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

[Peter Maydell]

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