>>> On 11/22/2014 at 12:25 AM, in message
<20141121162505.gf3...@noname.redhat.com>, Kevin Wolf <kw...@redhat.com> wrote:
> 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 <cy...@suse.com>
> > Reviewed-by: Eric Blake <ebl...@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.
I'm not disagreeing revert this patch if there is mistake. But here I need
to add some explanation to seek a correct future solution:
'Nocow' is not a specific attribute to raw-posix. This option is valid
to general disk images types, like 'raw', 'qcow2', 'qcow', 'vhdx', 'vpc',
'vmdk', etc. The reason why we only add 'nocow' in .createoptions in
raw-posix.c is: when creating all those file types, the creating point
is always in raw-posix.c, and since the NOCOW attr must be handled
in creating point, so we can only handle it in raw-posix.c file.
That's why we only add 'nocow' to raw-posix.c, but not other file types.
In creating, it will append two createoptions (like creating qcow2 file,
it will append qcow2 createoptions and also raw_posix createoptions),
so add 'nocow' to raw-posix is enough. Add 'nocow' to each file types
will touch many files.
- Chunyan
>
> Kevin
>
>
>
