diff mbox series

[v5,4/5] parallels: Replace fprintf by qemu_log in check

Message ID 20230529151503.34006-5-alexander.ivanov@virtuozzo.com
State New
Headers show
Series parallels: Add duplication check, repair at open, fix bugs | expand

Commit Message

Alexander Ivanov May 29, 2023, 3:15 p.m. UTC 
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(-)

Comments

Hanna Czenczek June 2, 2023, 2:48 p.m. UTC | #1 
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(-)
Alexander Ivanov June 9, 2023, 10:36 a.m. UTC | #2 
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(-)
>
Peter Maydell June 9, 2023, 10:59 a.m. UTC | #3 
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 mbox series

Patch

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++;