Re: [Qemu-devel] [PATCH 20/20] vdi: say why an image is bad

[Fam Zheng]

On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Instead of just putting it in debugging output, we can now put the
> value in an Error.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  block/vdi.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index 68e152c..3859e49 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -399,39 +399,46 @@ static int vdi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          ret = -EINVAL;
>          goto fail;
>      } else if (header.version != VDI_VERSION_1_1) {
> -        logout("unsupported version %u.%u\n",
> -               header.version >> 16, header.version & 0xffff);
> +        error_setg(errp, "unsupported VDI image (version %u.%u)\n",
> +                   header.version >> 16, header.version & 0xffff);
>          ret = -ENOTSUP;
>          goto fail;
>      } else if (header.offset_bmap % SECTOR_SIZE != 0) {
>          /* We only support block maps which start on a sector boundary. */
> -        logout("unsupported block map offset 0x%x B\n", header.offset_bmap);
> +        error_setg(errp, "unsupported VDI image (unaligned block map offset 
> 0x%x)\n",
> +                   header.offset_bmap);
>          ret = -ENOTSUP;
>          goto fail;
>      } else if (header.offset_data % SECTOR_SIZE != 0) {
>          /* We only support data blocks which start on a sector boundary. */
> -        logout("unsupported data offset 0x%x B\n", header.offset_data);
> +        error_setg(errp, "unsupported VDI image (unaligned data offset 
> 0x%x)\n",
> +                   header.offset_data);
>          ret = -ENOTSUP;
>          goto fail;
>      } else if (header.sector_size != SECTOR_SIZE) {
> -        logout("unsupported sector size %u B\n", header.sector_size);
> +        error_setg(errp, "unsupported VDI image (sector size %u is not 
> %u)\n",
> +                   header.sector_size, SECTOR_SIZE);
>          ret = -ENOTSUP;
>          goto fail;
>      } else if (header.block_size != 1 * MiB) {
> -        logout("unsupported block size %u B\n", header.block_size);
> +        error_setg(errp, "unsupported VDI image (sector size %u is not 
> %u)\n",

s/sector size/block size/

> +                   header.block_size, 1 * MiB);
>          ret = -ENOTSUP;
>          goto fail;
>      } else if (header.disk_size >
>                 (uint64_t)header.blocks_in_image * header.block_size) {
> -        logout("unsupported disk size %" PRIu64 " B\n", header.disk_size);
> +        error_setg(errp, "unsupported VDI image (disk size %" PRIu64 ", "
> +                   "image bitmap has room for %" PRIu64 ")\n",
> +                   header.disk_size,
> +                   (uint64_t)header.blocks_in_image * header.block_size);

Improved, good!

>          ret = -ENOTSUP;
>          goto fail;
>      } else if (!uuid_is_null(header.uuid_link)) {
> -        logout("link uuid != 0, unsupported\n");
> +        error_setg(errp, "unsupported VDI image (non-NULL link UUID)\n");
>          ret = -ENOTSUP;
>          goto fail;
>      } else if (!uuid_is_null(header.uuid_parent)) {
> -        logout("parent uuid != 0, unsupported\n");
> +        error_setg(errp, "unsupported VDI image (non-NULL parent UUID)\n");
>          ret = -ENOTSUP;
>          goto fail;
>      }

There are also unnecessary ending "\n" in the messages.

Fam