| Message ID | 20230529151503.34006-5-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallels: Add duplication check, repair at open, fix bugs | expand |
On 29.05.23 17:15, Alexander Ivanov wrote: > If the check is called during normal work, tracking of the check must be > present in VM logs to have some clues if something going wrong with user's > data. I understand stderr counts as part of the VM log, doesn’t it? I thought stderr is generally logged, and naïvely, it seems like the better fit to me, because it conveys more urgency than the standard log (which, judging from its callers, looks mostly like a debug log). Hanna > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > Reviewed-by: Denis V. Lunev <den@openvz.org> > --- > block/parallels.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-)
On 6/2/23 16:48, Hanna Czenczek wrote: > On 29.05.23 17:15, Alexander Ivanov wrote: >> If the check is called during normal work, tracking of the check must be >> present in VM logs to have some clues if something going wrong with >> user's >> data. > > I understand stderr counts as part of the VM log, doesn’t it? I > thought stderr is generally logged, and naïvely, it seems like the > better fit to me, because it conveys more urgency than the standard > log (which, judging from its callers, looks mostly like a debug log). > > Hanna We want to add some image checks to parallels_open(). It means that it will be used not only in qemu-img and we may lose these messages if a log file is specified. Stderr is not duplicated to the log file. > >> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> >> Reviewed-by: Denis V. Lunev <den@openvz.org> >> --- >> block/parallels.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >
On Mon, 29 May 2023 at 16:16, Alexander Ivanov <alexander.ivanov@virtuozzo.com> wrote: > > If the check is called during normal work, tracking of the check must be > present in VM logs to have some clues if something going wrong with user's > data. > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > Reviewed-by: Denis V. Lunev <den@openvz.org> > --- > block/parallels.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index 9fa1f93973..d64e8007d5 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -42,6 +42,7 @@ > #include "qemu/bswap.h" > #include "qemu/bitmap.h" > #include "qemu/memalign.h" > +#include "qemu/log-for-trace.h" > #include "migration/blocker.h" > #include "parallels.h" > > @@ -436,8 +437,8 @@ static void parallels_check_unclean(BlockDriverState *bs, > return; > } > > - fprintf(stderr, "%s image was not closed correctly\n", > - fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR"); > + qemu_log("%s image was not closed correctly\n", > + fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR"); Generally speaking you shouldn't directly call qemu_log(). Instead call qemu_log_mask() and pass it the relevant QEMU_LOG_* constant for whichever kind of -d debug logging this is. The raw qemu_log() function is for the odd case of expensive logging where you want to say if (log category foo enabled) { do expensive calculation; qemu_log(result1); qemu_log(result2); } But this doesn't look like the kind of logging we would usually do with qemu_log(). Notably, the default for the qemu_log machinery is to not output anything at all: it's only enabled if the user explicitly turns on debug logging with a -d option. You probably want warn_report() here. thanks -- PMM
diff --git a/block/parallels.c b/block/parallels.c index 9fa1f93973..d64e8007d5 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -42,6 +42,7 @@ #include "qemu/bswap.h" #include "qemu/bitmap.h" #include "qemu/memalign.h" +#include "qemu/log-for-trace.h" #include "migration/blocker.h" #include "parallels.h" @@ -436,8 +437,8 @@ static void parallels_check_unclean(BlockDriverState *bs, return; } - fprintf(stderr, "%s image was not closed correctly\n", - fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR"); + qemu_log("%s image was not closed correctly\n", + fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR"); res->corruptions++; if (fix & BDRV_FIX_ERRORS) { /* parallels_close will do the job right */ @@ -464,8 +465,8 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res, for (i = 0; i < s->bat_size; i++) { off = bat2sect(s, i) << BDRV_SECTOR_BITS; if (off + s->cluster_size > size) { - fprintf(stderr, "%s cluster %u is outside image\n", - fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); + qemu_log("%s cluster %u is outside image\n", + fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); res->corruptions++; if (fix & BDRV_FIX_ERRORS) { parallels_set_bat_entry(s, i, 0); @@ -551,8 +552,8 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res, count = DIV_ROUND_UP(leak_size, s->cluster_size); res->leaks += count; - fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n", - fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size); + qemu_log("%s space leaked at the end of the image %" PRId64 "\n", + fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size); if (fix & BDRV_FIX_LEAKS) { res->leaks_fixed += count; @@ -603,9 +604,8 @@ static int parallels_check_duplicate(BlockDriverState *bs, cluster_index = host_cluster_index(s, off); if (test_bit(cluster_index, bitmap)) { /* this cluster duplicates another one */ - fprintf(stderr, - "%s duplicate offset in BAT entry %u\n", - *fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); + qemu_log("%s duplicate offset in BAT entry %u\n", + *fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i); res->corruptions++;