Re: [PATCH v2 1/2] qemu-img: Add --target-is-zero to convert

[David Edmondson]

Eric Blake  writes:

> On 1/24/20 4:34 AM, David Edmondson wrote:
>> In many cases the target of a convert operation is a newly provisioned
>> target that the user knows is blank (filled with zeroes). In this
>> situation there is no requirement for qemu-img to wastefully zero out
>> the entire device.
>> 
>> Add a new option, --target-is-zero, allowing the user to indicate that
>> an existing target device is already zero filled.
>> 
>> Signed-off-by: David Edmondson 
>> ---
>>   qemu-img-cmds.hx |  4 ++--
>>   qemu-img.c   | 25 ++---
>>   qemu-img.texi|  4 
>>   3 files changed, 28 insertions(+), 5 deletions(-)
>
> I'm working up a patch series that tries to auto-set this flag without 
> user interaction where possible (for example, if lseek(fd, 0, SEEK_DATA) 
> returns EOF, or if fstat() reports 0 blocks allocated, or if qcow2 sees 
> no L2 tables allocated, or a proposed extension to NBD passes on the 
> same...).  I may rebase my series on top of your patch and tweak things 
> in yours accordingly.
>
> But as it stands, the idea makes sense to me; even if we add ways for 
> some images to efficiently report initial state (and our existing 
> bdrv_has_zero_init() is NOT such a method), there are enough other 
> scenarios where the knob will be the only way to let qemu-img know the 
> intent.

Having qemu-img figure things out on its own is obviously desirable, but
I agree that there are enough cases where this won't be possible and,
given the resulting performance improvement, it will still be useful to
allow the caller to force things.

>> +case OPTION_TARGET_IS_ZERO:
>> +/*
>> + * The user asserting that the target is blank has the
>> + * same effect as the target driver supporting zero
>> + * initialisation.
>
> Hmm. A git grep shows that 'initialization' has 200 hits, 
> 'initialisation' has only 29. But I think it's a US vs. UK thing, so I 
> don't care which spelling you use.

Yes, it's British English spelling. It was unconscious - I'll switch if
there is an existing policy.

> Reviewed-by: Eric Blake 

Thanks.

If the conversion of the documentation to rST is imminent then I'll wait
for that before submitting a followup with corresponding changes applied
to the new docs.

dme.
-- 
I'd come on over but I haven't got a raincoat.

Re: [PATCH v2 2/2] doc: Use @code rather than @var for options

[David Edmondson]

Eric Blake  writes:

> On 1/24/20 4:34 AM, David Edmondson wrote:
>> Texinfo defines @var for metasyntactic variables and such terms are
>> shown in upper-case or italics in the output of makeinfo. When
>> considering an option to a command, such as "-n", upper-casing is
>> undesirable as it may confuse the reader or be in conflict with the
>> equivalent upper-case option.
>> 
>> Replace the use of @var for options with @code to avoid this.
>> 
>> Signed-off-by: David Edmondson 
>> ---
>>   qemu-img.texi | 16 
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> Is this patch still needed given Peter's recent push to move to rST 
> documentation?

No, it would be obviated by those changes.

>> 
>> diff --git a/qemu-img.texi b/qemu-img.texi
>> index 3b6dfd8682..6b4a1ac961 100644
>> --- a/qemu-img.texi
>> +++ b/qemu-img.texi
>> @@ -74,13 +74,13 @@ keys.
>>   @item --image-opts
>>   Indicates that the source @var{filename} parameter is to be interpreted as 
>> a
>>   full option string, not a plain filename. This parameter is mutually
>> -exclusive with the @var{-f} parameter.
>> +exclusive with the @code{-f} parameter.
>>   
>
>
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org

dme.
-- 
Don't you know you're never going to get to France.

[PULL 2/2] tests/ide-test: Create a single unit-test covering more PRDT cases

[John Snow]

From: Alexander Popov 

Fuzzing the Linux kernel with syzkaller allowed to find how to crash qemu
using a special SCSI_IOCTL_SEND_COMMAND. It hits the assertion in
ide_dma_cb() introduced in the commit a718978ed58a in July 2015.
Currently this bug is not reproduced by the unit tests.

Let's improve the ide-test to cover more PRDT cases including one
that causes this particular qemu crash.

The test is developed according to the Programming Interface for
Bus Master IDE Controller (Revision 1.0 5/16/94).

Signed-off-by: Alexander Popov 
Message-id: 20191223175117.508990-3-alex.po...@linux.com
Signed-off-by: John Snow 
---
 tests/qtest/ide-test.c | 176 ++---
 1 file changed, 75 insertions(+), 101 deletions(-)

diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 0277e7d5a9..5cfd97f915 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -445,104 +445,81 @@ static void test_bmdma_trim(void)
 test_bmdma_teardown(qts);
 }
 
-static void test_bmdma_short_prdt(void)
+/*
+ * This test is developed according to the Programming Interface for
+ * Bus Master IDE Controller (Revision 1.0 5/16/94)
+ */
+static void test_bmdma_various_prdts(void)
 {
-QTestState *qts;
-QPCIDevice *dev;
-QPCIBar bmdma_bar, ide_bar;
-uint8_t status;
-
-PrdtEntry prdt[] = {
-{
-.addr = 0,
-.size = cpu_to_le32(0x10 | PRDT_EOT),
-},
-};
-
-qts = test_bmdma_setup();
-
-dev = get_pci_device(qts, _bar, _bar);
-
-/* Normal request */
-status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
-  prdt, ARRAY_SIZE(prdt), NULL);
-g_assert_cmphex(status, ==, 0);
-assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-
-/* Abort the request before it completes */
-status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 1,
-  prdt, ARRAY_SIZE(prdt), NULL);
-g_assert_cmphex(status, ==, 0);
-assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-free_pci_device(dev);
-test_bmdma_teardown(qts);
-}
-
-static void test_bmdma_one_sector_short_prdt(void)
-{
-QTestState *qts;
-QPCIDevice *dev;
-QPCIBar bmdma_bar, ide_bar;
-uint8_t status;
-
-/* Read 2 sectors but only give 1 sector in PRDT */
-PrdtEntry prdt[] = {
-{
-.addr = 0,
-.size = cpu_to_le32(0x200 | PRDT_EOT),
-},
-};
-
-qts = test_bmdma_setup();
-
-dev = get_pci_device(qts, _bar, _bar);
-
-/* Normal request */
-status = send_dma_request(qts, CMD_READ_DMA, 0, 2,
-  prdt, ARRAY_SIZE(prdt), NULL);
-g_assert_cmphex(status, ==, 0);
-assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-
-/* Abort the request before it completes */
-status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 2,
-  prdt, ARRAY_SIZE(prdt), NULL);
-g_assert_cmphex(status, ==, 0);
-assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-free_pci_device(dev);
-test_bmdma_teardown(qts);
-}
-
-static void test_bmdma_long_prdt(void)
-{
-QTestState *qts;
-QPCIDevice *dev;
-QPCIBar bmdma_bar, ide_bar;
-uint8_t status;
-
-PrdtEntry prdt[] = {
-{
-.addr = 0,
-.size = cpu_to_le32(0x1000 | PRDT_EOT),
-},
-};
-
-qts = test_bmdma_setup();
-
-dev = get_pci_device(qts, _bar, _bar);
-
-/* Normal request */
-status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
-  prdt, ARRAY_SIZE(prdt), NULL);
-g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
-assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-
-/* Abort the request before it completes */
-status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 1,
-  prdt, ARRAY_SIZE(prdt), NULL);
-g_assert_cmphex(status, ==, BM_STS_INTR);
-assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-free_pci_device(dev);
-test_bmdma_teardown(qts);
+int sectors = 0;
+uint32_t size = 0;
+
+for (sectors = 1; sectors <= 256; sectors *= 2) {
+QTestState *qts = NULL;
+QPCIDevice *dev = NULL;
+QPCIBar bmdma_bar, ide_bar;
+
+qts = test_bmdma_setup();
+dev = get_pci_device(qts, _bar, _bar);
+
+for (size = 0; size < 65536; size += 256) {
+uint32_t req_size = sectors * 512;
+uint32_t prd_size = size & 0xfffe; /* bit 0 is always set to 0 */
+uint8_t ret = 0;
+uint8_t req_status = 0;
+uint8_t abort_req_status = 0;
+PrdtEntry prdt[] = {
+{
+.addr = 0,
+.size = cpu_to_le32(size | PRDT_EOT),
+},
+};
+
+/* A value of zero in PRD size 

[PULL 1/2] ide: Fix incorrect handling of some PRDTs in ide_dma_cb()

[John Snow]

From: Alexander Popov 

The commit a718978ed58a from July 2015 introduced the assertion which
implies that the size of successful DMA transfers handled in ide_dma_cb()
should be multiple of 512 (the size of a sector). But guest systems can
initiate DMA transfers that don't fit this requirement.

For fixing that let's check the number of bytes prepared for the transfer
by the prepare_buf() handler. The code in ide_dma_cb() must behave
according to the Programming Interface for Bus Master IDE Controller
(Revision 1.0 5/16/94):
1. If PRDs specified a smaller size than the IDE transfer
   size, then the Interrupt and Active bits in the Controller
   status register are not set (Error Condition).
2. If the size of the physical memory regions was equal to
   the IDE device transfer size, the Interrupt bit in the
   Controller status register is set to 1, Active bit is set to 0.
3. If PRDs specified a larger size than the IDE transfer size,
   the Interrupt and Active bits in the Controller status register
   are both set to 1.

Signed-off-by: Alexander Popov 
Reviewed-by: Kevin Wolf 
Message-id: 20191223175117.508990-2-alex.po...@linux.com
Signed-off-by: John Snow 
---
 hw/ide/core.c | 30 ++
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 754ff4dc34..8eb766 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -849,6 +849,7 @@ static void ide_dma_cb(void *opaque, int ret)
 int64_t sector_num;
 uint64_t offset;
 bool stay_active = false;
+int32_t prep_size = 0;
 
 if (ret == -EINVAL) {
 ide_dma_error(s);
@@ -863,13 +864,15 @@ static void ide_dma_cb(void *opaque, int ret)
 }
 }
 
-n = s->io_buffer_size >> 9;
-if (n > s->nsector) {
-/* The PRDs were longer than needed for this request. Shorten them so
- * we don't get a negative remainder. The Active bit must remain set
- * after the request completes. */
+if (s->io_buffer_size > s->nsector * 512) {
+/*
+ * The PRDs were longer than needed for this request.
+ * The Active bit must remain set after the request completes.
+ */
 n = s->nsector;
 stay_active = true;
+} else {
+n = s->io_buffer_size >> 9;
 }
 
 sector_num = ide_get_sector(s);
@@ -892,9 +895,20 @@ static void ide_dma_cb(void *opaque, int ret)
 n = s->nsector;
 s->io_buffer_index = 0;
 s->io_buffer_size = n * 512;
-if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) {
-/* The PRDs were too short. Reset the Active bit, but don't raise an
- * interrupt. */
+prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size);
+/* prepare_buf() must succeed and respect the limit */
+assert(prep_size >= 0 && prep_size <= n * 512);
+
+/*
+ * Now prep_size stores the number of bytes in the sglist, and
+ * s->io_buffer_size stores the number of bytes described by the PRDs.
+ */
+
+if (prep_size < n * 512) {
+/*
+ * The PRDs are too short for this request. Error condition!
+ * Reset the Active bit and don't raise the interrupt.
+ */
 s->status = READY_STAT | SEEK_STAT;
 dma_buf_commit(s, 0);
 goto eot;
-- 
2.21.0

[PULL 0/2] Ide patches

[John Snow]

The following changes since commit 105b07f1ba462ec48b27e5cb74ddf81c6a79364c:

  Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20200127' into 
staging (2020-01-27 13:02:36 +)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 59805ae92dfe4f67105e36b539d567caec4f8304:

  tests/ide-test: Create a single unit-test covering more PRDT cases 
(2020-01-27 17:07:31 -0500)


Pull request



Alexander Popov (2):
  ide: Fix incorrect handling of some PRDTs in ide_dma_cb()
  tests/ide-test: Create a single unit-test covering more PRDT cases

 hw/ide/core.c  |  30 +--
 tests/qtest/ide-test.c | 176 ++---
 2 files changed, 97 insertions(+), 109 deletions(-)

-- 
2.21.0

Re: [PATCH v2 1/2] qemu-img: Add --target-is-zero to convert

[Eric Blake]

On 1/24/20 4:34 AM, David Edmondson wrote:

In many cases the target of a convert operation is a newly provisioned
target that the user knows is blank (filled with zeroes). In this
situation there is no requirement for qemu-img to wastefully zero out
the entire device.

Add a new option, --target-is-zero, allowing the user to indicate that
an existing target device is already zero filled.

Signed-off-by: David Edmondson 
---
  qemu-img-cmds.hx |  4 ++--
  qemu-img.c   | 25 ++---
  qemu-img.texi|  4 
  3 files changed, 28 insertions(+), 5 deletions(-)


I'm working up a patch series that tries to auto-set this flag without 
user interaction where possible (for example, if lseek(fd, 0, SEEK_DATA) 
returns EOF, or if fstat() reports 0 blocks allocated, or if qcow2 sees 
no L2 tables allocated, or a proposed extension to NBD passes on the 
same...).  I may rebase my series on top of your patch and tweak things 
in yours accordingly.


But as it stands, the idea makes sense to me; even if we add ways for 
some images to efficiently report initial state (and our existing 
bdrv_has_zero_init() is NOT such a method), there are enough other 
scenarios where the knob will be the only way to let qemu-img know the 
intent.




+case OPTION_TARGET_IS_ZERO:
+/*
+ * The user asserting that the target is blank has the
+ * same effect as the target driver supporting zero
+ * initialisation.


Hmm. A git grep shows that 'initialization' has 200 hits, 
'initialisation' has only 29. But I think it's a US vs. UK thing, so I 
don't care which spelling you use.




@@ -2247,6 +2256,11 @@ static int img_convert(int argc, char **argv)
  warn_report("This will become an error in future QEMU versions.");
  }
  
+if (s.has_zero_init && !skip_create) {

+error_report("--target-is-zero requires use of -n flag");
+goto fail_getopt;
+}
+


Makes sense, although we could perhaps relax it to also work even when 
the -n flag is supplied IF the destination image supports my proposal 
for a new status bit set when an image is known to be opened with all 
zero content.



  s.src_num = argc - optind - 1;
  out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
  
@@ -2380,6 +2394,11 @@ static int img_convert(int argc, char **argv)

  }
  s.target_has_backing = (bool) out_baseimg;
  
+if (s.has_zero_init && s.target_has_backing) {

+error_report("Cannot use --target-is-zero with a backing file");
+goto out;
+}
+


Makes sense, although we could perhaps relax it to also work even when 
there is a backing file IF the backing file supports my proposal for a 
new status bit set when an image is known to be opened with all zero 
content.


As my patch proposal is still not submitted, I'm fine if yours lands as-is:

Reviewed-by: Eric Blake 

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

Re: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

[John Snow]

On 1/27/20 3:43 PM, Peter Krempa wrote:
> On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
>>
>>
>> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
>>> This patch series is bunch of cleanups
>>> to the hmp monitor code.
>>>
>>> This series only touched blockdev related hmp handlers.
>>>
>>> No functional changes expected other that
>>> light error message changes by the last patch.
>>>
>>> This was inspired by this bugzilla:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1719169
>>>
>>> Basically some users still parse hmp error messages,
>>> and they would like to have them prefixed with 'Error:'
>>>
>>
>> HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
>> like consistency in my UIs (it's useful for human eyes, too), but I'd
>> like to know more about the request.
> 
> That's true as long as there's an stable replacement ... see below.
> 

Thanks for the context!

>>
>> Is this request coming from libvirt? Can we wean them off of this
>> interface? What do they need as a replacement?
> 
> There are 5 commands that libvirt still has HMP interfaces for:
> 
> drive_add
> drive_del
> 
> savevm
> loadvm
> delvm
> 
> From upstream point of view there's no value in adding the 'error'
> prefix to drive_add/drive_del as libvirt now uses blockdev-add/del QMP
> command instead which have implicit error propagation.
> 

As thought.

> There are no replacements for the internal snapshot commands, but they
> reported the 'error' prefix for some time even before this series.
> 
> Said that, please don't break savevm/loadvm/delvm until a QMP
> replacement is added.
> 

Yes, noted. I wonder where userfaultfd write support is these days...

> The bug was reported at the time when libvirt didn't use blockdev yet,
> but at this point it's pointless from our side. This wouldn't even fix
> the scenario when old (pre-5.10) libvirt would use new qemu because the
> drive-add handler never checked the error prefix.
> 
> [1] 
> https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD
> 

Thank you for the report from libvirtville :)

--js

Re: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

[Peter Krempa]

On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
> 
> 
> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
> > This patch series is bunch of cleanups
> > to the hmp monitor code.
> > 
> > This series only touched blockdev related hmp handlers.
> > 
> > No functional changes expected other that
> > light error message changes by the last patch.
> > 
> > This was inspired by this bugzilla:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1719169
> > 
> > Basically some users still parse hmp error messages,
> > and they would like to have them prefixed with 'Error:'
> > 
> 
> HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
> like consistency in my UIs (it's useful for human eyes, too), but I'd
> like to know more about the request.

That's true as long as there's an stable replacement ... see below.

> 
> Is this request coming from libvirt? Can we wean them off of this
> interface? What do they need as a replacement?

There are 5 commands that libvirt still has HMP interfaces for:

drive_add
drive_del

savevm
loadvm
delvm

>From upstream point of view there's no value in adding the 'error'
prefix to drive_add/drive_del as libvirt now uses blockdev-add/del QMP
command instead which have implicit error propagation.

There are no replacements for the internal snapshot commands, but they
reported the 'error' prefix for some time even before this series.

Said that, please don't break savevm/loadvm/delvm until a QMP
replacement is added.

The bug was reported at the time when libvirt didn't use blockdev yet,
but at this point it's pointless from our side. This wouldn't even fix
the scenario when old (pre-5.10) libvirt would use new qemu because the
drive-add handler never checked the error prefix.

[1] 
https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD

Re: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

[John Snow]

On 1/27/20 5:36 AM, Maxim Levitsky wrote:
> This patch series is bunch of cleanups
> to the hmp monitor code.
> 
> This series only touched blockdev related hmp handlers.
> 
> No functional changes expected other that
> light error message changes by the last patch.
> 
> This was inspired by this bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=1719169
> 
> Basically some users still parse hmp error messages,
> and they would like to have them prefixed with 'Error:'
> 

HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
like consistency in my UIs (it's useful for human eyes, too), but I'd
like to know more about the request.

Is this request coming from libvirt? Can we wean them off of this
interface? What do they need as a replacement?

(Is blockdev not enough?)

--js

Re: [PATCH v2 2/2] doc: Use @code rather than @var for options

[Eric Blake]

On 1/24/20 4:34 AM, David Edmondson wrote:

Texinfo defines @var for metasyntactic variables and such terms are
shown in upper-case or italics in the output of makeinfo. When
considering an option to a command, such as "-n", upper-casing is
undesirable as it may confuse the reader or be in conflict with the
equivalent upper-case option.

Replace the use of @var for options with @code to avoid this.

Signed-off-by: David Edmondson 
---
  qemu-img.texi | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)


Is this patch still needed given Peter's recent push to move to rST 
documentation?




diff --git a/qemu-img.texi b/qemu-img.texi
index 3b6dfd8682..6b4a1ac961 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -74,13 +74,13 @@ keys.
  @item --image-opts
  Indicates that the source @var{filename} parameter is to be interpreted as a
  full option string, not a plain filename. This parameter is mutually
-exclusive with the @var{-f} parameter.
+exclusive with the @code{-f} parameter.
  



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

[PULL 09/13] blockdev: Return bs to the proper context on snapshot abort

[Kevin Wolf]

From: Sergio Lopez 

external_snapshot_abort() calls to bdrv_set_backing_hd(), which
returns state->old_bs to the main AioContext, as it's intended to be
used then the BDS is going to be released. As that's not the case when
aborting an external snapshot, return it to the AioContext it was
before the call.

This issue can be triggered by issuing a transaction with two actions,
a proper blockdev-snapshot-sync and a bogus one, so the second will
trigger a transaction abort. This results in a crash with an stack
trace like this one:

 #0  0x7fa1048b28df in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:50
 #1  0x7fa10489ccf5 in __GI_abort () at abort.c:79
 #2  0x7fa10489cbc9 in __assert_fail_base
 (fmt=0x7fa104a03300 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
assertion=0x5572240b44d8 "bdrv_get_aio_context(old_bs) == 
bdrv_get_aio_context(new_bs)", file=0x557224014d30 "block.c", line=2240, 
function=) at assert.c:92
 #3  0x7fa1048aae96 in __GI___assert_fail
 (assertion=assertion@entry=0x5572240b44d8 "bdrv_get_aio_context(old_bs) == 
bdrv_get_aio_context(new_bs)", file=file@entry=0x557224014d30 "block.c", 
line=line@entry=2240, function=function@entry=0x5572240b5d60 
<__PRETTY_FUNCTION__.31620> "bdrv_replace_child_noperm") at assert.c:101
 #4  0x557223e631f8 in bdrv_replace_child_noperm (child=0x557225b9c980, 
new_bs=new_bs@entry=0x557225c42e40) at block.c:2240
 #5  0x557223e68be7 in bdrv_replace_node (from=0x557226951a60, 
to=0x557225c42e40, errp=0x5572247d6138 ) at block.c:4196
 #6  0x557223d069c4 in external_snapshot_abort (common=0x557225d7e170) at 
blockdev.c:1731
 #7  0x557223d069c4 in external_snapshot_abort (common=0x557225d7e170) at 
blockdev.c:1717
 #8  0x557223d09013 in qmp_transaction (dev_list=, 
has_props=, props=0x557225cc7d70, 
errp=errp@entry=0x7ffe704c0c98) at blockdev.c:2360
 #9  0x557223e32085 in qmp_marshal_transaction (args=, 
ret=, errp=0x7ffe704c0d08) at qapi/qapi-commands-transaction.c:44
 #10 0x557223ee798c in do_qmp_dispatch (errp=0x7ffe704c0d00, 
allow_oob=, request=, cmds=0x5572247d3cc0 
) at qapi/qmp-dispatch.c:132
 #11 0x557223ee798c in qmp_dispatch (cmds=0x5572247d3cc0 , 
request=, allow_oob=) at qapi/qmp-dispatch.c:175
 #12 0x557223e06141 in monitor_qmp_dispatch (mon=0x557225c69ff0, 
req=) at monitor/qmp.c:120
 #13 0x557223e0678a in monitor_qmp_bh_dispatcher (data=) at 
monitor/qmp.c:209
 #14 0x557223f2f366 in aio_bh_call (bh=0x557225b9dc60) at util/async.c:117
 #15 0x557223f2f366 in aio_bh_poll (ctx=ctx@entry=0x557225b9c840) at 
util/async.c:117
 #16 0x557223f32754 in aio_dispatch (ctx=0x557225b9c840) at 
util/aio-posix.c:459
 #17 0x557223f2f242 in aio_ctx_dispatch (source=, 
callback=, user_data=) at util/async.c:260
 #18 0x7fa10913467d in g_main_dispatch (context=0x557225c28e80) at 
gmain.c:3176
 #19 0x7fa10913467d in g_main_context_dispatch 
(context=context@entry=0x557225c28e80) at gmain.c:3829
 #20 0x557223f31808 in glib_pollfds_poll () at util/main-loop.c:219
 #21 0x557223f31808 in os_host_main_loop_wait (timeout=) at 
util/main-loop.c:242
 #22 0x557223f31808 in main_loop_wait (nonblocking=) at 
util/main-loop.c:518
 #23 0x557223d13201 in main_loop () at vl.c:1828
 #24 0x557223bbfb82 in main (argc=, argv=, 
envp=) at vl.c:4504

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1779036
Signed-off-by: Sergio Lopez 
Signed-off-by: Kevin Wolf 
---
 blockdev.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index d4ef6cd452..4cd9a58d36 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1731,6 +1731,8 @@ static void external_snapshot_abort(BlkActionState 
*common)
 if (state->new_bs) {
 if (state->overlay_appended) {
 AioContext *aio_context;
+AioContext *tmp_context;
+int ret;
 
 aio_context = bdrv_get_aio_context(state->old_bs);
 aio_context_acquire(aio_context);
@@ -1738,6 +1740,25 @@ static void external_snapshot_abort(BlkActionState 
*common)
 bdrv_ref(state->old_bs);   /* we can't let bdrv_set_backind_hd()
   close state->old_bs; we need it */
 bdrv_set_backing_hd(state->new_bs, NULL, _abort);
+
+/*
+ * The call to bdrv_set_backing_hd() above returns state->old_bs to
+ * the main AioContext. As we're still going to be using it, return
+ * it to the AioContext it was before.
+ */
+tmp_context = bdrv_get_aio_context(state->old_bs);
+if (aio_context != tmp_context) {
+aio_context_release(aio_context);
+aio_context_acquire(tmp_context);
+
+ret = bdrv_try_set_aio_context(state->old_bs,
+   aio_context, NULL);
+assert(ret == 0);
+
+aio_context_release(tmp_context);

[PULL 06/13] blockdev: honor bdrv_try_set_aio_context() context requirements

[Kevin Wolf]

From: Sergio Lopez 

bdrv_try_set_aio_context() requires that the old context is held, and
the new context is not held. Fix all the occurrences where it's not
done this way.

Suggested-by: Max Reitz 
Signed-off-by: Sergio Lopez 
Signed-off-by: Kevin Wolf 
---
 blockdev.c | 68 +++---
 1 file changed, 60 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 152a0f7454..1dacbc20ec 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1535,6 +1535,7 @@ static void external_snapshot_prepare(BlkActionState 
*common,
  DO_UPCAST(ExternalSnapshotState, common, common);
 TransactionAction *action = common->action;
 AioContext *aio_context;
+AioContext *old_context;
 int ret;
 
 /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
@@ -1675,7 +1676,16 @@ static void external_snapshot_prepare(BlkActionState 
*common,
 goto out;
 }
 
+/* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+old_context = bdrv_get_aio_context(state->new_bs);
+aio_context_release(aio_context);
+aio_context_acquire(old_context);
+
 ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp);
+
+aio_context_release(old_context);
+aio_context_acquire(aio_context);
+
 if (ret < 0) {
 goto out;
 }
@@ -1775,11 +1785,13 @@ static void drive_backup_prepare(BlkActionState 
*common, Error **errp)
 BlockDriverState *target_bs;
 BlockDriverState *source = NULL;
 AioContext *aio_context;
+AioContext *old_context;
 QDict *options;
 Error *local_err = NULL;
 int flags;
 int64_t size;
 bool set_backing_hd = false;
+int ret;
 
 assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
 backup = common->action->u.drive_backup.data;
@@ -1868,6 +1880,21 @@ static void drive_backup_prepare(BlkActionState *common, 
Error **errp)
 goto out;
 }
 
+/* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+old_context = bdrv_get_aio_context(target_bs);
+aio_context_release(aio_context);
+aio_context_acquire(old_context);
+
+ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+if (ret < 0) {
+bdrv_unref(target_bs);
+aio_context_release(old_context);
+return;
+}
+
+aio_context_release(old_context);
+aio_context_acquire(aio_context);
+
 if (set_backing_hd) {
 bdrv_set_backing_hd(target_bs, source, _err);
 if (local_err) {
@@ -1947,6 +1974,8 @@ static void blockdev_backup_prepare(BlkActionState 
*common, Error **errp)
 BlockDriverState *bs;
 BlockDriverState *target_bs;
 AioContext *aio_context;
+AioContext *old_context;
+int ret;
 
 assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
 backup = common->action->u.blockdev_backup.data;
@@ -1961,7 +1990,18 @@ static void blockdev_backup_prepare(BlkActionState 
*common, Error **errp)
 return;
 }
 
+/* Honor bdrv_try_set_aio_context() context acquisition requirements. */
 aio_context = bdrv_get_aio_context(bs);
+old_context = bdrv_get_aio_context(target_bs);
+aio_context_acquire(old_context);
+
+ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+if (ret < 0) {
+aio_context_release(old_context);
+return;
+}
+
+aio_context_release(old_context);
 aio_context_acquire(aio_context);
 state->bs = bs;
 
@@ -3562,7 +3602,6 @@ static BlockJob *do_backup_common(BackupCommon *backup,
 BlockJob *job = NULL;
 BdrvDirtyBitmap *bmap = NULL;
 int job_flags = JOB_DEFAULT;
-int ret;
 
 if (!backup->has_speed) {
 backup->speed = 0;
@@ -3586,11 +3625,6 @@ static BlockJob *do_backup_common(BackupCommon *backup,
 backup->compress = false;
 }
 
-ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
-if (ret < 0) {
-return NULL;
-}
-
 if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) ||
 (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL)) {
 /* done before desugaring 'incremental' to print the right message */
@@ -3825,6 +3859,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
 BlockDriverState *bs;
 BlockDriverState *source, *target_bs;
 AioContext *aio_context;
+AioContext *old_context;
 BlockMirrorBackingMode backing_mode;
 Error *local_err = NULL;
 QDict *options = NULL;
@@ -3937,12 +3972,22 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
(arg->mode == NEW_IMAGE_MODE_EXISTING ||
 !bdrv_has_zero_init(target_bs)));
 
+
+/* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+old_context = bdrv_get_aio_context(target_bs);
+aio_context_release(aio_context);
+aio_context_acquire(old_context);
+
 ret = bdrv_try_set_aio_context(target_bs, aio_context, 

[PULL 12/13] iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)

[Kevin Wolf]

From: Felipe Franciosi 

When querying an iSCSI server for the provisioning status of blocks (via
GET LBA STATUS), Qemu only validates that the response descriptor zero's
LBA matches the one requested. Given the SCSI spec allows servers to
respond with the status of blocks beyond the end of the LUN, Qemu may
have its heap corrupted by clearing/setting too many bits at the end of
its allocmap for the LUN.

A malicious guest in control of the iSCSI server could carefully program
Qemu's heap (by selectively setting the bitmap) and then smash it.

This limits the number of bits that iscsi_co_block_status() will try to
update in the allocmap so it can't overflow the bitmap.

Fixes: CVE-2020-1711
Cc: qemu-sta...@nongnu.org
Signed-off-by: Felipe Franciosi 
Signed-off-by: Peter Turschmid 
Signed-off-by: Raphael Norwitz 
Signed-off-by: Kevin Wolf 
---
 block/iscsi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 2aea7e3f13..cbd57294ab 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -701,7 +701,7 @@ static int coroutine_fn 
iscsi_co_block_status(BlockDriverState *bs,
 struct scsi_get_lba_status *lbas = NULL;
 struct scsi_lba_status_descriptor *lbasd = NULL;
 struct IscsiTask iTask;
-uint64_t lba;
+uint64_t lba, max_bytes;
 int ret;
 
 iscsi_co_init_iscsitask(iscsilun, );
@@ -721,6 +721,7 @@ static int coroutine_fn 
iscsi_co_block_status(BlockDriverState *bs,
 }
 
 lba = offset / iscsilun->block_size;
+max_bytes = (iscsilun->num_blocks - lba) * iscsilun->block_size;
 
 qemu_mutex_lock(>mutex);
 retry:
@@ -764,7 +765,7 @@ retry:
 goto out_unlock;
 }
 
-*pnum = (int64_t) lbasd->num_blocks * iscsilun->block_size;
+*pnum = MIN((int64_t) lbasd->num_blocks * iscsilun->block_size, max_bytes);
 
 if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
 lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
-- 
2.20.1

[PULL 11/13] block/backup: fix memory leak in bdrv_backup_top_append()

[Kevin Wolf]

From: Eiichi Tsukata 

bdrv_open_driver() allocates bs->opaque according to drv->instance_size.
There is no need to allocate it and overwrite opaque in
bdrv_backup_top_append().

Reproducer:

  $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 valgrind -q 
--leak-check=full tests/test-replication -p /replication/secondary/start
  ==29792== 24 bytes in 1 blocks are definitely lost in loss record 52 of 226
  ==29792==at 0x483AB1A: calloc (vg_replace_malloc.c:762)
  ==29792==by 0x4B07CE0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
  ==29792==by 0x12BAB9: bdrv_open_driver (block.c:1289)
  ==29792==by 0x12BEA9: bdrv_new_open_driver (block.c:1359)
  ==29792==by 0x1D15CB: bdrv_backup_top_append (backup-top.c:190)
  ==29792==by 0x1CC11A: backup_job_create (backup.c:439)
  ==29792==by 0x1CD542: replication_start (replication.c:544)
  ==29792==by 0x1401B9: replication_start_all (replication.c:52)
  ==29792==by 0x128B50: test_secondary_start (test-replication.c:427)
  ...

Fixes: 7df7868b9640 ("block: introduce backup-top filter driver")
Signed-off-by: Eiichi Tsukata 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Kevin Wolf 
---
 block/backup-top.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/backup-top.c b/block/backup-top.c
index b8d863ff08..9aed2eb4c0 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -196,7 +196,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState 
*source,
 }
 
 top->total_sectors = source->total_sectors;
-top->opaque = state = g_new0(BDRVBackupTopState, 1);
+state = top->opaque;
 
 bdrv_ref(target);
 state->target = bdrv_attach_child(top, target, "target", _file, 
errp);
-- 
2.20.1

[PULL 07/13] block/backup-top: Don't acquire context while dropping top

[Kevin Wolf]

From: Sergio Lopez 

All paths that lead to bdrv_backup_top_drop(), except for the call
from backup_clean(), imply that the BDS AioContext has already been
acquired, so doing it there too can potentially lead to QEMU hanging
on AIO_WAIT_WHILE().

An easy way to trigger this situation is by issuing a two actions
transaction, with a proper and a bogus blockdev-backup, so the second
one will trigger a rollback. This will trigger a hang with an stack
trace like this one:

 #0  0x7fb680c75016 in __GI_ppoll (fds=0x55e74580f7c0, nfds=1, 
timeout=,
 timeout@entry=0x0, sigmask=sigmask@entry=0x0) at 
../sysdeps/unix/sysv/linux/ppoll.c:39
 #1  0x55e743386e09 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=)
 at /usr/include/bits/poll2.h:77
 #2  0x55e743386e09 in qemu_poll_ns
 (fds=, nfds=, timeout=) at 
util/qemu-timer.c:336
 #3  0x55e743388dc4 in aio_poll (ctx=0x55e7458925d0, 
blocking=blocking@entry=true)
 at util/aio-posix.c:669
 #4  0x55e743305dea in bdrv_flush (bs=bs@entry=0x55e74593c0d0) at 
block/io.c:2878
 #5  0x55e7432be58e in bdrv_close (bs=0x55e74593c0d0) at block.c:4017
 #6  0x55e7432be58e in bdrv_delete (bs=) at block.c:4262
 #7  0x55e7432be58e in bdrv_unref (bs=bs@entry=0x55e74593c0d0) at 
block.c:5644
 #8  0x55e743316b9b in bdrv_backup_top_drop (bs=bs@entry=0x55e74593c0d0) at 
block/backup-top.c:273
 #9  0x55e74331461f in backup_job_create
 (job_id=0x0, bs=bs@entry=0x55e7458d5820, 
target=target@entry=0x55e74589f640, speed=0, sync_mode=MIRROR_SYNC_MODE_FULL, 
sync_bitmap=sync_bitmap@entry=0x0, bitmap_mode=BITMAP_SYNC_MODE_ON_SUCCESS, 
compress=false, filter_node_name=0x0, on_source_error=BLOCKDEV_ON_ERROR_REPORT, 
on_target_error=BLOCKDEV_ON_ERROR_REPORT, creation_flags=0, cb=0x0, opaque=0x0, 
txn=0x0, errp=0x7ffddfd1efb0) at block/backup.c:478
 #10 0x55e74315bc52 in do_backup_common
 (backup=backup@entry=0x55e746c066d0, bs=bs@entry=0x55e7458d5820, 
target_bs=target_bs@entry=0x55e74589f640, 
aio_context=aio_context@entry=0x55e7458a91e0, txn=txn@entry=0x0, 
errp=errp@entry=0x7ffddfd1efb0)
 at blockdev.c:3580
 #11 0x55e74315c37c in do_blockdev_backup
 (backup=backup@entry=0x55e746c066d0, txn=0x0, 
errp=errp@entry=0x7ffddfd1efb0)
 at 
/usr/src/debug/qemu-kvm-4.2.0-2.module+el8.2.0+5135+ed3b2489.x86_64/./qapi/qapi-types-block-core.h:1492
 #12 0x55e74315c449 in blockdev_backup_prepare (common=0x55e746a8de90, 
errp=0x7ffddfd1f018)
 at blockdev.c:1885
 #13 0x55e743160152 in qmp_transaction
 (dev_list=, has_props=, 
props=0x55e7467fe2c0, errp=errp@entry=0x7ffddfd1f088) at blockdev.c:2340
 #14 0x55e743287ff5 in qmp_marshal_transaction
 (args=, ret=, errp=0x7ffddfd1f0f8)
 at qapi/qapi-commands-transaction.c:44
 #15 0x55e74333de6c in do_qmp_dispatch
 (errp=0x7ffddfd1f0f0, allow_oob=, request=, 
cmds=0x55e743c28d60 ) at qapi/qmp-dispatch.c:132
 #16 0x55e74333de6c in qmp_dispatch
 (cmds=0x55e743c28d60 , request=, 
allow_oob=)
 at qapi/qmp-dispatch.c:175
 #17 0x55e74325c061 in monitor_qmp_dispatch (mon=0x55e745908030, 
req=)
 at monitor/qmp.c:145
 #18 0x55e74325c6fa in monitor_qmp_bh_dispatcher (data=) at 
monitor/qmp.c:234
 #19 0x55e743385866 in aio_bh_call (bh=0x55e745807ae0) at util/async.c:117
 #20 0x55e743385866 in aio_bh_poll (ctx=ctx@entry=0x55e7458067a0) at 
util/async.c:117
 #21 0x55e743388c54 in aio_dispatch (ctx=0x55e7458067a0) at 
util/aio-posix.c:459
 #22 0x55e743385742 in aio_ctx_dispatch
 (source=, callback=, user_data=) at util/async.c:260
 #23 0x7fb68543e67d in g_main_dispatch (context=0x55e745893a40) at 
gmain.c:3176
 #24 0x7fb68543e67d in g_main_context_dispatch 
(context=context@entry=0x55e745893a40) at gmain.c:3829
 #25 0x55e743387d08 in glib_pollfds_poll () at util/main-loop.c:219
 #26 0x55e743387d08 in os_host_main_loop_wait (timeout=) at 
util/main-loop.c:242
 #27 0x55e743387d08 in main_loop_wait (nonblocking=) at 
util/main-loop.c:518
 #28 0x55e74316a3c1 in main_loop () at vl.c:1828
 #29 0x55e743016a72 in main (argc=, argv=, 
envp=)
 at vl.c:4504

Fix this by not acquiring the AioContext there, and ensuring all paths
leading to it have it already acquired (backup_clean()).

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1782111
Signed-off-by: Sergio Lopez 
Signed-off-by: Kevin Wolf 
---
 block/backup-top.c | 5 -
 block/backup.c | 3 +++
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/block/backup-top.c b/block/backup-top.c
index 818d3f26b4..b8d863ff08 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -255,9 +255,6 @@ append_failed:
 void bdrv_backup_top_drop(BlockDriverState *bs)
 {
 BDRVBackupTopState *s = bs->opaque;
-AioContext *aio_context = bdrv_get_aio_context(bs);
-
-aio_context_acquire(aio_context);
 
 bdrv_drained_begin(bs);
 
@@ -271,6 +268,4 @@ void bdrv_backup_top_drop(BlockDriverState *bs)
 bdrv_drained_end(bs);
 
 

[PULL 05/13] blockdev: unify qmp_blockdev_backup and blockdev-backup transaction paths

[Kevin Wolf]

From: Sergio Lopez 

Issuing a blockdev-backup from qmp_blockdev_backup takes a slightly
different path than when it's issued from a transaction. In the code,
this is manifested as some redundancy between do_blockdev_backup() and
blockdev_backup_prepare().

This change unifies both paths, merging do_blockdev_backup() and
blockdev_backup_prepare(), and changing qmp_blockdev_backup() to
create a transaction instead of calling do_backup_common() direcly.

As a side-effect, now qmp_blockdev_backup() is executed inside a
drained section, as it happens when creating a blockdev-backup
transaction. This change is visible from the user's perspective, as
the job gets paused and immediately resumed before starting the actual
work.

Signed-off-by: Sergio Lopez 
Reviewed-by: Max Reitz 
Reviewed-by: Kevin Wolf 
Signed-off-by: Kevin Wolf 
---
 blockdev.c | 60 --
 1 file changed, 13 insertions(+), 47 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5e85fc042e..152a0f7454 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1940,16 +1940,13 @@ typedef struct BlockdevBackupState {
 BlockJob *job;
 } BlockdevBackupState;
 
-static BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
-Error **errp);
-
 static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
 BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
 BlockdevBackup *backup;
-BlockDriverState *bs, *target;
+BlockDriverState *bs;
+BlockDriverState *target_bs;
 AioContext *aio_context;
-Error *local_err = NULL;
 
 assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
 backup = common->action->u.blockdev_backup.data;
@@ -1959,8 +1956,8 @@ static void blockdev_backup_prepare(BlkActionState 
*common, Error **errp)
 return;
 }
 
-target = bdrv_lookup_bs(backup->target, backup->target, errp);
-if (!target) {
+target_bs = bdrv_lookup_bs(backup->target, backup->target, errp);
+if (!target_bs) {
 return;
 }
 
@@ -1971,13 +1968,10 @@ static void blockdev_backup_prepare(BlkActionState 
*common, Error **errp)
 /* Paired with .clean() */
 bdrv_drained_begin(state->bs);
 
-state->job = do_blockdev_backup(backup, common->block_job_txn, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-goto out;
-}
+state->job = do_backup_common(qapi_BlockdevBackup_base(backup),
+  bs, target_bs, aio_context,
+  common->block_job_txn, errp);
 
-out:
 aio_context_release(aio_context);
 }
 
@@ -3695,41 +3689,13 @@ XDbgBlockGraph *qmp_x_debug_query_block_graph(Error 
**errp)
 return bdrv_get_xdbg_block_graph(errp);
 }
 
-BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
- Error **errp)
+void qmp_blockdev_backup(BlockdevBackup *backup, Error **errp)
 {
-BlockDriverState *bs;
-BlockDriverState *target_bs;
-AioContext *aio_context;
-BlockJob *job;
-
-bs = bdrv_lookup_bs(backup->device, backup->device, errp);
-if (!bs) {
-return NULL;
-}
-
-target_bs = bdrv_lookup_bs(backup->target, backup->target, errp);
-if (!target_bs) {
-return NULL;
-}
-
-aio_context = bdrv_get_aio_context(bs);
-aio_context_acquire(aio_context);
-
-job = do_backup_common(qapi_BlockdevBackup_base(backup),
-   bs, target_bs, aio_context, txn, errp);
-
-aio_context_release(aio_context);
-return job;
-}
-
-void qmp_blockdev_backup(BlockdevBackup *arg, Error **errp)
-{
-BlockJob *job;
-job = do_blockdev_backup(arg, NULL, errp);
-if (job) {
-job_start(>job);
-}
+TransactionAction action = {
+.type = TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP,
+.u.blockdev_backup.data = backup,
+};
+blockdev_do_action(, errp);
 }
 
 /* Parameter check and block job starting for drive mirroring.
-- 
2.20.1

[PULL 04/13] blockdev: unify qmp_drive_backup and drive-backup transaction paths

[Kevin Wolf]

From: Sergio Lopez 

Issuing a drive-backup from qmp_drive_backup takes a slightly
different path than when it's issued from a transaction. In the code,
this is manifested as some redundancy between do_drive_backup() and
drive_backup_prepare().

This change unifies both paths, merging do_drive_backup() and
drive_backup_prepare(), and changing qmp_drive_backup() to create a
transaction instead of calling do_backup_common() direcly.

As a side-effect, now qmp_drive_backup() is executed inside a drained
section, as it happens when creating a drive-backup transaction. This
change is visible from the user's perspective, as the job gets paused
and immediately resumed before starting the actual work.

Also fix tests 141, 185 and 219 to cope with the extra
JOB_STATUS_CHANGE lines.

Signed-off-by: Sergio Lopez 
Reviewed-by: Kevin Wolf 
Signed-off-by: Kevin Wolf 
---
 blockdev.c | 224 +
 tests/qemu-iotests/141.out |   2 +
 tests/qemu-iotests/185.out |   2 +
 tests/qemu-iotests/219 |   7 +-
 tests/qemu-iotests/219.out |   8 ++
 5 files changed, 117 insertions(+), 126 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 553e315972..5e85fc042e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1761,39 +1761,128 @@ typedef struct DriveBackupState {
 BlockJob *job;
 } DriveBackupState;
 
-static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
-Error **errp);
+static BlockJob *do_backup_common(BackupCommon *backup,
+  BlockDriverState *bs,
+  BlockDriverState *target_bs,
+  AioContext *aio_context,
+  JobTxn *txn, Error **errp);
 
 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
 DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
-BlockDriverState *bs;
 DriveBackup *backup;
+BlockDriverState *bs;
+BlockDriverState *target_bs;
+BlockDriverState *source = NULL;
 AioContext *aio_context;
+QDict *options;
 Error *local_err = NULL;
+int flags;
+int64_t size;
+bool set_backing_hd = false;
 
 assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
 backup = common->action->u.drive_backup.data;
 
+if (!backup->has_mode) {
+backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+}
+
 bs = bdrv_lookup_bs(backup->device, backup->device, errp);
 if (!bs) {
 return;
 }
 
+if (!bs->drv) {
+error_setg(errp, "Device has no medium");
+return;
+}
+
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 
 /* Paired with .clean() */
 bdrv_drained_begin(bs);
 
-state->bs = bs;
+if (!backup->has_format) {
+backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ?
+ NULL : (char *) bs->drv->format_name;
+}
+
+/* Early check to avoid creating target */
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
+goto out;
+}
+
+flags = bs->open_flags | BDRV_O_RDWR;
+
+/*
+ * See if we have a backing HD we can use to create our new image
+ * on top of.
+ */
+if (backup->sync == MIRROR_SYNC_MODE_TOP) {
+source = backing_bs(bs);
+if (!source) {
+backup->sync = MIRROR_SYNC_MODE_FULL;
+}
+}
+if (backup->sync == MIRROR_SYNC_MODE_NONE) {
+source = bs;
+flags |= BDRV_O_NO_BACKING;
+set_backing_hd = true;
+}
+
+size = bdrv_getlength(bs);
+if (size < 0) {
+error_setg_errno(errp, -size, "bdrv_getlength failed");
+goto out;
+}
+
+if (backup->mode != NEW_IMAGE_MODE_EXISTING) {
+assert(backup->format);
+if (source) {
+bdrv_refresh_filename(source);
+bdrv_img_create(backup->target, backup->format, source->filename,
+source->drv->format_name, NULL,
+size, flags, false, _err);
+} else {
+bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL,
+size, flags, false, _err);
+}
+}
 
-state->job = do_drive_backup(backup, common->block_job_txn, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 goto out;
 }
 
+options = qdict_new();
+qdict_put_str(options, "discard", "unmap");
+qdict_put_str(options, "detect-zeroes", "unmap");
+if (backup->format) {
+qdict_put_str(options, "driver", backup->format);
+}
+
+target_bs = bdrv_open(backup->target, NULL, options, flags, errp);
+if (!target_bs) {
+goto out;
+}
+
+if (set_backing_hd) {
+bdrv_set_backing_hd(target_bs, source, _err);
+if (local_err) {
+goto unref;
+}
+}
+
+state->bs = bs;
+
+state->job = 

[PULL 13/13] iscsi: Don't access non-existent scsi_lba_status_descriptor

[Kevin Wolf]

In iscsi_co_block_status(), we may have received num_descriptors == 0
from the iscsi server. Therefore, we can't unconditionally access
lbas->descriptors[0]. Add the missing check.

Signed-off-by: Kevin Wolf 
Reviewed-by: Felipe Franciosi 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: John Snow 
Reviewed-by: Peter Lieven 
---
 block/iscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index cbd57294ab..c8feaa2f0e 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -753,7 +753,7 @@ retry:
 }
 
 lbas = scsi_datain_unmarshall(iTask.task);
-if (lbas == NULL) {
+if (lbas == NULL || lbas->num_descriptors == 0) {
 ret = -EIO;
 goto out_unlock;
 }
-- 
2.20.1

[PULL 10/13] iotests: Test handling of AioContexts with some blockdev actions

[Kevin Wolf]

From: Sergio Lopez 

Includes the following tests:

 - Adding a dirty bitmap.
   * RHBZ: 1782175

 - Starting a drive-mirror to an NBD-backed target.
   * RHBZ: 1746217, 1773517

 - Aborting an external snapshot transaction.
   * RHBZ: 1779036

 - Aborting a blockdev backup transaction.
   * RHBZ: 1782111

For each one of them, a VM with a number of disks running in an
IOThread AioContext is used.

Signed-off-by: Sergio Lopez 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/281 | 247 +
 tests/qemu-iotests/281.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 253 insertions(+)
 create mode 100755 tests/qemu-iotests/281
 create mode 100644 tests/qemu-iotests/281.out

diff --git a/tests/qemu-iotests/281 b/tests/qemu-iotests/281
new file mode 100755
index 00..269d583b2c
--- /dev/null
+++ b/tests/qemu-iotests/281
@@ -0,0 +1,247 @@
+#!/usr/bin/env python
+#
+# Test cases for blockdev + IOThread interactions
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import iotests
+from iotests import qemu_img
+
+image_len = 64 * 1024 * 1024
+
+# Test for RHBZ#1782175
+class TestDirtyBitmapIOThread(iotests.QMPTestCase):
+drive0_img = os.path.join(iotests.test_dir, 'drive0.img')
+images = { 'drive0': drive0_img }
+
+def setUp(self):
+for name in self.images:
+qemu_img('create', '-f', iotests.imgfmt,
+ self.images[name], str(image_len))
+
+self.vm = iotests.VM()
+self.vm.add_object('iothread,id=iothread0')
+
+for name in self.images:
+self.vm.add_blockdev('driver=file,filename=%s,node-name=file_%s'
+ % (self.images[name], name))
+self.vm.add_blockdev('driver=qcow2,file=file_%s,node-name=%s'
+ % (name, name))
+
+self.vm.launch()
+self.vm.qmp('x-blockdev-set-iothread',
+node_name='drive0', iothread='iothread0',
+force=True)
+
+def tearDown(self):
+self.vm.shutdown()
+for name in self.images:
+os.remove(self.images[name])
+
+def test_add_dirty_bitmap(self):
+result = self.vm.qmp(
+'block-dirty-bitmap-add',
+node='drive0',
+name='bitmap1',
+persistent=True,
+)
+
+self.assert_qmp(result, 'return', {})
+
+
+# Test for RHBZ#1746217 & RHBZ#1773517
+class TestNBDMirrorIOThread(iotests.QMPTestCase):
+nbd_sock = os.path.join(iotests.sock_dir, 'nbd.sock')
+drive0_img = os.path.join(iotests.test_dir, 'drive0.img')
+mirror_img = os.path.join(iotests.test_dir, 'mirror.img')
+images = { 'drive0': drive0_img, 'mirror': mirror_img }
+
+def setUp(self):
+for name in self.images:
+qemu_img('create', '-f', iotests.imgfmt,
+ self.images[name], str(image_len))
+
+self.vm_src = iotests.VM(path_suffix='src')
+self.vm_src.add_object('iothread,id=iothread0')
+self.vm_src.add_blockdev('driver=file,filename=%s,node-name=file0'
+  % (self.drive0_img))
+self.vm_src.add_blockdev('driver=qcow2,file=file0,node-name=drive0')
+self.vm_src.launch()
+self.vm_src.qmp('x-blockdev-set-iothread',
+node_name='drive0', iothread='iothread0',
+force=True)
+
+self.vm_tgt = iotests.VM(path_suffix='tgt')
+self.vm_tgt.add_object('iothread,id=iothread0')
+self.vm_tgt.add_blockdev('driver=file,filename=%s,node-name=file0'
+  % (self.mirror_img))
+self.vm_tgt.add_blockdev('driver=qcow2,file=file0,node-name=drive0')
+self.vm_tgt.launch()
+self.vm_tgt.qmp('x-blockdev-set-iothread',
+node_name='drive0', iothread='iothread0',
+force=True)
+
+def tearDown(self):
+self.vm_src.shutdown()
+self.vm_tgt.shutdown()
+for name in self.images:
+os.remove(self.images[name])
+
+def test_nbd_mirror(self):
+result = self.vm_tgt.qmp(
+'nbd-server-start',
+addr={
+'type': 'unix',
+'data': { 'path': self.nbd_sock }
+}
+)
+

[PULL 08/13] blockdev: Acquire AioContext on dirty bitmap functions

[Kevin Wolf]

From: Sergio Lopez 

Dirty map addition and removal functions are not acquiring to BDS
AioContext, while they may call to code that expects it to be
acquired.

This may trigger a crash with a stack trace like this one:

 #0  0x7f0ef146370f in __GI_raise (sig=sig@entry=6)
 at ../sysdeps/unix/sysv/linux/raise.c:50
 #1  0x7f0ef144db25 in __GI_abort () at abort.c:79
 #2  0x565022294dce in error_exit
 (err=, msg=msg@entry=0x56502243a730 <__func__.16350> 
"qemu_mutex_unlock_impl") at util/qemu-thread-posix.c:36
 #3  0x5650222950ba in qemu_mutex_unlock_impl
 (mutex=mutex@entry=0x5650244b0240, file=file@entry=0x565022439adf 
"util/async.c", line=line@entry=526) at util/qemu-thread-posix.c:108
 #4  0x565022290029 in aio_context_release
 (ctx=ctx@entry=0x5650244b01e0) at util/async.c:526
 #5  0x56502221cd08 in bdrv_can_store_new_dirty_bitmap
 (bs=bs@entry=0x5650244dc820, name=name@entry=0x56502481d360 "bitmap1", 
granularity=granularity@entry=65536, errp=errp@entry=0x7fff22831718)
 at block/dirty-bitmap.c:542
 #6  0x56502206ae53 in qmp_block_dirty_bitmap_add
 (errp=0x7fff22831718, disabled=false, has_disabled=, 
persistent=, has_persistent=true, granularity=65536, 
has_granularity=, name=0x56502481d360 "bitmap1", node=) at blockdev.c:2894
 #7  0x56502206ae53 in qmp_block_dirty_bitmap_add
 (node=, name=0x56502481d360 "bitmap1", 
has_granularity=, granularity=, 
has_persistent=true, persistent=, has_disabled=false, 
disabled=false, errp=0x7fff22831718) at blockdev.c:2856
 #8  0x5650221847a3 in qmp_marshal_block_dirty_bitmap_add
 (args=, ret=, errp=0x7fff22831798)
 at qapi/qapi-commands-block-core.c:651
 #9  0x565022247e6c in do_qmp_dispatch
 (errp=0x7fff22831790, allow_oob=, request=, 
cmds=0x565022b32d60 ) at qapi/qmp-dispatch.c:132
 #10 0x565022247e6c in qmp_dispatch
 (cmds=0x565022b32d60 , request=, 
allow_oob=) at qapi/qmp-dispatch.c:175
 #11 0x565022166061 in monitor_qmp_dispatch
 (mon=0x56502450faa0, req=) at monitor/qmp.c:145
 #12 0x5650221666fa in monitor_qmp_bh_dispatcher
 (data=) at monitor/qmp.c:234
 #13 0x56502228f866 in aio_bh_call (bh=0x56502440eae0)
 at util/async.c:117
 #14 0x56502228f866 in aio_bh_poll (ctx=ctx@entry=0x56502440d7a0)
 at util/async.c:117
 #15 0x565022292c54 in aio_dispatch (ctx=0x56502440d7a0)
 at util/aio-posix.c:459
 #16 0x56502228f742 in aio_ctx_dispatch
 (source=, callback=, user_data=) at util/async.c:260
 #17 0x7f0ef5ce667d in g_main_dispatch (context=0x56502449aa40)
 at gmain.c:3176
 #18 0x7f0ef5ce667d in g_main_context_dispatch
 (context=context@entry=0x56502449aa40) at gmain.c:3829
 #19 0x565022291d08 in glib_pollfds_poll () at util/main-loop.c:219
 #20 0x565022291d08 in os_host_main_loop_wait
 (timeout=) at util/main-loop.c:242
 #21 0x565022291d08 in main_loop_wait (nonblocking=)
 at util/main-loop.c:518
 #22 0x5650220743c1 in main_loop () at vl.c:1828
 #23 0x565021f20a72 in main
 (argc=, argv=, envp=)
 at vl.c:4504

Fix this by acquiring the AioContext at qmp_block_dirty_bitmap_add()
and qmp_block_dirty_bitmap_add().

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1782175
Signed-off-by: Sergio Lopez 
Signed-off-by: Kevin Wolf 
---
 blockdev.c | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1dacbc20ec..d4ef6cd452 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2984,6 +2984,7 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 {
 BlockDriverState *bs;
 BdrvDirtyBitmap *bitmap;
+AioContext *aio_context;
 
 if (!name || name[0] == '\0') {
 error_setg(errp, "Bitmap name cannot be empty");
@@ -2995,11 +2996,14 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 return;
 }
 
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+
 if (has_granularity) {
 if (granularity < 512 || !is_power_of_2(granularity)) {
 error_setg(errp, "Granularity must be power of 2 "
  "and at least 512");
-return;
+goto out;
 }
 } else {
 /* Default to cluster size, if available: */
@@ -3017,12 +3021,12 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 if (persistent &&
 !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
 {
-return;
+goto out;
 }
 
 bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
 if (bitmap == NULL) {
-return;
+goto out;
 }
 
 if (disabled) {
@@ -3030,6 +3034,9 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 }
 
 bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
+
+out:
+aio_context_release(aio_context);
 }
 
 static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
@@ 

[PULL 03/13] blockdev: fix coding style issues in drive_backup_prepare

[Kevin Wolf]

From: Sergio Lopez 

Fix a couple of minor coding style issues in drive_backup_prepare.

Signed-off-by: Sergio Lopez 
Reviewed-by: Max Reitz 
Reviewed-by: Kevin Wolf 
Signed-off-by: Kevin Wolf 
---
 blockdev.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8e029e9c01..553e315972 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3620,7 +3620,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
JobTxn *txn,
 
 if (!backup->has_format) {
 backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ?
- NULL : (char*) bs->drv->format_name;
+ NULL : (char *) bs->drv->format_name;
 }
 
 /* Early check to avoid creating target */
@@ -3630,8 +3630,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
JobTxn *txn,
 
 flags = bs->open_flags | BDRV_O_RDWR;
 
-/* See if we have a backing HD we can use to create our new image
- * on top of. */
+/*
+ * See if we have a backing HD we can use to create our new image
+ * on top of.
+ */
 if (backup->sync == MIRROR_SYNC_MODE_TOP) {
 source = backing_bs(bs);
 if (!source) {
-- 
2.20.1

[PULL 02/13] iotests: Add more "skip_if_unsupported" statements to the python tests

[Kevin Wolf]

From: Thomas Huth 

The python code already contains a possibility to skip tests if the
corresponding driver is not available in the qemu binary - use it
in more spots to avoid that the tests are failing if the driver has
been disabled.

While we're at it, we can now also remove some of the old checks that
were using iotests.supports_quorum() - and which were apparently not
working as expected since the tests aborted instead of being skipped
when "quorum" was missing in the QEMU binary.

Signed-off-by: Thomas Huth 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/030 |  4 +---
 tests/qemu-iotests/040 |  2 ++
 tests/qemu-iotests/041 | 39 +++
 tests/qemu-iotests/245 |  2 ++
 4 files changed, 8 insertions(+), 39 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index be35bde06f..0990681c1e 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -530,6 +530,7 @@ class TestQuorum(iotests.QMPTestCase):
 children = []
 backing = []
 
+@iotests.skip_if_unsupported(['quorum'])
 def setUp(self):
 opts = ['driver=quorum', 'vote-threshold=2']
 
@@ -560,9 +561,6 @@ class TestQuorum(iotests.QMPTestCase):
 os.remove(img)
 
 def test_stream_quorum(self):
-if not iotests.supports_quorum():
-return
-
 self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', 
self.children[0]),
 qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', 
self.backing[0]),
 'image file map matches backing file before 
streaming')
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 762ad1ebcb..74f62c3c4a 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -106,6 +106,7 @@ class TestSingleDrive(ImageCommitTestCase):
 self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 
524288', backing_img).find("verification failed"))
 self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 
524288', backing_img).find("verification failed"))
 
+@iotests.skip_if_unsupported(['throttle'])
 def test_commit_with_filter_and_quit(self):
 result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
 self.assert_qmp(result, 'return', {})
@@ -125,6 +126,7 @@ class TestSingleDrive(ImageCommitTestCase):
 self.has_quit = True
 
 # Same as above, but this time we add the filter after starting the job
+@iotests.skip_if_unsupported(['throttle'])
 def test_commit_plus_filter_and_quit(self):
 result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
 self.assert_qmp(result, 'return', {})
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index d7be30b62b..c07437fda1 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -871,6 +871,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
 image_len = 1 * 1024 * 1024 # MB
 IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
 
+@iotests.skip_if_unsupported(['quorum'])
 def setUp(self):
 self.vm = iotests.VM()
 
@@ -891,9 +892,8 @@ class TestRepairQuorum(iotests.QMPTestCase):
 #assemble the quorum block device from the individual files
 args = { "driver": "quorum", "node-name": "quorum0",
  "vote-threshold": 2, "children": [ "img0", "img1", "img2" ] }
-if iotests.supports_quorum():
-result = self.vm.qmp("blockdev-add", **args)
-self.assert_qmp(result, 'return', {})
+result = self.vm.qmp("blockdev-add", **args)
+self.assert_qmp(result, 'return', {})
 
 
 def tearDown(self):
@@ -906,9 +906,6 @@ class TestRepairQuorum(iotests.QMPTestCase):
 pass
 
 def test_complete(self):
-if not iotests.supports_quorum():
-return
-
 self.assert_no_active_block_jobs()
 
 result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
@@ -925,9 +922,6 @@ class TestRepairQuorum(iotests.QMPTestCase):
 'target image does not match source after mirroring')
 
 def test_cancel(self):
-if not iotests.supports_quorum():
-return
-
 self.assert_no_active_block_jobs()
 
 result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
@@ -942,9 +936,6 @@ class TestRepairQuorum(iotests.QMPTestCase):
 self.vm.shutdown()
 
 def test_cancel_after_ready(self):
-if not iotests.supports_quorum():
-return
-
 self.assert_no_active_block_jobs()
 
 result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',
@@ -961,9 +952,6 @@ class TestRepairQuorum(iotests.QMPTestCase):
 'target image does not match source after mirroring')
 
 def test_pause(self):
-if not iotests.supports_quorum():
-return
-
 self.assert_no_active_block_jobs()
 
 result = 

[PULL 01/13] iotests.py: Let wait_migration wait even more

[Kevin Wolf]

From: Max Reitz 

The "migration completed" event may be sent (on the source, to be
specific) before the migration is actually completed, so the VM runstate
will still be "finish-migrate" instead of "postmigrate".  So ask the
users of VM.wait_migration() to specify the final runstate they desire
and then poll the VM until it has reached that state.  (This should be
over very quickly, so busy polling is fine.)

Without this patch, I see intermittent failures in the new iotest 280
under high system load.  I have not yet seen such failures with other
iotests that use VM.wait_migration() and query-status afterwards, but
maybe they just occur even more rarely, or it is because they also wait
on the destination VM to be running.

Signed-off-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 6 +-
 tests/qemu-iotests/234| 8 
 tests/qemu-iotests/262| 4 ++--
 tests/qemu-iotests/280| 2 +-
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 13fd8b5cd2..0b62c42851 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -668,12 +668,16 @@ class VM(qtest.QEMUQtestMachine):
 }
 ]))
 
-def wait_migration(self):
+def wait_migration(self, expect_runstate):
 while True:
 event = self.event_wait('MIGRATION')
 log(event, filters=[filter_qmp_event])
 if event['data']['status'] == 'completed':
 break
+# The event may occur in finish-migrate, so wait for the expected
+# post-migration runstate
+while self.qmp('query-status')['return']['status'] != expect_runstate:
+pass
 
 def node_info(self, node_name):
 nodes = self.qmp('query-named-block-nodes')
diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
index 34c818c485..59a7f949ec 100755
--- a/tests/qemu-iotests/234
+++ b/tests/qemu-iotests/234
@@ -69,9 +69,9 @@ with iotests.FilePath('img') as img_path, \
 iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo_a)))
 with iotests.Timeout(3, 'Migration does not complete'):
 # Wait for the source first (which includes setup=setup)
-vm_a.wait_migration()
+vm_a.wait_migration('postmigrate')
 # Wait for the destination second (which does not)
-vm_b.wait_migration()
+vm_b.wait_migration('running')
 
 iotests.log(vm_a.qmp('query-migrate')['return']['status'])
 iotests.log(vm_b.qmp('query-migrate')['return']['status'])
@@ -98,9 +98,9 @@ with iotests.FilePath('img') as img_path, \
 iotests.log(vm_b.qmp('migrate', uri='exec:cat >%s' % (fifo_b)))
 with iotests.Timeout(3, 'Migration does not complete'):
 # Wait for the source first (which includes setup=setup)
-vm_b.wait_migration()
+vm_b.wait_migration('postmigrate')
 # Wait for the destination second (which does not)
-vm_a.wait_migration()
+vm_a.wait_migration('running')
 
 iotests.log(vm_a.qmp('query-migrate')['return']['status'])
 iotests.log(vm_b.qmp('query-migrate')['return']['status'])
diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262
index 0963daa806..bbcb5260a6 100755
--- a/tests/qemu-iotests/262
+++ b/tests/qemu-iotests/262
@@ -71,9 +71,9 @@ with iotests.FilePath('img') as img_path, \
 iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo)))
 with iotests.Timeout(3, 'Migration does not complete'):
 # Wait for the source first (which includes setup=setup)
-vm_a.wait_migration()
+vm_a.wait_migration('postmigrate')
 # Wait for the destination second (which does not)
-vm_b.wait_migration()
+vm_b.wait_migration('running')
 
 iotests.log(vm_a.qmp('query-migrate')['return']['status'])
 iotests.log(vm_b.qmp('query-migrate')['return']['status'])
diff --git a/tests/qemu-iotests/280 b/tests/qemu-iotests/280
index 0b1fa8e1d8..85e9114c5e 100755
--- a/tests/qemu-iotests/280
+++ b/tests/qemu-iotests/280
@@ -45,7 +45,7 @@ with iotests.FilePath('base') as base_path , \
 vm.qmp_log('migrate', uri='exec:cat > /dev/null')
 
 with iotests.Timeout(3, 'Migration does not complete'):
-vm.wait_migration()
+vm.wait_migration('postmigrate')
 
 iotests.log('\nVM is now stopped:')
 iotests.log(vm.qmp('query-migrate')['return']['status'])
-- 
2.20.1

[PULL 00/13] Block layer patches

[Kevin Wolf]

The following changes since commit 105b07f1ba462ec48b27e5cb74ddf81c6a79364c:

  Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20200127' into 
staging (2020-01-27 13:02:36 +)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 5fbf1d56c24018772e900a40a0955175ff82f35c:

  iscsi: Don't access non-existent scsi_lba_status_descriptor (2020-01-27 
17:19:53 +0100)


Block layer patches:

- iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)
- AioContext fixes in QMP commands for backup and bitmaps
- iotests fixes


Eiichi Tsukata (1):
  block/backup: fix memory leak in bdrv_backup_top_append()

Felipe Franciosi (1):
  iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)

Kevin Wolf (1):
  iscsi: Don't access non-existent scsi_lba_status_descriptor

Max Reitz (1):
  iotests.py: Let wait_migration wait even more

Sergio Lopez (8):
  blockdev: fix coding style issues in drive_backup_prepare
  blockdev: unify qmp_drive_backup and drive-backup transaction paths
  blockdev: unify qmp_blockdev_backup and blockdev-backup transaction paths
  blockdev: honor bdrv_try_set_aio_context() context requirements
  block/backup-top: Don't acquire context while dropping top
  blockdev: Acquire AioContext on dirty bitmap functions
  blockdev: Return bs to the proper context on snapshot abort
  iotests: Test handling of AioContexts with some blockdev actions

Thomas Huth (1):
  iotests: Add more "skip_if_unsupported" statements to the python tests

 block/backup-top.c|   7 +-
 block/backup.c|   3 +
 block/iscsi.c |   7 +-
 blockdev.c| 393 +++---
 tests/qemu-iotests/iotests.py |   6 +-
 tests/qemu-iotests/030|   4 +-
 tests/qemu-iotests/040|   2 +
 tests/qemu-iotests/041|  39 +
 tests/qemu-iotests/141.out|   2 +
 tests/qemu-iotests/185.out|   2 +
 tests/qemu-iotests/219|   7 +-
 tests/qemu-iotests/219.out|   8 +
 tests/qemu-iotests/234|   8 +-
 tests/qemu-iotests/245|   2 +
 tests/qemu-iotests/262|   4 +-
 tests/qemu-iotests/280|   2 +-
 tests/qemu-iotests/281| 247 ++
 tests/qemu-iotests/281.out|   5 +
 tests/qemu-iotests/group  |   1 +
 19 files changed, 510 insertions(+), 239 deletions(-)
 create mode 100755 tests/qemu-iotests/281
 create mode 100644 tests/qemu-iotests/281.out

Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Kevin Wolf]

Am 27.01.2020 um 14:33 hat Markus Armbruster geschrieben:
> Maxim Levitsky  writes:
> 
> > On Wed, 2019-11-27 at 09:08 +0100, Markus Armbruster wrote:
> >> I think it makes sense to collect *all* block HMP stuff here.
> >> 
> >> Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...
> >> 
> >> I guess hmp_change() has to stay there, because it's both block and ui.
> >> 
> >> Left in blockdev.c: hmp_drive_add_node().
> >
> > Thank you very much. I added these and bunch more to my patchset.
> >
> >> 
> >> Quick grep for possible files to check:
> >> 
> >> $ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
> >> MAINTAINERS
> >> blockdev-hmp-cmds.c
> >> 
> >
> >> blockdev.c
> > hmp_drive_add_node is there and I moved it too.
> >
> >
> >> cpus.c
> > Nothing suspicious
> >
> >> dump/dump.c
> > qmp_dump_guest_memory is only monitor reference there I think
> >
> >> hw/display/qxl.c
> > No way that is related to the block layer
> >
> >> hw/scsi/vhost-scsi.c
> > All right, the monitor_fd_param is an interesting thing.
> > Not related to block though.
> >
> >> hw/usb/dev-storage.c
> > All right, this for no reason includes monitor/monitor.h,
> > added patch to remove this because why not.
> >
> >> include/monitor/monitor.h
> > Nothing suspicious
> >
> >> migration/migration.c
> > Nothing suspicious
> >
> >> monitor/hmp-cmds.c
> > Added hmp_qemu_io
> >
> > Maybe I need to add hmp_delvm too?
> > savevm/delvm do old style snapshots
> > which are stored to the first block device
> 
> One foot in the block subsystem, the other foot in the migration
> subsystem.  I'm not sure where this should go.  Kevin?

Moving it to block is not an obvious improvement, so I think I'd leave
it alone.

Kevin

Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Mon, 2020-01-27 at 14:33 +0100, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > On Wed, 2019-11-27 at 09:08 +0100, Markus Armbruster wrote:
> > > I think it makes sense to collect *all* block HMP stuff here.
> > > 
> > > Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...
> > > 
> > > I guess hmp_change() has to stay there, because it's both block and ui.
> > > 
> > > Left in blockdev.c: hmp_drive_add_node().
> > 
> > Thank you very much. I added these and bunch more to my patchset.
> > 
> > > 
> > > Quick grep for possible files to check:
> > > 
> > > $ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
> > > MAINTAINERS
> > > blockdev-hmp-cmds.c
> > > 
> > > blockdev.c
> > 
> > hmp_drive_add_node is there and I moved it too.
> > 
> > 
> > > cpus.c
> > 
> > Nothing suspicious
> > 
> > > dump/dump.c
> > 
> > qmp_dump_guest_memory is only monitor reference there I think
> > 
> > > hw/display/qxl.c
> > 
> > No way that is related to the block layer
> > 
> > > hw/scsi/vhost-scsi.c
> > 
> > All right, the monitor_fd_param is an interesting thing.
> > Not related to block though.
> > 
> > > hw/usb/dev-storage.c
> > 
> > All right, this for no reason includes monitor/monitor.h,
> > added patch to remove this because why not.
> > 
> > > include/monitor/monitor.h
> > 
> > Nothing suspicious
> > 
> > > migration/migration.c
> > 
> > Nothing suspicious
> > 
> > > monitor/hmp-cmds.c
> > 
> > Added hmp_qemu_io
> > 
> > Maybe I need to add hmp_delvm too?
> > savevm/delvm do old style snapshots
> > which are stored to the first block device
> 
> One foot in the block subsystem, the other foot in the migration
> subsystem.  I'm not sure where this should go.  Kevin?
> 
> > > monitor/hmp.c
> > 
> > There are some block references in monitor_find_completion,
> > but I guess it is not worth it to move that
> > 
> > > monitor/misc.c
> > 
> > vm_completion for delvm/loadvm.
> 
> Having completion close to whatever it completes would be nice, I guess.
> 
> When in doubt, leave the savevm / delvm stuff alone.
Yep.

> 
> > > monitor/qmp-cmds.c
> > 
> > Nothing hmp related at first glance.
> > 
> > > qdev-monitor.c
> > 
> > blk_by_qdev_id - used by both hmp and qmp code
> > 
> > > vl.c
> > 
> > Hopefully nothing hmp+block related, I searched the file for
> > few things but I can't be fully sure.
> > Out of the curiosity do you know why this file is called like that,
> > since it hosts qemu main(), shouldn't it be called main.c ?
> 
> Its first commit 0824d6fc67 "for hard core developpers only: a new user
> mode linux project :-)" calls the executable "vl", and has
> 
> void help(void)
> {
> printf("Virtual Linux version " QEMU_VERSION ", Copyright (c) 2003 
> Fabrice Bellard\n"
>"usage: vl [-h] bzImage initrd [kernel parameters...]\n"
>"\n"
> [...]
> exit(1);
> }
> 
> The executable was renamed soon after.  I guess the source file name has
> made people wonder ever since.
Nice :-)

> 
> > 
> > Best regards and thanks for the detailed review!
> > Maxim Levitsky
> 
> You're welcome!

I hope we can move forward with this patch series as well.

Best regards,
Maxim Levitsky

Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Maxim Levitsky]

On Mon, 2020-01-27 at 14:44 +0100, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > On Wed, 2019-11-27 at 09:38 +0100, Markus Armbruster wrote:
> > > Title is too long.  blockdev-hmp-cmds.c will become
> > > block/monitor/block-hmp-cmds.c in v2.  With this in mind, suggest
> > > 
> > > block/monitor: Prefer to use hmp_handle_error() to report HMP errors
> > > 
> > > Maxim Levitsky  writes:
> > > 
> > > > This way they all will be prefixed with 'Error:' which some parsers
> > > > (e.g libvirt need)
> > > 
> > > Sadly, "all" is far from true.  Consider
> > > 
> > > void hmp_drive_add(Monitor *mon, const QDict *qdict)
> > > {
> > > Error *err = NULL;
> > > DriveInfo *dinfo = NULL;
> > > QemuOpts *opts;
> > > MachineClass *mc;
> > > const char *optstr = qdict_get_str(qdict, "opts");
> > > bool node = qdict_get_try_bool(qdict, "node", false);
> > > 
> > > if (node) {
> > > hmp_drive_add_node(mon, optstr);
> > > return;
> > > }
> > > 
> > > opts = drive_def(optstr);
> > > if (!opts)
> > > return;
> > > 
> > > 
> > > hmp_drive_add_node() uses error_report() and error_report_err().  Easy
> > > enough to fix if you move the function here, as I suggested in my review
> > > of PATCH 8.
> > 
> > To be honest that involves exporting the monitor_bdrv_states variable and
> > bds_tree_init, which were both static before, but I created a patch that 
> > does that,
> > If that is all right, I'll squash it with some of my patches.
> > 
> > 
> > > 
> > > drive_def() is a wrapper around qemu_opts_parse_noisily(), which uses
> > > error_report_err().  You can't change qemu_opts_parse_noisily() to use
> > > hmp_handle_error().  You'd have to convert drive_def() to Error, which
> > > involves switching it to qemu_opts_parse() + qemu_opts_print_help().
> > > 
> > > These are just the first two error paths in this file.  There's much
> > > more.  Truly routing all HMP errors through hmp_handle_error() takes a
> > > *massive* Error conversion effort, with a high risk of missing Error
> > > conversions, followed by a never-ending risk of non-Error stuff creeping
> > > in.
> > 
> > Oops. Active can of worms is detected. Take cover!
> 
> :)
> 
> > > There must be an easier way.
> > > 
> > > Consider vreport():
> > > 
> > > switch (type) {
> > > case REPORT_TYPE_ERROR:
> > > break;
> > > case REPORT_TYPE_WARNING:
> > > error_printf("warning: ");
> > > break;
> > > case REPORT_TYPE_INFO:
> > > error_printf("info: ");
> > > break;
> > > }
> > > 
> > > Adding the prefix here (either unconditionally, or if cur_mon) covers
> > > all HMP errors reported with error_report() & friends in one blow.
> > 
> > This is a very good idea.
> > If feels like this should be done unconditionally, although that will
> > break probably some scripts that depend on exact value of the error message 
> > (but to be honest,
> > scripts shouldn't be doing that in first place).
> > 
> > Doing that with cur_mon (took me some time to figure out what that is) will
> > limit the damage but its a bit of a hack.
> > 
> > 
> > I think that this is a very good change anyway though so if everyone agrees,
> > I will be more that happy to do this change.
> > Thoughts?
> 
> I think adding an "error: " tag has been proposed before.
> 
> I dislike overly decorated error messages, because decoration tends to
> obscure information.
> 
> However, when there's significant non-error output, or even uncertainty
> of what's an error and what's something else, decoration can help.
Yes, also this way it is consistent

> 
> Perhaps you can give some examples where the proposed decoration helps.
It helps to tag most monitor messages with error prefix which was the root 
cause of
me starting to work on this refactoring.
You suggested this, and I kind of like that idea.

> 
> > > That leaves the ones that are still reported with monitor_printf().
> > > Converting those to error_report() looks far more tractable to me.
> > 
> > Yep, in fact I grepped the tree for monitor_printf and there are not
> > that much instances of this used for error reporting, so it might
> > be possible to have 'error' prefix on all monitor errors that way
> > and not only for the block layer.
> 
> I figure "all" would be more useful than "just for the block layer".
Yep, the cover letter is outdated, now this patch series touch way
more that the block layer.

Best regards,
Maxim Levitsky

Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Markus Armbruster]

Maxim Levitsky  writes:

> On Wed, 2019-11-27 at 09:38 +0100, Markus Armbruster wrote:
>> Title is too long.  blockdev-hmp-cmds.c will become
>> block/monitor/block-hmp-cmds.c in v2.  With this in mind, suggest
>> 
>> block/monitor: Prefer to use hmp_handle_error() to report HMP errors
>> 
>> Maxim Levitsky  writes:
>> 
>> > This way they all will be prefixed with 'Error:' which some parsers
>> > (e.g libvirt need)
>> 
>> Sadly, "all" is far from true.  Consider
>> 
>> void hmp_drive_add(Monitor *mon, const QDict *qdict)
>> {
>> Error *err = NULL;
>> DriveInfo *dinfo = NULL;
>> QemuOpts *opts;
>> MachineClass *mc;
>> const char *optstr = qdict_get_str(qdict, "opts");
>> bool node = qdict_get_try_bool(qdict, "node", false);
>> 
>> if (node) {
>> hmp_drive_add_node(mon, optstr);
>> return;
>> }
>> 
>> opts = drive_def(optstr);
>> if (!opts)
>> return;
>> 
>> 
>> hmp_drive_add_node() uses error_report() and error_report_err().  Easy
>> enough to fix if you move the function here, as I suggested in my review
>> of PATCH 8.
> To be honest that involves exporting the monitor_bdrv_states variable and
> bds_tree_init, which were both static before, but I created a patch that does 
> that,
> If that is all right, I'll squash it with some of my patches.
>
>
>> 
>> drive_def() is a wrapper around qemu_opts_parse_noisily(), which uses
>> error_report_err().  You can't change qemu_opts_parse_noisily() to use
>> hmp_handle_error().  You'd have to convert drive_def() to Error, which
>> involves switching it to qemu_opts_parse() + qemu_opts_print_help().
>> 
>> These are just the first two error paths in this file.  There's much
>> more.  Truly routing all HMP errors through hmp_handle_error() takes a
>> *massive* Error conversion effort, with a high risk of missing Error
>> conversions, followed by a never-ending risk of non-Error stuff creeping
>> in.
> Oops. Active can of worms is detected. Take cover!

:)

>> There must be an easier way.
>> 
>> Consider vreport():
>> 
>> switch (type) {
>> case REPORT_TYPE_ERROR:
>> break;
>> case REPORT_TYPE_WARNING:
>> error_printf("warning: ");
>> break;
>> case REPORT_TYPE_INFO:
>> error_printf("info: ");
>> break;
>> }
>> 
>> Adding the prefix here (either unconditionally, or if cur_mon) covers
>> all HMP errors reported with error_report() & friends in one blow.
>
> This is a very good idea.
> If feels like this should be done unconditionally, although that will
> break probably some scripts that depend on exact value of the error message 
> (but to be honest,
> scripts shouldn't be doing that in first place).
>
> Doing that with cur_mon (took me some time to figure out what that is) will
> limit the damage but its a bit of a hack.
>
>
> I think that this is a very good change anyway though so if everyone agrees,
> I will be more that happy to do this change.
> Thoughts?

I think adding an "error: " tag has been proposed before.

I dislike overly decorated error messages, because decoration tends to
obscure information.

However, when there's significant non-error output, or even uncertainty
of what's an error and what's something else, decoration can help.

Perhaps you can give some examples where the proposed decoration helps.

>> That leaves the ones that are still reported with monitor_printf().
>> Converting those to error_report() looks far more tractable to me.
> Yep, in fact I grepped the tree for monitor_printf and there are not
> that much instances of this used for error reporting, so it might
> be possible to have 'error' prefix on all monitor errors that way
> and not only for the block layer.

I figure "all" would be more useful than "just for the block layer".

[...]

Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Markus Armbruster]

Maxim Levitsky  writes:

> On Wed, 2019-11-27 at 09:08 +0100, Markus Armbruster wrote:
>> I think it makes sense to collect *all* block HMP stuff here.
>> 
>> Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...
>> 
>> I guess hmp_change() has to stay there, because it's both block and ui.
>> 
>> Left in blockdev.c: hmp_drive_add_node().
>
> Thank you very much. I added these and bunch more to my patchset.
>
>> 
>> Quick grep for possible files to check:
>> 
>> $ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
>> MAINTAINERS
>> blockdev-hmp-cmds.c
>> 
>
>> blockdev.c
> hmp_drive_add_node is there and I moved it too.
>
>
>> cpus.c
> Nothing suspicious
>
>> dump/dump.c
> qmp_dump_guest_memory is only monitor reference there I think
>
>> hw/display/qxl.c
> No way that is related to the block layer
>
>> hw/scsi/vhost-scsi.c
> All right, the monitor_fd_param is an interesting thing.
> Not related to block though.
>
>> hw/usb/dev-storage.c
> All right, this for no reason includes monitor/monitor.h,
> added patch to remove this because why not.
>
>> include/monitor/monitor.h
> Nothing suspicious
>
>> migration/migration.c
> Nothing suspicious
>
>> monitor/hmp-cmds.c
> Added hmp_qemu_io
>
> Maybe I need to add hmp_delvm too?
> savevm/delvm do old style snapshots
> which are stored to the first block device

One foot in the block subsystem, the other foot in the migration
subsystem.  I'm not sure where this should go.  Kevin?

>> monitor/hmp.c
> There are some block references in monitor_find_completion,
> but I guess it is not worth it to move that
>
>> monitor/misc.c
> vm_completion for delvm/loadvm.

Having completion close to whatever it completes would be nice, I guess.

When in doubt, leave the savevm / delvm stuff alone.

>> monitor/qmp-cmds.c
> Nothing hmp related at first glance.
>
>> qdev-monitor.c
> blk_by_qdev_id - used by both hmp and qmp code
>
>> vl.c
> Hopefully nothing hmp+block related, I searched the file for
> few things but I can't be fully sure.
> Out of the curiosity do you know why this file is called like that,
> since it hosts qemu main(), shouldn't it be called main.c ?

Its first commit 0824d6fc67 "for hard core developpers only: a new user
mode linux project :-)" calls the executable "vl", and has

void help(void)
{
printf("Virtual Linux version " QEMU_VERSION ", Copyright (c) 2003 
Fabrice Bellard\n"
   "usage: vl [-h] bzImage initrd [kernel parameters...]\n"
   "\n"
[...]
exit(1);
}

The executable was renamed soon after.  I guess the source file name has
made people wonder ever since.

>
> Best regards and thanks for the detailed review!
>   Maxim Levitsky

You're welcome!

Re: [PATCH v2 4/4] vhost-user-blk: default num_queues to -smp N

[Cornelia Huck]

On Fri, 24 Jan 2020 10:01:59 +
Stefan Hajnoczi  wrote:

> Automatically size the number of request virtqueues to match the number
> of vCPUs.  This ensures that completion interrupts are handled on the
> same vCPU that submitted the request.  No IPI is necessary to complete
> an I/O request and performance is improved.

Ok, that one is pci-only anyway.

> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/block/vhost-user-blk.c  | 6 +-
>  hw/core/machine.c  | 1 +
>  hw/virtio/vhost-user-blk-pci.c | 6 ++
>  include/hw/virtio/vhost-user-blk.h | 2 ++
>  4 files changed, 14 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH v2 3/4] virtio-blk: default num_queues to -smp N

[Cornelia Huck]

On Fri, 24 Jan 2020 10:01:58 +
Stefan Hajnoczi  wrote:

> Automatically size the number of request virtqueues to match the number
> of vCPUs.  This ensures that completion interrupts are handled on the
> same vCPU that submitted the request.  No IPI is necessary to complete
> an I/O request and performance is improved.

Same comment regarding other transports.

> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/block/virtio-blk.c  | 6 +-
>  hw/core/machine.c  | 1 +
>  hw/virtio/virtio-blk-pci.c | 9 -
>  include/hw/virtio/virtio-blk.h | 2 ++
>  4 files changed, 16 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH v2 2/4] virtio-scsi: default num_queues to -smp N

[Cornelia Huck]

On Fri, 24 Jan 2020 10:01:57 +
Stefan Hajnoczi  wrote:

> Automatically size the number of request virtqueues to match the number

"If the pci transport is used, ..." ?

> of vCPUs.  This ensures that completion interrupts are handled on the
> same vCPU that submitted the request.  No IPI is necessary to complete
> an I/O request and performance is improved.

"For other transports, the number of request queues continues to
default to 1." ?

> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/core/machine.c   |  3 +++
>  hw/scsi/vhost-scsi.c|  3 ++-
>  hw/scsi/vhost-user-scsi.c   |  3 ++-
>  hw/scsi/virtio-scsi.c   |  6 +-
>  hw/virtio/vhost-scsi-pci.c  | 10 --
>  hw/virtio/vhost-user-scsi-pci.c | 10 --
>  hw/virtio/virtio-scsi-pci.c | 10 --
>  include/hw/virtio/virtio-scsi.h |  2 ++
>  8 files changed, 38 insertions(+), 9 deletions(-)
> 
(...)
> diff --git a/hw/virtio/vhost-scsi-pci.c b/hw/virtio/vhost-scsi-pci.c
> index e8dfbfc60f..38a8f0c3ef 100644
> --- a/hw/virtio/vhost-scsi-pci.c
> +++ b/hw/virtio/vhost-scsi-pci.c
> @@ -17,6 +17,7 @@
>  #include "qemu/osdep.h"
>  
>  #include "standard-headers/linux/virtio_pci.h"
> +#include "hw/boards.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/virtio/vhost-scsi.h"
>  #include "qapi/error.h"
> @@ -47,10 +48,15 @@ static void vhost_scsi_pci_realize(VirtIOPCIProxy 
> *vpci_dev, Error **errp)
>  {
>  VHostSCSIPCI *dev = VHOST_SCSI_PCI(vpci_dev);
>  DeviceState *vdev = DEVICE(>vdev);
> -VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
> +VirtIOSCSIConf *conf = >vdev.parent_obj.parent_obj.conf;
> +
> +/* 1:1 vq to vcpu mapping is ideal because it avoids IPIs */
> +if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
> +conf->num_queues = current_machine->smp.cpus;

This now maps the request vqs 1:1 to the vcpus. What about the fixed
vqs? If they don't really matter, amend the comment to explain that?

> +}
>  
>  if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> -vpci_dev->nvectors = vs->conf.num_queues + 3;
> +vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1;
>  }
>  
>  qdev_set_parent_bus(vdev, BUS(_dev->bus));

Re: [PATCH v2 1/4] virtio-scsi: introduce a constant for fixed virtqueues

[Cornelia Huck]

On Fri, 24 Jan 2020 10:01:56 +
Stefan Hajnoczi  wrote:

> The event and control virtqueues are always present, regardless of the
> multi-queue configuration.  Define a constant so that virtqueue number
> calculations are easier to read.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/scsi/vhost-user-scsi.c   | 2 +-
>  hw/scsi/virtio-scsi.c   | 7 ---
>  include/hw/virtio/virtio-scsi.h | 3 +++
>  3 files changed, 8 insertions(+), 4 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH] iotests/279: Fix for non-qcow2 formats

[Thomas Huth]

On 19/12/2019 15.42, Max Reitz wrote:
> First, driver=qcow2 will not work so well with non-qcow2 formats (and
> this test claims to support qcow, qed, and vmdk).
> 
> Second, vmdk will always report the backing file format to be vmdk.
> Filter that out so the output looks like for all other formats.
> 
> Third, the flat vmdk subformats do not support backing files, so they
> will not work with this test.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/279 | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/279 b/tests/qemu-iotests/279
> index 6682376808..30d29b1cb2 100755
> --- a/tests/qemu-iotests/279
> +++ b/tests/qemu-iotests/279
> @@ -38,6 +38,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  _supported_fmt qcow qcow2 vmdk qed
>  _supported_proto file
>  _supported_os Linux
> +_unsupported_imgopts "subformat=monolithicFlat" \
> + "subformat=twoGbMaxExtentFlat" \
>  
>  TEST_IMG="$TEST_IMG.base" _make_test_img 64M
>  TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base"
> @@ -45,11 +47,12 @@ _make_test_img -b "$TEST_IMG.mid"
>  
>  echo
>  echo '== qemu-img info --backing-chain =='
> -_img_info --backing-chain | _filter_img_info
> +_img_info --backing-chain | _filter_img_info | grep -v 'backing file format'
>  
>  echo
>  echo '== qemu-img info --backing-chain --image-opts =='
> -TEST_IMG="driver=qcow2,file.driver=file,file.filename=$TEST_IMG" _img_info 
> --backing-chain --image-opts | _filter_img_info
> +TEST_IMG="driver=$IMGFMT,file.driver=file,file.filename=$TEST_IMG" _img_info 
> --backing-chain --image-opts \
> +| _filter_img_info | grep -v 'backing file format'
>  
>  # success, all done
>  echo "*** done"
> 

This fixes the problems with "check -qed 279" and "check -vmdk 279" for me.

Tested-by: Thomas Huth 

Re: [PATCH 9/9] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Maxim Levitsky]

On Wed, 2019-11-27 at 09:38 +0100, Markus Armbruster wrote:
> Title is too long.  blockdev-hmp-cmds.c will become
> block/monitor/block-hmp-cmds.c in v2.  With this in mind, suggest
> 
> block/monitor: Prefer to use hmp_handle_error() to report HMP errors
> 
> Maxim Levitsky  writes:
> 
> > This way they all will be prefixed with 'Error:' which some parsers
> > (e.g libvirt need)
> 
> Sadly, "all" is far from true.  Consider
> 
> void hmp_drive_add(Monitor *mon, const QDict *qdict)
> {
> Error *err = NULL;
> DriveInfo *dinfo = NULL;
> QemuOpts *opts;
> MachineClass *mc;
> const char *optstr = qdict_get_str(qdict, "opts");
> bool node = qdict_get_try_bool(qdict, "node", false);
> 
> if (node) {
> hmp_drive_add_node(mon, optstr);
> return;
> }
> 
> opts = drive_def(optstr);
> if (!opts)
> return;
> 
> 
> hmp_drive_add_node() uses error_report() and error_report_err().  Easy
> enough to fix if you move the function here, as I suggested in my review
> of PATCH 8.
To be honest that involves exporting the monitor_bdrv_states variable and
bds_tree_init, which were both static before, but I created a patch that does 
that,
If that is all right, I'll squash it with some of my patches.


> 
> drive_def() is a wrapper around qemu_opts_parse_noisily(), which uses
> error_report_err().  You can't change qemu_opts_parse_noisily() to use
> hmp_handle_error().  You'd have to convert drive_def() to Error, which
> involves switching it to qemu_opts_parse() + qemu_opts_print_help().
> 
> These are just the first two error paths in this file.  There's much
> more.  Truly routing all HMP errors through hmp_handle_error() takes a
> *massive* Error conversion effort, with a high risk of missing Error
> conversions, followed by a never-ending risk of non-Error stuff creeping
> in.
Oops. Active can of worms is detected. Take cover!

> 
> There must be an easier way.
> 
> Consider vreport():
> 
> switch (type) {
> case REPORT_TYPE_ERROR:
> break;
> case REPORT_TYPE_WARNING:
> error_printf("warning: ");
> break;
> case REPORT_TYPE_INFO:
> error_printf("info: ");
> break;
> }
> 
> Adding the prefix here (either unconditionally, or if cur_mon) covers
> all HMP errors reported with error_report() & friends in one blow.

This is a very good idea.
If feels like this should be done unconditionally, although that will
break probably some scripts that depend on exact value of the error message 
(but to be honest,
scripts shouldn't be doing that in first place).

Doing that with cur_mon (took me some time to figure out what that is) will
limit the damage but its a bit of a hack.


I think that this is a very good change anyway though so if everyone agrees,
I will be more that happy to do this change.
Thoughts?

> 
> That leaves the ones that are still reported with monitor_printf().
> Converting those to error_report() looks far more tractable to me.
Yep, in fact I grepped the tree for monitor_printf and there are not
that much instances of this used for error reporting, so it might
be possible to have 'error' prefix on all monitor errors that way
and not only for the block layer.

> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  blockdev-hmp-cmds.c | 35 +--
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> > 
> > diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> > index c943dccd03..197994716f 100644
> > --- a/blockdev-hmp-cmds.c
> > +++ b/blockdev-hmp-cmds.c
> > @@ -59,7 +59,6 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
> >  mc = MACHINE_GET_CLASS(current_machine);
> >  dinfo = drive_new(opts, mc->block_default_type, );
> >  if (err) {
> > -error_report_err(err);
> >  qemu_opts_del(opts);
> >  goto err;
> >  }
> > @@ -73,7 +72,7 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
> >  monitor_printf(mon, "OK\n");
> >  break;
> >  default:
> > -monitor_printf(mon, "Can't hot-add drive to type %d\n", 
> > dinfo->type);
> > +error_setg(, "Can't hot-add drive to type %d", dinfo->type);
> >  goto err;
> >  }
> >  return;
> > @@ -84,6 +83,7 @@ err:
> >  monitor_remove_blk(blk);
> >  blk_unref(blk);
> >  }
> > +hmp_handle_error(mon, );
> >  }
> >  
> >  void hmp_drive_del(Monitor *mon, const QDict *qdict)
> > @@ -105,14 +105,14 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> >  
> >  blk = blk_by_name(id);
> >  if (!blk) {
> > -error_report("Device '%s' not found", id);
> > -return;
> > +error_setg(_err, "Device '%s' not found", id);
> > +goto err;
> 
> Having to create Error objects just so we can use hmp_handle_error() is
> awkward.  Tolerable if using hmp_handle_error() improves matters.  I'm
> not sure it does.

Well, if we go with 

Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 09:08 +0100, Markus Armbruster wrote:
> I think it makes sense to collect *all* block HMP stuff here.
> 
> Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...
> 
> I guess hmp_change() has to stay there, because it's both block and ui.
> 
> Left in blockdev.c: hmp_drive_add_node().

Thank you very much. I added these and bunch more to my patchset.

> 
> Quick grep for possible files to check:
> 
> $ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
> MAINTAINERS
> blockdev-hmp-cmds.c
> 

> blockdev.c
hmp_drive_add_node is there and I moved it too.


> cpus.c
Nothing suspicious

> dump/dump.c
qmp_dump_guest_memory is only monitor reference there I think

> hw/display/qxl.c
No way that is related to the block layer

> hw/scsi/vhost-scsi.c
All right, the monitor_fd_param is an interesting thing.
Not related to block though.

> hw/usb/dev-storage.c
All right, this for no reason includes monitor/monitor.h,
added patch to remove this because why not.

> include/monitor/monitor.h
Nothing suspicious

> migration/migration.c
Nothing suspicious

> monitor/hmp-cmds.c
Added hmp_qemu_io

Maybe I need to add hmp_delvm too?
savevm/delvm do old style snapshots
which are stored to the first block device


> monitor/hmp.c
There are some block references in monitor_find_completion,
but I guess it is not worth it to move that

> monitor/misc.c
vm_completion for delvm/loadvm.

> monitor/qmp-cmds.c
Nothing hmp related at first glance.

> qdev-monitor.c
blk_by_qdev_id - used by both hmp and qmp code

> vl.c
Hopefully nothing hmp+block related, I searched the file for
few things but I can't be fully sure.
Out of the curiosity do you know why this file is called like that,
since it hosts qemu main(), shouldn't it be called main.c ?


Best regards and thanks for the detailed review!
Maxim Levitsky

Re: [PATCH 4/9] monitor: move hmp_drive_mirror and hmp_drive_backup to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:22 +0100, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  blockdev-hmp-cmds.c | 61 +
> >  monitor/hmp-cmds.c  | 58 --
> >  2 files changed, 61 insertions(+), 58 deletions(-)
> 
> Licensing issue: blockdev-hmp-cmds.c is BSD, the code you moved there is
> GPLv2+.  The combined work must be GPLv2+.
> 
> 

Fixed, thanks!

Best regards,
Maxim Levitsky

Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:50 +0100, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  blockdev-hmp-cmds.c | 97 -
> >  blockdev.c  | 95 
> >  2 files changed, 96 insertions(+), 96 deletions(-)
> 
> hmp_drive_add() and hmp_drive_del() are now in the same .c, which feels
> right.  Their declarations are still in separate .h.  Suggest to move
> hmp_drive_add() from sysemu/sysemu.h to sysemu/blockdev.h.  Or maybe
> create a separate .h for the block HMP stuff, just like you created a
> separate .c.
> 
> 

Agree, I totally forgot about the headers.
I added include/block/blockdev-hmp-cmds.h for that now.

Best regards,
Maxim Levitsky

Re: [PATCH 5/9] monitor: move hmp_block_job* to blockdev-hmp-cmd.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:24 +0100, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  blockdev-hmp-cmds.c | 52 +
> >  monitor/hmp-cmds.c  | 52 -
> >  2 files changed, 52 insertions(+), 52 deletions(-)
> > 
> > diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> > index 5ae899a324..e333de27b1 100644
> > --- a/blockdev-hmp-cmds.c
> > +++ b/blockdev-hmp-cmds.c
> > @@ -238,3 +238,55 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
> >  hmp_handle_error(mon, );
> >  }
> >  
> > +
> 
> Is this extra blank line intentional?

Yes, it gives me 1 black line spacing between all functions.

Best regards,
Maxim Levitsky

Re: [PATCH 3/9] monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:29 +0100, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  blockdev-hmp-cmds.c | 97 -
> >  blockdev.c  | 95 
> >  2 files changed, 96 insertions(+), 96 deletions(-)
> > 
> > diff --git a/blockdev-hmp-cmds.c b/blockdev-hmp-cmds.c
> > index 21ff6fa9a9..8884618238 100644
> > --- a/blockdev-hmp-cmds.c
> > +++ b/blockdev-hmp-cmds.c
> > @@ -33,7 +33,7 @@
> >  #include "sysemu/sysemu.h"
> >  #include "monitor/monitor.h"
> >  #include "block/block_int.h"
> > -
> > +#include "qapi/qapi-commands-block.h"
> 
> I prefer keeping qapi/ stuff together.  Please add this right before
> #include "qapi/qmp/qdict.h".

Absolutely no problem!

Best regards,
Maxim Levitsky

Re: [PATCH 1/9] monitor: uninline add_init_drive

[Maxim Levitsky]

On Wed, 2019-11-27 at 08:13 +0100, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > This is only used by hmp_drive_add.
> > The code is just a bit shorter this way.
> > 
> > No functional changes
> > 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  device-hotplug.c | 33 +
> >  1 file changed, 13 insertions(+), 20 deletions(-)
> > 
> > diff --git a/device-hotplug.c b/device-hotplug.c
> > index f01d53774b..5ce73f0cff 100644
> > --- a/device-hotplug.c
> > +++ b/device-hotplug.c
> > @@ -34,42 +34,35 @@
> >  #include "monitor/monitor.h"
> >  #include "block/block_int.h"
> >  
> > -static DriveInfo *add_init_drive(const char *optstr)
> > +
> > +void hmp_drive_add(Monitor *mon, const QDict *qdict)
> >  {
> >  Error *err = NULL;
> > -DriveInfo *dinfo;
> > +DriveInfo *dinfo = NULL;
> 
> Superfluous initializer.
True, fixed now.
> 
> >  QemuOpts *opts;
> >  MachineClass *mc;
> > +const char *optstr = qdict_get_str(qdict, "opts");
> > +bool node = qdict_get_try_bool(qdict, "node", false);
> > +
> > +if (node) {
> > +hmp_drive_add_node(mon, optstr);
> > +return;
> > +}
> >  
> >  opts = drive_def(optstr);
> >  if (!opts)
> > -return NULL;
> > +return;
> >  
> >  mc = MACHINE_GET_CLASS(current_machine);
> >  dinfo = drive_new(opts, mc->block_default_type, );
> >  if (err) {
> >  error_report_err(err);
> >  qemu_opts_del(opts);
> > -return NULL;
> > -}
> > -
> > -return dinfo;
> > -}
> > -
> > -void hmp_drive_add(Monitor *mon, const QDict *qdict)
> > -{
> > -DriveInfo *dinfo = NULL;
> > -const char *opts = qdict_get_str(qdict, "opts");
> > -bool node = qdict_get_try_bool(qdict, "node", false);
> > -
> > -if (node) {
> > -hmp_drive_add_node(mon, opts);
> > -return;
> > +goto err;
> >  }
> >  
> > -dinfo = add_init_drive(opts);
> >  if (!dinfo) {
> > -goto err;
> > +return;
> >  }
> >  
> >  switch (dinfo->type) {
> 
> Reviewed-by: Markus Armbruster 
> 
> 

Best regards,
Maxim Levitsky

Re: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

[Maxim Levitsky]

On Mon, 2020-01-27 at 02:55 -0800, no-re...@patchew.org wrote:
> Patchew URL: 
> https://patchew.org/QEMU/20200127103647.17761-1-mlevi...@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:

All of these errors are either from code that I moved, and I wanted to
move it as is and from update of iotests, which shows that some of our
error messages end with space, which is stripped by iotest filter,
but once you update the iotest output, it appears again. I'll fix that
in later version of that WIP patch.

Best regards,
Maxim Levitsky

> 
> Type: series
> Message-id: 20200127103647.17761-1-mlevi...@redhat.com
> Subject: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  - [tag update]  
> patchew/1579779525-20065-1-git-send-email-imamm...@redhat.com -> 
> patchew/1579779525-20065-1-git-send-email-imamm...@redhat.com
>  - [tag update]  patchew/20200124100159.736209-1-stefa...@redhat.com -> 
> patchew/20200124100159.736209-1-stefa...@redhat.com
>  - [tag update]  patchew/20200124162606.8787-1-peter.mayd...@linaro.org 
> -> patchew/20200124162606.8787-1-peter.mayd...@linaro.org
>  - [tag update]  patchew/20200124172954.28481-1-peter.mayd...@linaro.org 
> -> patchew/20200124172954.28481-1-peter.mayd...@linaro.org
>  * [new tag] patchew/20200127103647.17761-1-mlevi...@redhat.com -> 
> patchew/20200127103647.17761-1-mlevi...@redhat.com
> Switched to a new branch 'test'
> 9d7e4e6 monitor/hmp: Prefer to use hmp_handle_error for error reporting in 
> block hmp commands
> a549971 add 'error' prefix to vreport
> 7c7da3a monitor: Move hmp_drive_add_node to block-hmp-cmds.c
> a2a0265 monitor/hmp: move hmp_info_block* to block-hmp-cmds.c
> d7f13de monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c
> c067499 monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c
> f5fab94 monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c
> 4cb26f9 monitor/hmp: move hmp_block_job* to block-hmp-cmds.c
> 97953d5 monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to 
> block-hmp-cmds.c
> 00dc3e8 monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c
> a4aa184 monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c
> d76374a monitor/hmp: uninline add_init_drive
> 13decc9 usb/dev-storage: remove unused include
> 
> === OUTPUT BEGIN ===
> 1/13 Checking commit 13decc9a539d (usb/dev-storage: remove unused include)
> 2/13 Checking commit d76374a8829d (monitor/hmp: uninline add_init_drive)
> 3/13 Checking commit a4aa1842d39b (monitor/hmp: rename device-hotplug.c to 
> block/monitor/block-hmp-cmds.c)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #58: 
> new file mode 100644
> 
> total: 0 errors, 1 warnings, 83 lines checked
> 
> Patch 3/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 4/13 Checking commit 00dc3e8c0cc5 (monitor/hmp: move hmp_drive_del and 
> hmp_commit to block-hmp-cmds.c)
> WARNING: Block comments use a leading /* on a separate line
> #81: FILE: block/monitor/block-hmp-cmds.c:119:
> +/* If this BlockBackend has a device attached to it, its refcount will be
> 
> total: 0 errors, 1 warnings, 234 lines checked
> 
> Patch 4/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 5/13 Checking commit 97953d583c25 (monitor/hmp: move hmp_drive_mirror and 
> hmp_drive_backup to block-hmp-cmds.c)
> 6/13 Checking commit 4cb26f9c9af1 (monitor/hmp: move hmp_block_job* to 
> block-hmp-cmds.c)
> 7/13 Checking commit f5fab9454aca (monitor/hmp: move hmp_snapshot_* to 
> block-hmp-cmds.c)
> WARNING: Block comments use a leading /* on a separate line
> #29: FILE: block/monitor/block-hmp-cmds.c:294:
> +/* In the future, if 'snapshot-file' is not specified, the snapshot
> 
> WARNING: Block comments use * on subsequent lines
> #30: FILE: block/monitor/block-hmp-cmds.c:295:
> +/* In the future, if 'snapshot-file' is not specified, the snapshot
> +   will be taken internally. Today it's actually required. */
> 
> WARNING: Block comments use a trailing */ on a separate line
> #30: FILE: block/monitor/block-hmp-cmds.c:295:
> +   will be taken internally. Today it's actually required. */
> 
> total: 0 errors, 3 warnings, 120 lines checked
> 
> Patch 7/13 has style problems, please review.  If any of these errors
> are false positives report them to the 

Re: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200127103647.17761-1-mlevi...@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20200127103647.17761-1-mlevi...@redhat.com
Subject: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  
patchew/1579779525-20065-1-git-send-email-imamm...@redhat.com -> 
patchew/1579779525-20065-1-git-send-email-imamm...@redhat.com
 - [tag update]  patchew/20200124100159.736209-1-stefa...@redhat.com -> 
patchew/20200124100159.736209-1-stefa...@redhat.com
 - [tag update]  patchew/20200124162606.8787-1-peter.mayd...@linaro.org -> 
patchew/20200124162606.8787-1-peter.mayd...@linaro.org
 - [tag update]  patchew/20200124172954.28481-1-peter.mayd...@linaro.org -> 
patchew/20200124172954.28481-1-peter.mayd...@linaro.org
 * [new tag] patchew/20200127103647.17761-1-mlevi...@redhat.com -> 
patchew/20200127103647.17761-1-mlevi...@redhat.com
Switched to a new branch 'test'
9d7e4e6 monitor/hmp: Prefer to use hmp_handle_error for error reporting in 
block hmp commands
a549971 add 'error' prefix to vreport
7c7da3a monitor: Move hmp_drive_add_node to block-hmp-cmds.c
a2a0265 monitor/hmp: move hmp_info_block* to block-hmp-cmds.c
d7f13de monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c
c067499 monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c
f5fab94 monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c
4cb26f9 monitor/hmp: move hmp_block_job* to block-hmp-cmds.c
97953d5 monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to 
block-hmp-cmds.c
00dc3e8 monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c
a4aa184 monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c
d76374a monitor/hmp: uninline add_init_drive
13decc9 usb/dev-storage: remove unused include

=== OUTPUT BEGIN ===
1/13 Checking commit 13decc9a539d (usb/dev-storage: remove unused include)
2/13 Checking commit d76374a8829d (monitor/hmp: uninline add_init_drive)
3/13 Checking commit a4aa1842d39b (monitor/hmp: rename device-hotplug.c to 
block/monitor/block-hmp-cmds.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#58: 
new file mode 100644

total: 0 errors, 1 warnings, 83 lines checked

Patch 3/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/13 Checking commit 00dc3e8c0cc5 (monitor/hmp: move hmp_drive_del and 
hmp_commit to block-hmp-cmds.c)
WARNING: Block comments use a leading /* on a separate line
#81: FILE: block/monitor/block-hmp-cmds.c:119:
+/* If this BlockBackend has a device attached to it, its refcount will be

total: 0 errors, 1 warnings, 234 lines checked

Patch 4/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/13 Checking commit 97953d583c25 (monitor/hmp: move hmp_drive_mirror and 
hmp_drive_backup to block-hmp-cmds.c)
6/13 Checking commit 4cb26f9c9af1 (monitor/hmp: move hmp_block_job* to 
block-hmp-cmds.c)
7/13 Checking commit f5fab9454aca (monitor/hmp: move hmp_snapshot_* to 
block-hmp-cmds.c)
WARNING: Block comments use a leading /* on a separate line
#29: FILE: block/monitor/block-hmp-cmds.c:294:
+/* In the future, if 'snapshot-file' is not specified, the snapshot

WARNING: Block comments use * on subsequent lines
#30: FILE: block/monitor/block-hmp-cmds.c:295:
+/* In the future, if 'snapshot-file' is not specified, the snapshot
+   will be taken internally. Today it's actually required. */

WARNING: Block comments use a trailing */ on a separate line
#30: FILE: block/monitor/block-hmp-cmds.c:295:
+   will be taken internally. Today it's actually required. */

total: 0 errors, 3 warnings, 120 lines checked

Patch 7/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/13 Checking commit c06749935535 (monitor/hmp: move hmp_nbd_server* to 
block-hmp-cmds.c)
WARNING: Block comments use a leading /* on a separate line
#60: FILE: block/monitor/block-hmp-cmds.c:363:
+/* Then try adding all block devices.  If one fails, close all and

total: 0 errors, 1 warnings, 217 lines checked

Patch 8/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/13 Checking commit d7f13ded16b4 (monitor/hmp: move remaining hmp_block* 
functions to block-hmp-cmds.c)

Re: [PATCH v3 01/13] usb/dev-storage: remove unused include

[Philippe Mathieu-Daudé]

On 1/27/20 11:36 AM, Maxim Levitsky wrote:

Signed-off-by: Maxim Levitsky 
---
  hw/usb/dev-storage.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 8545193488..50d12244ab 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -19,7 +19,6 @@
  #include "hw/scsi/scsi.h"
  #include "ui/console.h"
  #include "migration/vmstate.h"
-#include "monitor/monitor.h"
  #include "sysemu/sysemu.h"
  #include "sysemu/block-backend.h"
  #include "qapi/visitor.h"



Reviewed-by: Philippe Mathieu-Daudé 

[PATCH v3 11/13] monitor: Move hmp_drive_add_node to block-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 block/monitor/block-hmp-cmds.c | 30 
 blockdev.c | 42 +++---
 include/block/block_int.h  |  5 ++--
 3 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index a4b1604aee..7bbe4e3814 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -33,6 +33,36 @@
 #include "monitor/hmp.h"
 #include "qemu-io.h"
 
+static void hmp_drive_add_node(Monitor *mon, const char *optstr)
+{
+QemuOpts *opts;
+QDict *qdict;
+Error *local_err = NULL;
+
+opts = qemu_opts_parse_noisily(_drive_opts, optstr, false);
+if (!opts) {
+return;
+}
+
+qdict = qemu_opts_to_qdict(opts, NULL);
+
+if (!qdict_get_try_str(qdict, "node-name")) {
+qobject_unref(qdict);
+error_report("'node-name' needs to be specified");
+goto out;
+}
+
+BlockDriverState *bs = bds_tree_init(qdict, _err);
+if (!bs) {
+error_report_err(local_err);
+goto out;
+}
+
+bdrv_set_monitor_owned(bs);
+out:
+qemu_opts_del(opts);
+}
+
 void hmp_drive_add(Monitor *mon, const QDict *qdict)
 {
 Error *err = NULL;
diff --git a/blockdev.c b/blockdev.c
index df43e0aaef..63805f34b5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -64,7 +64,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/throttle-options.h"
 
-static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
+QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
 QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
 
 static int do_open_tray(const char *blk_name, const char *qdev_id,
@@ -75,6 +75,11 @@ static void blockdev_insert_medium(bool has_device, const 
char *device,
bool has_id, const char *id,
const char *node_name, Error **errp);
 
+void bdrv_set_monitor_owned(BlockDriverState *bs)
+{
+QTAILQ_INSERT_TAIL(_bdrv_states, bs, monitor_list);
+}
+
 static const char *const if_name[IF_COUNT] = {
 [IF_NONE] = "none",
 [IF_IDE] = "ide",
@@ -652,7 +657,7 @@ err_no_opts:
 }
 
 /* Takes the ownership of bs_opts */
-static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
+BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
 {
 int bdrv_flags = 0;
 
@@ -4201,37 +4206,6 @@ out:
 aio_context_release(aio_context);
 }
 
-void hmp_drive_add_node(Monitor *mon, const char *optstr)
-{
-QemuOpts *opts;
-QDict *qdict;
-Error *local_err = NULL;
-
-opts = qemu_opts_parse_noisily(_drive_opts, optstr, false);
-if (!opts) {
-return;
-}
-
-qdict = qemu_opts_to_qdict(opts, NULL);
-
-if (!qdict_get_try_str(qdict, "node-name")) {
-qobject_unref(qdict);
-error_report("'node-name' needs to be specified");
-goto out;
-}
-
-BlockDriverState *bs = bds_tree_init(qdict, _err);
-if (!bs) {
-error_report_err(local_err);
-goto out;
-}
-
-QTAILQ_INSERT_TAIL(_bdrv_states, bs, monitor_list);
-
-out:
-qemu_opts_del(opts);
-}
-
 void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 {
 BlockDriverState *bs;
@@ -4261,7 +4235,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
**errp)
 goto fail;
 }
 
-QTAILQ_INSERT_TAIL(_bdrv_states, bs, monitor_list);
+bdrv_set_monitor_owned(bs);
 
 fail:
 visit_free(v);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dd033d0b37..10df257a61 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1217,8 +1217,6 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 BlockCompletionFunc *cb, void *opaque,
 JobTxn *txn, Error **errp);
 
-void hmp_drive_add_node(Monitor *mon, const char *optstr);
-
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
   const char *child_name,
   const BdrvChildRole *child_role,
@@ -1320,4 +1318,7 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, 
uint64_t src_offset,
 
 int refresh_total_sectors(BlockDriverState *bs, int64_t hint);
 
+void bdrv_set_monitor_owned(BlockDriverState *bs);
+BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp);
+
 #endif /* BLOCK_INT_H */
-- 
2.17.2

[PATCH v3 13/13] monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands

[Maxim Levitsky]

This way they all will be prefixed with 'Error:' which some parsers
(e.g libvirt) need

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1719169

Signed-off-by: Maxim Levitsky 
---
 block/monitor/block-hmp-cmds.c | 35 --
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 7bbe4e3814..5b060d380d 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -84,7 +84,6 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
 mc = MACHINE_GET_CLASS(current_machine);
 dinfo = drive_new(opts, mc->block_default_type, );
 if (err) {
-error_report_err(err);
 qemu_opts_del(opts);
 goto err;
 }
@@ -98,7 +97,7 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
 monitor_printf(mon, "OK\n");
 break;
 default:
-monitor_printf(mon, "Can't hot-add drive to type %d\n", dinfo->type);
+error_setg(, "Can't hot-add drive to type %d", dinfo->type);
 goto err;
 }
 return;
@@ -109,6 +108,7 @@ err:
 monitor_remove_blk(blk);
 blk_unref(blk);
 }
+hmp_handle_error(mon, err);
 }
 
 void hmp_drive_del(Monitor *mon, const QDict *qdict)
@@ -130,14 +130,14 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
 
 blk = blk_by_name(id);
 if (!blk) {
-error_report("Device '%s' not found", id);
-return;
+error_setg(_err, "Device '%s' not found", id);
+goto err;
 }
 
 if (!blk_legacy_dinfo(blk)) {
-error_report("Deleting device added with blockdev-add"
- " is not supported");
-return;
+error_setg(_err,
+   "Deleting device added with blockdev-add is not supported");
+goto err;
 }
 
 aio_context = blk_get_aio_context(blk);
@@ -146,9 +146,8 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
 bs = blk_bs(blk);
 if (bs) {
 if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, _err)) {
-error_report_err(local_err);
 aio_context_release(aio_context);
-return;
+goto err;
 }
 
 blk_remove_bs(blk);
@@ -169,12 +168,15 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
 }
 
 aio_context_release(aio_context);
+err:
+hmp_handle_error(mon, local_err);
 }
 
 void hmp_commit(Monitor *mon, const QDict *qdict)
 {
 const char *device = qdict_get_str(qdict, "device");
 BlockBackend *blk;
+Error *local_err = NULL;
 int ret;
 
 if (!strcmp(device, "all")) {
@@ -185,12 +187,12 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
 
 blk = blk_by_name(device);
 if (!blk) {
-error_report("Device '%s' not found", device);
-return;
+error_setg(_err, "Device '%s' not found", device);
+goto err;
 }
 if (!blk_is_available(blk)) {
-error_report("Device '%s' has no medium", device);
-return;
+error_setg(_err, "Device '%s' has no medium", device);
+goto err;
 }
 
 bs = blk_bs(blk);
@@ -202,8 +204,13 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
 aio_context_release(aio_context);
 }
 if (ret < 0) {
-error_report("'commit' error for '%s': %s", device, strerror(-ret));
+error_setg(_err,
+   "'commit' error for '%s': %s", device, strerror(-ret));
+goto err;
 }
+return;
+err:
+hmp_handle_error(mon, local_err);
 }
 
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
-- 
2.17.2

[PATCH v3 08/13] monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 block/monitor/block-hmp-cmds.c | 88 ++
 include/block/block-hmp-commands.h |  5 ++
 include/monitor/hmp.h  |  4 --
 monitor/hmp-cmds.c | 87 -
 4 files changed, 93 insertions(+), 91 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 9aa94ea6e0..df0178d0f9 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -22,8 +22,10 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
+#include "qemu/sockets.h"
 #include "sysemu/sysemu.h"
 #include "monitor/monitor.h"
+#include "block/nbd.h"
 #include "block/block_int.h"
 #include "block/block-hmp-commands.h"
 #include "monitor/hmp.h"
@@ -327,3 +329,89 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, 
const QDict *qdict)
true, name, );
 hmp_handle_error(mon, err);
 }
+
+void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
+{
+const char *uri = qdict_get_str(qdict, "uri");
+bool writable = qdict_get_try_bool(qdict, "writable", false);
+bool all = qdict_get_try_bool(qdict, "all", false);
+Error *local_err = NULL;
+BlockInfoList *block_list, *info;
+SocketAddress *addr;
+
+if (writable && !all) {
+error_setg(_err, "-w only valid together with -a");
+goto exit;
+}
+
+/* First check if the address is valid and start the server.  */
+addr = socket_parse(uri, _err);
+if (local_err != NULL) {
+goto exit;
+}
+
+nbd_server_start(addr, NULL, NULL, _err);
+qapi_free_SocketAddress(addr);
+if (local_err != NULL) {
+goto exit;
+}
+
+if (!all) {
+return;
+}
+
+/* Then try adding all block devices.  If one fails, close all and
+ * exit.
+ */
+block_list = qmp_query_block(NULL);
+
+for (info = block_list; info; info = info->next) {
+if (!info->value->has_inserted) {
+continue;
+}
+
+qmp_nbd_server_add(info->value->device, false, NULL,
+   true, writable, false, NULL, _err);
+
+if (local_err != NULL) {
+qmp_nbd_server_stop(NULL);
+break;
+}
+}
+
+qapi_free_BlockInfoList(block_list);
+
+exit:
+hmp_handle_error(mon, local_err);
+}
+
+void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, "device");
+const char *name = qdict_get_try_str(qdict, "name");
+bool writable = qdict_get_try_bool(qdict, "writable", false);
+Error *local_err = NULL;
+
+qmp_nbd_server_add(device, !!name, name, true, writable,
+   false, NULL, _err);
+hmp_handle_error(mon, local_err);
+}
+
+void hmp_nbd_server_remove(Monitor *mon, const QDict *qdict)
+{
+const char *name = qdict_get_str(qdict, "name");
+bool force = qdict_get_try_bool(qdict, "force", false);
+Error *err = NULL;
+
+/* Rely on NBD_SERVER_REMOVE_MODE_SAFE being the default */
+qmp_nbd_server_remove(name, force, NBD_SERVER_REMOVE_MODE_HARD, );
+hmp_handle_error(mon, err);
+}
+
+void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
+{
+Error *err = NULL;
+
+qmp_nbd_server_stop();
+hmp_handle_error(mon, err);
+}
diff --git a/include/block/block-hmp-commands.h 
b/include/block/block-hmp-commands.h
index 3fc2daf3a9..721b9a1978 100644
--- a/include/block/block-hmp-commands.h
+++ b/include/block/block-hmp-commands.h
@@ -21,4 +21,9 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
 
+void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
+void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
+void hmp_nbd_server_remove(Monitor *mon, const QDict *qdict);
+void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
+
 #endif
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 6d34e29bb6..736a969131 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -94,10 +94,6 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
 void hmp_sendkey(Monitor *mon, const QDict *qdict);
 void hmp_screendump(Monitor *mon, const QDict *qdict);
-void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
-void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
-void hmp_nbd_server_remove(Monitor *mon, const QDict *qdict);
-void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
 void hmp_chardev_add(Monitor *mon, const QDict *qdict);
 void hmp_chardev_change(Monitor *mon, const QDict *qdict);
 void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 46b46b6dd7..67d2ca8a4c 100644
--- a/monitor/hmp-cmds.c

[PATCH v3 09/13] monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 block/monitor/block-hmp-cmds.c | 138 +
 include/block/block-hmp-commands.h |   9 ++
 include/monitor/hmp.h  |   6 --
 monitor/hmp-cmds.c | 137 
 4 files changed, 147 insertions(+), 143 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index df0178d0f9..60d63bfe18 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -29,6 +29,7 @@
 #include "block/block_int.h"
 #include "block/block-hmp-commands.h"
 #include "monitor/hmp.h"
+#include "qemu-io.h"
 
 void hmp_drive_add(Monitor *mon, const QDict *qdict)
 {
@@ -415,3 +416,140 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
 qmp_nbd_server_stop();
 hmp_handle_error(mon, err);
 }
+
+void hmp_block_resize(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, "device");
+int64_t size = qdict_get_int(qdict, "size");
+Error *err = NULL;
+
+qmp_block_resize(true, device, false, NULL, size, );
+hmp_handle_error(mon, err);
+}
+
+void hmp_block_stream(Monitor *mon, const QDict *qdict)
+{
+Error *error = NULL;
+const char *device = qdict_get_str(qdict, "device");
+const char *base = qdict_get_try_str(qdict, "base");
+int64_t speed = qdict_get_try_int(qdict, "speed", 0);
+
+qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
+ false, NULL, qdict_haskey(qdict, "speed"), speed, true,
+ BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
+ );
+
+hmp_handle_error(mon, error);
+}
+
+void hmp_block_passwd(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, "device");
+const char *password = qdict_get_str(qdict, "password");
+Error *err = NULL;
+
+qmp_block_passwd(true, device, false, NULL, password, );
+hmp_handle_error(mon, err);
+}
+
+void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
+{
+Error *err = NULL;
+char *device = (char *) qdict_get_str(qdict, "device");
+BlockIOThrottle throttle = {
+.bps = qdict_get_int(qdict, "bps"),
+.bps_rd = qdict_get_int(qdict, "bps_rd"),
+.bps_wr = qdict_get_int(qdict, "bps_wr"),
+.iops = qdict_get_int(qdict, "iops"),
+.iops_rd = qdict_get_int(qdict, "iops_rd"),
+.iops_wr = qdict_get_int(qdict, "iops_wr"),
+};
+
+/* qmp_block_set_io_throttle has separate parameters for the
+ * (deprecated) block device name and the qdev ID but the HMP
+ * version has only one, so we must decide which one to pass. */
+if (blk_by_name(device)) {
+throttle.has_device = true;
+throttle.device = device;
+} else {
+throttle.has_id = true;
+throttle.id = device;
+}
+
+qmp_block_set_io_throttle(, );
+hmp_handle_error(mon, err);
+}
+
+void hmp_eject(Monitor *mon, const QDict *qdict)
+{
+bool force = qdict_get_try_bool(qdict, "force", false);
+const char *device = qdict_get_str(qdict, "device");
+Error *err = NULL;
+
+qmp_eject(true, device, false, NULL, true, force, );
+hmp_handle_error(mon, err);
+}
+
+void hmp_qemu_io(Monitor *mon, const QDict *qdict)
+{
+BlockBackend *blk;
+BlockBackend *local_blk = NULL;
+bool qdev = qdict_get_try_bool(qdict, "qdev", false);
+const char* device = qdict_get_str(qdict, "device");
+const char* command = qdict_get_str(qdict, "command");
+Error *err = NULL;
+int ret;
+
+if (qdev) {
+blk = blk_by_qdev_id(device, );
+if (!blk) {
+goto fail;
+}
+} else {
+blk = blk_by_name(device);
+if (!blk) {
+BlockDriverState *bs = bdrv_lookup_bs(NULL, device, );
+if (bs) {
+blk = local_blk = blk_new(bdrv_get_aio_context(bs),
+  0, BLK_PERM_ALL);
+ret = blk_insert_bs(blk, bs, );
+if (ret < 0) {
+goto fail;
+}
+} else {
+goto fail;
+}
+}
+}
+
+/*
+ * Notably absent: Proper permission management. This is sad, but it seems
+ * almost impossible to achieve without changing the semantics and thereby
+ * limiting the use cases of the qemu-io HMP command.
+ *
+ * In an ideal world we would unconditionally create a new BlockBackend for
+ * qemuio_command(), but we have commands like 'reopen' and want them to
+ * take effect on the exact BlockBackend whose name the user passed instead
+ * of just on a temporary copy of it.
+ *
+ * Another problem is that deleting the temporary BlockBackend involves
+ * draining all requests on it first, but some qemu-iotests cases want to
+ * issue multiple aio_read/write requests and expect them to complete in
+ * the 

[PATCH v3 10/13] monitor/hmp: move hmp_info_block* to block-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 block/monitor/block-hmp-cmds.c | 388 +
 include/block/block-hmp-commands.h |   4 +
 include/monitor/hmp.h  |   4 -
 monitor/hmp-cmds.c | 388 -
 4 files changed, 392 insertions(+), 392 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 60d63bfe18..a4b1604aee 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -23,11 +23,13 @@
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "qemu/sockets.h"
+#include "qemu/cutils.h"
 #include "sysemu/sysemu.h"
 #include "monitor/monitor.h"
 #include "block/nbd.h"
 #include "block/block_int.h"
 #include "block/block-hmp-commands.h"
+#include "block/qapi.h"
 #include "monitor/hmp.h"
 #include "qemu-io.h"
 
@@ -553,3 +555,389 @@ fail:
 blk_unref(local_blk);
 hmp_handle_error(mon, err);
 }
+
+static void print_block_info(Monitor *mon, BlockInfo *info,
+ BlockDeviceInfo *inserted, bool verbose)
+{
+ImageInfo *image_info;
+
+assert(!info || !info->has_inserted || info->inserted == inserted);
+
+if (info && *info->device) {
+monitor_printf(mon, "%s", info->device);
+if (inserted && inserted->has_node_name) {
+monitor_printf(mon, " (%s)", inserted->node_name);
+}
+} else {
+assert(info || inserted);
+monitor_printf(mon, "%s",
+   inserted && inserted->has_node_name ? 
inserted->node_name
+   : info && info->has_qdev ? info->qdev
+   : "");
+}
+
+if (inserted) {
+monitor_printf(mon, ": %s (%s%s%s)\n",
+   inserted->file,
+   inserted->drv,
+   inserted->ro ? ", read-only" : "",
+   inserted->encrypted ? ", encrypted" : "");
+} else {
+monitor_printf(mon, ": [not inserted]\n");
+}
+
+if (info) {
+if (info->has_qdev) {
+monitor_printf(mon, "Attached to:  %s\n", info->qdev);
+}
+if (info->has_io_status && info->io_status != 
BLOCK_DEVICE_IO_STATUS_OK) {
+monitor_printf(mon, "I/O status:   %s\n",
+   BlockDeviceIoStatus_str(info->io_status));
+}
+
+if (info->removable) {
+monitor_printf(mon, "Removable device: %slocked, tray %s\n",
+   info->locked ? "" : "not ",
+   info->tray_open ? "open" : "closed");
+}
+}
+
+
+if (!inserted) {
+return;
+}
+
+monitor_printf(mon, "Cache mode:   %s%s%s\n",
+   inserted->cache->writeback ? "writeback" : "writethrough",
+   inserted->cache->direct ? ", direct" : "",
+   inserted->cache->no_flush ? ", ignore flushes" : "");
+
+if (inserted->has_backing_file) {
+monitor_printf(mon,
+   "Backing file: %s "
+   "(chain depth: %" PRId64 ")\n",
+   inserted->backing_file,
+   inserted->backing_file_depth);
+}
+
+if (inserted->detect_zeroes != BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF) {
+monitor_printf(mon, "Detect zeroes:%s\n",
+BlockdevDetectZeroesOptions_str(inserted->detect_zeroes));
+}
+
+if (inserted->bps  || inserted->bps_rd  || inserted->bps_wr  ||
+inserted->iops || inserted->iops_rd || inserted->iops_wr)
+{
+monitor_printf(mon, "I/O throttling:   bps=%" PRId64
+" bps_rd=%" PRId64  " bps_wr=%" PRId64
+" bps_max=%" PRId64
+" bps_rd_max=%" PRId64
+" bps_wr_max=%" PRId64
+" iops=%" PRId64 " iops_rd=%" PRId64
+" iops_wr=%" PRId64
+" iops_max=%" PRId64
+" iops_rd_max=%" PRId64
+" iops_wr_max=%" PRId64
+" iops_size=%" PRId64
+" group=%s\n",
+inserted->bps,
+inserted->bps_rd,
+inserted->bps_wr,
+inserted->bps_max,
+inserted->bps_rd_max,
+inserted->bps_wr_max,
+inserted->iops,
+inserted->iops_rd,
+inserted->iops_wr,
+inserted->iops_max,
+inserted->iops_rd_max,
+inserted->iops_wr_max,
+inserted->iops_size,
+inserted->group);
+}
+
+if (verbose) {
+monitor_printf(mon, "\nImages:\n");
+image_info = inserted->image;
+while (1) {
+  

[PATCH v3 04/13] monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 block/monitor/block-hmp-cmds.c | 97 +-
 blockdev.c | 95 -
 include/block/block-hmp-commands.h |  3 +
 include/sysemu/blockdev.h  |  4 --
 4 files changed, 99 insertions(+), 100 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index c65aaa86ea..9614c67e77 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -12,6 +12,7 @@
 #include "hw/boards.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
+#include "qapi/qapi-commands-block.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/error.h"
 #include "qemu/config-file.h"
@@ -21,7 +22,6 @@
 #include "block/block_int.h"
 #include "block/block-hmp-commands.h"
 
-
 void hmp_drive_add(Monitor *mon, const QDict *qdict)
 {
 Error *err = NULL;
@@ -69,3 +69,98 @@ err:
 blk_unref(blk);
 }
 }
+
+void hmp_drive_del(Monitor *mon, const QDict *qdict)
+{
+const char *id = qdict_get_str(qdict, "id");
+BlockBackend *blk;
+BlockDriverState *bs;
+AioContext *aio_context;
+Error *local_err = NULL;
+
+bs = bdrv_find_node(id);
+if (bs) {
+qmp_blockdev_del(id, _err);
+if (local_err) {
+error_report_err(local_err);
+}
+return;
+}
+
+blk = blk_by_name(id);
+if (!blk) {
+error_report("Device '%s' not found", id);
+return;
+}
+
+if (!blk_legacy_dinfo(blk)) {
+error_report("Deleting device added with blockdev-add"
+ " is not supported");
+return;
+}
+
+aio_context = blk_get_aio_context(blk);
+aio_context_acquire(aio_context);
+
+bs = blk_bs(blk);
+if (bs) {
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, _err)) {
+error_report_err(local_err);
+aio_context_release(aio_context);
+return;
+}
+
+blk_remove_bs(blk);
+}
+
+/* Make the BlockBackend and the attached BlockDriverState anonymous */
+monitor_remove_blk(blk);
+
+/* If this BlockBackend has a device attached to it, its refcount will be
+ * decremented when the device is removed; otherwise we have to do so here.
+ */
+if (blk_get_attached_dev(blk)) {
+/* Further I/O must not pause the guest */
+blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT,
+ BLOCKDEV_ON_ERROR_REPORT);
+} else {
+blk_unref(blk);
+}
+
+aio_context_release(aio_context);
+}
+
+void hmp_commit(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, "device");
+BlockBackend *blk;
+int ret;
+
+if (!strcmp(device, "all")) {
+ret = blk_commit_all();
+} else {
+BlockDriverState *bs;
+AioContext *aio_context;
+
+blk = blk_by_name(device);
+if (!blk) {
+error_report("Device '%s' not found", device);
+return;
+}
+if (!blk_is_available(blk)) {
+error_report("Device '%s' has no medium", device);
+return;
+}
+
+bs = blk_bs(blk);
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+
+ret = bdrv_commit(bs);
+
+aio_context_release(aio_context);
+}
+if (ret < 0) {
+error_report("'commit' error for '%s': %s", device, strerror(-ret));
+}
+}
diff --git a/blockdev.c b/blockdev.c
index 8e029e9c01..df43e0aaef 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1074,41 +1074,6 @@ static BlockBackend *qmp_get_blk(const char *blk_name, 
const char *qdev_id,
 return blk;
 }
 
-void hmp_commit(Monitor *mon, const QDict *qdict)
-{
-const char *device = qdict_get_str(qdict, "device");
-BlockBackend *blk;
-int ret;
-
-if (!strcmp(device, "all")) {
-ret = blk_commit_all();
-} else {
-BlockDriverState *bs;
-AioContext *aio_context;
-
-blk = blk_by_name(device);
-if (!blk) {
-error_report("Device '%s' not found", device);
-return;
-}
-if (!blk_is_available(blk)) {
-error_report("Device '%s' has no medium", device);
-return;
-}
-
-bs = blk_bs(blk);
-aio_context = bdrv_get_aio_context(bs);
-aio_context_acquire(aio_context);
-
-ret = bdrv_commit(bs);
-
-aio_context_release(aio_context);
-}
-if (ret < 0) {
-error_report("'commit' error for '%s': %s", device, strerror(-ret));
-}
-}
-
 static void blockdev_do_action(TransactionAction *action, Error **errp)
 {
 TransactionActionList list;
@@ -3101,66 +3066,6 @@ BlockDirtyBitmapSha256 
*qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
 return ret;
 }
 
-void hmp_drive_del(Monitor *mon, const QDict *qdict)
-{
-const char *id = qdict_get_str(qdict, "id");
-BlockBackend *blk;
-

[PATCH v3 05/13] monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to block-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 block/monitor/block-hmp-cmds.c | 64 ++
 include/block/block-hmp-commands.h |  3 ++
 include/monitor/hmp.h  |  2 -
 monitor/hmp-cmds.c | 58 ---
 4 files changed, 67 insertions(+), 60 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 9614c67e77..ae3890aaab 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -2,6 +2,10 @@
  * Blockdev HMP commands
  *
  * Copyright (c) 2004 Fabrice Bellard
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   
  *
  * This work is licensed under the terms of the GNU GPL, version 2.
  * or (at your option) any later version.
@@ -15,12 +19,14 @@
 #include "qapi/qapi-commands-block.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "sysemu/sysemu.h"
 #include "monitor/monitor.h"
 #include "block/block_int.h"
 #include "block/block-hmp-commands.h"
+#include "monitor/hmp.h"
 
 void hmp_drive_add(Monitor *mon, const QDict *qdict)
 {
@@ -164,3 +170,61 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
 error_report("'commit' error for '%s': %s", device, strerror(-ret));
 }
 }
+
+void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
+{
+const char *filename = qdict_get_str(qdict, "target");
+const char *format = qdict_get_try_str(qdict, "format");
+bool reuse = qdict_get_try_bool(qdict, "reuse", false);
+bool full = qdict_get_try_bool(qdict, "full", false);
+Error *err = NULL;
+DriveMirror mirror = {
+.device = (char *)qdict_get_str(qdict, "device"),
+.target = (char *)filename,
+.has_format = !!format,
+.format = (char *)format,
+.sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
+.has_mode = true,
+.mode = reuse ? NEW_IMAGE_MODE_EXISTING : 
NEW_IMAGE_MODE_ABSOLUTE_PATHS,
+.unmap = true,
+};
+
+if (!filename) {
+error_setg(, QERR_MISSING_PARAMETER, "target");
+hmp_handle_error(mon, err);
+return;
+}
+qmp_drive_mirror(, );
+hmp_handle_error(mon, err);
+}
+
+void hmp_drive_backup(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, "device");
+const char *filename = qdict_get_str(qdict, "target");
+const char *format = qdict_get_try_str(qdict, "format");
+bool reuse = qdict_get_try_bool(qdict, "reuse", false);
+bool full = qdict_get_try_bool(qdict, "full", false);
+bool compress = qdict_get_try_bool(qdict, "compress", false);
+Error *err = NULL;
+DriveBackup backup = {
+.device = (char *)device,
+.target = (char *)filename,
+.has_format = !!format,
+.format = (char *)format,
+.sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
+.has_mode = true,
+.mode = reuse ? NEW_IMAGE_MODE_EXISTING : 
NEW_IMAGE_MODE_ABSOLUTE_PATHS,
+.has_compress = !!compress,
+.compress = compress,
+};
+
+if (!filename) {
+error_setg(, QERR_MISSING_PARAMETER, "target");
+hmp_handle_error(mon, err);
+return;
+}
+
+qmp_drive_backup(, );
+hmp_handle_error(mon, err);
+}
diff --git a/include/block/block-hmp-commands.h 
b/include/block/block-hmp-commands.h
index c5e394c0fc..fcaf753118 100644
--- a/include/block/block-hmp-commands.h
+++ b/include/block/block-hmp-commands.h
@@ -8,4 +8,7 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict);
 void hmp_commit(Monitor *mon, const QDict *qdict);
 void hmp_drive_del(Monitor *mon, const QDict *qdict);
 
+void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
+void hmp_drive_backup(Monitor *mon, const QDict *qdict);
+
 #endif
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 3d329853b2..c1b363ee57 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -64,8 +64,6 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
-void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
-void hmp_drive_backup(Monitor *mon, const QDict *qdict);
 void hmp_loadvm(Monitor *mon, const QDict *qdict);
 void hmp_savevm(Monitor *mon, const QDict *qdict);
 void hmp_delvm(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index d0e0af893a..a70bcb1d16 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1337,64 +1337,6 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict)
 hmp_handle_error(mon, err);
 }
 
-void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
-{
-const char *filename = qdict_get_str(qdict, "target");
-const char *format = 

[PATCH v3 07/13] monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 block/monitor/block-hmp-cmds.c | 47 ++
 include/block/block-hmp-commands.h |  4 +++
 include/monitor/hmp.h  |  3 --
 monitor/hmp-cmds.c | 47 --
 4 files changed, 51 insertions(+), 50 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index ed3c350143..9aa94ea6e0 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -280,3 +280,50 @@ void hmp_block_job_complete(Monitor *mon, const QDict 
*qdict)
 
 hmp_handle_error(mon, error);
 }
+
+void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, "device");
+const char *filename = qdict_get_try_str(qdict, "snapshot-file");
+const char *format = qdict_get_try_str(qdict, "format");
+bool reuse = qdict_get_try_bool(qdict, "reuse", false);
+enum NewImageMode mode;
+Error *err = NULL;
+
+if (!filename) {
+/* In the future, if 'snapshot-file' is not specified, the snapshot
+   will be taken internally. Today it's actually required. */
+error_setg(, QERR_MISSING_PARAMETER, "snapshot-file");
+hmp_handle_error(mon, err);
+return;
+}
+
+mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+qmp_blockdev_snapshot_sync(true, device, false, NULL,
+   filename, false, NULL,
+   !!format, format,
+   true, mode, );
+hmp_handle_error(mon, err);
+}
+
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, "device");
+const char *name = qdict_get_str(qdict, "name");
+Error *err = NULL;
+
+qmp_blockdev_snapshot_internal_sync(device, name, );
+hmp_handle_error(mon, err);
+}
+
+void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+const char *device = qdict_get_str(qdict, "device");
+const char *name = qdict_get_str(qdict, "name");
+const char *id = qdict_get_try_str(qdict, "id");
+Error *err = NULL;
+
+qmp_blockdev_snapshot_delete_internal_sync(device, !!id, id,
+   true, name, );
+hmp_handle_error(mon, err);
+}
diff --git a/include/block/block-hmp-commands.h 
b/include/block/block-hmp-commands.h
index ea6578a5f6..3fc2daf3a9 100644
--- a/include/block/block-hmp-commands.h
+++ b/include/block/block-hmp-commands.h
@@ -17,4 +17,8 @@ void hmp_block_job_pause(Monitor *mon, const QDict *qdict);
 void hmp_block_job_resume(Monitor *mon, const QDict *qdict);
 void hmp_block_job_complete(Monitor *mon, const QDict *qdict);
 
+void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
+
 #endif
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 592ce0ccfe..6d34e29bb6 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -61,9 +61,6 @@ void hmp_set_link(Monitor *mon, const QDict *qdict);
 void hmp_block_passwd(Monitor *mon, const QDict *qdict);
 void hmp_balloon(Monitor *mon, const QDict *qdict);
 void hmp_block_resize(Monitor *mon, const QDict *qdict);
-void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
-void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
-void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
 void hmp_loadvm(Monitor *mon, const QDict *qdict);
 void hmp_savevm(Monitor *mon, const QDict *qdict);
 void hmp_delvm(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 996ce96430..46b46b6dd7 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1337,53 +1337,6 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict)
 hmp_handle_error(mon, err);
 }
 
-void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
-{
-const char *device = qdict_get_str(qdict, "device");
-const char *filename = qdict_get_try_str(qdict, "snapshot-file");
-const char *format = qdict_get_try_str(qdict, "format");
-bool reuse = qdict_get_try_bool(qdict, "reuse", false);
-enum NewImageMode mode;
-Error *err = NULL;
-
-if (!filename) {
-/* In the future, if 'snapshot-file' is not specified, the snapshot
-   will be taken internally. Today it's actually required. */
-error_setg(, QERR_MISSING_PARAMETER, "snapshot-file");
-hmp_handle_error(mon, err);
-return;
-}
-
-mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-qmp_blockdev_snapshot_sync(true, device, false, NULL,
-   filename, false, NULL,
-   !!format, format,
-   true, mode, );
-hmp_handle_error(mon, err);
-}

[PATCH v3 06/13] monitor/hmp: move hmp_block_job* to block-hmp-cmds.c

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 block/monitor/block-hmp-cmds.c | 52 ++
 include/block/block-hmp-commands.h |  6 
 include/monitor/hmp.h  |  5 ---
 monitor/hmp-cmds.c | 52 --
 4 files changed, 58 insertions(+), 57 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index ae3890aaab..ed3c350143 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -228,3 +228,55 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
 qmp_drive_backup(, );
 hmp_handle_error(mon, err);
 }
+
+void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
+{
+Error *error = NULL;
+const char *device = qdict_get_str(qdict, "device");
+int64_t value = qdict_get_int(qdict, "speed");
+
+qmp_block_job_set_speed(device, value, );
+
+hmp_handle_error(mon, error);
+}
+
+void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
+{
+Error *error = NULL;
+const char *device = qdict_get_str(qdict, "device");
+bool force = qdict_get_try_bool(qdict, "force", false);
+
+qmp_block_job_cancel(device, true, force, );
+
+hmp_handle_error(mon, error);
+}
+
+void hmp_block_job_pause(Monitor *mon, const QDict *qdict)
+{
+Error *error = NULL;
+const char *device = qdict_get_str(qdict, "device");
+
+qmp_block_job_pause(device, );
+
+hmp_handle_error(mon, error);
+}
+
+void hmp_block_job_resume(Monitor *mon, const QDict *qdict)
+{
+Error *error = NULL;
+const char *device = qdict_get_str(qdict, "device");
+
+qmp_block_job_resume(device, );
+
+hmp_handle_error(mon, error);
+}
+
+void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
+{
+Error *error = NULL;
+const char *device = qdict_get_str(qdict, "device");
+
+qmp_block_job_complete(device, );
+
+hmp_handle_error(mon, error);
+}
diff --git a/include/block/block-hmp-commands.h 
b/include/block/block-hmp-commands.h
index fcaf753118..ea6578a5f6 100644
--- a/include/block/block-hmp-commands.h
+++ b/include/block/block-hmp-commands.h
@@ -11,4 +11,10 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict);
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
 void hmp_drive_backup(Monitor *mon, const QDict *qdict);
 
+void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
+void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
+void hmp_block_job_pause(Monitor *mon, const QDict *qdict);
+void hmp_block_job_resume(Monitor *mon, const QDict *qdict);
+void hmp_block_job_complete(Monitor *mon, const QDict *qdict);
+
 #endif
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index c1b363ee57..592ce0ccfe 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -87,11 +87,6 @@ void hmp_eject(Monitor *mon, const QDict *qdict);
 void hmp_change(Monitor *mon, const QDict *qdict);
 void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
 void hmp_block_stream(Monitor *mon, const QDict *qdict);
-void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
-void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
-void hmp_block_job_pause(Monitor *mon, const QDict *qdict);
-void hmp_block_job_resume(Monitor *mon, const QDict *qdict);
-void hmp_block_job_complete(Monitor *mon, const QDict *qdict);
 void hmp_migrate(Monitor *mon, const QDict *qdict);
 void hmp_device_add(Monitor *mon, const QDict *qdict);
 void hmp_device_del(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a70bcb1d16..996ce96430 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1975,58 +1975,6 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
 hmp_handle_error(mon, error);
 }
 
-void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
-{
-Error *error = NULL;
-const char *device = qdict_get_str(qdict, "device");
-int64_t value = qdict_get_int(qdict, "speed");
-
-qmp_block_job_set_speed(device, value, );
-
-hmp_handle_error(mon, error);
-}
-
-void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
-{
-Error *error = NULL;
-const char *device = qdict_get_str(qdict, "device");
-bool force = qdict_get_try_bool(qdict, "force", false);
-
-qmp_block_job_cancel(device, true, force, );
-
-hmp_handle_error(mon, error);
-}
-
-void hmp_block_job_pause(Monitor *mon, const QDict *qdict)
-{
-Error *error = NULL;
-const char *device = qdict_get_str(qdict, "device");
-
-qmp_block_job_pause(device, );
-
-hmp_handle_error(mon, error);
-}
-
-void hmp_block_job_resume(Monitor *mon, const QDict *qdict)
-{
-Error *error = NULL;
-const char *device = qdict_get_str(qdict, "device");
-
-qmp_block_job_resume(device, );
-
-hmp_handle_error(mon, error);
-}
-
-void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
-{
-Error *error = NULL;
-const char *device = qdict_get_str(qdict, "device");
-
-

[PATCH v3 02/13] monitor/hmp: uninline add_init_drive

[Maxim Levitsky]

This is only used by hmp_drive_add.
The code is just a bit shorter this way.

No functional changes

Signed-off-by: Maxim Levitsky 
Reviewed-by: Markus Armbruster 
---
 device-hotplug.c | 31 ---
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/device-hotplug.c b/device-hotplug.c
index f01d53774b..554e4d98db 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -34,42 +34,35 @@
 #include "monitor/monitor.h"
 #include "block/block_int.h"
 
-static DriveInfo *add_init_drive(const char *optstr)
+
+void hmp_drive_add(Monitor *mon, const QDict *qdict)
 {
 Error *err = NULL;
 DriveInfo *dinfo;
 QemuOpts *opts;
 MachineClass *mc;
+const char *optstr = qdict_get_str(qdict, "opts");
+bool node = qdict_get_try_bool(qdict, "node", false);
+
+if (node) {
+hmp_drive_add_node(mon, optstr);
+return;
+}
 
 opts = drive_def(optstr);
 if (!opts)
-return NULL;
+return;
 
 mc = MACHINE_GET_CLASS(current_machine);
 dinfo = drive_new(opts, mc->block_default_type, );
 if (err) {
 error_report_err(err);
 qemu_opts_del(opts);
-return NULL;
-}
-
-return dinfo;
-}
-
-void hmp_drive_add(Monitor *mon, const QDict *qdict)
-{
-DriveInfo *dinfo = NULL;
-const char *opts = qdict_get_str(qdict, "opts");
-bool node = qdict_get_try_bool(qdict, "node", false);
-
-if (node) {
-hmp_drive_add_node(mon, opts);
-return;
+goto err;
 }
 
-dinfo = add_init_drive(opts);
 if (!dinfo) {
-goto err;
+return;
 }
 
 switch (dinfo->type) {
-- 
2.17.2

[PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

[Maxim Levitsky]

This patch series is bunch of cleanups
to the hmp monitor code.

This series only touched blockdev related hmp handlers.

No functional changes expected other that
light error message changes by the last patch.

This was inspired by this bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1719169

Basically some users still parse hmp error messages,
and they would like to have them prefixed with 'Error:'

In commit 66363e9a43f649360a3f74d2805c9f864da027eb we added
the hmp_handle_error which does exactl that but some hmp handlers
don't use it.

In this patch series, I moved all the block related hmp handlers
into blockdev-hmp-cmds.c, and then made them use this function
to report the errors.

I hope I didn't change too much code, I just felt that if
I touch this code, I can also make it easier to find these
handlers, that were scattered over 3 different files.

Changes from V1:
   * move the handlers to block/monitor/block-hmp-cmds.c
   * tiny cleanup for the commit messages

Changes from V2:
   * Moved all the function prototypes to new header (blockdev-hmp-cmds.h)
   * Set the license of blockdev-hmp-cmds.c to GPLv2+
   * Moved hmp_snapshot_* functions to blockdev-hmp-cmds.c
   * Moved hmp_drive_add_node to blockdev-hmp-cmds.c
 (this change needed some new exports, thus in separate new patch)
   * Moved hmp_qemu_io and hmp_eject to blockdev-hmp-cmds.c
   * Added 'error:' prefix to vreport, and updated the iotests
 This is invasive change, but really feels like the right one
   * Added minor refactoring patch that drops an unused #include

Best regards,
Maxim Levitsky

Maxim Levitsky (13):
  usb/dev-storage: remove unused include
  monitor/hmp: uninline add_init_drive
  monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c
  monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c
  monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to
block-hmp-cmds.c
  monitor/hmp: move hmp_block_job* to block-hmp-cmds.c
  monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c
  monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c
  monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c
  monitor/hmp: move hmp_info_block* to block-hmp-cmds.c
  monitor: Move hmp_drive_add_node to block-hmp-cmds.c
  add 'error' prefix to vreport
  monitor/hmp: Prefer to use hmp_handle_error for error reporting in
block hmp commands

 MAINTAINERS|   1 +
 Makefile.objs  |   2 +-
 block/Makefile.objs|   1 +
 block/monitor/Makefile.objs|   1 +
 block/monitor/block-hmp-cmds.c | 980 +
 blockdev.c | 137 +---
 device-hotplug.c   |  91 ---
 hw/usb/dev-storage.c   |   1 -
 include/block/block-hmp-commands.h |  42 ++
 include/block/block_int.h  |   5 +-
 include/monitor/hmp.h  |  24 -
 include/sysemu/blockdev.h  |   4 -
 include/sysemu/sysemu.h|   3 -
 monitor/hmp-cmds.c | 771 +--
 monitor/misc.c |   1 +
 tests/qemu-iotests/020.out |   2 +-
 tests/qemu-iotests/026.out | 260 
 tests/qemu-iotests/036.out |  16 +-
 tests/qemu-iotests/043.out |   6 +-
 tests/qemu-iotests/049.out |  30 +-
 tests/qemu-iotests/051.pc.out  | 150 ++---
 tests/qemu-iotests/054.out |   4 +-
 tests/qemu-iotests/060.out |  20 +-
 tests/qemu-iotests/061.out |  26 +-
 tests/qemu-iotests/069.out |   2 +-
 tests/qemu-iotests/071.out |   4 +-
 tests/qemu-iotests/074.out |   4 +-
 tests/qemu-iotests/079.out |   2 +-
 tests/qemu-iotests/080.out |  72 +--
 tests/qemu-iotests/081.out |   2 +-
 tests/qemu-iotests/082.out |  38 +-
 tests/qemu-iotests/083.out |  68 +-
 tests/qemu-iotests/098.out |   8 +-
 tests/qemu-iotests/103.out |  14 +-
 tests/qemu-iotests/106.out |   4 +-
 tests/qemu-iotests/111.out |   2 +-
 tests/qemu-iotests/112.out |  24 +-
 tests/qemu-iotests/113.out |   6 +-
 tests/qemu-iotests/114.out |   2 +-
 tests/qemu-iotests/122.out |   4 +-
 tests/qemu-iotests/133.out |  30 +-
 tests/qemu-iotests/137.out |  28 +-
 tests/qemu-iotests/140.out |   2 +-
 tests/qemu-iotests/142.out |  38 +-
 tests/qemu-iotests/143.out |   2 +-
 tests/qemu-iotests/153.out | 118 ++--
 tests/qemu-iotests/162.out |  10 +-
 tests/qemu-iotests/172.out |  16 +-
 tests/qemu-iotests/178.out.qcow2   |  30 +-
 tests/qemu-iotests/178.out.raw |  26 +-
 tests/qemu-iotests/182.out |   2 +-
 tests/qemu-iotests/187.out |   6 +-
 tests/qemu-iotests/188.out |   2 +-
 tests/qemu-iotests/197.out |   2 +-
 tests/qemu-iotests/205 |   2 +-
 tests/qemu-iotests/215.out |   2 +-
 

[PATCH v3 03/13] monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c

[Maxim Levitsky]

These days device-hotplug.c only contains the hmp_drive_add
In the next patch, rest of hmp_drive* functions will be moved
there.

Also change the license of that file to GPL2+ since most
of the code that will be moved there is under that license

Also add block-hmp-commands.h to contain prototypes of these
functions

Signed-off-by: Maxim Levitsky 
---
 MAINTAINERS   |  1 +
 Makefile.objs |  2 +-
 block/Makefile.objs   |  1 +
 block/monitor/Makefile.objs   |  1 +
 .../monitor/block-hmp-cmds.c  | 23 ---
 include/block/block-hmp-commands.h|  8 +++
 include/sysemu/sysemu.h   |  3 ---
 monitor/misc.c|  1 +
 8 files changed, 18 insertions(+), 22 deletions(-)
 create mode 100644 block/monitor/Makefile.objs
 rename device-hotplug.c => block/monitor/block-hmp-cmds.c (55%)
 create mode 100644 include/block/block-hmp-commands.h

diff --git a/MAINTAINERS b/MAINTAINERS
index f6511d5120..5d50d09ad8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1882,6 +1882,7 @@ Block QAPI, monitor, command line
 M: Markus Armbruster 
 S: Supported
 F: blockdev.c
+F: blockdev-hmp-cmds.c
 F: block/qapi.c
 F: qapi/block*.json
 F: qapi/transaction.json
diff --git a/Makefile.objs b/Makefile.objs
index ff396b9209..15209eb6b5 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -48,7 +48,7 @@ common-obj-y += dump/
 common-obj-y += job-qmp.o
 common-obj-y += monitor/
 common-obj-y += net/
-common-obj-y += qdev-monitor.o device-hotplug.o
+common-obj-y += qdev-monitor.o
 common-obj-$(CONFIG_WIN32) += os-win32.o
 common-obj-$(CONFIG_POSIX) += os-posix.o
 
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 330529b0b7..3f65544a6b 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -44,6 +44,7 @@ block-obj-y += crypto.o
 block-obj-y += aio_task.o
 block-obj-y += backup-top.o
 block-obj-y += filter-compress.o
+common-obj-y += monitor/
 
 common-obj-y += stream.o
 
diff --git a/block/monitor/Makefile.objs b/block/monitor/Makefile.objs
new file mode 100644
index 00..0a74f9a8b5
--- /dev/null
+++ b/block/monitor/Makefile.objs
@@ -0,0 +1 @@
+common-obj-y += block-hmp-cmds.o
diff --git a/device-hotplug.c b/block/monitor/block-hmp-cmds.c
similarity index 55%
rename from device-hotplug.c
rename to block/monitor/block-hmp-cmds.c
index 554e4d98db..c65aaa86ea 100644
--- a/device-hotplug.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -1,25 +1,11 @@
 /*
- * QEMU device hotplug helpers
+ * Blockdev HMP commands
  *
  * Copyright (c) 2004 Fabrice Bellard
  *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to 
deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * or (at your option) any later version.
+ * See the COPYING file in the top-level directory.
  */
 
 #include "qemu/osdep.h"
@@ -33,6 +19,7 @@
 #include "sysemu/sysemu.h"
 #include "monitor/monitor.h"
 #include "block/block_int.h"
+#include "block/block-hmp-commands.h"
 
 
 void hmp_drive_add(Monitor *mon, const QDict *qdict)
diff --git a/include/block/block-hmp-commands.h 
b/include/block/block-hmp-commands.h
new file mode 100644
index 00..4f9033a8a6
--- /dev/null
+++ b/include/block/block-hmp-commands.h
@@ -0,0 +1,8 @@
+#ifndef BLOCK_HMP_COMMANDS_H
+#define BLOCK_HMP_COMMANDS_H
+
+/* HMP commands related to the block layer*/
+
+void hmp_drive_add(Monitor *mon, const QDict *qdict);
+
+#endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 80c57fdc4e..c48635666d 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -68,9 +68,6 @@ extern int nb_option_roms;
 extern const char *prom_envs[MAX_PROM_ENVS];
 extern unsigned int nb_prom_envs;
 
-/* generic hotplug */
-void hmp_drive_add(Monitor *mon, const QDict *qdict);
-
 /* pcie aer error injection */
 void hmp_pcie_aer_inject_error(Monitor *mon, const QDict 

[PATCH v3 01/13] usb/dev-storage: remove unused include

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky 
---
 hw/usb/dev-storage.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 8545193488..50d12244ab 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -19,7 +19,6 @@
 #include "hw/scsi/scsi.h"
 #include "ui/console.h"
 #include "migration/vmstate.h"
-#include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/block-backend.h"
 #include "qapi/visitor.h"
-- 
2.17.2

Re: [PATCH v2 0/4] virtio-pci: enable blk and scsi multi-queue by default

[Stefano Garzarella]

On Fri, Jan 24, 2020 at 10:01:55AM +, Stefan Hajnoczi wrote:
> v2:
>  * Let the virtio-DEVICE-pci device select num-queues because the optimal
>multi-queue configuration may differ between virtio-pci, virtio-mmio, and
>virtio-ccw [Cornelia]
> 
> Enabling multi-queue on virtio-pci storage devices improves performance on SMP
> guests because the completion interrupt is handled on the vCPU that submitted
> the I/O request.  This avoids IPIs inside the guest.
> 
> Note that performance is unchanged in these cases:
> 1. Uniprocessor guests.  They don't have IPIs.
> 2. Application threads might be scheduled on the sole vCPU that handles
>completion interrupts purely by chance.  (This is one reason why benchmark
>results can vary noticably between runs.)
> 3. Users may bind the application to the vCPU that handles completion
>interrupts.
> 
> Set the number of queues to the number of vCPUs by default.  Older machine
> types continue to default to 1 queue for live migration compatibility.
> 
> This patch improves IOPS by 1-4% on an Intel Optane SSD with 4 vCPUs, -drive
> aio=native, and fio bs=4k direct=1 rw=randread.
> 
> Stefan Hajnoczi (4):
>   virtio-scsi: introduce a constant for fixed virtqueues
>   virtio-scsi: default num_queues to -smp N
>   virtio-blk: default num_queues to -smp N
>   vhost-user-blk: default num_queues to -smp N

The series looks good to me:

Reviewed-by: Stefano Garzarella 

Thanks,
Stefano

Re: [PATCH v2 2/8] hxtool: Support SRST/ERST directives

[Philippe Mathieu-Daudé]

On 1/24/20 5:26 PM, Peter Maydell wrote:

We want to add support for including rST document fragments
in our .hx files, in the same way we currently have texinfo
fragments. These will be delimited by SRST and ERST directives,
in the same way the texinfo is delimited by STEXI/ETEXI.
The rST fragments will not be extracted by the hxtool
script, but by a different mechanism, so all we need to
do in hxtool is have it ignore all the text inside a
SRST/ERST section, with suitable error-checking for
mismatched rST-vs-texi fragment delimiters.

The resulting effective state machine has only three states:
  * flag = 0, rstflag = 0 : reading section for C output
  * flag = 1, rstflag = 0 : reading texi fragment
  * flag = 0, rstflag = 1 : reading rST fragment
and flag = 1, rstflag = 1 is not possible. Using two
variables makes the parallel between the rST handling and
the texi handling clearer; in any case all this code will
be deleted once we've converted entirely to rST.

Signed-off-by: Peter Maydell 
Reviewed-by: Richard Henderson 
---
  scripts/hxtool | 33 -
  1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/scripts/hxtool b/scripts/hxtool
index 7d7c4289e32..0003e7b673d 100644
--- a/scripts/hxtool
+++ b/scripts/hxtool
@@ -7,7 +7,7 @@ hxtoh()
  case $str in
  HXCOMM*)
  ;;
-STEXI*|ETEXI*) flag=$(($flag^1))
+STEXI*|ETEXI*|SRST*|ERST*) flag=$(($flag^1))
  ;;
  *)
  test $flag -eq 1 && printf "%s\n" "$str"
@@ -27,12 +27,17 @@ print_texi_heading()
  hxtotexi()
  {
  flag=0
+rstflag=0
  line=1
  while read -r str; do
  case "$str" in
  HXCOMM*)
  ;;
  STEXI*)
+if test $rstflag -eq 1 ; then
+printf "line %d: syntax error: expected ERST, found '%s'\n" "$line" 
"$str" >&2
+exit 1
+fi
  if test $flag -eq 1 ; then
  printf "line %d: syntax error: expected ETEXI, found '%s'\n" "$line" 
"$str" >&2
  exit 1
@@ -40,12 +45,38 @@ hxtotexi()
  flag=1
  ;;
  ETEXI*)
+if test $rstflag -eq 1 ; then
+printf "line %d: syntax error: expected ERST, found '%s'\n" "$line" 
"$str" >&2
+exit 1
+fi
  if test $flag -ne 1 ; then
  printf "line %d: syntax error: expected STEXI, found '%s'\n" "$line" 
"$str" >&2
  exit 1
  fi
  flag=0
  ;;
+SRST*)
+if test $rstflag -eq 1 ; then
+printf "line %d: syntax error: expected ERST, found '%s'\n" "$line" 
"$str" >&2
+exit 1
+fi
+if test $flag -eq 1 ; then
+printf "line %d: syntax error: expected ETEXI, found '%s'\n" "$line" 
"$str" >&2
+exit 1
+fi
+rstflag=1
+;;
+ERST*)
+if test $flag -eq 1 ; then
+printf "line %d: syntax error: expected ETEXI, found '%s'\n" "$line" 
"$str" >&2
+exit 1
+fi
+if test $rstflag -ne 1 ; then
+printf "line %d: syntax error: expected SRST, found '%s'\n" "$line" 
"$str" >&2
+exit 1
+fi
+rstflag=0
+;;
  DEFHEADING*)
  print_texi_heading "$(expr "$str" : "DEFHEADING(\(.*\))")"
  ;;



Reviewed-by: Philippe Mathieu-Daudé