Re: [Qemu-devel] [RESEND PATCH V3] qemu-img info: show nocow info

[Kevin Wolf <kwolf@redhat.com>]

Am 30.07.2014 um 04:55 hat Chunyan Liu geschrieben:
> Add nocow info in 'qemu-img info' output to show whether the file
> currently has NOCOW flag set or not.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> Resend for QEMU 2.2. Change json version comment. Add Reviewed-by.
> 
>  block/qapi.c         | 25 +++++++++++++++++++++++++
>  qapi/block-core.json |  5 ++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)

Unfortunately, I have never been CCed on this patch and I saw the code
it added only now. I think it doesn't work correctly and has the wrong
design, so I would prefer to revert it for 2.2 rather than making a bad
API stable.

> diff --git a/block/qapi.c b/block/qapi.c
> index f44f6b4..aa53f19 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -28,6 +28,13 @@
>  #include "qapi-visit.h"
>  #include "qapi/qmp-output-visitor.h"
>  #include "qapi/qmp/types.h"
> +#ifdef __linux__
> +#include <linux/fs.h>
> +#include <sys/ioctl.h>
> +#ifndef FS_NOCOW_FL
> +#define FS_NOCOW_FL                     0x00800000 /* Do not cow file */
> +#endif
> +#endif
>  
>  BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
>  {
> @@ -173,6 +180,20 @@ void bdrv_query_image_info(BlockDriverState *bs,
>      Error *err = NULL;
>      ImageInfo *info = g_new0(ImageInfo, 1);
>  
> +#ifdef __linux__
> +    int fd, attr;
> +
> +    /* get NOCOW info */
> +    fd = qemu_open(bs->filename, O_RDONLY | O_NONBLOCK);
> +    if (fd >= 0) {
> +        if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0 && (attr & FS_NOCOW_FL)) {
> +            info->has_nocow = true;
> +            info->nocow = true;
> +        }
> +        qemu_close(fd);
> +    }
> +#endif

Code like this has no business in the general block layer. This belongs
in raw-posix, which already has an open fd, and nowhere else. There you
wouldn't have to rely on bs->filename, which may be empty or contain
something different from a local filename, e.g. a URL. The approach of
this patch also doesn't work if the file has been renamed or deleted
since qemu opened it (e.g. because of -snapshot).

In short: This code may happen to work in some special cases, but
generally speaking, it is wrong.

>      bdrv_get_geometry(bs, &total_sectors);
>  
>      info->filename        = g_strdup(bs->filename);
> @@ -625,4 +646,8 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>          func_fprintf(f, "Format specific information:\n");
>          bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>      }
> +
> +    if (info->has_nocow && info->nocow) {
> +        func_fprintf(f, "NOCOW flag: set\n");
> +    }
>  }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e378653..72b4015 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -115,6 +115,8 @@
>  # @format-specific: #optional structure supplying additional format-specific
>  # information (since 1.7)
>  #
> +# @nocow: #optional info of whether NOCOW flag is set or not. (since 2.2)
> +#
>  # Since: 1.3
>  #
>  ##
> @@ -126,7 +128,8 @@
>             '*backing-filename': 'str', '*full-backing-filename': 'str',
>             '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
>             '*backing-image': 'ImageInfo',
> -           '*format-specific': 'ImageInfoSpecific' } }
> +           '*format-specific': 'ImageInfoSpecific',
> +           '*nocow': 'bool' } }

As this field makes only sense for raw-posix, it should be moved to
ImageInfoSpecific. ("format-specific" is also an unfortunate misnomer,
it should be "driver-specific", but it's too late to fix that.)

Kevin