Re: [Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file

[Peter Xu]

On Tue, Jun 06, 2017 at 06:42:18PM +0100, Dr. David Alan Gilbert wrote:
> * Kevin Wolf (kw...@redhat.com) wrote:
> > Am 06.06.2017 um 07:24 hat QingFeng Hao geschrieben:
> > > In load_snapshot, mis->from_src_file is freed twice, the first free is by
> > > qemu_fclose, the second is by migration_incoming_state_destroy and
> > > it causes Illegal instruction exception. The fix is just to remove the
> > > first free.
> > > 
> > > This problem is found by qemu-iotests case 068 since commit
> > > "660819b migration: shut src return path unconditionally". The error is:
> > > 068 1s ... - output mismatch (see 068.out.bad)
> > > --- tests/qemu-iotests/068.out2017-05-06 01:00:26.417270437 
> > > +0200
> > > +++ 068.out.bad   2017-06-03 13:59:55.360274640 +0200
> > > @@ -6,6 +6,8 @@
> > >  QEMU X.Y.Z monitor - type 'help' for more information
> > >  (qemu) savevm 0
> > >  (qemu) quit
> > > +./common.config: line 107: 242472 Illegal instruction (core 
> > > dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
> > > +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> > > +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> > >  QEMU X.Y.Z monitor - type 'help' for more information
> > > -(qemu) quit
> > > -*** done
> > > +(qemu) *** done
> > > 
> > > Signed-off-by: QingFeng Hao 
> > > Reviewed-by: Dr. David Alan Gilbert 
> > > Reviewed-by: Peter Xu 
> > 
> > Dave, as you only gave R-b rather than merging the patch, should this be
> > merged through the block tree?
> 
> I'm happy for it to go via block but also happy for it to go via
> migration; Juan is mostly doing the migration set at the moment since
> they're dominated by his cleanups.
> 
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index 9c320f59d0..853e14e34e 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
> > >  
> > >  aio_context_acquire(aio_context);
> > >  ret = qemu_loadvm_state(f);
> > > -qemu_fclose(f);
> > >  aio_context_release(aio_context);
> > >  
> > >  migration_incoming_state_destroy();
> > 
> > Did we check other callers of migration_incoming_state_destroy()?
> > 
> > For example, qmp_xen_load_devices_state() looks suspicious, too.
> 
> Hmm, it looks suspicious in the opposite direction; it never sets
> mis->from_src_file as was added by b4b076da into the load_snapshot path.

Agree.

Does qmp_xen_load_devices_state() needs to call
migration_incoming_state_destroy() after all? Since the latter
function only cleanups MigrationIncomingState and looks like the
former xen code didn't really use it at all.

> 
> > I can't tell for postcopy_ram_listen_thread() - commit 660819b didn't
> > seem to remove a qemu_fclose() call there, but I can't see one left
> > behind either. Was the file leaked before commit 660819b or am I
> > missing something?
> 
> I don't think there's a problem in the postcopy path, although hmm was
> I missing a close before?
> 
> Dave
> > 
> > Kevin
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

-- 
Peter Xu

Re: [Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file

[QingFeng Hao]

在 2017/6/6 20:49, Kevin Wolf 写道:

Am 06.06.2017 um 07:24 hat QingFeng Hao geschrieben:

In load_snapshot, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.

This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
 --- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
 +++ 068.out.bad2017-06-03 13:59:55.360274640 +0200
 @@ -6,6 +6,8 @@
  QEMU X.Y.Z monitor - type 'help' for more information
  (qemu) savevm 0
  (qemu) quit
 +./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n 
"${QEMU_NEED_PID}" ]; then
 +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
 +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
  QEMU X.Y.Z monitor - type 'help' for more information
 -(qemu) quit
 -*** done
 +(qemu) *** done

Signed-off-by: QingFeng Hao 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Peter Xu 

Dave, as you only gave R-b rather than merging the patch, should this be
merged through the block tree?


diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
  
  aio_context_acquire(aio_context);

  ret = qemu_loadvm_state(f);
-qemu_fclose(f);
  aio_context_release(aio_context);
  
  migration_incoming_state_destroy();

Did we check other callers of migration_incoming_state_destroy()?

For example, qmp_xen_load_devices_state() looks suspicious, too.
Good reminder! Yes, I checked it and there is no assignment of 
from_src_file there and f
is opened locally, so I think that qemu_fclose doesn't impact 
migration_incoming_state_destroy.

migration_incoming_state_destroy is called in 4 places:
process_incoming_migration_bh, postcopy_ram_listen_thread, 
qmp_xen_load_devices_state
and load_snapshot. process_incoming_migration_bh is launched by 
process_incoming_migration_co

whose qemu_fclose is removed by commit 660819b.
For postcopy_ram_listen_thread, I didn't see where it calls qemu_fclose.
Actually to simplify the check for the problem, I just searched where 
from_src_file
is assigned to and got 2 places: process_incoming_migration_co and 
load_snapshot.
qemu_fclose in the first function is removed by commit 660819b, and 
qemu_fclose in the
second is removed by this one. I think a potential risk might be opaque 
is closed
by anywhere else than process_incoming_migration_co, but there is legacy 
qemu_close

before commit 660819b, so the risk might be low? thanks :)


I can't tell for postcopy_ram_listen_thread() - commit 660819b didn't
seem to remove a qemu_fclose() call there, but I can't see one left
behind either. Was the file leaked before commit 660819b or am I
missing something?
I don't think so because loadvm_postcopy_handle_listen creates thread 
postcopy_ram_listen_thread
and passes mis->from_src_file as its arg, which will be closed by 
migration_incoming_state_destroy.
What confuses me is in the series function calls of 
qemu_loadvm_state_main etc, argument f looks
to be redundant as mis already contains from_src_file which equals to f. 
Furthermore, mis may be

also redundant as it can be got via migration_incoming_get_current. Thanks!


Kevin



--
Regards
QingFeng Hao

Re: [Qemu-block] Virtio-BLK/SCSI write requests and data payload checksums

[Paolo Bonzini]

- Original Message -
> From: "Peter Lieven" 
> To: "qemu block" 
> Cc: "Paolo Bonzini" , stefa...@redhat.com, 
> kw...@redhat.com, "Max Reitz" ,
> "Michael S. Tsirkin" 
> Sent: Tuesday, June 6, 2017 10:13:51 PM
> Subject: Virtio-BLK/SCSI write requests and data payload checksums
> 
> Hi,
> 
> 
> ich have spend several hours debugging a strange checksum error issue and
> finally found the cause, but I am totally unsure if whats happening
> is correct or not.
> 
> Imagine a Protocol like iSCSI which has a Data Digest and which receives its
> data via zero copy straight from the guest kernel through Qemu and
> 
> then out of the socket to the network. It calculates the CRC32C data digest
> from the buffer it gets from Qemu. It totally relies thereby that the buffer
> is not mangled after the write request has been submitted. But it seems this
> assumption is not true.
> 
> [...] the contents of the buffer submitted to Qemu change while Qemu processes
> the write.
> In the end several write requests follow and the /etc/network/interfaces file
> has the right content, but
> is this at all legal?

Yes, it's ugly but it's legal.  It probably doesn't happen on real hardware
that computes the checksum after or during DMA and has some kind of buffer
inside the board.  But on virt there is only one copy until we reach the actual
physical hardware.

Paolo

> If yes, checksum calculation of payload and zero copy
> would not work together.

[Qemu-block] Virtio-BLK/SCSI write requests and data payload checksums

[Peter Lieven]

Hi,


ich have spend several hours debugging a strange checksum error issue and 
finally found the cause, but I am totally unsure if whats happening

is correct or not.


Imagine a Protocol like iSCSI which has a Data Digest and which receives its 
data via zero copy straight from the guest kernel through Qemu and

then out of the socket to the network. It calculates the CRC32C data digest 
from the buffer it gets from Qemu. It totally relies thereby that the buffer

is not mangled after the write request has been submitted. But it seems this 
assumption is not true.


In my some virtual Debian based Virtual Machines I somewhere have the following 
code to generate a new network config:


---8<---

echo "# interfaces(5) file used by ifup(8) and ifdown(8)" 
>/etc/network/interfaces
echo >>/etc/network/interfaces
echo "auto lo" >>/etc/network/interfaces
echo "iface lo inet loopback" >>/etc/network/interfaces
echo >>/etc/network/interfaces
echo "auto eth0" >>/etc/network/interfaces
echo "iface eth0 inet $IP_CONFIG" >>/etc/network/interfaces

if [ "$IP_CONFIG" = "static" ]; then
 echo " address $IP_ADDRESS" >>/etc/network/interfaces
 echo " netmask $IP_NETMASK" >>/etc/network/interfaces
 echo " gateway $IP_GW" >>/etc/network/interfaces
 echo " dns-nameservers $IP_DNS1 $IP_DNS2" >>/etc/network/interfaces
fi

--->8---


I would suspect several /write cycles one for each echo line. Maybe some of the 
requests consolidated together.
However, the frist write request I see looks like this:

#offset=3316645888 bytes=4096 flags=0 niov=1 crc32c=0xeabc59bf

The submitted buffer has the following contents when I enter the write function:
---8<---
# interfaces(5) file used by ifup(8) and ifdown(8)

auto lo
--->8---

When the write completes the buffer has a different checksum and the following 
contents:
---8<---
# interfaces(5) file used by ifup(8) and ifdown(8)

auto lo
iface lo inet loopback

auto eth0
--->8---

So the contents of the buffer submitted to Qemu change while Qemu processes the 
write.
In the end several write requests follow and the /etc/network/interfaces file 
has the right content, but
is this at all legal? If yes, checksum calculation of payload and zero copy 
would not work together.

Thanks,
Peter

Re: [Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file

[Juan Quintela]

Kevin Wolf  wrote:
> Am 06.06.2017 um 07:24 hat QingFeng Hao geschrieben:
>> In load_snapshot, mis->from_src_file is freed twice, the first free is by
>> qemu_fclose, the second is by migration_incoming_state_destroy and
>> it causes Illegal instruction exception. The fix is just to remove the
>> first free.
>> 
>> This problem is found by qemu-iotests case 068 since commit
>> "660819b migration: shut src return path unconditionally". The error is:
>> 068 1s ... - output mismatch (see 068.out.bad)
>> --- tests/qemu-iotests/068.out   2017-05-06 01:00:26.417270437 +0200
>> +++ 068.out.bad  2017-06-03 13:59:55.360274640 +0200
>> @@ -6,6 +6,8 @@
>>  QEMU X.Y.Z monitor - type 'help' for more information
>>  (qemu) savevm 0
>>  (qemu) quit
>> +./common.config: line 107: 242472 Illegal instruction (core dumped) 
>> ( if [ -n "${QEMU_NEED_PID}" ]; then
>> +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>  QEMU X.Y.Z monitor - type 'help' for more information
>> -(qemu) quit
>> -*** done
>> +(qemu) *** done
>> 
>> Signed-off-by: QingFeng Hao 
>> Reviewed-by: Dr. David Alan Gilbert 
>> Reviewed-by: Peter Xu 
>
> Dave, as you only gave R-b rather than merging the patch, should this be
> merged through the block tree?

I got it.  I will send a pull request later Today.


>
> Did we check other callers of migration_incoming_state_destroy()?
>
> For example, qmp_xen_load_devices_state() looks suspicious, too.
>
> I can't tell for postcopy_ram_listen_thread() - commit 660819b didn't
> seem to remove a qemu_fclose() call there, but I can't see one left
> behind either. Was the file leaked before commit 660819b or am I
> missing something?

I will take a look at those.

Thanks, Juan.

Re: [Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file

[Dr. David Alan Gilbert]

* Kevin Wolf (kw...@redhat.com) wrote:
> Am 06.06.2017 um 07:24 hat QingFeng Hao geschrieben:
> > In load_snapshot, mis->from_src_file is freed twice, the first free is by
> > qemu_fclose, the second is by migration_incoming_state_destroy and
> > it causes Illegal instruction exception. The fix is just to remove the
> > first free.
> > 
> > This problem is found by qemu-iotests case 068 since commit
> > "660819b migration: shut src return path unconditionally". The error is:
> > 068 1s ... - output mismatch (see 068.out.bad)
> > --- tests/qemu-iotests/068.out  2017-05-06 01:00:26.417270437 +0200
> > +++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200
> > @@ -6,6 +6,8 @@
> >  QEMU X.Y.Z monitor - type 'help' for more information
> >  (qemu) savevm 0
> >  (qemu) quit
> > +./common.config: line 107: 242472 Illegal instruction (core 
> > dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
> > +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> > +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> >  QEMU X.Y.Z monitor - type 'help' for more information
> > -(qemu) quit
> > -*** done
> > +(qemu) *** done
> > 
> > Signed-off-by: QingFeng Hao 
> > Reviewed-by: Dr. David Alan Gilbert 
> > Reviewed-by: Peter Xu 
> 
> Dave, as you only gave R-b rather than merging the patch, should this be
> merged through the block tree?

I'm happy for it to go via block but also happy for it to go via
migration; Juan is mostly doing the migration set at the moment since
they're dominated by his cleanups.

> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 9c320f59d0..853e14e34e 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
> >  
> >  aio_context_acquire(aio_context);
> >  ret = qemu_loadvm_state(f);
> > -qemu_fclose(f);
> >  aio_context_release(aio_context);
> >  
> >  migration_incoming_state_destroy();
> 
> Did we check other callers of migration_incoming_state_destroy()?
> 
> For example, qmp_xen_load_devices_state() looks suspicious, too.

Hmm, it looks suspicious in the opposite direction; it never sets
mis->from_src_file as was added by b4b076da into the load_snapshot path.

> I can't tell for postcopy_ram_listen_thread() - commit 660819b didn't
> seem to remove a qemu_fclose() call there, but I can't see one left
> behind either. Was the file leaked before commit 660819b or am I
> missing something?

I don't think there's a problem in the postcopy path, although hmm was
I missing a close before?

Dave
> 
> Kevin
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-block] [PATCH v3 0/3] qemu-img check: format allocation info

[Vladimir Sementsov-Ogievskiy]

Hi all.

See 01 patch for the doc.

v3: - improve docs
- rename fields
- add 'zero' type of underlying file portions status. It as these
  areas cannot be presented as 'discarded', but they are not
  occupying real space, so we don't want to present them as
  'allocated data' too.
- remove last patch. It is not needed after introducing new naming
  for the fields

v2: fix build error, gcc things that some variables may be used
uninitialized (actually they didn't).

v1: 

These series is a replacement for "qemu-img check: unallocated size"
series.

There was a question, should we account allocated clusters in qcow2 but
actually holes in underalying file as allocated or not. Instead of
hiding this information in one-number statistic I've decided to print
the whole information, 5 numbers:

For allocated by top-level format driver (qcow2 for ex.) clusters, 3
numbers: number of bytes, which are:
 - allocated in underlying file
 - holes in underlying file
 - after end of underlying file

To account other areas of underlying file, 2 more numbers of bytes:
 - unallocated by top-level driver but allocated in underlying file
 - unallocated by top-level driver and holes in underlying file

Vladimir Sementsov-Ogievskiy (3):
  block: add bdrv_get_format_alloc_stat format interface
  qcow2: add .bdrv_get_format_alloc_stat
  qemu-img check: add format allocation info

 block.c   |  16 +
 block/qcow2-refcount.c| 152 ++
 block/qcow2.c |   2 +
 block/qcow2.h |   2 +
 include/block/block.h |   3 +
 include/block/block_int.h |   2 +
 qapi/block-core.json  |  61 ++-
 qemu-img.c|  42 +
 8 files changed, 279 insertions(+), 1 deletion(-)

-- 
2.11.1

[Qemu-block] [PATCH v3 1/3] block: add bdrv_get_format_alloc_stat format interface

[Vladimir Sementsov-Ogievskiy]

The function should collect statistics, about used/unused by top-level
format driver space (in its .file) and allocation status
(data/zero/discarded/after-eof) of corresponding areas in this .file.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c   | 16 ++
 include/block/block.h |  3 +++
 include/block/block_int.h |  2 ++
 qapi/block-core.json  | 55 +++
 4 files changed, 76 insertions(+)

diff --git a/block.c b/block.c
index 50ba264143..7d720ae0c2 100644
--- a/block.c
+++ b/block.c
@@ -3407,6 +3407,22 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState 
*bs)
 }
 
 /**
+ * Collect format allocation info. See BlockFormatAllocInfo definition in
+ * qapi/block-core.json.
+ */
+int bdrv_get_format_alloc_stat(BlockDriverState *bs, BlockFormatAllocInfo 
*bfai)
+{
+BlockDriver *drv = bs->drv;
+if (!drv) {
+return -ENOMEDIUM;
+}
+if (drv->bdrv_get_format_alloc_stat) {
+return drv->bdrv_get_format_alloc_stat(bs, bfai);
+}
+return -ENOTSUP;
+}
+
+/**
  * Return number of sectors on success, -errno on error.
  */
 int64_t bdrv_nb_sectors(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e92d8..646376a772 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -335,6 +335,9 @@ typedef enum {
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 
+int bdrv_get_format_alloc_stat(BlockDriverState *bs,
+   BlockFormatAllocInfo *bfai);
+
 /* The units of offset and total_work_size may be chosen arbitrarily by the
  * block driver; total_work_size may change during the course of the amendment
  * operation */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d3724cce6..458c715e99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -208,6 +208,8 @@ struct BlockDriver {
 int64_t (*bdrv_getlength)(BlockDriverState *bs);
 bool has_variable_length;
 int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
+int (*bdrv_get_format_alloc_stat)(BlockDriverState *bs,
+  BlockFormatAllocInfo *bfai);
 
 int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
 uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ea0b3e8b13..fd7b52bd69 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -139,6 +139,61 @@
'*format-specific': 'ImageInfoSpecific' } }
 
 ##
+# @BlockFormatAllocInfo:
+#
+#
+# Allocation relations between format file and underlying protocol file.
+# All fields are in bytes.
+#
+# There are two types of the format file portions: 'used' and 'unused'. It's up
+# to the format how to interpret these types. For now the only format 
supporting
+# the feature is Qcow2 and for this case 'used' are clusters with positive
+# refcount and unused a clusters with zero refcount. Described portions include
+# all format file allocations, not only virtual disk data (metadata, internal
+# snapshots, etc. are included).
+#
+# For the underlying file there are native block-status types of the portions:
+#  - data: allocated data
+#  - zero: read-as-zero holes
+#  - discarded: not allocated
+# 4th additional type is 'overrun', which is for the format file portions 
beyond
+# the end of the underlying file.
+#
+# So, the fields are:
+#
+# @used-data: used by the format file and backed by data in the underlying file
+#
+# @used-zero: used by the format file and backed by a hole in the underlying
+# file
+#
+# @used-discarded: used by the format file but actually unallocated in the
+#  underlying file
+#
+# @used-overrun: used by the format file beyond the end of the underlying file
+#
+# @unused-data: allocated data in the underlying file not used by the format
+#
+# @unused-zero: holes in the underlying file not used by the format file
+#
+# @unused-discarded: unallocated areas in the underlying file not used by the
+#format file
+#
+# Note: sum of 6 fields {used,unused}-{data,zero,discarded} is equal to the
+#   length of the underlying file.
+#
+# Since: 2.10
+#
+##
+{ 'struct': 'BlockFormatAllocInfo',
+  'data': {'used-data':'uint64',
+   'used-zero':'uint64',
+   'used-discarded':   'uint64',
+   'used-overrun': 'uint64',
+   'unused-data':  'uint64',
+   'unused-zero':  'uint64',
+   'unused-discarded': 'uint64' } }
+
+##
 # @ImageCheck:
 #
 # Information about a QEMU image file check
-- 
2.11.1

[Qemu-block] [PATCH v3 2/3] qcow2: add .bdrv_get_format_alloc_stat

[Vladimir Sementsov-Ogievskiy]

Realize .bdrv_get_format_alloc_stat interface.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-refcount.c | 152 +
 block/qcow2.c  |   2 +
 block/qcow2.h  |   2 +
 3 files changed, 156 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 7c06061aae..fd6027f0c2 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2931,3 +2931,155 @@ done:
 qemu_vfree(new_refblock);
 return ret;
 }
+
+typedef struct FormatAllocStatCo {
+BlockDriverState *bs;
+BlockFormatAllocInfo *bfai;
+int64_t ret;
+} FormatAllocStatCo;
+
+static void coroutine_fn qcow2_get_format_alloc_stat_entry(void *opaque)
+{
+int ret = 0;
+FormatAllocStatCo *nbco = opaque;
+BlockDriverState *bs = nbco->bs;
+BDRVQcow2State *s = bs->opaque;
+BlockFormatAllocInfo *bfai = nbco->bfai;
+int64_t cluster, file_sectors, sector;
+int refcount_block_offset;
+uint32_t i;
+bool used = false, f_data = false, f_zero = false;
+int dif, num = 0, f_num = 0;
+
+memset(bfai, 0, sizeof(*bfai));
+
+file_sectors = bdrv_nb_sectors(bs->file->bs);
+if (file_sectors < 0) {
+nbco->ret = file_sectors;
+return;
+}
+
+qemu_co_mutex_lock(>lock);
+
+for (sector = 0; sector < file_sectors; sector += dif) {
+if (f_num == 0) {
+BlockDriverState *file;
+ret = bdrv_get_block_status(bs->file->bs, sector,
+file_sectors - sector, _num, );
+if (ret < 0) {
+goto fail;
+}
+f_data = ret & BDRV_BLOCK_DATA;
+f_zero = ret & BDRV_BLOCK_ZERO;
+}
+
+if (num == 0) {
+uint64_t refcount;
+assert(((sector << BDRV_SECTOR_BITS) & (s->cluster_size - 1)) == 
0);
+ret = qcow2_get_refcount(
+bs, (sector << BDRV_SECTOR_BITS) >> s->cluster_bits, 
);
+if (ret < 0) {
+goto fail;
+}
+used = refcount > 0;
+num = s->cluster_size >> BDRV_SECTOR_BITS;
+}
+
+dif = MIN(f_num, MIN(num, file_sectors - sector));
+if (used) {
+if (f_data) {
+bfai->used_data += dif;
+} else if (f_zero) {
+bfai->used_zero += dif;
+} else {
+bfai->used_discarded += dif;
+}
+} else {
+if (f_data) {
+bfai->unused_data += dif;
+} else if (f_zero) {
+bfai->unused_zero += dif;
+} else {
+bfai->unused_discarded += dif;
+}
+}
+f_num -= dif;
+num -= dif;
+}
+
+assert(f_num == 0);
+
+if (used) {
+bfai->used_overrun += num;
+}
+
+cluster = size_to_clusters(s, sector << BDRV_SECTOR_BITS);
+refcount_block_offset = cluster & (s->refcount_block_size - 1);
+for (i = cluster >> s->refcount_block_bits;
+ i <= s->max_refcount_table_index; i++)
+{
+int j;
+
+if (!(s->refcount_table[i] & REFT_OFFSET_MASK)) {
+refcount_block_offset = 0;
+continue;
+}
+
+for (j = refcount_block_offset; j < s->refcount_block_size; j++) {
+uint64_t refcount;
+cluster = (i << s->refcount_block_bits) + j;
+
+ret = qcow2_get_refcount(bs, cluster, );
+if (ret < 0) {
+goto fail;
+}
+if (refcount > 0) {
+bfai->used_overrun++;
+}
+}
+
+refcount_block_offset = 0;
+}
+
+qemu_co_mutex_unlock(>lock);
+
+bfai->used_data = bfai->used_data << BDRV_SECTOR_BITS;
+bfai->used_zero = bfai->used_zero << BDRV_SECTOR_BITS;
+bfai->used_discarded = bfai->used_discarded << BDRV_SECTOR_BITS;
+bfai->used_overrun = bfai->used_overrun << BDRV_SECTOR_BITS;
+
+bfai->unused_data = bfai->unused_data << BDRV_SECTOR_BITS;
+bfai->unused_zero = bfai->unused_zero << BDRV_SECTOR_BITS;
+bfai->unused_discarded = bfai->unused_discarded << BDRV_SECTOR_BITS;
+
+nbco->ret = 0;
+return;
+
+fail:
+nbco->ret = ret;
+qemu_co_mutex_unlock(>lock);
+}
+
+/* qcow2_get_format_alloc_stat()
+ * Fills @bfai struct. In case of failure @bfai content is unpredicted.
+ */
+int qcow2_get_format_alloc_stat(BlockDriverState *bs,
+BlockFormatAllocInfo *bfai)
+{
+FormatAllocStatCo nbco = {
+.bs = bs,
+.bfai = bfai,
+.ret = -EINPROGRESS
+};
+
+if (qemu_in_coroutine()) {
+qcow2_get_format_alloc_stat_entry();
+} else {
+Coroutine *co =
+qemu_coroutine_create(qcow2_get_format_alloc_stat_entry, );
+qemu_coroutine_enter(co);
+BDRV_POLL_WHILE(bs, nbco.ret == -EINPROGRESS);
+}
+
+return nbco.ret;
+}

[Qemu-block] [PATCH v3 3/3] qemu-img check: add format allocation info

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json |  6 +-
 qemu-img.c   | 42 ++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index fd7b52bd69..b9b9952541 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -232,6 +232,9 @@
 #   field is present if the driver for the image format
 #   supports it
 #
+# @format-alloc-info: Format-allocation information, see
+# BlockFormatAllocInfo description. (Since: 2.10)
+#
 # Since: 1.4
 #
 ##
@@ -240,7 +243,8 @@
'*image-end-offset': 'int', '*corruptions': 'int', '*leaks': 'int',
'*corruptions-fixed': 'int', '*leaks-fixed': 'int',
'*total-clusters': 'int', '*allocated-clusters': 'int',
-   '*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
+   '*fragmented-clusters': 'int', '*compressed-clusters': 'int',
+   '*format-alloc-info': 'BlockFormatAllocInfo' } }
 
 ##
 # @MapEntry:
diff --git a/qemu-img.c b/qemu-img.c
index b506839ef0..b3adf9a1a2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -560,6 +560,35 @@ static void dump_json_image_check(ImageCheck *check, bool 
quiet)
 QDECREF(str);
 }
 
+static void dump_human_format_alloc_info(BlockFormatAllocInfo *bfai, bool 
quiet)
+{
+char *used_data = size_to_str(bfai->used_data);
+char *used_zero = size_to_str(bfai->used_zero);
+char *used_discarded = size_to_str(bfai->used_discarded);
+char *used_overrun = size_to_str(bfai->used_overrun);
+
+char *unused_data = size_to_str(bfai->unused_data);
+char *unused_zero = size_to_str(bfai->unused_zero);
+char *unused_discarded = size_to_str(bfai->unused_discarded);
+
+qprintf(quiet,
+"Format allocation info (including metadata):\n"
+"   datazero   discarded   after-eof\n"
+"used %10s  %10s  %10s  %10s\n"
+"unused   %10s  %10s  %10s\n",
+used_data, used_zero, used_discarded, used_overrun,
+unused_data, unused_zero, unused_discarded);
+
+g_free(used_data);
+g_free(used_zero);
+g_free(used_discarded);
+g_free(used_overrun);
+
+g_free(unused_data);
+g_free(unused_zero);
+g_free(unused_discarded);
+}
+
 static void dump_human_image_check(ImageCheck *check, bool quiet)
 {
 if (!(check->corruptions || check->leaks || check->check_errors)) {
@@ -601,6 +630,10 @@ static void dump_human_image_check(ImageCheck *check, bool 
quiet)
 qprintf(quiet,
 "Image end offset: %" PRId64 "\n", check->image_end_offset);
 }
+
+if (check->has_format_alloc_info) {
+dump_human_format_alloc_info(check->format_alloc_info, quiet);
+}
 }
 
 static int collect_image_check(BlockDriverState *bs,
@@ -611,6 +644,7 @@ static int collect_image_check(BlockDriverState *bs,
 {
 int ret;
 BdrvCheckResult result;
+BlockFormatAllocInfo *bfai = g_new0(BlockFormatAllocInfo, 1);
 
 ret = bdrv_check(bs, , fix);
 if (ret < 0) {
@@ -639,6 +673,14 @@ static int collect_image_check(BlockDriverState *bs,
 check->compressed_clusters  = result.bfi.compressed_clusters;
 check->has_compressed_clusters  = result.bfi.compressed_clusters != 0;
 
+ret = bdrv_get_format_alloc_stat(bs, bfai);
+if (ret < 0) {
+qapi_free_BlockFormatAllocInfo(bfai);
+} else {
+check->has_format_alloc_info = true;
+check->format_alloc_info = bfai;
+}
+
 return 0;
 }
 
-- 
2.11.1

Re: [Qemu-block] [PATCH] qemu-img: supplement the omitted 'disk size' bytes info

[sochin.jiang]

Please ignore this patch, I realize that there are some unfixed problems,
like iotest, etc.

On 2017/6/6 15:03, sochin.jiang wrote:
> From: "sochin.jiang" 
>
>  Supplement the omitted 'disk size' bytes information when using
>
>  'qemu-img info',it is useful in some occasion. Anyhow, it looks
>
>  strange that 'virtual size' has bytes while actual 'disk size'
>
>  does not.
>
> Signed-off-by: sochin.jiang 
> ---
>  block/qapi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index a40922e..9bb5956 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -703,10 +703,11 @@ void bdrv_image_info_dump(fprintf_function 
> func_fprintf, void *f,
>   "image: %s\n"
>   "file format: %s\n"
>   "virtual size: %s (%" PRId64 " bytes)\n"
> - "disk size: %s\n",
> + "disk size: %s (%" PRId64 " bytes)\n",
>   info->filename, info->format, size_buf,
>   info->virtual_size,
> - dsize_buf);
> + dsize_buf,
> + info->actual_size);
>  
>  if (info->has_encrypted && info->encrypted) {
>  func_fprintf(f, "encrypted: yes\n");

Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/4] qemu-io: Don't die on second open

[Kevin Wolf]

Am 05.06.2017 um 21:58 hat Eric Blake geschrieben:
> Also, I'm seeing a segfault on 68 that exists on current master:
> 
> 068 1s ... - output mismatch (see 068.out.bad)
> --- /home/eblake/qemu/tests/qemu-iotests/068.out  2017-06-01
> 17:01:25.113094957 -0500
> +++ 068.out.bad   2017-06-05 14:55:54.140835402 -0500
> @@ -6,6 +6,8 @@
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) savevm 0
>  (qemu) quit
> +./common.config: line 107: 1 Segmentation fault  (core dumped)
> ( if [ -n "${QEMU_NEED_PID}" ]; then
> +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) quit
> -*** done
> +(qemu) *** done

This should be fixed with "qemu/migration: fix the migration bug found
by qemu-iotests case 068".

Kevin


pgpI68ofbNlHU.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file

[Kevin Wolf]

Am 06.06.2017 um 07:24 hat QingFeng Hao geschrieben:
> In load_snapshot, mis->from_src_file is freed twice, the first free is by
> qemu_fclose, the second is by migration_incoming_state_destroy and
> it causes Illegal instruction exception. The fix is just to remove the
> first free.
> 
> This problem is found by qemu-iotests case 068 since commit
> "660819b migration: shut src return path unconditionally". The error is:
> 068 1s ... - output mismatch (see 068.out.bad)
> --- tests/qemu-iotests/068.out2017-05-06 01:00:26.417270437 +0200
> +++ 068.out.bad   2017-06-03 13:59:55.360274640 +0200
> @@ -6,6 +6,8 @@
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) savevm 0
>  (qemu) quit
> +./common.config: line 107: 242472 Illegal instruction (core dumped) 
> ( if [ -n "${QEMU_NEED_PID}" ]; then
> +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) quit
> -*** done
> +(qemu) *** done
> 
> Signed-off-by: QingFeng Hao 
> Reviewed-by: Dr. David Alan Gilbert 
> Reviewed-by: Peter Xu 

Dave, as you only gave R-b rather than merging the patch, should this be
merged through the block tree?

> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9c320f59d0..853e14e34e 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
>  
>  aio_context_acquire(aio_context);
>  ret = qemu_loadvm_state(f);
> -qemu_fclose(f);
>  aio_context_release(aio_context);
>  
>  migration_incoming_state_destroy();

Did we check other callers of migration_incoming_state_destroy()?

For example, qmp_xen_load_devices_state() looks suspicious, too.

I can't tell for postcopy_ram_listen_thread() - commit 660819b didn't
seem to remove a qemu_fclose() call there, but I can't see one left
behind either. Was the file leaked before commit 660819b or am I
missing something?

Kevin

Re: [Qemu-block] [PATCH v4 0/4] more blkdebug tweaks

[Kevin Wolf]

Am 05.06.2017 um 22:38 hat Eric Blake geschrieben:
> I found a crasher and some odd behavior while rebasing my
> bdrv_get_block_status series, so I figured I'd get these things
> fixed first.  This is based on top of Max's block branch.
> 
> Available as a tag at:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4
> 
> Since v3:
> - check all qemu-iotests (patch 1)

Whoops, somehow I ended up applying and reviewing v4 while looking at
the v3 thread... Looks like there really aren't any missing test case
updates any more.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v3 0/4] more blkdebug tweaks

[Kevin Wolf]

Am 05.06.2017 um 21:08 hat Eric Blake geschrieben:
> I found a crasher and some odd behavior while rebasing my
> bdrv_get_block_status series, so I figured I'd get these things
> fixed first.  This is based on top of Max's block branch.
> 
> Available as a tag at:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v3
> 
> Since v2:
> - defer the original patch 3 to a later series
> - rebase to make sure test 177 passes at all times (I didn't
> pinpoint which recent merge caused it, but the test now produces
> json:{...} instead of blkdebug:... during patch 2)
> - add more documentation of BDRV_BLOCK_RAW
> - add R-b where appropriate
> 
> 001/4:[] [--] 'qemu-io: Don't die on second open'
> 002/4:[0002] [FC] 'block: Guarantee that *file is set on 
> bdrv_get_block_status()'
> 003/4:[0006] [FC] 'block: Simplify use of BDRV_BLOCK_RAW'
> 004/4:[0002] [FC] 'blkdebug: Support .bdrv_co_get_block_status'

Patches 2-4:
Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH 1/4] block: add bdrv_get_format_alloc_stat format interface

[Eric Blake]

On 06/02/2017 10:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.05.2017 17:53, Eric Blake wrote:
>> On 05/30/2017 05:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> The function should collect statistics, about allocted/unallocated by
>>> top-level format driver space (in its .file) and allocation status
>>> (allocated/hole/after eof) of corresponding areas in this .file.
>>>

>>> +# @BlockFormatAllocInfo:
>>> +#
>>> +# Information about allocations, including metadata. All fields are
>>> in bytes.
> 
> s/All fields are in bytes./All fields are in bytes and aligned by sector
> (512 bytes)./

I wouldn't even promise sector alignment. We probably happen to have
sector alignment (especially for qcow2, since the smallest cluster size
permitted is sector aligned), but I see no inherent reason why we can't
support sub-sector values if we are reporting in bytes.

> 
> - ok? to emphasize that there is nothing about clusters... Or may be
> better to write that they are aligned by byte.

I think "All fields are in bytes" is sufficient.


>>> +{ 'struct': 'BlockFormatAllocInfo',
>>> +  'data': {'alloc_alloc':'uint64',
>>> +   'alloc_hole': 'uint64',
>>> +   'alloc_overhead': 'uint64',
>>> +   'hole_alloc': 'uint64',
>>> +   'hole_hole':  'uint64' } }
>> The idea seems okay, but the naming needs to be fixed.  Also, I'm not
>> sure if we need all 5, or if 4 is enough; and I'm not sure if we have
>> the right names ("how does alloc-hole differ from hole-alloc?"), or if
>> we can come up with something more descriptive.  Particularly since
>> "hole-" is not a hole in the filesystem sense, so much as unused
>> clusters.  But I'm also not coming up with better names to suggest at
>> the moment.
>>
> how about:
> 
> used-allocated
> used-discarded
> used-overrun
> 
> unused-allocated
> unused-discarded

Those work for me.

> 
> 
> also, do you mention that your detailed wordings should be included into
> block-core.json or you just clarify things?

Good documentation is worth the effort. I don't know if you want all of
my details in block-core.json, but giving a better overview of how each
statistic is possible does make it easier to visualize what the
statistic is actually counting.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] block/qcow.c: Fix memory leak in qcow_create()

[Kevin Wolf]

Am 05.06.2017 um 15:55 hat Peter Maydell geschrieben:
> Coverity points out that the code path in qcow_create() for
> the magic "fat:" backing file name leaks the memory used to
> store the filename (CID 1307771). Free the memory before
> we overwrite the pointer.
> 
> Signed-off-by: Peter Maydell 

Thanks, applied to the block branch.

Kevin

[Qemu-block] [PATCH 2/2] nvme: Check PSDT flag for admin and rw command

[Qu Wenruo]

Check PRP or SGL for Data Transfer(PSDT) flag for admin and read/write
commands.

For NVMe-over-PCIE, admin sq entry should not set SGL bits.
and rw sq entry PSDT flag should not be set beyond controller support.

Although currently either linux kernel NVMe-over-PCIE host driver uses
SGL nor qemu nvme controller supports it, it's never a bad idea to
enhance the check.

Signed-off-by: Qu Wenruo 
---
 hw/block/nvme.c | 11 +++
 hw/block/nvme.h |  7 ++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fa1069160e..b2579594d9 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -284,6 +284,10 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, 
NvmeCmd *cmd,
 block_acct_invalid(blk_get_stats(n->conf.blk), acct);
 return NVME_LBA_RANGE | NVME_DNR;
 }
+if (!n->id_ctrl.sgls && NVME_CMD_FLAGS_PSDT(rw->flags)) {
+block_acct_invalid(blk_get_stats(n->conf.blk), acct);
+return NVME_INVALID_FIELD | NVME_DNR;
+}
 
 if (nvme_map_prp(>qsg, prp1, prp2, data_size, n)) {
 block_acct_invalid(blk_get_stats(n->conf.blk), acct);
@@ -618,6 +622,11 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd 
*cmd, NvmeRequest *req)
 
 static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
 {
+/* Admin cmd for NVMe-over-PCIE should NOT use SGL */
+if (NVME_CMD_FLAGS_PSDT(cmd->flags)) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
 switch (cmd->opcode) {
 case NVME_ADM_CMD_DELETE_SQ:
 return nvme_del_sq(n, cmd);
@@ -967,6 +976,8 @@ static int nvme_init(PCIDevice *pci_dev)
 if (blk_enable_write_cache(n->conf.blk)) {
 id->vwc = 1;
 }
+/* TODO: Support SGL in NVMe-over-PCIE in both qemu and kernel */
+id->sgls = 0;
 
 n->bar.cap = 0;
 NVME_CAP_SET_MQES(n->bar.cap, 0x7ff);
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 26be663d2d..caf21c5f94 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -229,9 +229,14 @@ typedef union NvmeDataPtr{
 NvmeKeyedSGLDescksgl;
 } NvmeDataPtr;
 
+#define _NVME_CMD_FLAGS_FUSE_MASK((1 << 0)  + (1 << 1))
+#define _NVME_CMD_FLAGS_PSDT_MASK((1 << 15) + (1 << 14))
+#define NVME_CMD_FLAGS_FUSE(flags)  ((flags) & _NVME_CMD_FLAGS_FUSE_MASK)
+#define NVME_CMD_FLAGS_PSDT(flags)  ((flags) & _NVME_CMD_FLAGS_PSDT_MASK)
+
 typedef struct NvmeCmd {
 uint8_t opcode;
-uint8_t fuse;
+uint8_t flags;
 uint16_tcid;
 uint32_tnsid;
 uint64_tres1;
-- 
2.13.0

[Qemu-block] [PATCH 1/2] nvme: Update header to include SGL related macros and members

[Qu Wenruo]

Update nvme header to catch up with kernel.
Most of the newly added members are from 1.2 and 1.3 spec, while the
status code is only kept the same with kernel (around 1.1 spec).

The major update is to add Scatter Gather List related macros and
members, for later SGL support implementation.

Signed-off-by: Qu Wenruo 
---
 hw/block/nvme.c |  16 
 hw/block/nvme.h | 122 +++-
 2 files changed, 111 insertions(+), 27 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 381dc7c5fb..fa1069160e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -270,8 +270,8 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, 
NvmeCmd *cmd,
 NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
 uint32_t nlb  = le32_to_cpu(rw->nlb) + 1;
 uint64_t slba = le64_to_cpu(rw->slba);
-uint64_t prp1 = le64_to_cpu(rw->prp1);
-uint64_t prp2 = le64_to_cpu(rw->prp2);
+uint64_t prp1 = le64_to_cpu(rw->dptr.prp1);
+uint64_t prp2 = le64_to_cpu(rw->dptr.prp2);
 
 uint8_t lba_index  = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
 uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
@@ -512,8 +512,8 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
 
 static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeIdentify *c)
 {
-uint64_t prp1 = le64_to_cpu(c->prp1);
-uint64_t prp2 = le64_to_cpu(c->prp2);
+uint64_t prp1 = le64_to_cpu(c->dptr.prp1);
+uint64_t prp2 = le64_to_cpu(c->dptr.prp2);
 
 return nvme_dma_read_prp(n, (uint8_t *)>id_ctrl, sizeof(n->id_ctrl),
 prp1, prp2);
@@ -523,8 +523,8 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeIdentify 
*c)
 {
 NvmeNamespace *ns;
 uint32_t nsid = le32_to_cpu(c->nsid);
-uint64_t prp1 = le64_to_cpu(c->prp1);
-uint64_t prp2 = le64_to_cpu(c->prp2);
+uint64_t prp1 = le64_to_cpu(c->dptr.prp1);
+uint64_t prp2 = le64_to_cpu(c->dptr.prp2);
 
 if (nsid == 0 || nsid > n->num_namespaces) {
 return NVME_INVALID_NSID | NVME_DNR;
@@ -539,8 +539,8 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, 
NvmeIdentify *c)
 {
 static const int data_len = 4096;
 uint32_t min_nsid = le32_to_cpu(c->nsid);
-uint64_t prp1 = le64_to_cpu(c->prp1);
-uint64_t prp2 = le64_to_cpu(c->prp2);
+uint64_t prp1 = le64_to_cpu(c->dptr.prp1);
+uint64_t prp2 = le64_to_cpu(c->dptr.prp2);
 uint32_t *list;
 uint16_t ret;
 int i, j = 0;
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index b4961d2547..26be663d2d 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -206,6 +206,29 @@ enum NvmeCmbszMask {
 #define NVME_CMBSZ_GETSIZE(cmbsz) \
 (NVME_CMBSZ_SZ(cmbsz) * (1 << (12 + 4 * NVME_CMBSZ_SZU(cmbsz
 
+typedef struct NvmeSGLDesc {
+uint64_taddr;
+uint32_tlength;
+uint8_t rsvd[3];
+uint8_t type;
+} NvmeSGLDesc;
+
+typedef struct NvmeKeyedSGLDesc {
+uint64_taddr;
+uint8_t length[3];
+uint8_t key[4];
+uint8_t type;
+} NvmeKeyedSGLDesc;
+
+typedef union NvmeDataPtr{
+struct {
+uint64_tprp1;
+uint64_tprp2;
+};
+NvmeSGLDesc sgl;
+NvmeKeyedSGLDescksgl;
+} NvmeDataPtr;
+
 typedef struct NvmeCmd {
 uint8_t opcode;
 uint8_t fuse;
@@ -213,8 +236,7 @@ typedef struct NvmeCmd {
 uint32_tnsid;
 uint64_tres1;
 uint64_tmptr;
-uint64_tprp1;
-uint64_tprp2;
+NvmeDataPtr dptr;
 uint32_tcdw10;
 uint32_tcdw11;
 uint32_tcdw12;
@@ -309,9 +331,10 @@ typedef struct NvmeIdentify {
 uint16_tcid;
 uint32_tnsid;
 uint64_trsvd2[2];
-uint64_tprp1;
-uint64_tprp2;
-uint32_tcns;
+NvmeDataPtr dptr;
+uint8_t cns;
+uint8_t rsvd3;
+uint16_tctrlid;
 uint32_trsvd11[5];
 } NvmeIdentify;
 
@@ -322,8 +345,7 @@ typedef struct NvmeRwCmd {
 uint32_tnsid;
 uint64_trsvd2;
 uint64_tmptr;
-uint64_tprp1;
-uint64_tprp2;
+NvmeDataPtr dptr;
 uint64_tslba;
 uint16_tnlb;
 uint16_tcontrol;
@@ -363,8 +385,7 @@ typedef struct NvmeDsmCmd {
 uint16_tcid;
 uint32_tnsid;
 uint64_trsvd2[2];
-uint64_tprp1;
-uint64_tprp2;
+NvmeDataPtr dptr;
 uint32_tnr;
 uint32_tattributes;
 uint32_trsvd12[4];
@@ -428,6 +449,13 @@ enum NvmeStatusCodes {
 NVME_CMD_ABORT_MISSING_FUSE = 0x000a,
 NVME_INVALID_NSID   = 0x000b,
 NVME_CMD_SEQ_ERROR  = 0x000c,
+NVME_SGL_INVALID_LAST   = 0x000d,
+NVME_SGL_INVALID_COUNT  = 0x000e,
+NVME_SGL_INVALID_DATA   = 0x000f,
+NVME_SGL_INVALID_METADATA   = 0x0010,
+NVME_SGL_INVALID_TYPE   = 0x0011,
+NVME_SGL_INVALID_OFFSET = 0x0016,
+NVME_SGL_INVALID_SUBTYPE= 0x0017,
 NVME_LBA_RANGE  = 0x0080,
 NVME_CAP_EXCEEDED   = 0x0081,
 NVME_NS_NOT_READY   

[Qemu-block] [PATCH] qemu-img: supplement the omitted 'disk size' bytes info

[sochin.jiang]

From: "sochin.jiang" 

 Supplement the omitted 'disk size' bytes information when using

 'qemu-img info',it is useful in some occasion. Anyhow, it looks

 strange that 'virtual size' has bytes while actual 'disk size'

 does not.

Signed-off-by: sochin.jiang 
---
 block/qapi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a40922e..9bb5956 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -703,10 +703,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, 
void *f,
  "image: %s\n"
  "file format: %s\n"
  "virtual size: %s (%" PRId64 " bytes)\n"
- "disk size: %s\n",
+ "disk size: %s (%" PRId64 " bytes)\n",
  info->filename, info->format, size_buf,
  info->virtual_size,
- dsize_buf);
+ dsize_buf,
+ info->actual_size);
 
 if (info->has_encrypted && info->encrypted) {
 func_fprintf(f, "encrypted: yes\n");
-- 
1.8.3.1