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

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

[Chunyan Liu]

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>
Acked-by: Eric Blake <eblake@redhat.com>
---
Resend for QEMU 2.2. Change json version comment.

 block/qapi.c         | 25 +++++++++++++++++++++++++
 qapi/block-core.json |  5 ++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

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
+
     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' } }
 
 ##
 # @ImageCheck:
-- 
1.8.4.5

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On 07/29/2014 01:18 AM, Chunyan Liu wrote:
> 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>
> Acked-by: Eric Blake <eblake@redhat.com>

Actually, this should be Reviewed-by, not Acked-by.  My understanding is
that "Reviewed-by" is stronger (I actually read through the patch, and
looked for possible problems) while "Acked-by" is weaker (a maintainer
stating that the work is taken in on the basis of third-party reviews
without actually reviewing the patch itself).

> ---
> Resend for QEMU 2.2. Change json version comment.
> 
>  block/qapi.c         | 25 +++++++++++++++++++++++++
>  qapi/block-core.json |  5 ++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Chun Yan Liu]

>>> On 7/29/2014 at 08:45 PM, in message <53D79771.1040006@redhat.com>, Eric Blake
<eblake@redhat.com> wrote: 
> On 07/29/2014 01:18 AM, Chunyan Liu wrote: 
> > 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> 
> > Acked-by: Eric Blake <eblake@redhat.com> 
>  
> Actually, this should be Reviewed-by, not Acked-by.  My understanding is 
> that "Reviewed-by" is stronger (I actually read through the patch, and 
> looked for possible problems) while "Acked-by" is weaker (a maintainer 
> stating that the work is taken in on the basis of third-party reviews 
> without actually reviewing the patch itself). 

Thanks for clarification. I corrected it and sent.

>  
> > --- 
> > Resend for QEMU 2.2. Change json version comment. 
> >  
> >  block/qapi.c         | 25 +++++++++++++++++++++++++ 
> >  qapi/block-core.json |  5 ++++- 
> >  2 files changed, 29 insertions(+), 1 deletion(-) 
> >  
>  
> --  
> Eric Blake   eblake redhat com    +1-919-301-3266 
> Libvirt virtualization library http://libvirt.org 
>  
>  

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

[Chunyan Liu]

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(-)

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
+
     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' } }
 
 ##
 # @ImageCheck:
-- 
1.8.4.5

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

On Wed, Jul 30, 2014 at 10:55:06AM +0800, Chunyan Liu wrote:
> 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(-)

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Kevin Wolf]

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

On 11/21/2014 09:25 AM, Kevin Wolf 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 <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.

Even though I reviewed the original, you make some valid points, and I
can agree to a revert to avoid baking in a mistake.

>> @@ -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.)

Yes, that alone is a compelling reason to back this out and get the
interface right.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Chun Yan Liu]

>>> On 11/22/2014 at 12:25 AM, in message
<20141121162505.GF3956@noname.redhat.com>, Kevin Wolf <kwolf@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 <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.

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

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

[Kevin Wolf]

Am 24.11.2014 um 04:11 hat Chun Yan Liu geschrieben:
> 
> 
> >>> On 11/22/2014 at 12:25 AM, in message
> <20141121162505.GF3956@noname.redhat.com>, Kevin Wolf <kwolf@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 <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.
> 
> 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.

I believe there are two misunderstandings here:

First, I'm not talking about create options, that was a different patch.
The create options are already in a single place in raw-posix.c, so
that's all fine. The patch I commented on is about query-block.

Second, a block device in qemu isn't just qcow2 or just vmdk. It has at
least two levels and is something like "qcow2 over file" or "vmdk over
http". The nocow field is in fact specific to raw-posix.c (which provides
the "file" driver), you won't get it for images backed by an http,
gluster or nbd driver. Which format driver (like qcow2, vmdk or raw) is
used on top of this, is completely independent from that.

Kevin