Re: [Qemu-block] [Qemu-devel] [PATCH 0/9] IDE: replace printfs with tracing

[no-reply]

Hi,

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

Message-id: 20170808183306.27474-1-js...@redhat.com
Subject: [Qemu-devel] [PATCH 0/9] IDE: replace printfs with tracing
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
56439344ef AHCI: remove DPRINTF macro
6ad85afba9 AHCI: pretty-print FIS to buffer instead of stderr
dfcd58be6d AHCI: Rework IRQ constants
77feb08d1d AHCI: Replace DPRINTF with trace-events
5657a4914e IDE: replace DEBUG_AIO with trace events
7708ba0158 ATAPI: Replace DEBUG_IDE_ATAPI with tracing events
c007da81b2 IDE: add tracing for data ports
07cc43a5ff IDE: Add register hints to tracing
a5a7b507be IDE: replace DEBUG_IDE with tracing system

=== OUTPUT BEGIN ===
Checking PATCH 1/9: IDE: replace DEBUG_IDE with tracing system...
ERROR: spaces required around that '|' (ctx:VxV)
#139: FILE: hw/ide/core.c:1189:
+if (reg_num != 7 && (s->status & (BUSY_STAT|DRQ_STAT))) {
^

ERROR: space required before the open parenthesis '('
#144: FILE: hw/ide/core.c:1193:
+switch(reg_num) {

ERROR: space required before the open parenthesis '('
#191: FILE: hw/ide/core.c:2058:
+switch(reg_num) {

total: 3 errors, 0 warnings, 335 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/9: IDE: Add register hints to tracing...
Checking PATCH 3/9: IDE: add tracing for data ports...
Checking PATCH 4/9: ATAPI: Replace DEBUG_IDE_ATAPI with tracing events...
Checking PATCH 5/9: IDE: replace DEBUG_AIO with trace events...
Checking PATCH 6/9: AHCI: Replace DPRINTF with trace-events...
ERROR: Hex numbers must be prefixed with '0x'
#543: FILE: hw/ide/trace-events:88:
+handle_reg_h2d_fis_pmp(void *s, int port, char b0, char b1, char b2) 
"ahci(%p)[%d]: Port Multiplier not supported, FIS: 0x%02x-%02x-%02x"

ERROR: Hex numbers must be prefixed with '0x'
#544: FILE: hw/ide/trace-events:89:
+handle_reg_h2d_fis_res(void *s, int port, char b0, char b1, char b2) 
"ahci(%p)[%d]: Reserved flags set in H2D Register FIS, FIS: 0x%02x-%02x-%02x"

ERROR: Hex numbers must be prefixed with '0x'
#550: FILE: hw/ide/trace-events:95:
+handle_cmd_unhandled_fis(void *s, int port, uint8_t b0, uint8_t b1, uint8_t 
b2) "ahci(%p)[%d]: unhandled FIS type. cmd_fis: 0x%02x-%02x-%02x"

total: 3 errors, 0 warnings, 500 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 7/9: AHCI: Rework IRQ constants...
Checking PATCH 8/9: AHCI: pretty-print FIS to buffer instead of stderr...
ERROR: Use g_assert or g_assert_not_reached
#35: FILE: hw/ide/ahci.c:658:
+g_assert_cmpint(cmd_len, <=, 0x100);

total: 1 errors, 0 warnings, 85 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 9/9: AHCI: remove DPRINTF macro...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-block] [Qemu-devel] [PATCH v2 10/13] vvfat: correctly generate numeric-tail of short file names

[Pranith Kumar]

On Mon, Aug 7, 2017 at 7:07 AM, Eric Blake  wrote:
> On 08/05/2017 01:52 PM, Pranith Kumar wrote:
>> FYI,
>>
>> This commit breaks the build with gcc-7:
>>
>>   CC  block/vvfat.o
>> qemu/block/vvfat.c: In function ‘read_directory’:
>> qemu/block/vvfat.c:605:37: error: ‘__builtin___sprintf_chk’ may write
>> a terminating nul past the end of the destination
>> [-Werror=format-overflow=]
>>  int len = sprintf(tail, "~%d", i);
>>  ^
>
> Doesn't commit 7c8730d45f6 fix that?

Indeed it does. I hadn't rebased my branch before reporting. Sorry for
the noise!

-- 
Pranith

Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[Philippe Mathieu-Daudé]

On 08/08/2017 05:00 PM, Eric Blake wrote:

On 08/08/2017 01:32 PM, John Snow wrote:

Out with the old, in with the new.

Signed-off-by: John Snow 
---



  hw/ide/piix.c | 11 
  hw/ide/trace-events   | 33 
  hw/ide/via.c  | 10 +++-


Hmm - should we tweak scripts/git.orderfile to prioritize trace-events
over .c files? Then again, right now it prioritizes all .c files before
anything that didn't match, so that things like trace-events will at
least avoid falling in the middle of a patch if you use the project's
orderfile.


It sounds like a good idea, although I'd rather prioritize .c, having 
trace-events at bottom. At least we can agree about top-to-bottom 
scripting here :)

Re: [Qemu-block] [Qemu-devel] [PATCH 4/9] ATAPI: Replace DEBUG_IDE_ATAPI with tracing events

[Eric Blake]

On 08/08/2017 01:33 PM, John Snow wrote:
> Goodbye, printfs.
> Hello, fancy printfs.
> 
> Signed-off-by: John Snow 
> ---
>  hw/ide/atapi.c| 64 
> +--
>  hw/ide/trace-events   | 19 ++
>  include/hw/ide/internal.h |  1 -
>  3 files changed, 42 insertions(+), 42 deletions(-)

> @@ -1330,16 +1310,18 @@ void ide_atapi_cmd(IDEState *s)
>  uint8_t *buf = s->io_buffer;
>  const struct AtapiCmd *cmd = _cmd_table[s->io_buffer[0]];
>  
> -#ifdef DEBUG_IDE_ATAPI
> -{
> +trace_ide_atapi_cmd(s, s->io_buffer[0]);
> +
> +if (TRACE_IDE_ATAPI_CMD_PACKET_ENABLED) {
> +/* Each pretty-printed byte needs two bytes and a space; */
> +char *ppacket = g_malloc(ATAPI_PACKET_SIZE * 3 + 1);

Nice use of an extra conditional to make a pretty trace without the
overhead when tracing is off.  (Oh yeah, you asked me on IRC how to do
it, and I pointed to commit bd6952a3 as the example)


> +++ b/hw/ide/trace-events
> @@ -6,6 +6,10 @@ ide_ioport_read(uint32_t addr, const char *reg, uint32_t 
> val, void *bus, void *s
>  ide_ioport_write(uint32_t addr, const char *reg, uint32_t val, void *bus, 
> void *s) "IDE PIO wr @ 0x%"PRIx32" (%s); val 0x%02"PRIx32"; bus %p IDEState 
> %p"

Wouldn't it be nice if our trace generator would let us line-wrap?

> +# Warning: Verbose

Cute.  Should you also add it to the ide_data_* traces, per the commit
message in 3/9?

> +ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) "IDEState: 
> %p; limit=0x%x packet: %s"
> \ No newline at end of file

Umm, that should be fixed (I know that 'diff' uses both / and \ to
differentiate whether it was the pre- or post-patch version that had the
issue, but never remember which is which - so the fix may be in an
earlier patch).  Also fix the trace-events rebase for 3/9; with that,
you can add

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4] tests: Avoid non-portable 'echo -ARG'

[Eric Blake]

On 08/08/2017 09:48 AM, Kevin Wolf wrote:

>>> Not completely sure why, but this broke the test with whitespace changes
>>> like this:
>>>
>>> -=== Running test case: mmap.elf -m 1.1M ===
>>> +=== Running test case: mmap.elf -m1.1M ===
>>
>> I guess that means I'm not regularly running tests/multiboot?  Is it not
>> part of 'make check' or qemu-iotests?
> 
> The problem is that it needs an i386 compiler to build the test kernels
> (and qemu-system-i386 or qemu-system-x86_64 binaries to execute them).
> 
> I guess we could check these conditions, though, and skip the test if we
> can't produce i386 binaries.

And when you CAN run the test, it litters 'git status' of an in-tree
build with:

Untracked files:
  (use "git add ..." to include in what will be committed)

tests/multiboot/mmap.elf
tests/multiboot/modules.elf
tests/multiboot/test.out

which ought to be fixed.

> 
>> Ah, I see the problem, and it's insidious.  We're using "...$@...", but
>> want to be using "...$*...".  $@ causes multiple arguments to be passed,
>> but printf %b is not concatenating those arguments; while $* uses only a
>> single argument.  We didn't notice it with echo -e, because echo inserts
>> a space between multiple arguments, just as you'd get a space with $*.
> 
> The thing that completely confused me here is that printf doesn't just
> ignore additional arguments as I would have expected, but just starts
> over with the format string, so that it does kind of work with multiple
> arguments and fails only subtly.

Both echo and printf accept more than one argument, but only echo
injects an automatic space between those arguments in the output.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 3/9] IDE: add tracing for data ports

[John Snow]

On 08/08/2017 04:30 PM, Eric Blake wrote:
> On 08/08/2017 01:33 PM, John Snow wrote:
>> To be used sparingly, but still interesting in the case of small
>> firmwares designed to reproduce bugs in QEMU IDE.
> 
> Is that because the trace would fire so frequently in normal usage that
> it will drown the user in noise?
> 

Yeah, and it's of little use for a real guest due to the volume and
relative unimportance of what that data actually is

>>
>> Signed-off-by: John Snow 
>> ---
>>  hw/ide/core.c | 12 +++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
> 
> Reviewed-by: Eric Blake 
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 3/9] IDE: add tracing for data ports

[Eric Blake]

On 08/08/2017 03:30 PM, Eric Blake wrote:
> On 08/08/2017 01:33 PM, John Snow wrote:
>> To be used sparingly, but still interesting in the case of small
>> firmwares designed to reproduce bugs in QEMU IDE.
> 
> Is that because the trace would fire so frequently in normal usage that
> it will drown the user in noise?
> 
>>
>> Signed-off-by: John Snow 
>> ---
>>  hw/ide/core.c | 12 +++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
> 
> Reviewed-by: Eric Blake 

Argh. I had two compose windows open at once. R-b is only valid if you
fix the time-traveling trace-events changes :)

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 3/9] IDE: add tracing for data ports

[Eric Blake]

On 08/08/2017 01:33 PM, John Snow wrote:
> To be used sparingly, but still interesting in the case of small
> firmwares designed to reproduce bugs in QEMU IDE.

Is that because the trace would fire so frequently in normal usage that
it will drown the user in noise?

> 
> Signed-off-by: John Snow 
> ---
>  hw/ide/core.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 3/9] IDE: add tracing for data ports

[John Snow]

On 08/08/2017 04:10 PM, Eric Blake wrote:
> On 08/08/2017 01:33 PM, John Snow wrote:
>> To be used sparingly, but still interesting in the case of small
>> firmwares designed to reproduce bugs in QEMU IDE.
>>
>> Signed-off-by: John Snow 
>> ---
>>  hw/ide/core.c | 12 +++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 6235bdf..29848ff 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -2251,6 +2251,8 @@ void ide_data_writew(void *opaque, uint32_t addr, 
>> uint32_t val)
>>  IDEState *s = idebus_active_if(bus);
>>  uint8_t *p;
>>  
>> +trace_ide_data_writew(addr, val, bus, s);
> 
> Umm, where's the trace-events addition for this?
> 

Accidentally traveled forward through time to patch 04.

Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[John Snow]

On 08/08/2017 04:00 PM, Eric Blake wrote:
> On 08/08/2017 01:32 PM, John Snow wrote:
>> Out with the old, in with the new.
>>
>> Signed-off-by: John Snow 
>> ---
> 
>>  hw/ide/piix.c | 11 
>>  hw/ide/trace-events   | 33 
>>  hw/ide/via.c  | 10 +++-
> 
> Hmm - should we tweak scripts/git.orderfile to prioritize trace-events
> over .c files? Then again, right now it prioritizes all .c files before
> anything that didn't match, so that things like trace-events will at
> least avoid falling in the middle of a patch if you use the project's
> orderfile.
> 

Sorry!

>> +++ b/hw/ide/cmd646.c
>> @@ -32,6 +32,7 @@
>>  #include "sysemu/dma.h"
>>  
>>  #include "hw/ide/pci.h"
>> +#include "trace.h"
>>  
>>  /* CMD646 specific */
>>  #define CFR 0x50
>> @@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
>>  val = 0xff;
>>  break;
>>  }
>> -#ifdef DEBUG_IDE
>> -printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val);
>> -#endif
> 
> Yay for killing code prone to bitrot.
> 

Yup, seeya later.

>> +++ b/hw/ide/core.c
> 
>> @@ -2054,18 +2044,18 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>  }
> 
>>  hob = 0;
>> -switch(addr) {
>> +switch(reg_num) {
> 
> Worth fixing the style to put space after switch while touching this?
> 

Yes.

>> +++ b/hw/ide/trace-events
>> @@ -0,0 +1,33 @@
>> +# See docs/devel/tracing.txt for syntax documentation.
>> +
>> +# hw/ide/core.c
> 
>> +
>> +# hw/ide/pci.c
>> +bmdma_reset(void) ""
> 
> An empty trace? Do all the backends support it?
> 

Oh, I don't know... I guess I can just repeat the function name in here,
or add something arbitrary.

>> +# hw/ide/cmd646.c
> 
> Worth sorting the sections by filename?
> 

Oh, you mean alphabetically?

I'd like to keep the BMDMA HBA tracers near each other, but otherwise I
can, yes.

> Whether or not you tweak based on my nits,
> 
> Reviewed-by: Eric Blake 
> 

There's likely to be many nits, so I'll accept all the critique.

John

Re: [Qemu-block] [Qemu-devel] [PATCH 3/9] IDE: add tracing for data ports

[Eric Blake]

On 08/08/2017 01:33 PM, John Snow wrote:
> To be used sparingly, but still interesting in the case of small
> firmwares designed to reproduce bugs in QEMU IDE.
> 
> Signed-off-by: John Snow 
> ---
>  hw/ide/core.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 6235bdf..29848ff 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2251,6 +2251,8 @@ void ide_data_writew(void *opaque, uint32_t addr, 
> uint32_t val)
>  IDEState *s = idebus_active_if(bus);
>  uint8_t *p;
>  
> +trace_ide_data_writew(addr, val, bus, s);

Umm, where's the trace-events addition for this?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 2/9] IDE: Add register hints to tracing

[Eric Blake]

On 08/08/2017 01:32 PM, John Snow wrote:
> Name the registers for tracing purposes.
> 
> Signed-off-by: John Snow 
> ---
>  hw/ide/core.c   | 88 
> +
>  hw/ide/trace-events |  4 +--
>  2 files changed, 70 insertions(+), 22 deletions(-)

> -case 1:
> - ide_clear_hob(bus);
> +case ATA_IOPORT_WR_FEATURES:
> +ide_clear_hob(bus);

Cool, fixing tab damage in the process.

I did a similar improvement to NBD traces, and it's definitely worth it
to have traces include names in addition to numbers.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[Eric Blake]

On 08/08/2017 01:32 PM, John Snow wrote:
> Out with the old, in with the new.
> 
> Signed-off-by: John Snow 
> ---

>  hw/ide/piix.c | 11 
>  hw/ide/trace-events   | 33 
>  hw/ide/via.c  | 10 +++-

Hmm - should we tweak scripts/git.orderfile to prioritize trace-events
over .c files? Then again, right now it prioritizes all .c files before
anything that didn't match, so that things like trace-events will at
least avoid falling in the middle of a patch if you use the project's
orderfile.

> +++ b/hw/ide/cmd646.c
> @@ -32,6 +32,7 @@
>  #include "sysemu/dma.h"
>  
>  #include "hw/ide/pci.h"
> +#include "trace.h"
>  
>  /* CMD646 specific */
>  #define CFR  0x50
> @@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
>  val = 0xff;
>  break;
>  }
> -#ifdef DEBUG_IDE
> -printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val);
> -#endif

Yay for killing code prone to bitrot.

> +++ b/hw/ide/core.c

> @@ -2054,18 +2044,18 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>  }

>  hob = 0;
> -switch(addr) {
> +switch(reg_num) {

Worth fixing the style to put space after switch while touching this?

> +++ b/hw/ide/trace-events
> @@ -0,0 +1,33 @@
> +# See docs/devel/tracing.txt for syntax documentation.
> +
> +# hw/ide/core.c

> +
> +# hw/ide/pci.c
> +bmdma_reset(void) ""

An empty trace? Do all the backends support it?

> +# hw/ide/cmd646.c

Worth sorting the sections by filename?

Whether or not you tweak based on my nits,

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 2/4] IDE: test flush on empty CDROM

[John Snow]

On 08/08/2017 03:20 PM, Eric Blake wrote:
> On 08/08/2017 12:57 PM, John Snow wrote:
>> From: Kevin Wolf 
>>
>> Signed-off-by: Kevin Wolf 
>> Signed-off-by: John Snow 
>> ---
>>  tests/ide-test.c | 19 +++
>>  1 file changed, 19 insertions(+)
>>
> 
>> +static void test_flush_empty_drive(void)
>> +{
>> +QPCIDevice *dev;
>> +QPCIBar bmdma_bar, ide_bar;
>> +
>> +ide_test_start("-device ide-cd,bus=ide.0");
>> +dev = get_pci_device(_bar, _bar);
>> +
>> +/* FLUSH CACHE command on device 0*/
> 
> Space before */
> 
> Reviewed-by: Eric Blake 
> 
> I agree with your assessment of 1 and 2 being 2.10 material.
> 

Yep, thanks. I just wanted to include Kevin's attempt at fixing the root
problem to make it clear that:

(A) The root problem is known and being worked on, but
(B) Is evidently not ready for prime time.

I'll stage 1 & 2 with your minor typo edit here, thank you.

--js

Re: [Qemu-block] [Qemu-devel] [PATCH 1/4] IDE: Do not flush empty CDROM drives

[Eric Blake]

On 08/08/2017 12:57 PM, John Snow wrote:
> The block backend changed in a way that flushing empty CDROM drives
> is now an error. Amend IDE to avoid doing so until the root problem
> can be addressed for 2.11.
> 
> Reported-by: Kieron Shorrock 
> Signed-off-by: John Snow 
> ---
>  hw/ide/core.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS

[John Snow]

On 08/08/2017 02:34 PM, Paolo Bonzini wrote:
> 
> 
> - Original Message -
>> From: "John Snow" 
>> To: qemu-block@nongnu.org
>> Cc: kw...@redhat.com, qemu-de...@nongnu.org, dgilb...@redhat.com, 
>> stefa...@redhat.com, pbonz...@redhat.com,
>> p...@redhat.com, "John Snow" 
>> Sent: Tuesday, August 8, 2017 7:57:10 PM
>> Subject: [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS
>>
>> From: Kevin Wolf 
>>
>> This allows us to detect errors in cache flushing (ENOMEDIUM)
>> without choking on a null dereference because we assume that
>> blk_bs(bb) is always defined.
>>
>> Signed-off-by: Kevin Wolf 
>> Signed-off-by: John Snow 
> 
> This is not enough, as discussed in the thread.
> 
> Paolo
> 

Sure, I amended Kevin's later fix and rolled it into one patch and split
the tests out. The cover letter states that this is busted, but I wanted
it on the list instead of buried in a now-unrelated thread.

So now it's here as a patch, can we keep discussion here instead of on
the other thread?

--John

Re: [Qemu-block] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS

[Paolo Bonzini]

- Original Message -
> From: "John Snow" 
> To: qemu-block@nongnu.org
> Cc: kw...@redhat.com, qemu-de...@nongnu.org, dgilb...@redhat.com, 
> stefa...@redhat.com, pbonz...@redhat.com,
> p...@redhat.com, "John Snow" 
> Sent: Tuesday, August 8, 2017 7:57:10 PM
> Subject: [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS
> 
> From: Kevin Wolf 
> 
> This allows us to detect errors in cache flushing (ENOMEDIUM)
> without choking on a null dereference because we assume that
> blk_bs(bb) is always defined.
> 
> Signed-off-by: Kevin Wolf 
> Signed-off-by: John Snow 

This is not enough, as discussed in the thread.

Paolo

> ---
>  block.c   |  2 +-
>  block/block-backend.c | 40 ++--
>  2 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ce9cce7..834b836 100644
> --- a/block.c
> +++ b/block.c
> @@ -4476,7 +4476,7 @@ out:
>  
>  AioContext *bdrv_get_aio_context(BlockDriverState *bs)
>  {
> -return bs->aio_context;
> +return bs ? bs->aio_context : qemu_get_aio_context();
>  }
>  
>  void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co)
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 968438c..efd7e92 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -68,6 +68,9 @@ struct BlockBackend {
>  NotifierList remove_bs_notifiers, insert_bs_notifiers;
>  
>  int quiesce_counter;
> +
> +/* Number of in-flight requests. Accessed with atomic ops. */
> +unsigned int in_flight;
>  };
>  
>  typedef struct BlockBackendAIOCB {
> @@ -1109,6 +1112,16 @@ int blk_make_zero(BlockBackend *blk, BdrvRequestFlags
> flags)
>  return bdrv_make_zero(blk->root, flags);
>  }
>  
> +static void blk_inc_in_flight(BlockBackend *blk)
> +{
> +atomic_inc(>in_flight);
> +}
> +
> +static void blk_dec_in_flight(BlockBackend *blk)
> +{
> +atomic_dec(>in_flight);
> +}
> +
>  static void error_callback_bh(void *opaque)
>  {
>  struct BlockBackendAIOCB *acb = opaque;
> @@ -1147,7 +1160,7 @@ static const AIOCBInfo blk_aio_em_aiocb_info = {
>  static void blk_aio_complete(BlkAioEmAIOCB *acb)
>  {
>  if (acb->has_returned) {
> -bdrv_dec_in_flight(acb->common.bs);
> +blk_dec_in_flight(acb->rwco.blk);
>  acb->common.cb(acb->common.opaque, acb->rwco.ret);
>  qemu_aio_unref(acb);
>  }
> @@ -1168,7 +1181,7 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk,
> int64_t offset, int bytes,
>  BlkAioEmAIOCB *acb;
>  Coroutine *co;
>  
> -bdrv_inc_in_flight(blk_bs(blk));
> +blk_inc_in_flight(blk);
>  acb = blk_aio_get(_aio_em_aiocb_info, blk, cb, opaque);
>  acb->rwco = (BlkRwCo) {
>  .blk= blk,
> @@ -1405,14 +1418,28 @@ int blk_flush(BlockBackend *blk)
>  
>  void blk_drain(BlockBackend *blk)
>  {
> -if (blk_bs(blk)) {
> -bdrv_drain(blk_bs(blk));
> +AioContext *ctx = blk_get_aio_context(blk);
> +
> +while (atomic_read(>in_flight)) {
> +aio_context_acquire(ctx);
> +aio_poll(ctx, false);
> +aio_context_release(ctx);
> +
> +if (blk_bs(blk)) {
> +bdrv_drain(blk_bs(blk));
> +}
>  }
>  }
>  
>  void blk_drain_all(void)
>  {
> -bdrv_drain_all();
> +BlockBackend *blk = NULL;
> +
> +bdrv_drain_all_begin();
> +while ((blk = blk_all_next(blk)) != NULL) {
> +blk_drain(blk);
> +}
> +bdrv_drain_all_end();
>  }
>  
>  void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
> @@ -1453,10 +1480,11 @@ static void send_qmp_error_event(BlockBackend *blk,
>   bool is_read, int error)
>  {
>  IoOperationType optype;
> +BlockDriverState *bs = blk_bs(blk);
>  
>  optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
>  qapi_event_send_block_io_error(blk_name(blk),
> -   bdrv_get_node_name(blk_bs(blk)), optype,
> +   bs ? bdrv_get_node_name(bs) : "", optype,
> action, blk_iostatus_is_enabled(blk),
> error == ENOSPC, strerror(error),
> _abort);
> --
> 2.9.4
> 
> 

[Qemu-block] [PATCH 6/9] AHCI: Replace DPRINTF with trace-events

[John Snow]

There are a few hangers-on that will be dealt with individually
in forthcoming patches.

Signed-off-by: John Snow 
---
 hw/ide/ahci.c   | 157 +++-
 hw/ide/trace-events |  52 -
 2 files changed, 119 insertions(+), 90 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 406a1b5..d5acceb 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -35,6 +35,7 @@
 #include "hw/ide/ahci_internal.h"
 
 #define DEBUG_AHCI 0
+#include "trace.h"
 
 #define DPRINTF(port, fmt, ...) \
 do { \
@@ -114,9 +115,9 @@ static uint32_t  ahci_port_read(AHCIState *s, int port, int 
offset)
 default:
 val = 0;
 }
-DPRINTF(port, "offset: 0x%x val: 0x%x\n", offset, val);
-return val;
 
+trace_ahci_port_read(s, port, offset, val);
+return val;
 }
 
 static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev)
@@ -125,7 +126,7 @@ static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev)
 PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state),
TYPE_PCI_DEVICE);
 
-DPRINTF(0, "raise irq\n");
+trace_ahci_irq_raise(s);
 
 if (pci_dev && msi_enabled(pci_dev)) {
 msi_notify(pci_dev, 0);
@@ -140,7 +141,7 @@ static void ahci_irq_lower(AHCIState *s, AHCIDevice *dev)
 PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state),
TYPE_PCI_DEVICE);
 
-DPRINTF(0, "lower irq\n");
+trace_ahci_irq_lower(s);
 
 if (!pci_dev || !msi_enabled(pci_dev)) {
 qemu_irq_lower(s->irq);
@@ -150,8 +151,7 @@ static void ahci_irq_lower(AHCIState *s, AHCIDevice *dev)
 static void ahci_check_irq(AHCIState *s)
 {
 int i;
-
-DPRINTF(-1, "check irq %#x\n", s->control_regs.irqstatus);
+uint32_t old_irq = s->control_regs.irqstatus;
 
 s->control_regs.irqstatus = 0;
 for (i = 0; i < s->ports; i++) {
@@ -160,7 +160,7 @@ static void ahci_check_irq(AHCIState *s)
 s->control_regs.irqstatus |= (1 << i);
 }
 }
-
+trace_ahci_check_irq(s, old_irq, s->control_regs.irqstatus);
 if (s->control_regs.irqstatus &&
 (s->control_regs.ghc & HOST_CTL_IRQ_EN)) {
 ahci_irq_raise(s, NULL);
@@ -240,7 +240,7 @@ static void  ahci_port_write(AHCIState *s, int port, int 
offset, uint32_t val)
 {
 AHCIPortRegs *pr = >dev[port].port_regs;
 
-DPRINTF(port, "offset: 0x%x val: 0x%x\n", offset, val);
+trace_ahci_port_write(s, port, offset, val);
 switch (offset) {
 case PORT_LST_ADDR:
 pr->lst_addr = val;
@@ -341,8 +341,6 @@ static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr)
 val = s->control_regs.version;
 break;
 }
-
-DPRINTF(-1, "(addr 0x%08X), val 0x%08X\n", (unsigned) addr, val);
 } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
(addr < (AHCI_PORT_REGS_START_ADDR +
 (s->ports * AHCI_PORT_ADDR_OFFSET_LEN {
@@ -350,6 +348,7 @@ static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr)
  addr & AHCI_PORT_ADDR_OFFSET_MASK);
 }
 
+trace_ahci_mem_read_32(s, addr, val);
 return val;
 }
 
@@ -379,8 +378,7 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr, 
unsigned size)
 val = (hi << 32 | lo) >> (ofst * 8);
 }
 
-DPRINTF(-1, "addr=0x%" HWADDR_PRIx " val=0x%" PRIx64 ", size=%d\n",
-addr, val, size);
+trace_ahci_mem_read(opaque, size, addr, val);
 return val;
 }
 
@@ -390,8 +388,7 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
 {
 AHCIState *s = opaque;
 
-DPRINTF(-1, "addr=0x%" HWADDR_PRIx " val=0x%" PRIx64 ", size=%d\n",
-addr, val, size);
+trace_ahci_mem_write(s, size, addr, val);
 
 /* Only aligned reads are allowed on AHCI */
 if (addr & 3) {
@@ -401,15 +398,12 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
 }
 
 if (addr < AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR) {
-DPRINTF(-1, "(addr 0x%08X), val 0x%08"PRIX64"\n", (unsigned) addr, 
val);
-
 switch (addr) {
 case HOST_CAP: /* R/WO, RO */
 /* FIXME handle R/WO */
 break;
 case HOST_CTL: /* R/W */
 if (val & HOST_CTL_RESET) {
-DPRINTF(-1, "HBA Reset\n");
 ahci_reset(s);
 } else {
 s->control_regs.ghc = (val & 0x3) | HOST_CTL_AHCI_EN;
@@ -427,7 +421,7 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
 /* FIXME report write? */
 break;
 default:
-DPRINTF(-1, "write to unknown register 0x%x\n", 
(unsigned)addr);
+trace_ahci_mem_write_unknown(s, addr);
 }
 } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
(addr < (AHCI_PORT_REGS_START_ADDR +

[Qemu-block] [PATCH 8/9] AHCI: pretty-print FIS to buffer instead of stderr

[John Snow]

The current FIS printing routines dump the FIS to screen. adjust this
such that it dumps to buffer instead, then use this ability to have
FIS dump mechanisms via trace-events instead of compiled defines.

Signed-off-by: John Snow 
---
 hw/ide/ahci.c   | 54 +++--
 hw/ide/trace-events |  3 +++
 2 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index c5a9b69..1a4d4db 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -644,20 +644,45 @@ static void ahci_reset_port(AHCIState *s, int port)
 ahci_init_d2h(d);
 }
 
-static void debug_print_fis(uint8_t *fis, int cmd_len)
+/* Buffer pretty output based on a raw FIS structure. */
+static void ahci_pretty_buffer_fis(uint8_t *fis, int cmd_len, char **out)
 {
-#if DEBUG_AHCI
+size_t bufsize;
+char *pbuf;
+char *pptr;
+size_t lines = DIV_ROUND_UP(cmd_len, 16);
+const char *preamble = "FIS:";
 int i;
 
-fprintf(stderr, "fis:");
+/* Total amount of memory to store FISes in HBA memory */
+g_assert_cmpint(cmd_len, <=, 0x100);
+g_assert(out);
+
+/* Printed like:
+ * FIS:\n
+ * 0x00: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee \n
+ * 0x10: ff \n
+ * \0
+ *
+ * Four bytes for the preamble, seven for each line prefix (including a
+ * newline to start a new line), three bytes for each source byte,
+ * a trailing newline and a terminal null byte.
+ */
+
+bufsize = strlen(preamble) + ((6 + 1) * lines) + (3 * cmd_len) + 1 + 1;
+pbuf = g_malloc(bufsize);
+pptr = pbuf;
+pptr += sprintf(pptr, "%s", preamble);
 for (i = 0; i < cmd_len; i++) {
 if ((i & 0xf) == 0) {
-fprintf(stderr, "\n%02x:",i);
+pptr += sprintf(pptr, "\n0x%02x: ", i);
 }
-fprintf(stderr, "%02x ",fis[i]);
+pptr += sprintf(pptr, "%02x ", fis[i]);
 }
-fprintf(stderr, "\n");
-#endif
+pptr += sprintf(pptr, "\n");
+pptr += 1; /* \0 */
+g_assert(pbuf + bufsize == pptr);
+*out = pbuf;
 }
 
 static bool ahci_map_fis_address(AHCIDevice *ad)
@@ -1201,7 +1226,12 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
  * table to ide_state->io_buffer */
 if (opts & AHCI_CMD_ATAPI) {
 memcpy(ide_state->io_buffer, _fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
-debug_print_fis(ide_state->io_buffer, 0x10);
+if (TRACE_HANDLE_REG_H2D_FIS_DUMP_ENABLED) {
+char *pretty_fis;
+ahci_pretty_buffer_fis(ide_state->io_buffer, 0x10, _fis);
+trace_handle_reg_h2d_fis_dump(s, port, pretty_fis);
+g_free(pretty_fis);
+}
 s->dev[port].done_atapi_packet = false;
 /* XXX send PIO setup FIS */
 }
@@ -1256,8 +1286,12 @@ static int handle_cmd(AHCIState *s, int port, uint8_t 
slot)
 trace_handle_cmd_badmap(s, port, cmd_len);
 goto out;
 }
-debug_print_fis(cmd_fis, 0x80);
-
+if (TRACE_HANDLE_CMD_FIS_DUMP_ENABLED) {
+char *pretty_fis;
+ahci_pretty_buffer_fis(cmd_fis, 0x80, _fis);
+trace_handle_cmd_fis_dump(s, port, pretty_fis);
+g_free(pretty_fis);
+}
 switch (cmd_fis[0]) {
 case SATA_FIS_TYPE_REGISTER_H2D:
 handle_reg_h2d_fis(s, port, slot, cmd_fis);
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 5b321e1..082e804 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -103,3 +103,6 @@ ahci_reset(void *s) "ahci(%p): HBA reset"
 allwinner_ahci_mem_read(void *s, void *a, uint64_t addr, uint64_t val, 
unsigned size) "ahci(%p): read a=%p addr=0x%"HWADDR_PRIx" val=0x%"PRIx64", 
size=%d"
 allwinner_ahci_mem_write(void *s, void *a, uint64_t addr, uint64_t val, 
unsigned size) "ahci(%p): write a=%p addr=0x%"HWADDR_PRIx" val=0x%"PRIx64", 
size=%d"
 
+# Warning: Verbose
+handle_reg_h2d_fis_dump(void *s, int port, char *fis) "ahci(%p)[%d]: %s"
+handle_cmd_fis_dump(void *s, int port, char *fis) "ahci(%p)[%d]: %s"
-- 
2.9.4

[Qemu-block] [PATCH 7/9] AHCI: Rework IRQ constants

[John Snow]

Create a new enum so that we can name the IRQ bits, which will make debugging
them a little nicer if we can print them out. Not handled in this patch, but
this will make it possible to get a nice debug printf detailing exactly which
status bits are set, as it can be multiple at any given time.

As a consequence of this patch, it is no longer possible to set multiple IRQ
codes at once, but nothing was utilizing this ability anyway.

Signed-off-by: John Snow 
---
 hw/ide/ahci.c  | 49 ++---
 hw/ide/ahci_internal.h | 44 +++-
 hw/ide/trace-events|  2 +-
 3 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d5acceb..c5a9b69 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -56,6 +56,27 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
 static void ahci_unmap_fis_address(AHCIDevice *ad);
 
+static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__END] = {
+[AHCI_PORT_IRQ_BIT_DHRS] = "DHRS",
+[AHCI_PORT_IRQ_BIT_PSS]  = "PSS",
+[AHCI_PORT_IRQ_BIT_DSS]  = "DSS",
+[AHCI_PORT_IRQ_BIT_SDBS] = "SDBS",
+[AHCI_PORT_IRQ_BIT_UFS]  = "UFS",
+[AHCI_PORT_IRQ_BIT_DPS]  = "DPS",
+[AHCI_PORT_IRQ_BIT_PCS]  = "PCS",
+[AHCI_PORT_IRQ_BIT_DMPS] = "DMPS",
+[8 ... 21]   = "RESERVED",
+[AHCI_PORT_IRQ_BIT_PRCS] = "PRCS",
+[AHCI_PORT_IRQ_BIT_IPMS] = "IPMS",
+[AHCI_PORT_IRQ_BIT_OFS]  = "OFS",
+[25] = "RESERVED",
+[AHCI_PORT_IRQ_BIT_INFS] = "INFS",
+[AHCI_PORT_IRQ_BIT_IFS]  = "IFS",
+[AHCI_PORT_IRQ_BIT_HBDS] = "HBDS",
+[AHCI_PORT_IRQ_BIT_HBFS] = "HBFS",
+[AHCI_PORT_IRQ_BIT_TFES] = "TFES",
+[AHCI_PORT_IRQ_BIT_CPDS] = "CPDS"
+};
 
 static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
 {
@@ -170,12 +191,18 @@ static void ahci_check_irq(AHCIState *s)
 }
 
 static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
- int irq_type)
+ enum AHCIPortIRQ irqbit)
 {
-DPRINTF(d->port_no, "trigger irq %#x -> %x\n",
-irq_type, d->port_regs.irq_mask & irq_type);
+g_assert(irqbit >= 0 && irqbit < 32);
+uint32_t irq = 1U << irqbit;
+uint32_t irqstat = d->port_regs.irq_stat | irq;
 
-d->port_regs.irq_stat |= irq_type;
+trace_ahci_trigger_irq(s, d->port_no,
+   AHCIPortIRQ_lookup[irqbit], irq,
+   d->port_regs.irq_stat, irqstat,
+   irqstat & d->port_regs.irq_mask);
+
+d->port_regs.irq_stat = irqstat;
 ahci_check_irq(s);
 }
 
@@ -718,7 +745,7 @@ static void ahci_write_fis_sdb(AHCIState *s, 
NCQTransferState *ncq_tfs)
 
 /* Trigger IRQ if interrupt bit is set (which currently, it always is) */
 if (sdb_fis->flags & 0x40) {
-ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
+ahci_trigger_irq(s, ad, AHCI_PORT_IRQ_BIT_SDBS);
 }
 }
 
@@ -761,10 +788,10 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t 
len)
 ad->port.ifs[0].status;
 
 if (pio_fis[2] & ERR_STAT) {
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
 }
 
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_PSS);
 }
 
 static bool ahci_write_fis_d2h(AHCIDevice *ad)
@@ -804,10 +831,10 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad)
 ad->port.ifs[0].status;
 
 if (d2h_fis[2] & ERR_STAT) {
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
 }
 
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
 return true;
 }
 
@@ -1082,7 +1109,7 @@ static void process_ncq_command(AHCIState *s, int port, 
uint8_t *cmd_fis,
  "is smaller than the requested size (0x%zx)",
  ncq_tfs->sglist.size, size);
 ncq_err(ncq_tfs);
-ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
+ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_OFS);
 return;
 } else if (ncq_tfs->sglist.size != size) {
 trace_process_ncq_command_large(s, port, tag,
@@ -1225,7 +1252,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t 
slot)
 trace_handle_cmd_badfis(s, port);
 return -1;
 } else if (cmd_len != 0x80) {
-ahci_trigger_irq(s, >dev[port], PORT_IRQ_HBUS_ERR);
+ahci_trigger_irq(s, >dev[port], AHCI_PORT_IRQ_BIT_HBFS);
 trace_handle_cmd_badmap(s, port, cmd_len);
 goto out;
 }
diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index 1e21169..7e67add 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -91,6 +91,31 @@
 #define PORT_CMD_ISSUE0x38 /* command 

[Qemu-block] [PATCH 5/9] IDE: replace DEBUG_AIO with trace events

[John Snow]

Signed-off-by: John Snow 
---
 hw/ide/atapi.c|  5 +
 hw/ide/core.c | 17 ++---
 hw/ide/trace-events   |  3 +++
 include/hw/ide/internal.h |  7 +--
 4 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 37fa699..b8fc51e 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -416,10 +416,7 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int 
ret)
 s->io_buffer_size = n * 2048;
 data_offset = 0;
 }
-#ifdef DEBUG_AIO
-printf("aio_read_cd: lba=%u n=%d\n", s->lba, n);
-#endif
-
+trace_ide_atapi_cmd_read_dma_cb_aio(s, s->lba, n);
 s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
 s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
 qemu_iovec_init_external(>bus->dma->qiov, >bus->dma->iov, 1);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 29848ff..1358029 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -58,6 +58,13 @@ static const int smart_attributes[][12] = {
 { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
 };
 
+const char *IDE_DMA_CMD_lookup[IDE_DMA__END] = {
+[IDE_DMA_READ] = "DMA_READ",
+[IDE_DMA_WRITE] = "DMA_WRITE",
+[IDE_DMA_TRIM] = "DMA TRIM",
+[IDE_DMA_ATAPI] = "DMA ATAPI"
+};
+
 static void ide_dummy_transfer_stop(IDEState *s);
 
 static void padstr(char *str, const char *src, int len)
@@ -860,10 +867,8 @@ static void ide_dma_cb(void *opaque, int ret)
 goto eot;
 }
 
-#ifdef DEBUG_AIO
-printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, cmd_cmd=%d\n",
-   sector_num, n, s->dma_cmd);
-#endif
+trace_ide_dma_cb(s, sector_num, n,
+ IDE_DMA_CMD_lookup[s->dma_cmd]);
 
 if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) &&
 !ide_sect_range_ok(s, sector_num, n)) {
@@ -2383,9 +2388,7 @@ void ide_bus_reset(IDEBus *bus)
 
 /* pending async DMA */
 if (bus->dma->aiocb) {
-#ifdef DEBUG_AIO
-printf("aio_cancel\n");
-#endif
+trace_ide_bus_reset_aio();
 blk_aio_cancel(bus->dma->aiocb);
 bus->dma->aiocb = NULL;
 }
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index ade1e76..6e33cb3 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -17,6 +17,8 @@ ide_cancel_dma_sync_remaining(void) "draining all remaining 
requests"
 ide_sector_read(int64_t sector_num, int nsectors) "sector=%"PRId64" 
nsectors=%d"
 ide_sector_write(int64_t sector_num, int nsectors) "sector=%"PRId64" 
nsectors=%d"
 ide_reset(void *s) "IDEstate %p"
+ide_bus_reset_aio(void) "aio_cancel"
+ide_dma_cb(void *s, int64_t sector_num, int n, const char *dma) "IDEState %p; 
sector_num=%"PRId64" n=%d cmd=%s"
 
 # hw/ide/pci.c
 bmdma_reset(void) ""
@@ -48,5 +50,6 @@ ide_atapi_cmd_reply_end_new(void *s, int status) "IDEState: 
%p; new transfer sta
 ide_atapi_cmd_check_status(void *s) "IDEState: %p"
 ide_atapi_cmd_read(void *s, const char *method, int lba, int nb_sectors) 
"IDEState: %p; read %s: LBA=%d nb_sectors=%d"
 ide_atapi_cmd(void *s, uint8_t cmd) "IDEState: %p; cmd: 0x%02x"
+ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p; aio 
read: lba=%d n=%d"
 # Warning: Verbose
 ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) "IDEState: 
%p; limit=0x%x packet: %s"
\ No newline at end of file
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 74efe8a..97e97a2 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -14,7 +14,6 @@
 #include "block/scsi.h"
 
 /* debug IDE devices */
-//#define DEBUG_AIO
 #define USE_DMA_CDROM
 
 typedef struct IDEBus IDEBus;
@@ -333,12 +332,16 @@ struct unreported_events {
 };
 
 enum ide_dma_cmd {
-IDE_DMA_READ,
+IDE_DMA__BEGIN = 0,
+IDE_DMA_READ = IDE_DMA__BEGIN,
 IDE_DMA_WRITE,
 IDE_DMA_TRIM,
 IDE_DMA_ATAPI,
+IDE_DMA__END
 };
 
+extern const char *IDE_DMA_CMD_lookup[IDE_DMA__END];
+
 #define ide_cmd_is_read(s) \
((s)->dma_cmd == IDE_DMA_READ)
 
-- 
2.9.4

[Qemu-block] [PATCH 4/9] ATAPI: Replace DEBUG_IDE_ATAPI with tracing events

[John Snow]

Goodbye, printfs.
Hello, fancy printfs.

Signed-off-by: John Snow 
---
 hw/ide/atapi.c| 64 +--
 hw/ide/trace-events   | 19 ++
 include/hw/ide/internal.h |  1 -
 3 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index fc1d19c..37fa699 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -27,6 +27,7 @@
 #include "hw/ide/internal.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/block-backend.h"
+#include "trace.h"
 
 #define ATAPI_SECTOR_BITS (2 + BDRV_SECTOR_BITS)
 #define ATAPI_SECTOR_SIZE (1 << ATAPI_SECTOR_BITS)
@@ -116,9 +117,7 @@ cd_read_sector_sync(IDEState *s)
 block_acct_start(blk_get_stats(s->blk), >acct,
  ATAPI_SECTOR_SIZE, BLOCK_ACCT_READ);
 
-#ifdef DEBUG_IDE_ATAPI
-printf("cd_read_sector_sync: lba=%d\n", s->lba);
-#endif
+trace_cd_read_sector_sync(s->lba);
 
 switch (s->cd_sector_size) {
 case 2048:
@@ -152,9 +151,7 @@ static void cd_read_sector_cb(void *opaque, int ret)
 {
 IDEState *s = opaque;
 
-#ifdef DEBUG_IDE_ATAPI
-printf("cd_read_sector_cb: lba=%d ret=%d\n", s->lba, ret);
-#endif
+trace_cd_read_sector_cb(s->lba, ret);
 
 if (ret < 0) {
 block_acct_failed(blk_get_stats(s->blk), >acct);
@@ -188,9 +185,7 @@ static int cd_read_sector(IDEState *s)
 s->iov.iov_len = ATAPI_SECTOR_SIZE;
 qemu_iovec_init_external(>qiov, >iov, 1);
 
-#ifdef DEBUG_IDE_ATAPI
-printf("cd_read_sector: lba=%d\n", s->lba);
-#endif
+trace_cd_read_sector(s->lba);
 
 block_acct_start(blk_get_stats(s->blk), >acct,
  ATAPI_SECTOR_SIZE, BLOCK_ACCT_READ);
@@ -213,9 +208,7 @@ void ide_atapi_cmd_ok(IDEState *s)
 
 void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc)
 {
-#ifdef DEBUG_IDE_ATAPI
-printf("atapi_cmd_error: sense=0x%x asc=0x%x\n", sense_key, asc);
-#endif
+trace_ide_atapi_cmd_error(s, sense_key, asc);
 s->error = sense_key << 4;
 s->status = READY_STAT | ERR_STAT;
 s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO | ATAPI_INT_REASON_CD;
@@ -252,19 +245,14 @@ static uint16_t atapi_byte_count_limit(IDEState *s)
 void ide_atapi_cmd_reply_end(IDEState *s)
 {
 int byte_count_limit, size, ret;
-#ifdef DEBUG_IDE_ATAPI
-printf("reply: tx_size=%d elem_tx_size=%d index=%d\n",
-   s->packet_transfer_size,
-   s->elementary_transfer_size,
-   s->io_buffer_index);
-#endif
+trace_ide_atapi_cmd_reply_end(s, s->packet_transfer_size,
+  s->elementary_transfer_size,
+  s->io_buffer_index);
 if (s->packet_transfer_size <= 0) {
 /* end of transfer */
 ide_atapi_cmd_ok(s);
 ide_set_irq(s->bus);
-#ifdef DEBUG_IDE_ATAPI
-printf("end of transfer, status=0x%x\n", s->status);
-#endif
+trace_ide_atapi_cmd_reply_end_eot(s, s->status);
 } else {
 /* see if a new sector must be read */
 if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) {
@@ -300,9 +288,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
 /* a new transfer is needed */
 s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
 byte_count_limit = atapi_byte_count_limit(s);
-#ifdef DEBUG_IDE_ATAPI
-printf("byte_count_limit=%d\n", byte_count_limit);
-#endif
+trace_ide_atapi_cmd_reply_end_bcl(s, byte_count_limit);
 size = s->packet_transfer_size;
 if (size > byte_count_limit) {
 /* byte count limit must be even if this case */
@@ -324,9 +310,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
 ide_transfer_start(s, s->io_buffer + s->io_buffer_index - size,
size, ide_atapi_cmd_reply_end);
 ide_set_irq(s->bus);
-#ifdef DEBUG_IDE_ATAPI
-printf("status=0x%x\n", s->status);
-#endif
+trace_ide_atapi_cmd_reply_end_new(s, s->status);
 }
 }
 }
@@ -368,9 +352,7 @@ static void ide_atapi_cmd_read_pio(IDEState *s, int lba, 
int nb_sectors,
 
 static void ide_atapi_cmd_check_status(IDEState *s)
 {
-#ifdef DEBUG_IDE_ATAPI
-printf("atapi_cmd_check_status\n");
-#endif
+trace_ide_atapi_cmd_check_status(s);
 s->error = MC_ERR | (UNIT_ATTENTION << 4);
 s->status = ERR_STAT;
 s->nsector = 0;
@@ -477,10 +459,8 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, 
int nb_sectors,
 static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors,
int sector_size)
 {
-#ifdef DEBUG_IDE_ATAPI
-printf("read %s: LBA=%d nb_sectors=%d\n", s->atapi_dma ? "dma" : "pio",
-lba, nb_sectors);
-#endif
+trace_ide_atapi_cmd_read(s, s->atapi_dma ? "dma" : "pio",
+ lba, nb_sectors);
 if (s->atapi_dma) {
 ide_atapi_cmd_read_dma(s, lba, nb_sectors, sector_size);
 } else {
@@ -1330,16 +1310,18 

[Qemu-block] [PATCH 3/9] IDE: add tracing for data ports

[John Snow]

To be used sparingly, but still interesting in the case of small
firmwares designed to reproduce bugs in QEMU IDE.

Signed-off-by: John Snow 
---
 hw/ide/core.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 6235bdf..29848ff 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2251,6 +2251,8 @@ void ide_data_writew(void *opaque, uint32_t addr, 
uint32_t val)
 IDEState *s = idebus_active_if(bus);
 uint8_t *p;
 
+trace_ide_data_writew(addr, val, bus, s);
+
 /* PIO data access allowed only when DRQ bit is set. The result of a write
  * during PIO out is indeterminate, just ignore it. */
 if (!(s->status & DRQ_STAT) || ide_is_pio_out(s)) {
@@ -2296,6 +2298,8 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr)
 s->status &= ~DRQ_STAT;
 s->end_transfer_func(s);
 }
+
+trace_ide_data_readw(addr, ret, bus, s);
 return ret;
 }
 
@@ -2305,6 +2309,8 @@ void ide_data_writel(void *opaque, uint32_t addr, 
uint32_t val)
 IDEState *s = idebus_active_if(bus);
 uint8_t *p;
 
+trace_ide_data_writel(addr, val, bus, s);
+
 /* PIO data access allowed only when DRQ bit is set. The result of a write
  * during PIO out is indeterminate, just ignore it. */
 if (!(s->status & DRQ_STAT) || ide_is_pio_out(s)) {
@@ -2335,7 +2341,8 @@ uint32_t ide_data_readl(void *opaque, uint32_t addr)
 /* PIO data access allowed only when DRQ bit is set. The result of a read
  * during PIO in is indeterminate, return 0 and don't move forward. */
 if (!(s->status & DRQ_STAT) || !ide_is_pio_out(s)) {
-return 0;
+ret = 0;
+goto out;
 }
 
 p = s->data_ptr;
@@ -2350,6 +2357,9 @@ uint32_t ide_data_readl(void *opaque, uint32_t addr)
 s->status &= ~DRQ_STAT;
 s->end_transfer_func(s);
 }
+
+out:
+trace_ide_data_readl(addr, ret, bus, s);
 return ret;
 }
 
-- 
2.9.4

[Qemu-block] [PATCH 0/9] IDE: replace printfs with tracing

[John Snow]

Wherever possible, replace all printfs with proper tracing.
In most places I've tried to do a straight replacement, but
forthcoming patches may calibrate the tracing to be a little nicer.

For now, it's nice to just remove the all-or-nothing tracing.

John Snow (9):
  IDE: replace DEBUG_IDE with tracing system
  IDE: Add register hints to tracing
  IDE: add tracing for data ports
  ATAPI: Replace DEBUG_IDE_ATAPI with tracing events
  IDE: replace DEBUG_AIO with trace events
  AHCI: Replace DPRINTF with trace-events
  AHCI: Rework IRQ constants
  AHCI: pretty-print FIS to buffer instead of stderr
  AHCI: remove DPRINTF macro

 Makefile.objs |   1 +
 hw/ide/ahci.c | 269 ++
 hw/ide/ahci_internal.h|  44 ++--
 hw/ide/atapi.c|  69 +---
 hw/ide/cmd646.c   |  10 +-
 hw/ide/core.c | 178 +++---
 hw/ide/pci.c  |  17 +--
 hw/ide/piix.c |  11 +-
 hw/ide/trace-events   | 108 +++
 hw/ide/via.c  |  10 +-
 include/hw/ide/internal.h |   9 +-
 11 files changed, 454 insertions(+), 272 deletions(-)
 create mode 100644 hw/ide/trace-events

-- 
2.9.4

[Qemu-block] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[John Snow]

Out with the old, in with the new.

Signed-off-by: John Snow 
---
 Makefile.objs |  1 +
 hw/ide/cmd646.c   | 10 +++-
 hw/ide/core.c | 65 +++
 hw/ide/pci.c  | 17 -
 hw/ide/piix.c | 11 
 hw/ide/trace-events   | 33 
 hw/ide/via.c  | 10 +++-
 include/hw/ide/internal.h |  1 -
 8 files changed, 78 insertions(+), 70 deletions(-)
 create mode 100644 hw/ide/trace-events

diff --git a/Makefile.objs b/Makefile.objs
index 24a4ea0..967c092 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -153,6 +153,7 @@ trace-events-subdirs += hw/acpi
 trace-events-subdirs += hw/arm
 trace-events-subdirs += hw/alpha
 trace-events-subdirs += hw/xen
+trace-events-subdirs += hw/ide
 trace-events-subdirs += ui
 trace-events-subdirs += audio
 trace-events-subdirs += net
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 9ebb8d4..86b2a8f 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -32,6 +32,7 @@
 #include "sysemu/dma.h"
 
 #include "hw/ide/pci.h"
+#include "trace.h"
 
 /* CMD646 specific */
 #define CFR0x50
@@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
 val = 0xff;
 break;
 }
-#ifdef DEBUG_IDE
-printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val);
-#endif
+
+trace_bmdma_read_cmd646(addr, val);
 return val;
 }
 
@@ -211,9 +211,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
 return;
 }
 
-#ifdef DEBUG_IDE
-printf("bmdma: writeb " TARGET_FMT_plx " : 0x%" PRIx64 "\n", addr, val);
-#endif
+trace_bmdma_write_cmd646(addr, val);
 switch(addr & 3) {
 case 0:
 bmdma_cmd_writeb(bm, val);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0b48b64..53fa084 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -36,6 +36,7 @@
 #include "qemu/cutils.h"
 
 #include "hw/ide/internal.h"
+#include "trace.h"
 
 /* These values were based on a Seagate ST3500418AS but have been modified
to make more sense in QEMU */
@@ -656,10 +657,7 @@ void ide_cancel_dma_sync(IDEState *s)
  * write requests) pending and we can avoid to drain. */
 QLIST_FOREACH(req, >buffered_requests, list) {
 if (!req->orphaned) {
-#ifdef DEBUG_IDE
-printf("%s: invoking cb %p of buffered request %p with"
-   " -ECANCELED\n", __func__, req->original_cb, req);
-#endif
+trace_ide_cancel_dma_sync_buffered(req->original_cb, req);
 req->original_cb(req->original_opaque, -ECANCELED);
 }
 req->orphaned = true;
@@ -678,9 +676,7 @@ void ide_cancel_dma_sync(IDEState *s)
  * aio operation with preadv/pwritev.
  */
 if (s->bus->dma->aiocb) {
-#ifdef DEBUG_IDE
-printf("%s: draining all remaining requests", __func__);
-#endif
+trace_ide_cancel_dma_sync_remaining();
 blk_drain(s->blk);
 assert(s->bus->dma->aiocb == NULL);
 }
@@ -741,9 +737,7 @@ static void ide_sector_read(IDEState *s)
 n = s->req_nb_sectors;
 }
 
-#if defined(DEBUG_IDE)
-printf("sector=%" PRId64 "\n", sector_num);
-#endif
+trace_ide_sector_read(sector_num, n);
 
 if (!ide_sect_range_ok(s, sector_num, n)) {
 ide_rw_error(s);
@@ -1005,14 +999,14 @@ static void ide_sector_write(IDEState *s)
 
 s->status = READY_STAT | SEEK_STAT | BUSY_STAT;
 sector_num = ide_get_sector(s);
-#if defined(DEBUG_IDE)
-printf("sector=%" PRId64 "\n", sector_num);
-#endif
+
 n = s->nsector;
 if (n > s->req_nb_sectors) {
 n = s->req_nb_sectors;
 }
 
+trace_ide_sector_write(sector_num, n);
+
 if (!ide_sect_range_ok(s, sector_num, n)) {
 ide_rw_error(s);
 block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_WRITE);
@@ -1186,18 +1180,17 @@ static void ide_clear_hob(IDEBus *bus)
 void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 {
 IDEBus *bus = opaque;
+IDEState *s = idebus_active_if(bus);
+int reg_num = addr & 7;
 
-#ifdef DEBUG_IDE
-printf("IDE: write addr=0x%x val=0x%02x\n", addr, val);
-#endif
-
-addr &= 7;
+trace_ide_ioport_write(addr, val, bus, s);
 
 /* ignore writes to command block while busy with previous command */
-if (addr != 7 && (idebus_active_if(bus)->status & (BUSY_STAT|DRQ_STAT)))
+if (reg_num != 7 && (s->status & (BUSY_STAT|DRQ_STAT))) {
 return;
+}
 
-switch(addr) {
+switch(reg_num) {
 case 0:
 break;
 case 1:
@@ -1253,9 +1246,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, 
uint32_t val)
 
 static void ide_reset(IDEState *s)
 {
-#ifdef DEBUG_IDE
-printf("ide: reset\n");
-#endif
+trace_ide_reset(s);
 
 if (s->pio_aiocb) {
 blk_aio_cancel(s->pio_aiocb);
@@ -2013,10 +2004,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
 IDEState *s;
 bool complete;
 
-#if defined(DEBUG_IDE)
-printf("ide: 

[Qemu-block] [PATCH 2/9] IDE: Add register hints to tracing

[John Snow]

Name the registers for tracing purposes.

Signed-off-by: John Snow 
---
 hw/ide/core.c   | 88 +
 hw/ide/trace-events |  4 +--
 2 files changed, 70 insertions(+), 22 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 53fa084..6235bdf 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1177,13 +1177,37 @@ static void ide_clear_hob(IDEBus *bus)
 bus->ifs[1].select &= ~(1 << 7);
 }
 
+/* IOport [W]rite [R]egisters */
+enum ATA_IOPORT_WR {
+ATA_IOPORT_WR_DATA = 0,
+ATA_IOPORT_WR_FEATURES = 1,
+ATA_IOPORT_WR_SECTOR_COUNT = 2,
+ATA_IOPORT_WR_SECTOR_NUMBER = 3,
+ATA_IOPORT_WR_CYLINDER_LOW = 4,
+ATA_IOPORT_WR_CYLINDER_HIGH = 5,
+ATA_IOPORT_WR_DEVICE_HEAD = 6,
+ATA_IOPORT_WR_COMMAND = 7,
+ATA_IOPORT_WR_NUM_REGISTERS,
+};
+
+const char *ATA_IOPORT_WR_lookup[ATA_IOPORT_WR_NUM_REGISTERS] = {
+[ATA_IOPORT_WR_DATA] = "Data",
+[ATA_IOPORT_WR_FEATURES] = "Features",
+[ATA_IOPORT_WR_SECTOR_COUNT] = "Sector Count",
+[ATA_IOPORT_WR_SECTOR_NUMBER] = "Sector Number",
+[ATA_IOPORT_WR_CYLINDER_LOW] = "Cylinder Low",
+[ATA_IOPORT_WR_CYLINDER_HIGH] = "Cylinder High",
+[ATA_IOPORT_WR_DEVICE_HEAD] = "Device/Head",
+[ATA_IOPORT_WR_COMMAND] = "Command"
+};
+
 void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 {
 IDEBus *bus = opaque;
 IDEState *s = idebus_active_if(bus);
 int reg_num = addr & 7;
 
-trace_ide_ioport_write(addr, val, bus, s);
+trace_ide_ioport_write(addr, ATA_IOPORT_WR_lookup[reg_num], val, bus, s);
 
 /* ignore writes to command block while busy with previous command */
 if (reg_num != 7 && (s->status & (BUSY_STAT|DRQ_STAT))) {
@@ -1193,43 +1217,43 @@ void ide_ioport_write(void *opaque, uint32_t addr, 
uint32_t val)
 switch(reg_num) {
 case 0:
 break;
-case 1:
-   ide_clear_hob(bus);
+case ATA_IOPORT_WR_FEATURES:
+ide_clear_hob(bus);
 /* NOTE: data is written to the two drives */
-   bus->ifs[0].hob_feature = bus->ifs[0].feature;
-   bus->ifs[1].hob_feature = bus->ifs[1].feature;
+bus->ifs[0].hob_feature = bus->ifs[0].feature;
+bus->ifs[1].hob_feature = bus->ifs[1].feature;
 bus->ifs[0].feature = val;
 bus->ifs[1].feature = val;
 break;
-case 2:
+case ATA_IOPORT_WR_SECTOR_COUNT:
ide_clear_hob(bus);
bus->ifs[0].hob_nsector = bus->ifs[0].nsector;
bus->ifs[1].hob_nsector = bus->ifs[1].nsector;
 bus->ifs[0].nsector = val;
 bus->ifs[1].nsector = val;
 break;
-case 3:
+case ATA_IOPORT_WR_SECTOR_NUMBER:
ide_clear_hob(bus);
bus->ifs[0].hob_sector = bus->ifs[0].sector;
bus->ifs[1].hob_sector = bus->ifs[1].sector;
 bus->ifs[0].sector = val;
 bus->ifs[1].sector = val;
 break;
-case 4:
+case ATA_IOPORT_WR_CYLINDER_LOW:
ide_clear_hob(bus);
bus->ifs[0].hob_lcyl = bus->ifs[0].lcyl;
bus->ifs[1].hob_lcyl = bus->ifs[1].lcyl;
 bus->ifs[0].lcyl = val;
 bus->ifs[1].lcyl = val;
 break;
-case 5:
+case ATA_IOPORT_WR_CYLINDER_HIGH:
ide_clear_hob(bus);
bus->ifs[0].hob_hcyl = bus->ifs[0].hcyl;
bus->ifs[1].hob_hcyl = bus->ifs[1].hcyl;
 bus->ifs[0].hcyl = val;
 bus->ifs[1].hcyl = val;
 break;
-case 6:
+case ATA_IOPORT_WR_DEVICE_HEAD:
/* FIXME: HOB readback uses bit 7 */
 bus->ifs[0].select = (val & ~0x10) | 0xa0;
 bus->ifs[1].select = (val | 0x10) | 0xa0;
@@ -1237,7 +1261,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, 
uint32_t val)
 bus->unit = (val >> 4) & 1;
 break;
 default:
-case 7:
+case ATA_IOPORT_WR_COMMAND:
 /* command */
 ide_exec_cmd(bus, val);
 break;
@@ -2044,6 +2068,30 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
 }
 }
 
+/* IOport [R]ead [R]egisters */
+enum ATA_IOPORT_RR {
+ATA_IOPORT_RR_DATA = 0,
+ATA_IOPORT_RR_ERROR = 1,
+ATA_IOPORT_RR_SECTOR_COUNT = 2,
+ATA_IOPORT_RR_SECTOR_NUMBER = 3,
+ATA_IOPORT_RR_CYLINDER_LOW = 4,
+ATA_IOPORT_RR_CYLINDER_HIGH = 5,
+ATA_IOPORT_RR_DEVICE_HEAD = 6,
+ATA_IOPORT_RR_STATUS = 7,
+ATA_IOPORT_RR_NUM_REGISTERS,
+};
+
+const char *ATA_IOPORT_RR_lookup[ATA_IOPORT_RR_NUM_REGISTERS] = {
+[ATA_IOPORT_RR_DATA] = "Data",
+[ATA_IOPORT_RR_ERROR] = "Error",
+[ATA_IOPORT_RR_SECTOR_COUNT] = "Sector Count",
+[ATA_IOPORT_RR_SECTOR_NUMBER] = "Sector Number",
+[ATA_IOPORT_RR_CYLINDER_LOW] = "Cylinder Low",
+[ATA_IOPORT_RR_CYLINDER_HIGH] = "Cylinder High",
+[ATA_IOPORT_RR_DEVICE_HEAD] = "Device/Head",
+[ATA_IOPORT_RR_STATUS] = "Status"
+};
+
 uint32_t ide_ioport_read(void *opaque, uint32_t addr)
 {
 IDEBus *bus = opaque;
@@ -2056,10 +2104,10 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr)
 //hob = s->select & (1 << 7);
 hob = 

[Qemu-block] [PATCH 4/4] block-backend: test flush op on empty backend

[John Snow]

From: Kevin Wolf 

Signed-off-by: Kevin Wolf 
Signed-off-by: John Snow 
---
 tests/Makefile.include |  2 ++
 tests/test-block-backend.c | 62 ++
 2 files changed, 64 insertions(+)
 create mode 100644 tests/test-block-backend.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index eb4895f..153494b 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -56,6 +56,7 @@ check-unit-y += tests/test-hbitmap$(EXESUF)
 gcov-files-test-hbitmap-y = blockjob.c
 check-unit-y += tests/test-blockjob$(EXESUF)
 check-unit-y += tests/test-blockjob-txn$(EXESUF)
+check-unit-y += tests/test-block-backend$(EXESUF)
 check-unit-y += tests/test-x86-cpuid$(EXESUF)
 # all code tested by test-x86-cpuid is inside topology.h
 gcov-files-test-x86-cpuid-y =
@@ -556,6 +557,7 @@ tests/test-aio-multithread$(EXESUF): 
tests/test-aio-multithread.o $(test-block-o
 tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-obj-y)
 tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) 
$(test-util-obj-y)
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o 
$(test-block-obj-y) $(test-util-obj-y)
+tests/test-block-backend$(EXESUF): tests/test-block-backend.o 
$(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
 tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) 
$(test-crypto-obj-y)
diff --git a/tests/test-block-backend.c b/tests/test-block-backend.c
new file mode 100644
index 000..5348781
--- /dev/null
+++ b/tests/test-block-backend.c
@@ -0,0 +1,62 @@
+/*
+ * BlockBackend tests
+ *
+ * Copyright (c) 2017 Kevin Wolf 
+ *
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block.h"
+#include "sysemu/block-backend.h"
+#include "qapi/error.h"
+
+static void test_drain_aio_error_flush_cb(void *opaque, int ret)
+{
+bool *completed = opaque;
+
+g_assert(ret == -ENOMEDIUM);
+*completed = true;
+}
+
+static void test_drain_aio_error(void)
+{
+BlockBackend *blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+BlockAIOCB *acb;
+bool completed = false;
+
+acb = blk_aio_flush(blk, test_drain_aio_error_flush_cb, );
+g_assert(acb != NULL);
+g_assert(completed == false);
+
+blk_drain(blk);
+g_assert(completed == true);
+}
+
+int main(int argc, char **argv)
+{
+bdrv_init();
+qemu_init_main_loop(_abort);
+
+g_test_init(, , NULL);
+
+g_test_add_func("/block-backend/drain_aio_error", test_drain_aio_error);
+
+return g_test_run();
+}
-- 
2.9.4

[Qemu-block] [PATCH 3/4] block-backend: shift in-flight counter to BB from BDS

[John Snow]

From: Kevin Wolf 

This allows us to detect errors in cache flushing (ENOMEDIUM)
without choking on a null dereference because we assume that
blk_bs(bb) is always defined.

Signed-off-by: Kevin Wolf 
Signed-off-by: John Snow 
---
 block.c   |  2 +-
 block/block-backend.c | 40 ++--
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index ce9cce7..834b836 100644
--- a/block.c
+++ b/block.c
@@ -4476,7 +4476,7 @@ out:
 
 AioContext *bdrv_get_aio_context(BlockDriverState *bs)
 {
-return bs->aio_context;
+return bs ? bs->aio_context : qemu_get_aio_context();
 }
 
 void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co)
diff --git a/block/block-backend.c b/block/block-backend.c
index 968438c..efd7e92 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -68,6 +68,9 @@ struct BlockBackend {
 NotifierList remove_bs_notifiers, insert_bs_notifiers;
 
 int quiesce_counter;
+
+/* Number of in-flight requests. Accessed with atomic ops. */
+unsigned int in_flight;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -1109,6 +1112,16 @@ int blk_make_zero(BlockBackend *blk, BdrvRequestFlags 
flags)
 return bdrv_make_zero(blk->root, flags);
 }
 
+static void blk_inc_in_flight(BlockBackend *blk)
+{
+atomic_inc(>in_flight);
+}
+
+static void blk_dec_in_flight(BlockBackend *blk)
+{
+atomic_dec(>in_flight);
+}
+
 static void error_callback_bh(void *opaque)
 {
 struct BlockBackendAIOCB *acb = opaque;
@@ -1147,7 +1160,7 @@ static const AIOCBInfo blk_aio_em_aiocb_info = {
 static void blk_aio_complete(BlkAioEmAIOCB *acb)
 {
 if (acb->has_returned) {
-bdrv_dec_in_flight(acb->common.bs);
+blk_dec_in_flight(acb->rwco.blk);
 acb->common.cb(acb->common.opaque, acb->rwco.ret);
 qemu_aio_unref(acb);
 }
@@ -1168,7 +1181,7 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, 
int64_t offset, int bytes,
 BlkAioEmAIOCB *acb;
 Coroutine *co;
 
-bdrv_inc_in_flight(blk_bs(blk));
+blk_inc_in_flight(blk);
 acb = blk_aio_get(_aio_em_aiocb_info, blk, cb, opaque);
 acb->rwco = (BlkRwCo) {
 .blk= blk,
@@ -1405,14 +1418,28 @@ int blk_flush(BlockBackend *blk)
 
 void blk_drain(BlockBackend *blk)
 {
-if (blk_bs(blk)) {
-bdrv_drain(blk_bs(blk));
+AioContext *ctx = blk_get_aio_context(blk);
+
+while (atomic_read(>in_flight)) {
+aio_context_acquire(ctx);
+aio_poll(ctx, false);
+aio_context_release(ctx);
+
+if (blk_bs(blk)) {
+bdrv_drain(blk_bs(blk));
+}
 }
 }
 
 void blk_drain_all(void)
 {
-bdrv_drain_all();
+BlockBackend *blk = NULL;
+
+bdrv_drain_all_begin();
+while ((blk = blk_all_next(blk)) != NULL) {
+blk_drain(blk);
+}
+bdrv_drain_all_end();
 }
 
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
@@ -1453,10 +1480,11 @@ static void send_qmp_error_event(BlockBackend *blk,
  bool is_read, int error)
 {
 IoOperationType optype;
+BlockDriverState *bs = blk_bs(blk);
 
 optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
 qapi_event_send_block_io_error(blk_name(blk),
-   bdrv_get_node_name(blk_bs(blk)), optype,
+   bs ? bdrv_get_node_name(bs) : "", optype,
action, blk_iostatus_is_enabled(blk),
error == ENOSPC, strerror(error),
_abort);
-- 
2.9.4

[Qemu-block] [PATCH 2/4] IDE: test flush on empty CDROM

[John Snow]

From: Kevin Wolf 

Signed-off-by: Kevin Wolf 
Signed-off-by: John Snow 
---
 tests/ide-test.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index bfd79dd..ffbfb04 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -689,6 +689,24 @@ static void test_flush_nodev(void)
 ide_test_quit();
 }
 
+static void test_flush_empty_drive(void)
+{
+QPCIDevice *dev;
+QPCIBar bmdma_bar, ide_bar;
+
+ide_test_start("-device ide-cd,bus=ide.0");
+dev = get_pci_device(_bar, _bar);
+
+/* FLUSH CACHE command on device 0*/
+qpci_io_writeb(dev, ide_bar, reg_device, 0);
+qpci_io_writeb(dev, ide_bar, reg_command, CMD_FLUSH_CACHE);
+
+/* Just testing that qemu doesn't crash... */
+
+free_pci_device(dev);
+ide_test_quit();
+}
+
 static void test_pci_retry_flush(void)
 {
 test_retry_flush("pc");
@@ -954,6 +972,7 @@ int main(int argc, char **argv)
 
 qtest_add_func("/ide/flush", test_flush);
 qtest_add_func("/ide/flush/nodev", test_flush_nodev);
+qtest_add_func("/ide/flush/empty_drive", test_flush_empty_drive);
 qtest_add_func("/ide/flush/retry_pci", test_pci_retry_flush);
 qtest_add_func("/ide/flush/retry_isa", test_isa_retry_flush);
 
-- 
2.9.4

[Qemu-block] [PATCH 0/4] IDE: Do not flush empty drives

[John Snow]

Patches one and two here are a 2.10 bandaid that avoids a crash.
Patches three and four are a more comprehensive fix as written by
Kevin in another discussion and are being posted here for the sake
of a discussion.

Patch three as written causes hangs in iotests 20, 39, 97, 98, 129,
153, 176, and 185. 124 actually segfaults.

For the purposes of 2.10, we'll likely just want patches 1 and 2
for now.

The problem in a nutshell: incrementing the in-flight counter of the
BDS from the BB layer assumes that every BB always has a BDS. That's
not true; and some devices like IDE have not in the past checked to
see if a given blk_ operation WOULD fail.

This culminates in a new regression where issuing a cache flush to a
CDROM (which is, for some reason, specification valid) will crash QEMU
due to a null dereference when attempting to atomically increment that
backend's in-flight counter.

John Snow (1):
  IDE: Do not flush empty CDROM drives

Kevin Wolf (3):
  IDE: test flush on empty CDROM
  block-backend: shift in-flight counter to BB from BDS
  block-backend: test flush op on empty backend

 block.c|  2 +-
 block/block-backend.c  | 40 +-
 hw/ide/core.c  | 11 +---
 tests/Makefile.include |  2 ++
 tests/ide-test.c   | 19 ++
 tests/test-block-backend.c | 62 ++
 6 files changed, 125 insertions(+), 11 deletions(-)
 create mode 100644 tests/test-block-backend.c

-- 
2.9.4

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: fix 185

[Eric Blake]

On 08/08/2017 10:16 AM, Kevin Wolf wrote:

 is sleep for ms portable?
>>> Sadly, sub-second sleep is a GNU coreutils feature; I suspect the BSD
>>> machines may fail to parse it.  (Of course, we could do some sort of
>>> 'sleep $SMALL', where $SMALL is 0.5 if sleep supports it, and 1 otherwise).
>>>
>> sleep for 1 second may lead to more then one request done before qemu quite.
> 
> _supported_os Linux
> 
> So do we really care about portability? And are all the other test cases
> working on the BSDs?

You've got valid points there - I suspect that a run of qemu-iotests on
BSD would turn up quite a few failures, some of which would be easy to
address; but it's not on my personal priority list.  I'm fine with an
approach of committing something that works where it was first tested,
then adding followup patches to improve portability, especially as long
as we are not currently getting anyone complaining about qemu-iotests
failures on BSD.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 03/56] monitor: Rewrite comment describing HMP .args_type

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  writes:
> 
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  monitor.c | 75 
> >> +++
> >>  1 file changed, 47 insertions(+), 28 deletions(-)
> >> 
> >> diff --git a/monitor.c b/monitor.c
> >> index e0f8801..8b54ba1 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -85,37 +85,56 @@
> >>  #endif
> >>  
> >>  /*
> >> - * Supported types:
> >> + * Command handlers (mon_cmd_t member @cmd) receive actual arguments
> >> + * in a QDict, which is built by the HMP core according to mon_cmd_t
> >> + * member @args_type.  It's a list of NAME:TYPE separated by comma.
> >>   *
> >> - * 'F'  filename
> >> - * 'B'  block device name
> >> - * 's'  string (accept optional quote)
> >> - * 'S'  it just appends the rest of the string (accept optional 
> >> quote)
> >> - * 'O'  option string of the form NAME=VALUE,...
> >> - *  parsed according to QemuOptsList given by its name
> >> - *  Example: 'device:O' uses qemu_device_opts.
> >> - *  Restriction: only lists with empty desc are supported
> >> - *  TODO lift the restriction
> >> - * 'i'  32 bit integer
> >> - * 'l'  target long (32 or 64 bit)
> >> - * 'M'  Non-negative target long (32 or 64 bit), in user mode the
> >> - *  value is multiplied by 2^20 (think Mebibyte)
> >> - * 'o'  octets (aka bytes)
> >> - *  user mode accepts an optional E, e, P, p, T, t, G, g, M, 
> >> m,
> >> - *  K, k suffix, which multiplies the value by 2^60 for 
> >> suffixes E
> >> - *  and e, 2^50 for suffixes P and p, 2^40 for suffixes T and 
> >> t,
> >> - *  2^30 for suffixes G and g, 2^20 for M and m, 2^10 for K 
> >> and k
> >> - * 'T'  double
> >> - *  user mode accepts an optional ms, us, ns suffix,
> >> - *  which divides the value by 1e3, 1e6, 1e9, respectively
> >> - * '/'  optional gdb-like print format (like "/10x")
> >> + * TYPEs that put a string value with key NAME into the QDict:
> >> + * 's'Argument is enclosed in '"' or delimited by whitespace.  In
> >> + *the former case, escapes \n \r \\ \' and \" are recognized.
> >> + * 'F'File name, like 's' except for completion.
> >> + * 'B'BlockBackend name, like 's' except for completion.
> >> + * 'S'Argument is the remainder of the line, less leading
> >> + *whitespace.
> >> +
> >>   *
> >> - * '?'  optional type (for all types, except '/')
> >> - * '.'  other form of optional type (for 'i' and 'l')
> >> - * 'b'  boolean
> >> - *  user mode accepts "on" or "off"
> >> - * '-'  optional parameter (eg. '-f')
> >> + * TYPEs that put an int64_t value with key NAME:
> >> + * 'l'Argument is an expression (QEMU pocket calculator).
> >> + * 'i'Like 'l' except value must fit into 32 bit unsigned.
> >> + * 'M'Like 'l' except value must not be negative and is multiplied
> >> + *by 2^20 (think "mebibyte").
> >>   *
> >> + * TYPEs that put an uint64_t value with key NAME:
> >> + * 'o'Argument is a size (think "octets").  Without suffix the
> >> + *value is multiplied by 2^20 (mebibytes), with suffix E or e
> >> + *by 2^60 (exbibytes), with P or p by 2^50 (pebibytes), with T
> >> + *or t by 2^40 (tebibytes), with G or g by 2^30 (gibibytes),
> >> + *with M or m by 2^10 (mebibytes), with K or k by 2^10
> >> + *(kibibytes).
> >
> > 'o' is messy.  It using qemu_strtosz_MiB which uses a 'double' intermediate
> > so I fear it can round.
> 
> It does, but only when you have more than 53 significant bits.
> 
> >  It also has a note it can't take all f's due to
> > an overflow from the conversion.
> 
> Correct, because values between 0xfc00 and 2^64-1 round up
> to 2^64.

Right, so these bother me not for normal sizes, but if we were to start
to use them for hex values with meanings, like addresses for example.
(Although I guess that's unlikely with the default assumption of MB)

> If it bothers you, feel free to explore the following: feed the string
> both to strtod() and to strtoll().  Whichever eats more characters wins.

Is the reason we're using strtod because we actively want users to be
able to say 3.5G ?  I guess that's a reason to keep it.

> This patch is of course just about better documenting what we have.  I
> was starting to type something like "repeating the (complex) contract of
> qemu_strtosz_MiB() here isn't so hot, let's include it by reference
> instead", but then I looked it up.  Pffft.
> 
> >Two things not mentioned are that
> > it also takes hex (as explicit 0x) and that it also 

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 29/56] block: Make BlockDirtyInfo byte count unsigned in QAPI/QMP

[Markus Armbruster]

John Snow  writes:

> On 08/07/2017 10:45 AM, Markus Armbruster wrote:
>> Byte counts should use QAPI type 'size' (uint64_t).  BlockDirtyInfo
>> member @count is 'int' (int64_t).  bdrv_query_dirty_bitmaps() computes
>> @count from bdrv_get_dirty_count() in uint64_t, then implicitly
>> converts to int64_t.  Before the commit before previous, the
>> conversion was in bdrv_get_dirty_count() instead.
>> 
>> Change member @count to 'size'.
>> 
>> query-block now reports @count values above 2^63-1 correctly instead
>> of their (negative) two's complement.
>> 
>> Signed-off-by: Markus Armbruster 
>
> Assuming there's no "gotcha" here introduced by changing the QAPI, then
> ACK; but you're the expert there, so I trust you!

Juan asked the same question on PATCH 15, see my reply there.

> Reviewed-by: John Snow 

Thanks!

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 15/56] migration: Make XBZRLE cache size unsigned in QAPI/QMP

[Markus Armbruster]

Juan Quintela  writes:

> Markus Armbruster  wrote:
>> Sizes should use QAPI type 'size' (uint64_t).  migrate-set-cache-size
>> parameter @value is 'int' (int64_t).  qmp_migrate_set_cache_size()
>> ensures it fits into size_t.  page_cache.c implicitly converts the
>> signed size to unsigned types (it can't quite decide whether to use
>> uint64_t or size_t for cache offsets, but that's not something I can
>> tackle now).
>>
>> XBZRLECacheStats member @cache-size and query-migrate-cache-size's
>> result are also 'int'.
>>
>> Change the migrate-set-cache-size parameter and the XBZRLECacheStats
>> members to 'size', fix up hmp_migrate_set_cache_size(), and tweak a
>> few variable types to reduce implicit conversions.
>>
>> migrate-set-cache-size now accepts size values between 2^63 and
>> 2^64-1.  It accepts negative values as before, because that's how the
>> QObject input visitor works for backward compatibility.
>>
>> So does HMP's migrate_set_cache_size.
>>
>> query-migrate and query-migrate-cache-size now report cache sizes
>> above 2^63-1 correctly instead of their (negative) two's complement.
>>
>> So does HMP's "info migrate_cache_size".  HMP's "info migrate" already
>> reported the cache size correctly, because it printed the signed
>> integer with PRIu32.
>>
>
> Reviewed-by: Juan Quintela 
>
>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index c8cceb9..ecabff6 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -646,7 +646,7 @@
>>  # Since: 1.2
>>  ##
>>  { 'struct': 'XBZRLECacheStats',
>> -  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
>> +  'data': {'cache-size': 'size', 'bytes': 'int', 'pages': 'int',
>> 'cache-miss': 'int', 'cache-miss-rate': 'number',
>> 'overflow': 'int' } }
>>  
>> @@ -2875,7 +2875,7 @@
>>  # <- { "return": {} }
>>  #
>>  ##
>> -{ 'command': 'migrate-set-cache-size', 'data': {'value': 'int'} }
>> +{ 'command': 'migrate-set-cache-size', 'data': {'value': 'size'} }
>>  
>>  ##
>>  # @query-migrate-cache-size:
>> @@ -2892,7 +2892,7 @@
>>  # <- { "return": 67108864 }
>>  #
>>  ##
>> -{ 'command': 'query-migrate-cache-size', 'returns': 'int' }
>> +{ 'command': 'query-migrate-cache-size', 'returns': 'size' }
>>  
>>  ##
>>  # @ObjectPropertyInfo:
>
> I am ussming this bits are backward compatible (I don't understand QMP
> to assure this)

I guess I should've explained this in the cover letter.

Until recent commit 5923f85, integers between INT64_MAX+1 and UINT64_MAX
did not work in QMP.  QEMU sent and accepted integers between INT64_MIN
and -1 instead.

The fix maintains strict compatibility for QMP input: negative integers
are accepted as before for backward compatibility.  Perhaps we can get
rid of this wart some day.

It does not maintain strict compatibility for QMP output: we now output
the correct integer.  We figure that's tolerable, because the obvious
way to parse the old output is strtoull(), and that does the right thing
for the new output when it does the right thing for the old output.

Fixing a QAPI type from 'int' to 'size' has the same compatibility
impact.

Questions?

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 03/56] monitor: Rewrite comment describing HMP .args_type

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> Signed-off-by: Markus Armbruster 
>> ---
>>  monitor.c | 75 
>> +++
>>  1 file changed, 47 insertions(+), 28 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index e0f8801..8b54ba1 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -85,37 +85,56 @@
>>  #endif
>>  
>>  /*
>> - * Supported types:
>> + * Command handlers (mon_cmd_t member @cmd) receive actual arguments
>> + * in a QDict, which is built by the HMP core according to mon_cmd_t
>> + * member @args_type.  It's a list of NAME:TYPE separated by comma.
>>   *
>> - * 'F'  filename
>> - * 'B'  block device name
>> - * 's'  string (accept optional quote)
>> - * 'S'  it just appends the rest of the string (accept optional 
>> quote)
>> - * 'O'  option string of the form NAME=VALUE,...
>> - *  parsed according to QemuOptsList given by its name
>> - *  Example: 'device:O' uses qemu_device_opts.
>> - *  Restriction: only lists with empty desc are supported
>> - *  TODO lift the restriction
>> - * 'i'  32 bit integer
>> - * 'l'  target long (32 or 64 bit)
>> - * 'M'  Non-negative target long (32 or 64 bit), in user mode the
>> - *  value is multiplied by 2^20 (think Mebibyte)
>> - * 'o'  octets (aka bytes)
>> - *  user mode accepts an optional E, e, P, p, T, t, G, g, M, m,
>> - *  K, k suffix, which multiplies the value by 2^60 for 
>> suffixes E
>> - *  and e, 2^50 for suffixes P and p, 2^40 for suffixes T and t,
>> - *  2^30 for suffixes G and g, 2^20 for M and m, 2^10 for K and 
>> k
>> - * 'T'  double
>> - *  user mode accepts an optional ms, us, ns suffix,
>> - *  which divides the value by 1e3, 1e6, 1e9, respectively
>> - * '/'  optional gdb-like print format (like "/10x")
>> + * TYPEs that put a string value with key NAME into the QDict:
>> + * 's'Argument is enclosed in '"' or delimited by whitespace.  In
>> + *the former case, escapes \n \r \\ \' and \" are recognized.
>> + * 'F'File name, like 's' except for completion.
>> + * 'B'BlockBackend name, like 's' except for completion.
>> + * 'S'Argument is the remainder of the line, less leading
>> + *whitespace.
>> +
>>   *
>> - * '?'  optional type (for all types, except '/')
>> - * '.'  other form of optional type (for 'i' and 'l')
>> - * 'b'  boolean
>> - *  user mode accepts "on" or "off"
>> - * '-'  optional parameter (eg. '-f')
>> + * TYPEs that put an int64_t value with key NAME:
>> + * 'l'Argument is an expression (QEMU pocket calculator).
>> + * 'i'Like 'l' except value must fit into 32 bit unsigned.
>> + * 'M'Like 'l' except value must not be negative and is multiplied
>> + *by 2^20 (think "mebibyte").
>>   *
>> + * TYPEs that put an uint64_t value with key NAME:
>> + * 'o'Argument is a size (think "octets").  Without suffix the
>> + *value is multiplied by 2^20 (mebibytes), with suffix E or e
>> + *by 2^60 (exbibytes), with P or p by 2^50 (pebibytes), with T
>> + *or t by 2^40 (tebibytes), with G or g by 2^30 (gibibytes),
>> + *with M or m by 2^10 (mebibytes), with K or k by 2^10
>> + *(kibibytes).
>
> 'o' is messy.  It using qemu_strtosz_MiB which uses a 'double' intermediate
> so I fear it can round.

It does, but only when you have more than 53 significant bits.

>  It also has a note it can't take all f's due to
> an overflow from the conversion.

Correct, because values between 0xfc00 and 2^64-1 round up
to 2^64.

If it bothers you, feel free to explore the following: feed the string
both to strtod() and to strtoll().  Whichever eats more characters wins.

This patch is of course just about better documenting what we have.  I
was starting to type something like "repeating the (complex) contract of
qemu_strtosz_MiB() here isn't so hot, let's include it by reference
instead", but then I looked it up.  Pffft.

>Two things not mentioned are that
> it also takes hex (as explicit 0x) and that it also does 'b' as a suffix
> to multiply by 1.  Those two combine in bad ways - i.e. 0x1b is 27MB,
> 1b is 1 byte (same for 'e').  These are probably OK except if you were
> to start replacing 'l' by 'o' because you really wanted 64bit addresses
> say.

I guess the sanest solution is not to recognize suffixes when the number
is hexadecimal.

> (I also wouldn't bother expanding the size names and powers).

I erred on the side of tedious clarity.  Feel free to suggest something
you like better.

>> + *
>> + * TYPEs that put a double value with key NAME:
>> + * 'T'Argument is a time 

Re: [Qemu-block] [RFC PATCH 32/56] hmp: Make block_set_io_throttle's arguments unsigned

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> The previous commit made them unsigned in QMP.  Switch HMP's args_type
> from 'l' to 'o'.  Loses support for expressions (QEMU pocket
> calculator), gains support for unit suffixes.  Negative values are no
> longer accepted and interpreted modulo 2^64.  Instead, values between
> 2^63 and 2^64-1 are now accepted.

But that also means all these values are assumed to be in MB by default?

Dave

> Signed-off-by: Markus Armbruster 
> ---
>  hmp-commands.hx | 2 +-
>  hmp.c   | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 46ce79c..bc3c066 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1668,7 +1668,7 @@ ETEXI
>  
>  {
>  .name   = "block_set_io_throttle",
> -.args_type  = 
> "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l",
> +.args_type  = 
> "device:B,bps:o,bps_rd:o,bps_wr:o,iops:l,iops_rd:l,iops_wr:l",
>  .params = "device bps bps_rd bps_wr iops iops_rd iops_wr",
>  .help   = "change I/O throttle limits for a block drive",
>  .cmd= hmp_block_set_io_throttle,
> diff --git a/hmp.c b/hmp.c
> index 3253674..599e816 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1764,9 +1764,9 @@ void hmp_block_set_io_throttle(Monitor *mon, const 
> QDict *qdict)
>  BlockIOThrottle throttle = {
>  .has_device = true,
>  .device = (char *) qdict_get_str(qdict, "device"),
> -.bps = qdict_get_int(qdict, "bps"),
> -.bps_rd = qdict_get_int(qdict, "bps_rd"),
> -.bps_wr = qdict_get_int(qdict, "bps_wr"),
> +.bps = qdict_get_uint(qdict, "bps"),
> +.bps_rd = qdict_get_uint(qdict, "bps_rd"),
> +.bps_wr = qdict_get_uint(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"),
> -- 
> 2.7.5
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-block] [PATCH v2 for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Vladimir Sementsov-Ogievskiy]

Do not communicate after the first error to avoid communicating throught
broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
in nbd_client_close.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all. Here is a patch, fixing a problem noted in
[PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
and
[PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
channel error
and discussed on list.

If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring and 
fixing'
on it (for 2.11). If not - I'll prefer not rebase the series, so, do not apply 
this
patch for 2.11.

v2: set eio_to_all in nbd_co_send_request and nbd_co_receive_reply in case of 
error

 block/nbd-client.h |  1 +
 block/nbd-client.c | 65 +++---
 2 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..28db9922c8 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {
 
 Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
 NBDReply reply;
+bool eio_to_all;
 } NBDClientSession;
 
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..a72cb7690a 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 
+client->eio_to_all = true;
+
 if (!client->ioc) { /* Already closed */
 return;
 }
@@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 Error *local_err = NULL;
 
 for (;;) {
+if (s->eio_to_all) {
+break;
+}
+
 assert(s->reply.handle == 0);
 ret = nbd_receive_reply(s->ioc, >reply, _err);
 if (ret < 0) {
 error_report_err(local_err);
 }
-if (ret <= 0) {
+if (ret <= 0 || s->eio_to_all) {
 break;
 }
 
@@ -107,6 +113,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 qemu_coroutine_yield();
 }
 
+s->eio_to_all = true;
 nbd_recv_coroutines_enter_all(s);
 s->read_reply_co = NULL;
 }
@@ -118,6 +125,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
 NBDClientSession *s = nbd_get_client_session(bs);
 int rc, ret, i;
 
+if (s->eio_to_all) {
+return -EIO;
+}
+
 qemu_co_mutex_lock(>send_mutex);
 while (s->in_flight == MAX_NBD_REQUESTS) {
 qemu_co_queue_wait(>free_sema, >send_mutex);
@@ -135,15 +146,15 @@ static int nbd_co_send_request(BlockDriverState *bs,
 assert(i < MAX_NBD_REQUESTS);
 request->handle = INDEX_TO_HANDLE(s, i);
 
-if (!s->ioc) {
+if (s->eio_to_all) {
 qemu_co_mutex_unlock(>send_mutex);
-return -EPIPE;
+return -EIO;
 }
 
 if (qiov) {
 qio_channel_set_cork(s->ioc, true);
 rc = nbd_send_request(s->ioc, request);
-if (rc >= 0) {
+if (rc >= 0 && !s->eio_to_all) {
 ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
   NULL);
 if (ret != request->len) {
@@ -155,7 +166,13 @@ static int nbd_co_send_request(BlockDriverState *bs,
 rc = nbd_send_request(s->ioc, request);
 }
 qemu_co_mutex_unlock(>send_mutex);
-return rc;
+
+if (rc < 0 || s->eio_to_all) {
+s->eio_to_all = true;
+return -EIO;
+}
+
+return 0;
 }
 
 static void nbd_co_receive_reply(NBDClientSession *s,
@@ -169,14 +186,16 @@ static void nbd_co_receive_reply(NBDClientSession *s,
 qemu_coroutine_yield();
 *reply = s->reply;
 if (reply->handle != request->handle ||
-!s->ioc) {
+!s->ioc || s->eio_to_all) {
 reply->error = EIO;
+s->eio_to_all = true;
 } else {
 if (qiov && reply->error == 0) {
 ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, true,
   NULL);
-if (ret != request->len) {
+if (ret != request->len || s->eio_to_all) {
 reply->error = EIO;
+s->eio_to_all = true;
 }
 }
 
@@ -225,8 +244,10 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t 
offset,
 } else {
 nbd_co_receive_reply(client, , , qiov);
 }
-nbd_coroutine_end(bs, );
-return -reply.error;
+if (request.handle != 0) {
+nbd_coroutine_end(bs, );
+}
+return client->eio_to_all ? -EIO : -reply.error;
 }
 
 int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
@@ -254,8 +275,10 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t 
offset,
 } else {
 nbd_co_receive_reply(client, , , NULL);
 }
-nbd_coroutine_end(bs, );
-return -reply.error;
+if (request.handle 

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10] throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

[Alberto Garcia]

On Tue 08 Aug 2017 05:11:27 PM CEST, Eric Blake wrote:
>> Why is this marked for-2.10?  Does it fix a bug?
>
> Theoretically, converting between int64_t and double loses precision
> on any values larger than 2^53.  In all practicality, though, if you
> expect throttling to be precise through 2^53 (approximately 16 orders
> of magnitude), you're crazy.

You cannot, THROTTLE_VALUE_MAX is set to 10^15, which is smaller than
2^50.

> So I'm also having a hard time seeing this as 2.10 material.

Right, as I said that was a wrong assumption from my side :) No need for
this in 2.10.

Berto

Re: [Qemu-block] [RFC PATCH 11/56] monitor: Drop unused HMP .args_type 'M'

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> The previous commit switched balloon from 'M' to 'o', rendering 'M'
> unused.  It was never used for anything else.  Drop it.
> 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  monitor.c | 11 +--
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 8b54ba1..3b2757e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -101,8 +101,6 @@
>   * TYPEs that put an int64_t value with key NAME:
>   * 'l'Argument is an expression (QEMU pocket calculator).
>   * 'i'Like 'l' except value must fit into 32 bit unsigned.
> - * 'M'Like 'l' except value must not be negative and is multiplied
> - *by 2^20 (think "mebibyte").
>   *
>   * TYPEs that put an uint64_t value with key NAME:
>   * 'o'Argument is a size (think "octets").  Without suffix the
> @@ -134,7 +132,7 @@
>   * '?'Argument is optional, nothing is put when it is absent
>   *(all types except 'O', '/', 'b').
>   * '.'Argument is optional, must be preceded by '.' if present
> - *(only types 'i', 'l', 'M')
> + *(only types 'i', 'l')
>   */
>  
>  typedef struct mon_cmd_t {
> @@ -2913,7 +2911,6 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>  break;
>  case 'i':
>  case 'l':
> -case 'M':
>  {
>  int64_t val;
>  
> @@ -2944,12 +2941,6 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>  monitor_printf(mon, "\'%s\' has failed: ", cmd->name);
>  monitor_printf(mon, "integer is for 32-bit values\n");
>  goto fail;
> -} else if (c == 'M') {
> -if (val < 0) {
> -monitor_printf(mon, "enter a positive value\n");
> -goto fail;
> -}
> -val <<= 20;
>  }
>  qdict_put_int(qdict, key, val);
>  }
> -- 
> 2.7.5
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: fix 185

[Kevin Wolf]

Am 08.08.2017 um 17:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 08.08.2017 18:07, Eric Blake wrote:
> > On 08/08/2017 04:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 
> > > > > > Throttling "guaranties" that there will not be more than one
> > > > > > request. But
> > > > > > what prevent less than one, i.e. zero, like in my reproduction?
> > > > > Yes, I understand. Can we somehow make sure that at least one 
> > > > > iteration
> > > > > is made? I'd really like to keep the functional test for block job
> > > > > throttling. I suppose a simple 'sleep 0.1' would do the trick, though
> > > > > it's not very clean.
> > > > > 
> > > > > Kevin
> > > > 
> > > > I've started with 'sleep 0.5', now there are >100 successful
> > > > iterations... The other way is to check in test that there was 0 or 1
> > > > requests, but for this it looks better to rewrite it in python.
> > > > 
> > > > 
> > > is sleep for ms portable?
> > Sadly, sub-second sleep is a GNU coreutils feature; I suspect the BSD
> > machines may fail to parse it.  (Of course, we could do some sort of
> > 'sleep $SMALL', where $SMALL is 0.5 if sleep supports it, and 1 otherwise).
> > 
> sleep for 1 second may lead to more then one request done before qemu quite.

_supported_os Linux

So do we really care about portability? And are all the other test cases
working on the BSDs?

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10] throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

[Eric Blake]

On 08/08/2017 05:00 AM, Stefan Hajnoczi wrote:
> On Mon, Aug 07, 2017 at 07:15:29PM +0300, Alberto Garcia wrote:
>> Both the throttling limits set with the throttling.iops-* and
>> throttling.bps-* options and their QMP equivalents defined in the
>> BlockIOThrottle struct are integer values.
>>
>> Those limits are also reported in the BlockDeviceInfo struct and they
>> are integers there as well.
>>
>> Therefore there's no reason to store them internally as double and do
>> the conversion everytime we're setting or querying them, so this patch
>> uses int64_t for those types.
>>

> Why is this marked for-2.10?  Does it fix a bug?

Theoretically, converting between int64_t and double loses precision on
any values larger than 2^53.  In all practicality, though, if you expect
throttling to be precise through 2^53 (approximately 16 orders of
magnitude), you're crazy.  I also don't see any change to potential
division-by-zero actions (where differences in NaN vs. SIGFPE each have
their own advantages, but changing between them can be construed as a
bug fix).  So I'm also having a hard time seeing this as 2.10 material.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: fix 185

[Vladimir Sementsov-Ogievskiy]

08.08.2017 18:07, Eric Blake wrote:

On 08/08/2017 04:04 AM, Vladimir Sementsov-Ogievskiy wrote:


Throttling "guaranties" that there will not be more than one
request. But
what prevent less than one, i.e. zero, like in my reproduction?

Yes, I understand. Can we somehow make sure that at least one iteration
is made? I'd really like to keep the functional test for block job
throttling. I suppose a simple 'sleep 0.1' would do the trick, though
it's not very clean.

Kevin


I've started with 'sleep 0.5', now there are >100 successful
iterations... The other way is to check in test that there was 0 or 1
requests, but for this it looks better to rewrite it in python.



is sleep for ms portable?

Sadly, sub-second sleep is a GNU coreutils feature; I suspect the BSD
machines may fail to parse it.  (Of course, we could do some sort of
'sleep $SMALL', where $SMALL is 0.5 if sleep supports it, and 1 otherwise).


sleep for 1 second may lead to more then one request done before qemu quite.


--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: fix 185

[Eric Blake]

On 08/08/2017 04:04 AM, Vladimir Sementsov-Ogievskiy wrote:

 Throttling "guaranties" that there will not be more than one
 request. But
 what prevent less than one, i.e. zero, like in my reproduction?
>>> Yes, I understand. Can we somehow make sure that at least one iteration
>>> is made? I'd really like to keep the functional test for block job
>>> throttling. I suppose a simple 'sleep 0.1' would do the trick, though
>>> it's not very clean.
>>>
>>> Kevin
>>
>>
>> I've started with 'sleep 0.5', now there are >100 successful
>> iterations... The other way is to check in test that there was 0 or 1
>> requests, but for this it looks better to rewrite it in python.
>>
>>
> 
> is sleep for ms portable?

Sadly, sub-second sleep is a GNU coreutils feature; I suspect the BSD
machines may fail to parse it.  (Of course, we could do some sort of
'sleep $SMALL', where $SMALL is 0.5 if sleep supports it, and 1 otherwise).

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 5/7] block: add throttle block filter driver

[Alberto Garcia]

On Tue 08 Aug 2017 04:56:20 PM CEST, Manos Pitsidianakis wrote:
>>> So basically if we have anonymous groups, we accept limits in the
>>> driver options but only without a group-name.
>>
>>In the commit message you do however have limits and a group name, is
>>that a mistake?
>>
>>-drive driver=throttle,file.filename=foo.qcow2, \
>>   limits.iops-total=...,throttle-group=bar
>
> Sorry this wasn't clear, I'm actually proposing to remove limits from
> the throttle driver options and only create/config throttle groups via
> -object/object-add.

Sorry I think it was me who misunderstood :-) Anyway in the new
command-line API I would be more inclined to have limits defined using
"-object throttle-group" and -drive would only reference the group id.

I understand that this implies that it wouldn't be possible to create
anonymous groups (at least not from the command line), is that a
problem?

Berto

Re: [Qemu-block] [PATCH] block: document semanatics of bdrv_co_preadv|pwritev

[Eric Blake]

On 08/08/2017 04:13 AM, Daniel P. Berrange wrote:
> On Tue, Aug 08, 2017 at 10:39:29AM +0800, Fam Zheng wrote:
>> On Fri, 08/04 16:49, Daniel P. Berrange wrote:
>>> This is odd.  In the bdrv_aligned_readv() it looks very much like
>>> we'll reference qiov->niov, if bytes != 0, so if qiov was NULL we
>>> would crash.
>>
>> It doesn't make sense if read doesn't have an iov, where should the data be
>> placed? :)
>>

> We can't remove it from the bdrv_co_pwritev() function, but we can remove
> it from bdrv_co_pwritev block driver callback AFAICT.

Sounds like a separate cleanup series to remove the length parameter for
both read and write (since we have write_zeroes), for the 2.11
timeframe.  You're right that the block layer does not have to have
quite the same interface as the driver callbacks.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Vladimir Sementsov-Ogievskiy]

08.08.2017 17:44, Eric Blake wrote:

On 08/08/2017 09:29 AM, Vladimir Sementsov-Ogievskiy wrote:

Do not communicate after the first error to avoid communicating throught
broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
in nbd_client_close.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all. Here is a patch, fixing a problem noted in
[PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
and
[PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
channel error
and discussed on list.

If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring and 
fixing'
on it (for 2.11). If not - I'll prefer not rebase the series, so, do not apply 
this
patch for 2.11.

It may be possible to do something even smaller:


  block/nbd-client.h |  1 +
  block/nbd-client.c | 58 +-
  2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..28db9922c8 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {
  
  Coroutine *recv_coroutine[MAX_NBD_REQUESTS];

  NBDReply reply;
+bool eio_to_all;
  } NBDClientSession;
  
  NBDClientSession *nbd_get_client_session(BlockDriverState *bs);

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..1282b2484e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
  {
  NBDClientSession *client = nbd_get_client_session(bs);
  
+client->eio_to_all = true;

+
  if (!client->ioc) { /* Already closed */
  return;
  }
@@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  Error *local_err = NULL;
  
  for (;;) {

+if (s->eio_to_all) {
+break;
+}
+

How is this conditional ever reached? nbd_read_reply_entry() is our
workhorse, that is supposed to run until the client is ready to disconnect.


I forget to set eio_to_all on error in sending request.




  assert(s->reply.handle == 0);
  ret = nbd_receive_reply(s->ioc, >reply, _err);
  if (ret < 0) {
  error_report_err(local_err);
  }
-if (ret <= 0) {
+if (ret <= 0 || s->eio_to_all) {
  break;
  }

This says we're now supposed to break out of the while loop.  So unless
something can set s->eio_to_all during
aio_co_wake(s->recv_coroutine[i]), the next iteration of the loop won't
hit the first conditional.

Meanwhile, even if we skip the first conditional, this second
conditional will still end the loop prior to acting on anything received
from the server (the difference being whether we called
nbd_receive_reply() one additional time, but I don't see that as getting
in the way of a clean exit).

But my question is whether we can just go with a simpler fix: if we ever
break out of the workhorse loop of nbd_read_reply_entry(), then (and
only then) is when we call nbd_recv_coroutines_enter_all() to mop up any
pending transactions before tearing down the coroutines.  Is there
something we can do in nbd_recv_coroutines_enter_all() that will
guarantee that our final entry into each coroutine will fail?


anyway, you should prevent the following new requests, using some 
additional variable or disconnect..


Also, I think, an error in sending requests should cause finishing of 
this loop in nbd_read_reply_entry, so, an error condition should be 
checked after each yield.





I'm still playing with the idea locally.



--
Best regards,
Vladimir

Re: [Qemu-block] [RFC PATCH 09/56] balloon: Make balloon size unsigned in QAPI/QMP

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Sizes should use QAPI type 'size' (uint64_t).  balloon parameter
> @value is 'int' (int64_t).  qmp_balloon() implicitly converts to
> ram_addr_t, i.e. uint64_t.  BALLOON_CHANGE parameter @actual and
> BalloonInfo member @actual are also 'int'.
> virtio_balloon_set_config() and virtio_balloon_stat() implicitly
> convert from ram_addr_t.
> 
> Change all three to 'size', and adjust the code using them.
> 
> balloon now accepts size values between 2^63 and 2^64-1.  It accepts
> negative values as before, because that's how the QObject input
> visitor works for backward compatibility.
> 
> Doing the same for HMP's balloon deserves its own commit (the next
> one).
> 
> BALLOON_CHANGE and query-balloon now report sizes above 2^63-1
> correctly instead of their (negative) two's complement.
> 
> So does HMP's "info balloon".
> 
> Signed-off-by: Markus Armbruster 
> ---
>  balloon.c| 2 +-
>  hmp.c| 2 +-
>  qapi-schema.json | 4 ++--
>  qapi/event.json  | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/balloon.c b/balloon.c
> index 1d720ff..2ecca76 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -102,7 +102,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
>  return info;
>  }
>  
> -void qmp_balloon(int64_t target, Error **errp)
> +void qmp_balloon(uint64_t target, Error **errp)
>  {
>  if (!have_balloon(errp)) {
>  return;

Can't you remove the:
  if (target <= 0) {

check?

(The type of the trace_balloon_event probably needs fixing
to be uint64_t rather than the unsigned long)

> diff --git a/hmp.c b/hmp.c
> index 8257dd0..4556045 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -781,7 +781,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict)
>  return;
>  }
>  
> -monitor_printf(mon, "balloon: actual=%" PRId64 "\n", info->actual >> 20);
> +monitor_printf(mon, "balloon: actual=%" PRIu64 "\n", info->actual >> 20);
>  
>  qapi_free_BalloonInfo(info);
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3ad2bc0..23eb60d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2003,7 +2003,7 @@
>  # Since: 0.14.0
>  #
>  ##
> -{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
> +{ 'struct': 'BalloonInfo', 'data': {'actual': 'size' } }
>  
>  ##
>  # @query-balloon:
> @@ -2603,7 +2603,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'balloon', 'data': {'value': 'int'} }
> +{ 'command': 'balloon', 'data': {'value': 'size'} }
>  
>  ##
>  # @Abort:
> diff --git a/qapi/event.json b/qapi/event.json
> index 6d22b02..9dfc70b 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -488,7 +488,7 @@
>  #
>  ##
>  { 'event': 'BALLOON_CHANGE',
> -  'data': { 'actual': 'int' } }
> +  'data': { 'actual': 'size' } }

I was going to ask whether that was a problem for any external users,
but there again libvirt looks like it reads it into an unsigned long
long.

Dave

>  ##
>  # @GUEST_PANICKED:
> -- 
> 2.7.5
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-block] [PATCH v3 5/7] block: add throttle block filter driver

[Manos Pitsidianakis]

On Tue, Aug 08, 2017 at 04:53:08PM +0200, Alberto Garcia wrote:

On Tue 08 Aug 2017 03:45:44 PM CEST, Manos Pitsidianakis wrote:

On Tue, Aug 08, 2017 at 03:13:36PM +0200, Alberto Garcia wrote:

On Mon 31 Jul 2017 11:54:41 AM CEST, Manos Pitsidianakis wrote:

block/throttle.c uses existing I/O throttle infrastructure inside a
block filter driver. I/O operations are intercepted in the filter's
read/write coroutines, and referred to block/throttle-groups.c

The driver can be used with the syntax
-drive driver=throttle,file.filename=foo.qcow2, \
limits.iops-total=...,throttle-group=bar


Sorry for not having noticed this earlier, but can't you define the
throttling group (and its limits) using -object throttle-group ... as
shown in the previous patch, and simply reference it here? Or would we
have two alternative ways of setting the throttling limits?

What happens if you have many -drive lines each one with a different set
of limits but with the same throttling group?


The limits of the last one to be processed will win.


That's what the current throttling API does, and I tend to agree that
it's not completely straightforward (a few people have asked me the same
question since this feature is available).

If we're going to add a new API we could eliminate this ambiguity. After
all the previous -drive throttling.iops-total=... would still be
available, wouldn't it?


Indeed, it already is.




So basically if we have anonymous groups, we accept limits in the
driver options but only without a group-name.


In the commit message you do however have limits and a group name, is
that a mistake?

   -drive driver=throttle,file.filename=foo.qcow2, \
  limits.iops-total=...,throttle-group=bar



Sorry this wasn't clear, I'm actually proposing to remove limits from 
the throttle driver options and only create/config throttle groups via 
-object/object-add.


signature.asc
Description: PGP signature

Re: [Qemu-block] [RFC PATCH 27/56] block/dirty-bitmap: Clean up signed vs. unsigned dirty counts

[Eric Blake]

On 08/07/2017 09:45 AM, Markus Armbruster wrote:
> hbitmap_count() returns uint64_t.
> 
> Clean up test-hbitmap.c to check its value with g_assert_cmpuint()
> instead of g_assert_cmpint().
> 
> bdrv_get_dirty_count() and bdrv_get_meta_dirty_count() return its
> value converted to int64_t.  Clean them up to return it unadulterated.
> 
> This moves the implicit conversion to some callers, so clean them up,
> too.
> 
> mirror_run() assigns the value of bdrv_get_meta_dirty_count() to a
> local int64_t variable.  Change it to uint64_t.  Signedness still gets
> mixed up in the computation of s->common.len, but messing with that is
> more than I can handle right now.
> 
> get_remaining_dirty() tallies bdrv_get_dirty_count() values in
> int64_t.  Its caller block_save_pending() converts it back to
> uint64_t.  Change get_remaining_dirty() to uint64_t.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  block/dirty-bitmap.c |  4 ++--
>  block/mirror.c   |  4 ++--
>  block/trace-events   |  8 
>  include/block/dirty-bitmap.h |  4 ++--
>  migration/block.c|  4 ++--
>  tests/test-hbitmap.c | 16 +---
>  6 files changed, 21 insertions(+), 19 deletions(-)

I don't know how much this will conflict with my pending work for
byte-based block status, but I suspect it may be easier for your RFC to
go in after my cleanups (I think you'll still have things to fix).

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 28/56] block: Widen dirty bitmap granularity to uint64_t for safety

[Eric Blake]

On 08/07/2017 08:55 PM, John Snow wrote:
> 
> 
> On 08/07/2017 10:45 AM, Markus Armbruster wrote:
>> Block dirty bitmaps represent granularity in bytes as uint32_t.  It
>> must be a power of two and a multiple of BDRV_SECTOR_SIZE.
>>
>> The trouble with uint32_t is computations like this one in
>> mirror_do_read():
>>
>> uint64_t max_bytes;
>>
>> max_bytes = s->granularity * s->max_iov;
>>
>> The operands of * are uint32_t and int, so the product is computed in
>> uint32_t (assuming 32 bit int), then zero-extended to uint64_t.
>>
>> Since granularity is generally combined with 64 bit file offsets, it's
>> best to make it 64 bits, too.  Less opportunity to screw up.

And definitely conflicts with my work on byte-based block status.


>>  
>> -uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap)
>> -{
>> -return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->meta);
>> -}
> 
> Why? Unused? Not cool enough to mention?

Already deleted as unused in my byte-based series.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 5/7] block: add throttle block filter driver

[Alberto Garcia]

On Tue 08 Aug 2017 03:45:44 PM CEST, Manos Pitsidianakis wrote:
> On Tue, Aug 08, 2017 at 03:13:36PM +0200, Alberto Garcia wrote:
>>On Mon 31 Jul 2017 11:54:41 AM CEST, Manos Pitsidianakis wrote:
>>> block/throttle.c uses existing I/O throttle infrastructure inside a
>>> block filter driver. I/O operations are intercepted in the filter's
>>> read/write coroutines, and referred to block/throttle-groups.c
>>>
>>> The driver can be used with the syntax
>>> -drive driver=throttle,file.filename=foo.qcow2, \
>>> limits.iops-total=...,throttle-group=bar
>>
>>Sorry for not having noticed this earlier, but can't you define the
>>throttling group (and its limits) using -object throttle-group ... as
>>shown in the previous patch, and simply reference it here? Or would we
>>have two alternative ways of setting the throttling limits?
>>
>>What happens if you have many -drive lines each one with a different set
>>of limits but with the same throttling group?
>
> The limits of the last one to be processed will win.

That's what the current throttling API does, and I tend to agree that
it's not completely straightforward (a few people have asked me the same
question since this feature is available).

If we're going to add a new API we could eliminate this ambiguity. After
all the previous -drive throttling.iops-total=... would still be
available, wouldn't it?

> So basically if we have anonymous groups, we accept limits in the
> driver options but only without a group-name.

In the commit message you do however have limits and a group name, is
that a mistake?

-drive driver=throttle,file.filename=foo.qcow2, \
   limits.iops-total=...,throttle-group=bar

Berto

Re: [Qemu-block] [PATCH v4] tests: Avoid non-portable 'echo -ARG'

[Kevin Wolf]

Am 08.08.2017 um 16:29 hat Eric Blake geschrieben:
> On 08/08/2017 08:54 AM, Kevin Wolf wrote:
> > Am 03.07.2017 um 20:09 hat Eric Blake geschrieben:
> >> POSIX says that backslashes in the arguments to 'echo', as well as
> >> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
> >> people should favor 'printf' instead.  This is definitely true where
> >> we do not control which shell is running (such as in makefile snippets
> >> or in documentation examples).  But even for scripts where we
> >> require bash (and therefore, where echo does what we want by default),
> >> it is still possible to use 'shopt -s xpg_echo' to change bash's
> >> behavior of echo.  And setting a good example never hurts when we are
> >> not sure if a snippet will be copied from a bash-only script to a
> >> general shell script (although I don't change the use of non-portable
> >> \e for ESC when we know the running shell is bash).
> >>
> 
> >> +++ b/tests/multiboot/run_test.sh
> >> @@ -26,7 +26,7 @@ run_qemu() {
> >>  local kernel=$1
> >>  shift
> >>
> >> -echo -e "\n\n=== Running test case: $kernel $@ ===\n" >> test.log
> >> +printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log
> >>
> >>  $QEMU \
> >>  -kernel $kernel \
> > 
> > Not completely sure why, but this broke the test with whitespace changes
> > like this:
> > 
> > -=== Running test case: mmap.elf -m 1.1M ===
> > +=== Running test case: mmap.elf -m1.1M ===
> 
> I guess that means I'm not regularly running tests/multiboot?  Is it not
> part of 'make check' or qemu-iotests?

The problem is that it needs an i386 compiler to build the test kernels
(and qemu-system-i386 or qemu-system-x86_64 binaries to execute them).

I guess we could check these conditions, though, and skip the test if we
can't produce i386 binaries.

> Ah, I see the problem, and it's insidious.  We're using "...$@...", but
> want to be using "...$*...".  $@ causes multiple arguments to be passed,
> but printf %b is not concatenating those arguments; while $* uses only a
> single argument.  We didn't notice it with echo -e, because echo inserts
> a space between multiple arguments, just as you'd get a space with $*.

The thing that completely confused me here is that printf doesn't just
ignore additional arguments as I would have expected, but just starts
over with the format string, so that it does kind of work with multiple
arguments and fails only subtly.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [RFC PATCH 03/56] monitor: Rewrite comment describing HMP .args_type

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonz...@redhat.com) wrote:
> On 08/08/2017 13:20, Dr. David Alan Gilbert wrote:
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  monitor.c | 75 
> >> +++
> >>  1 file changed, 47 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index e0f8801..8b54ba1 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -85,37 +85,56 @@
> >>  #endif
> >>  
> >>  /*
> >> - * Supported types:
> >> + * Command handlers (mon_cmd_t member @cmd) receive actual arguments
> >> + * in a QDict, which is built by the HMP core according to mon_cmd_t
> >> + * member @args_type.  It's a list of NAME:TYPE separated by comma.
> >>   *
> >> - * 'F'  filename
> >> - * 'B'  block device name
> >> - * 's'  string (accept optional quote)
> >> - * 'S'  it just appends the rest of the string (accept optional 
> >> quote)
> >> - * 'O'  option string of the form NAME=VALUE,...
> >> - *  parsed according to QemuOptsList given by its name
> >> - *  Example: 'device:O' uses qemu_device_opts.
> >> - *  Restriction: only lists with empty desc are supported
> >> - *  TODO lift the restriction
> >> - * 'i'  32 bit integer
> >> - * 'l'  target long (32 or 64 bit)
> >> - * 'M'  Non-negative target long (32 or 64 bit), in user mode the
> >> - *  value is multiplied by 2^20 (think Mebibyte)
> >> - * 'o'  octets (aka bytes)
> >> - *  user mode accepts an optional E, e, P, p, T, t, G, g, M, 
> >> m,
> >> - *  K, k suffix, which multiplies the value by 2^60 for 
> >> suffixes E
> >> - *  and e, 2^50 for suffixes P and p, 2^40 for suffixes T and 
> >> t,
> >> - *  2^30 for suffixes G and g, 2^20 for M and m, 2^10 for K 
> >> and k
> >> - * 'T'  double
> >> - *  user mode accepts an optional ms, us, ns suffix,
> >> - *  which divides the value by 1e3, 1e6, 1e9, respectively
> >> - * '/'  optional gdb-like print format (like "/10x")
> >> + * TYPEs that put a string value with key NAME into the QDict:
> >> + * 's'Argument is enclosed in '"' or delimited by whitespace.  In
> >> + *the former case, escapes \n \r \\ \' and \" are recognized.
> >> + * 'F'File name, like 's' except for completion.
> >> + * 'B'BlockBackend name, like 's' except for completion.
> >> + * 'S'Argument is the remainder of the line, less leading
> >> + *whitespace.
> >> +
> >>   *
> >> - * '?'  optional type (for all types, except '/')
> >> - * '.'  other form of optional type (for 'i' and 'l')
> >> - * 'b'  boolean
> >> - *  user mode accepts "on" or "off"
> >> - * '-'  optional parameter (eg. '-f')
> >> + * TYPEs that put an int64_t value with key NAME:
> >> + * 'l'Argument is an expression (QEMU pocket calculator).
> >> + * 'i'Like 'l' except value must fit into 32 bit unsigned.
> >> + * 'M'Like 'l' except value must not be negative and is multiplied
> >> + *by 2^20 (think "mebibyte").
> >>   *
> >> + * TYPEs that put an uint64_t value with key NAME:
> >> + * 'o'Argument is a size (think "octets").  Without suffix the
> >> + *value is multiplied by 2^20 (mebibytes), with suffix E or e
> >> + *by 2^60 (exbibytes), with P or p by 2^50 (pebibytes), with T
> >> + *or t by 2^40 (tebibytes), with G or g by 2^30 (gibibytes),
> >> + *with M or m by 2^10 (mebibytes), with K or k by 2^10
> >> + *(kibibytes).
> > 
> > 'o' is messy.  It using qemu_strtosz_MiB which uses a 'double' intermediate
> > so I fear it can round.  It also has a note it can't take all f's due to
> > an overflow from the conversion.   Two things not mentioned are that
> > it also takes hex (as explicit 0x) and that it also does 'b' as a suffix
> > to multiply by 1.  Those two combine in bad ways - i.e. 0x1b is 27MB,
> > 1b is 1 byte (same for 'e').  These are probably OK except if you were
> > to start replacing 'l' by 'o' because you really wanted 64bit addresses
> > say.
> > (I also wouldn't bother expanding the size names and powers).
> > 
> >> + *
> >> + * TYPEs that put a double value with key NAME:
> >> + * 'T'Argument is a time in seconds.  With optional ms, us, ns
> >> + *suffix, the value divided by 1e3, 1e6, 1e9 respectively.
> >> + *
> >> + * TYPEs that put a bool value with key NAME:
> >> + * 'b'Argument is either "on" (true) or "off" (false).
> >> + * '-' CHAR
> >> + *Argument is either "-CHAR" (true) or absent (false).
> > 
> > I found the previous description clearer.
> > 
> >> + * TYPEs that put multiple values:
> >> + * 'O'Option string of the form NAME=VALUE,... parsed according to
> >> + *the QemuOptsList given by its name.
> >> + *  

Re: [Qemu-block] [PATCH for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Eric Blake]

On 08/08/2017 09:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> Do not communicate after the first error to avoid communicating throught
> broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
> in nbd_client_close.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
> 
> Hi all. Here is a patch, fixing a problem noted in
> [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
> and
> [PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
> channel error
> and discussed on list.
> 
> If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring 
> and fixing'
> on it (for 2.11). If not - I'll prefer not rebase the series, so, do not 
> apply this
> patch for 2.11.

It may be possible to do something even smaller:

> 
>  block/nbd-client.h |  1 +
>  block/nbd-client.c | 58 
> +-
>  2 files changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index df80771357..28db9922c8 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -29,6 +29,7 @@ typedef struct NBDClientSession {
>  
>  Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
>  NBDReply reply;
> +bool eio_to_all;
>  } NBDClientSession;
>  
>  NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 25dd28406b..1282b2484e 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>  {
>  NBDClientSession *client = nbd_get_client_session(bs);
>  
> +client->eio_to_all = true;
> +
>  if (!client->ioc) { /* Already closed */
>  return;
>  }
> @@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void 
> *opaque)
>  Error *local_err = NULL;
>  
>  for (;;) {
> +if (s->eio_to_all) {
> +break;
> +}
> +

How is this conditional ever reached? nbd_read_reply_entry() is our
workhorse, that is supposed to run until the client is ready to disconnect.

>  assert(s->reply.handle == 0);
>  ret = nbd_receive_reply(s->ioc, >reply, _err);
>  if (ret < 0) {
>  error_report_err(local_err);
>  }
> -if (ret <= 0) {
> +if (ret <= 0 || s->eio_to_all) {
>  break;
>  }

This says we're now supposed to break out of the while loop.  So unless
something can set s->eio_to_all during
aio_co_wake(s->recv_coroutine[i]), the next iteration of the loop won't
hit the first conditional.

Meanwhile, even if we skip the first conditional, this second
conditional will still end the loop prior to acting on anything received
from the server (the difference being whether we called
nbd_receive_reply() one additional time, but I don't see that as getting
in the way of a clean exit).

But my question is whether we can just go with a simpler fix: if we ever
break out of the workhorse loop of nbd_read_reply_entry(), then (and
only then) is when we call nbd_recv_coroutines_enter_all() to mop up any
pending transactions before tearing down the coroutines.  Is there
something we can do in nbd_recv_coroutines_enter_all() that will
guarantee that our final entry into each coroutine will fail?

I'm still playing with the idea locally.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Vladimir Sementsov-Ogievskiy]

08.08.2017 17:29, Vladimir Sementsov-Ogievskiy wrote:

Do not communicate after the first error to avoid communicating throught
broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
in nbd_client_close.


worth add: To simplify things, return -EIO in case of disconnect too.

(refers to:

-if (!s->ioc) {
+if (s->eio_to_all) {
 qemu_co_mutex_unlock(>send_mutex);
-return -EPIPE;
+return -EIO;
 }

and to:

@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 
+client->eio_to_all = true;

+

However, I'm unsure about, how s->ioc may be NULL in first chunk, and if 
it possible, why
it is not checked everywhere. read/write after bdrv_close? Should it be 
handled on higher level?)




Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all. Here is a patch, fixing a problem noted in
[PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
and
[PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
channel error
and discussed on list.

If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring and 
fixing'
on it (for 2.11). If not - I'll prefer not rebase the series, so, do not apply 
this
patch for 2.11.

  block/nbd-client.h |  1 +
  block/nbd-client.c | 58 +-
  2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..28db9922c8 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {
  
  Coroutine *recv_coroutine[MAX_NBD_REQUESTS];

  NBDReply reply;
+bool eio_to_all;
  } NBDClientSession;
  
  NBDClientSession *nbd_get_client_session(BlockDriverState *bs);

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..1282b2484e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
  {
  NBDClientSession *client = nbd_get_client_session(bs);
  
+client->eio_to_all = true;

+
  if (!client->ioc) { /* Already closed */
  return;
  }
@@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  Error *local_err = NULL;
  
  for (;;) {

+if (s->eio_to_all) {
+break;
+}
+
  assert(s->reply.handle == 0);
  ret = nbd_receive_reply(s->ioc, >reply, _err);
  if (ret < 0) {
  error_report_err(local_err);
  }
-if (ret <= 0) {
+if (ret <= 0 || s->eio_to_all) {
  break;
  }
  
@@ -107,6 +113,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)

  qemu_coroutine_yield();
  }
  
+s->eio_to_all = true;

  nbd_recv_coroutines_enter_all(s);
  s->read_reply_co = NULL;
  }
@@ -118,6 +125,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
  NBDClientSession *s = nbd_get_client_session(bs);
  int rc, ret, i;
  
+if (s->eio_to_all) {

+return -EIO;
+}
+
  qemu_co_mutex_lock(>send_mutex);
  while (s->in_flight == MAX_NBD_REQUESTS) {
  qemu_co_queue_wait(>free_sema, >send_mutex);
@@ -135,15 +146,15 @@ static int nbd_co_send_request(BlockDriverState *bs,
  assert(i < MAX_NBD_REQUESTS);
  request->handle = INDEX_TO_HANDLE(s, i);
  
-if (!s->ioc) {

+if (s->eio_to_all) {
  qemu_co_mutex_unlock(>send_mutex);
-return -EPIPE;
+return -EIO;
  }
  
  if (qiov) {

  qio_channel_set_cork(s->ioc, true);
  rc = nbd_send_request(s->ioc, request);
-if (rc >= 0) {
+if (rc >= 0 && !s->eio_to_all) {
  ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
NULL);
  if (ret != request->len) {
@@ -155,7 +166,8 @@ static int nbd_co_send_request(BlockDriverState *bs,
  rc = nbd_send_request(s->ioc, request);
  }
  qemu_co_mutex_unlock(>send_mutex);
-return rc;
+
+return s->eio_to_all ? -EIO : rc;
  }
  
  static void nbd_co_receive_reply(NBDClientSession *s,

@@ -169,13 +181,13 @@ static void nbd_co_receive_reply(NBDClientSession *s,
  qemu_coroutine_yield();
  *reply = s->reply;
  if (reply->handle != request->handle ||
-!s->ioc) {
+!s->ioc || s->eio_to_all) {
  reply->error = EIO;
  } else {
  if (qiov && reply->error == 0) {
  ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, true,
NULL);
-if (ret != request->len) {
+if (ret != request->len || s->eio_to_all) {
  reply->error = EIO;
  }
  }
@@ -225,8 +237,10 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t 
offset,
  } else {
  

Re: [Qemu-block] [RFC PATCH 07/56] cpus: Make memsave, pmemsave sizes, addresses unsigned in QAPI/QMP

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Sizes, virtual and physical addresses should use QAPI type 'size'
> (uint64_t).  memsave, pmemsave parameters @val, @size are 'int'
> (int64_t).  qmp_memsave() and qmp_pmemsave() implicitly convert to
> target_ulong or hwaddr.
> 
> Change the parameters to 'size'.
> 
> Both commands now accept size and address values between 2^63 and
> 2^64-1.  They accept negative values as before, because that's how the
> QObject input visitor works for backward compatibility.
> 
> The HMP commands' size parameters remain uint32_t, as HMP args_type
> strings can't do uint64_t byte counts: 'l' is signed, and 'o'
> multiplies by 2^20.  Their address parameters remain int64_t for the
> same reason.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  cpus.c   | 6 +++---
>  qapi-schema.json | 5 +++--
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 9bed61e..8c5ee05 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1947,14 +1947,14 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  return head;
>  }
>  
> -void qmp_memsave(int64_t addr, int64_t size, const char *filename,
> +void qmp_memsave(uint64_t addr, uint64_t size, const char *filename,
>   bool has_cpu, int64_t cpu_index, Error **errp)
>  {
>  FILE *f;
>  uint32_t l;
>  CPUState *cpu;
>  uint8_t buf[1024];
> -int64_t orig_addr = addr, orig_size = size;
> +uint64_t orig_addr = addr, orig_size = size;
>  
>  if (!has_cpu) {
>  cpu_index = 0;

a little bit further down is a:
error_setg(errp, "Invalid addr 0x%016" PRIx64 "/size %" PRId64
 " specified", orig_addr, orig_size);

that PRId64 should be a PRIu64 now

However, other than that;


Reviewed-by: Dr. David Alan Gilbert 

> @@ -1994,7 +1994,7 @@ exit:
>  fclose(f);
>  }
>  
> -void qmp_pmemsave(int64_t addr, int64_t size, const char *filename,
> +void qmp_pmemsave(uint64_t addr, uint64_t size, const char *filename,
>Error **errp)
>  {
>  FILE *f;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f4a71df..80458fa 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2460,7 +2460,8 @@
>  #
>  ##
>  { 'command': 'memsave',
> -  'data': {'val': 'int', 'size': 'int', 'filename': 'str', '*cpu-index': 
> 'int'} }
> +  'data': {'val': 'size', 'size': 'size', 'filename': 'str',
> +   '*cpu-index': 'int'} }
>  
>  ##
>  # @pmemsave:
> @@ -2489,7 +2490,7 @@
>  #
>  ##
>  { 'command': 'pmemsave',
> -  'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
> +  'data': {'val': 'size', 'size': 'size', 'filename': 'str'} }
>  
>  ##
>  # @cont:
> -- 
> 2.7.5
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-block] [Qemu-devel] [PATCH] virtio-blk: handle blk_getlength() errors

[Fam Zheng]

On Tue, 08/08 13:22, Stefan Hajnoczi wrote:
> If blk_getlength() fails in virtio_blk_update_config() consider the disk
> image length to be 0 bytes.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/block/virtio-blk.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index b750bd8b53..a16ac75090 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -730,6 +730,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> uint8_t *config)
>  BlockConf *conf = >conf.conf;
>  struct virtio_blk_config blkcfg;
>  uint64_t capacity;
> +int64_t length;
>  int blk_size = conf->logical_block_size;
>  
>  blk_get_geometry(s->blk, );
> @@ -752,7 +753,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
> uint8_t *config)
>   * divided by 512 - instead it is the amount of blk_size blocks
>   * per track (cylinder).
>   */
> -if (blk_getlength(s->blk) /  conf->heads / conf->secs % blk_size) {
> +length = blk_getlength(s->blk);
> +if (length > 0 && length / conf->heads / conf->secs % blk_size) {
>  blkcfg.geometry.sectors = conf->secs & ~s->sector_mask;
>  } else {
>  blkcfg.geometry.sectors = conf->secs;
> -- 
> 2.13.3
> 
> 

Reviewed-by: Fam Zheng 

[Qemu-block] [PATCH for 2.10] block/nbd-client: always return EIO on and after the first io channel error

[Vladimir Sementsov-Ogievskiy]

Do not communicate after the first error to avoid communicating throught
broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on
in nbd_client_close.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi all. Here is a patch, fixing a problem noted in
[PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
and
[PATCH 17/17] block/nbd-client: always return EIO on and after the first io 
channel error
and discussed on list.

If it will be applied to 2.10, then I'll rebase my 'nbd client refactoring and 
fixing'
on it (for 2.11). If not - I'll prefer not rebase the series, so, do not apply 
this
patch for 2.11.

 block/nbd-client.h |  1 +
 block/nbd-client.c | 58 +-
 2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index df80771357..28db9922c8 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {
 
 Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
 NBDReply reply;
+bool eio_to_all;
 } NBDClientSession;
 
 NBDClientSession *nbd_get_client_session(BlockDriverState *bs);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..1282b2484e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 {
 NBDClientSession *client = nbd_get_client_session(bs);
 
+client->eio_to_all = true;
+
 if (!client->ioc) { /* Already closed */
 return;
 }
@@ -74,12 +76,16 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 Error *local_err = NULL;
 
 for (;;) {
+if (s->eio_to_all) {
+break;
+}
+
 assert(s->reply.handle == 0);
 ret = nbd_receive_reply(s->ioc, >reply, _err);
 if (ret < 0) {
 error_report_err(local_err);
 }
-if (ret <= 0) {
+if (ret <= 0 || s->eio_to_all) {
 break;
 }
 
@@ -107,6 +113,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
 qemu_coroutine_yield();
 }
 
+s->eio_to_all = true;
 nbd_recv_coroutines_enter_all(s);
 s->read_reply_co = NULL;
 }
@@ -118,6 +125,10 @@ static int nbd_co_send_request(BlockDriverState *bs,
 NBDClientSession *s = nbd_get_client_session(bs);
 int rc, ret, i;
 
+if (s->eio_to_all) {
+return -EIO;
+}
+
 qemu_co_mutex_lock(>send_mutex);
 while (s->in_flight == MAX_NBD_REQUESTS) {
 qemu_co_queue_wait(>free_sema, >send_mutex);
@@ -135,15 +146,15 @@ static int nbd_co_send_request(BlockDriverState *bs,
 assert(i < MAX_NBD_REQUESTS);
 request->handle = INDEX_TO_HANDLE(s, i);
 
-if (!s->ioc) {
+if (s->eio_to_all) {
 qemu_co_mutex_unlock(>send_mutex);
-return -EPIPE;
+return -EIO;
 }
 
 if (qiov) {
 qio_channel_set_cork(s->ioc, true);
 rc = nbd_send_request(s->ioc, request);
-if (rc >= 0) {
+if (rc >= 0 && !s->eio_to_all) {
 ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, false,
   NULL);
 if (ret != request->len) {
@@ -155,7 +166,8 @@ static int nbd_co_send_request(BlockDriverState *bs,
 rc = nbd_send_request(s->ioc, request);
 }
 qemu_co_mutex_unlock(>send_mutex);
-return rc;
+
+return s->eio_to_all ? -EIO : rc;
 }
 
 static void nbd_co_receive_reply(NBDClientSession *s,
@@ -169,13 +181,13 @@ static void nbd_co_receive_reply(NBDClientSession *s,
 qemu_coroutine_yield();
 *reply = s->reply;
 if (reply->handle != request->handle ||
-!s->ioc) {
+!s->ioc || s->eio_to_all) {
 reply->error = EIO;
 } else {
 if (qiov && reply->error == 0) {
 ret = nbd_rwv(s->ioc, qiov->iov, qiov->niov, request->len, true,
   NULL);
-if (ret != request->len) {
+if (ret != request->len || s->eio_to_all) {
 reply->error = EIO;
 }
 }
@@ -225,8 +237,10 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t 
offset,
 } else {
 nbd_co_receive_reply(client, , , qiov);
 }
-nbd_coroutine_end(bs, );
-return -reply.error;
+if (request.handle != 0) {
+nbd_coroutine_end(bs, );
+}
+return client->eio_to_all ? -EIO : -reply.error;
 }
 
 int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
@@ -254,8 +268,10 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t 
offset,
 } else {
 nbd_co_receive_reply(client, , , NULL);
 }
-nbd_coroutine_end(bs, );
-return -reply.error;
+if (request.handle != 0) {
+nbd_coroutine_end(bs, );
+}
+return client->eio_to_all ? -EIO : -reply.error;
 }
 
 int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
@@ -288,8 +304,10 @@ int 

Re: [Qemu-block] [RFC PATCH 03/56] monitor: Rewrite comment describing HMP .args_type

[Paolo Bonzini]

On 08/08/2017 13:20, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> Signed-off-by: Markus Armbruster 
>> ---
>>  monitor.c | 75 
>> +++
>>  1 file changed, 47 insertions(+), 28 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index e0f8801..8b54ba1 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -85,37 +85,56 @@
>>  #endif
>>  
>>  /*
>> - * Supported types:
>> + * Command handlers (mon_cmd_t member @cmd) receive actual arguments
>> + * in a QDict, which is built by the HMP core according to mon_cmd_t
>> + * member @args_type.  It's a list of NAME:TYPE separated by comma.
>>   *
>> - * 'F'  filename
>> - * 'B'  block device name
>> - * 's'  string (accept optional quote)
>> - * 'S'  it just appends the rest of the string (accept optional 
>> quote)
>> - * 'O'  option string of the form NAME=VALUE,...
>> - *  parsed according to QemuOptsList given by its name
>> - *  Example: 'device:O' uses qemu_device_opts.
>> - *  Restriction: only lists with empty desc are supported
>> - *  TODO lift the restriction
>> - * 'i'  32 bit integer
>> - * 'l'  target long (32 or 64 bit)
>> - * 'M'  Non-negative target long (32 or 64 bit), in user mode the
>> - *  value is multiplied by 2^20 (think Mebibyte)
>> - * 'o'  octets (aka bytes)
>> - *  user mode accepts an optional E, e, P, p, T, t, G, g, M, m,
>> - *  K, k suffix, which multiplies the value by 2^60 for 
>> suffixes E
>> - *  and e, 2^50 for suffixes P and p, 2^40 for suffixes T and t,
>> - *  2^30 for suffixes G and g, 2^20 for M and m, 2^10 for K and 
>> k
>> - * 'T'  double
>> - *  user mode accepts an optional ms, us, ns suffix,
>> - *  which divides the value by 1e3, 1e6, 1e9, respectively
>> - * '/'  optional gdb-like print format (like "/10x")
>> + * TYPEs that put a string value with key NAME into the QDict:
>> + * 's'Argument is enclosed in '"' or delimited by whitespace.  In
>> + *the former case, escapes \n \r \\ \' and \" are recognized.
>> + * 'F'File name, like 's' except for completion.
>> + * 'B'BlockBackend name, like 's' except for completion.
>> + * 'S'Argument is the remainder of the line, less leading
>> + *whitespace.
>> +
>>   *
>> - * '?'  optional type (for all types, except '/')
>> - * '.'  other form of optional type (for 'i' and 'l')
>> - * 'b'  boolean
>> - *  user mode accepts "on" or "off"
>> - * '-'  optional parameter (eg. '-f')
>> + * TYPEs that put an int64_t value with key NAME:
>> + * 'l'Argument is an expression (QEMU pocket calculator).
>> + * 'i'Like 'l' except value must fit into 32 bit unsigned.
>> + * 'M'Like 'l' except value must not be negative and is multiplied
>> + *by 2^20 (think "mebibyte").
>>   *
>> + * TYPEs that put an uint64_t value with key NAME:
>> + * 'o'Argument is a size (think "octets").  Without suffix the
>> + *value is multiplied by 2^20 (mebibytes), with suffix E or e
>> + *by 2^60 (exbibytes), with P or p by 2^50 (pebibytes), with T
>> + *or t by 2^40 (tebibytes), with G or g by 2^30 (gibibytes),
>> + *with M or m by 2^10 (mebibytes), with K or k by 2^10
>> + *(kibibytes).
> 
> 'o' is messy.  It using qemu_strtosz_MiB which uses a 'double' intermediate
> so I fear it can round.  It also has a note it can't take all f's due to
> an overflow from the conversion.   Two things not mentioned are that
> it also takes hex (as explicit 0x) and that it also does 'b' as a suffix
> to multiply by 1.  Those two combine in bad ways - i.e. 0x1b is 27MB,
> 1b is 1 byte (same for 'e').  These are probably OK except if you were
> to start replacing 'l' by 'o' because you really wanted 64bit addresses
> say.
> (I also wouldn't bother expanding the size names and powers).
> 
>> + *
>> + * TYPEs that put a double value with key NAME:
>> + * 'T'Argument is a time in seconds.  With optional ms, us, ns
>> + *suffix, the value divided by 1e3, 1e6, 1e9 respectively.
>> + *
>> + * TYPEs that put a bool value with key NAME:
>> + * 'b'Argument is either "on" (true) or "off" (false).
>> + * '-' CHAR
>> + *Argument is either "-CHAR" (true) or absent (false).
> 
> I found the previous description clearer.
> 
>> + * TYPEs that put multiple values:
>> + * 'O'Option string of the form NAME=VALUE,... parsed according to
>> + *the QemuOptsList given by its name.
>> + *Example: 'device:O' uses qemu_device_opts.
>> + *Restriction: only lists with empty desc are supported.
>> + *Puts all the NAME=VALUE.
>> + * '/'Gdb-like print format (like "/10x"), always optional.
>> + *Puts keys "count", 

Re: [Qemu-block] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Eric Blake]

On 08/08/2017 03:28 AM, Kevin Wolf wrote:
> Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
>> This also requires changing the return type of get_cluster_offset()
>> and adjusting all callers.
>>
>> Use osdep.h macros instead of open-coded rounding while in the
>> area.
>>
>> Reported-by: Markus Armbruster 
>> Signed-off-by: Eric Blake 
>> ---

> qcow2 ended up with this prototype:
> 
> int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>  unsigned int *bytes, uint64_t *cluster_offset)
> 
> Separating a full uin64_t offset by-reference parameter and an 0/-errno
> status return code feels a little cleaner to me.
> 
> And in fact, it's not only cleaner, but bit 63 is QCOW_OFLAG_COMPRESSED,
> so this patch breaks compressed images.

Looks like I'll be sending a v2, then.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 for-2.10 0/4] improved --version/--help tweaks

[Jeff Cody]

On Tue, Aug 08, 2017 at 08:50:55AM -0500, Eric Blake wrote:
> On 08/07/2017 07:08 PM, John Snow wrote:
> > 
> > 
> > On 08/03/2017 12:33 PM, Eric Blake wrote:
> >> Not sure if this should go through Kevin's block tree, Paolo's
> >> miscellaneous patches, or if I should just do a pull request
> >> myself (since patch 4 includes a change to qemu-nbd)
> >>
> 
> > 
> > Nothing to keep the commands from going out of order again, it looks
> > like -- can this be added as a comment or otherwise scripted as a
> > ./hey_it_looks_like_you_are_about_to_release_qemu.sh script that makes
> > sure we've dotted the 'i's and crossed the 't's?
> 
> We don't add subcommands to qemu-img all that frequently; I think a
> comment is okay without having to figure out where to script things.
> 
> > 
> > Lastly, Didn't we fix the certificate issue? (I thought Jeff had) -- and
> > even if not, it's still the correct address to send people to IMO. Let
> > people report to us if the SSL certificate appears to be broken.
> 
> That can be a followup-patch.
> 

Probably no need for one, really.  We force redirect to SSL, so the http://
versions will still end up https://.

And to confirm - the SSL certificates are now all in order.  The issue on
the website I think you were referencing in that commit message is because
we still had two URL resources pulling in non-ssl assets (Google fonts, and
jquery), so browsers blocked those mixed requests.  But that has been
updated.


-Jeff

[Qemu-block] [PULL 17/18] qemu-iotests: Test reopen between read-only and read-write

[Kevin Wolf]

This serves as a regression test for the bugs that were just fixed for
bdrv_reopen() between read-only and read-write mode.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 tests/qemu-iotests/187 | 69 ++
 tests/qemu-iotests/187.out | 18 
 tests/qemu-iotests/group   |  1 +
 3 files changed, 88 insertions(+)
 create mode 100755 tests/qemu-iotests/187
 create mode 100644 tests/qemu-iotests/187.out

diff --git a/tests/qemu-iotests/187 b/tests/qemu-iotests/187
new file mode 100755
index 00..7bb783363c
--- /dev/null
+++ b/tests/qemu-iotests/187
@@ -0,0 +1,69 @@
+#!/bin/bash
+#
+# Test switching between read-only and read-write
+#
+# Copyright (C) 2017 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 .
+#
+
+# creator
+owner=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+rm -f "$TEST_IMG.2"
+rm -f "$TEST_IMG.3"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=64M
+_make_test_img $size
+
+echo
+echo "Start from read-only"
+echo
+
+$QEMU_IO -r -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -r -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -r -c 'reopen -w' -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | 
_filter_qemu_io
+
+echo
+echo "Start from read-write"
+echo
+
+$QEMU_IO -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'reopen -r' -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | 
_filter_qemu_io
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out
new file mode 100644
index 00..68fb944cd5
--- /dev/null
+++ b/tests/qemu-iotests/187.out
@@ -0,0 +1,18 @@
+QA output created by 187
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+
+Start from read-only
+
+Block node is read-only
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Block node is read-only
+
+Start from read-write
+
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+write failed: Operation not permitted
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 823811076d..1848077932 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -182,6 +182,7 @@
 183 rw auto migration
 185 rw auto
 186 rw auto
+187 rw auto
 188 rw auto quick
 189 rw auto
 190 rw auto quick
-- 
2.13.4

[Qemu-block] [PULL 18/18] block/nfs: fix mutex assertion in nfs_file_close()

[Kevin Wolf]

From: Jeff Cody 

Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion
checks for when qemu_mutex() functions are called without the
corresponding qemu_mutex_init() having initialized the mutex.

This uncovered a latent bug in qemu's nfs driver - in
nfs_client_close(), the NFSClient structure is overwritten with zeros,
prior to the mutex being destroyed.

Go ahead and destroy the mutex in nfs_client_close(), and change where
we call qemu_mutex_init() so that it is correctly balanced.

There are also a couple of memory leaks obscured by the memset, so this
fixes those as well.

Finally, we should be able to get rid of the memset(), as it isn't
necessary.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Jeff Cody 
Reviewed-by: Peter Lieven 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 block/nfs.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index d8db419957..bec16b72a6 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -433,19 +433,23 @@ static void nfs_client_close(NFSClient *client)
 if (client->context) {
 if (client->fh) {
 nfs_close(client->context, client->fh);
+client->fh = NULL;
 }
 aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
false, NULL, NULL, NULL, NULL);
 nfs_destroy_context(client->context);
+client->context = NULL;
 }
-memset(client, 0, sizeof(NFSClient));
+g_free(client->path);
+qemu_mutex_destroy(>mutex);
+qapi_free_NFSServer(client->server);
+client->server = NULL;
 }
 
 static void nfs_file_close(BlockDriverState *bs)
 {
 NFSClient *client = bs->opaque;
 nfs_client_close(client);
-qemu_mutex_destroy(>mutex);
 }
 
 static NFSServer *nfs_config(QDict *options, Error **errp)
@@ -498,6 +502,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict 
*options,
 struct stat st;
 char *file = NULL, *strp = NULL;
 
+qemu_mutex_init(>mutex);
 opts = qemu_opts_create(_opts, NULL, 0, _abort);
 qemu_opts_absorb_qdict(opts, options, _err);
 if (local_err) {
@@ -660,7 +665,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict 
*options, int flags,
 if (ret < 0) {
 return ret;
 }
-qemu_mutex_init(>mutex);
+
 bs->total_sectors = ret;
 ret = 0;
 return ret;
-- 
2.13.4

[Qemu-block] [PULL 14/18] block: Allow reopen rw without BDRV_O_ALLOW_RDWR

[Kevin Wolf]

BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally
reopen a node read-write temporarily because the user requested
read-write for the top-level image, but qemu decided that read-only is
enough for this node (a backing file).

bdrv_reopen() is different, it is also used for cases where the user
changed their mind and wants to update the options. There is no reason
to forbid making a node read-write in that case.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 include/block/block.h |  3 ++-
 block.c   | 11 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 34770bb33a..ab80195378 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -436,7 +436,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, 
BlockDriverState *base,
 
 bool bdrv_is_read_only(BlockDriverState *bs);
 bool bdrv_is_writable(BlockDriverState *bs);
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+   bool ignore_allow_rdw, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index ab908cdc50..2711c3dd3b 100644
--- a/block.c
+++ b/block.c
@@ -246,7 +246,8 @@ bool bdrv_is_writable(BlockDriverState *bs)
 return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE);
 }
 
-int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
+   bool ignore_allow_rdw, Error **errp)
 {
 /* Do not set read_only if copy_on_read is enabled */
 if (bs->copy_on_read && read_only) {
@@ -256,7 +257,9 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool 
read_only, Error **errp)
 }
 
 /* Do not clear read_only if it is prohibited */
-if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) {
+if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR) &&
+!ignore_allow_rdw)
+{
 error_setg(errp, "Node '%s' is read only",
bdrv_get_device_or_node_name(bs));
 return -EPERM;
@@ -269,7 +272,7 @@ int bdrv_set_read_only(BlockDriverState *bs, bool 
read_only, Error **errp)
 {
 int ret = 0;
 
-ret = bdrv_can_set_read_only(bs, read_only, errp);
+ret = bdrv_can_set_read_only(bs, read_only, false, errp);
 if (ret < 0) {
 return ret;
 }
@@ -2907,7 +2910,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
  * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
  * not set, or if the BDS still has copy_on_read enabled */
 read_only = !(reopen_state->flags & BDRV_O_RDWR);
-ret = bdrv_can_set_read_only(reopen_state->bs, read_only, _err);
+ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, 
_err);
 if (local_err) {
 error_propagate(errp, local_err);
 goto error;
-- 
2.13.4

[Qemu-block] [PULL 16/18] qemu-io: Allow reopen read-write

[Kevin Wolf]

This allows qemu-iotests to test the switch between read-only and
read-write mode for block devices.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 qemu-io-cmds.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 3eb42c6728..2811a89099 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1920,6 +1920,7 @@ static void reopen_help(void)
 " 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a qcow2 
image\n"
 "\n"
 " -r, -- Reopen the image read-only\n"
+" -w, -- Reopen the image read-write\n"
 " -c, -- Change the cache mode to the given value\n"
 " -o, -- Changes block driver options (cf. 'open' command)\n"
 "\n");
@@ -1942,7 +1943,7 @@ static const cmdinfo_t reopen_cmd = {
.argmin = 0,
.argmax = -1,
.cfunc  = reopen_f,
-   .args   = "[-r] [-c cache] [-o options]",
+   .args   = "[(-r|-w)] [-c cache] [-o options]",
.oneline= "reopens an image with new options",
.help   = reopen_help,
 };
@@ -1955,11 +1956,12 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 int c;
 int flags = bs->open_flags;
 bool writethrough = !blk_enable_write_cache(blk);
+bool has_rw_option = false;
 
 BlockReopenQueue *brq;
 Error *local_err = NULL;
 
-while ((c = getopt(argc, argv, "c:o:r")) != -1) {
+while ((c = getopt(argc, argv, "c:o:rw")) != -1) {
 switch (c) {
 case 'c':
 if (bdrv_parse_cache_mode(optarg, , ) < 0) {
@@ -1974,7 +1976,20 @@ static int reopen_f(BlockBackend *blk, int argc, char 
**argv)
 }
 break;
 case 'r':
+if (has_rw_option) {
+error_report("Only one -r/-w option may be given");
+return 0;
+}
 flags &= ~BDRV_O_RDWR;
+has_rw_option = true;
+break;
+case 'w':
+if (has_rw_option) {
+error_report("Only one -r/-w option may be given");
+return 0;
+}
+flags |= BDRV_O_RDWR;
+has_rw_option = true;
 break;
 default:
 qemu_opts_reset(_opts);
-- 
2.13.4

[Qemu-block] [PULL 13/18] block: Fix order in bdrv_replace_child()

[Kevin Wolf]

Commit 8ee03995 refactored the code incorrectly and broke the release of
permissions on the old BDS. Instead of changing the permissions to the
new required values after removing the old BDS from the list of
children, it only re-obtains the permissions it already had.

Change the order of operations so that the old BDS is removed again
before calculating the new required permissions.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index ce9cce7b3c..ab908cdc50 100644
--- a/block.c
+++ b/block.c
@@ -1933,6 +1933,8 @@ static void bdrv_replace_child(BdrvChild *child, 
BlockDriverState *new_bs)
 BlockDriverState *old_bs = child->bs;
 uint64_t perm, shared_perm;
 
+bdrv_replace_child_noperm(child, new_bs);
+
 if (old_bs) {
 /* Update permissions for old node. This is guaranteed to succeed
  * because we're just taking a parent away, so we're loosening
@@ -1942,8 +1944,6 @@ static void bdrv_replace_child(BdrvChild *child, 
BlockDriverState *new_bs)
 bdrv_set_perm(old_bs, perm, shared_perm);
 }
 
-bdrv_replace_child_noperm(child, new_bs);
-
 if (new_bs) {
 bdrv_get_cumulative_perm(new_bs, , _perm);
 bdrv_set_perm(new_bs, perm, shared_perm);
-- 
2.13.4

[Qemu-block] [PULL 15/18] block: Set BDRV_O_ALLOW_RDWR during rw reopen

[Kevin Wolf]

Reopening an image should be consistent with opening it, so we should
set BDRV_O_ALLOW_RDWR for any image that is reopened read-write like in
bdrv_open_inherit().

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
---
 block.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 2711c3dd3b..3615a6809e 100644
--- a/block.c
+++ b/block.c
@@ -2729,8 +2729,11 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 bdrv_join_options(bs, options, old_options);
 QDECREF(old_options);
 
-/* bdrv_open() masks this flag out */
+/* bdrv_open_inherit() sets and clears some additional flags internally */
 flags &= ~BDRV_O_PROTOCOL;
+if (flags & BDRV_O_RDWR) {
+flags |= BDRV_O_ALLOW_RDWR;
+}
 
 QLIST_FOREACH(child, >children, next) {
 QDict *new_child_options;
-- 
2.13.4

[Qemu-block] [PULL 12/18] parallels: drop check that bdrv_truncate() is working

[Kevin Wolf]

From: "Denis V. Lunev" 

This would be actually strange and error prone. If truncate() nowadays
will fail, there is something fatally wrong. Let's check for that during
the actual work.

The only fallback case is when the file is not zero initialized. In this
case we should switch to preallocation via fallocate().

Signed-off-by: Denis V. Lunev 
CC: Markus Armbruster 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
---
 block/parallels.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 6794e53c0b..e1e06d23cc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -703,9 +703,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto fail_options;
 }
 
-if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) ||
-bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs),
-  PREALLOC_MODE_OFF, NULL) != 0) {
+if (!bdrv_has_zero_init(bs->file->bs)) {
 s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
 }
 
-- 
2.13.4

[Qemu-block] [PULL 10/18] block: respect error code from bdrv_getlength in handle_aiocb_write_zeroes

[Kevin Wolf]

From: "Denis V. Lunev" 

Original idea beyond the code in question was the following: we have failed
to write zeroes with fallocate(FALLOC_FL_ZERO_RANGE) as the simplest
approach and via fallocate(FALLOC_FL_PUNCH_HOLE)/fallocate(0). We have the
only chance now: if the request comes beyond end of the file. Thus we
should calculate file length and respect the error code from that op.

Signed-off-by: Denis V. Lunev 
CC: Markus Armbruster 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/file-posix.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index cfbb236f6f..f4de022ae0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1339,6 +1339,9 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
*aiocb)
 #if defined(CONFIG_FALLOCATE) || defined(CONFIG_XFS)
 BDRVRawState *s = aiocb->bs->opaque;
 #endif
+#ifdef CONFIG_FALLOCATE
+int64_t len;
+#endif
 
 if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
 return handle_aiocb_write_zeroes_block(aiocb);
@@ -1381,7 +1384,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
*aiocb)
 #endif
 
 #ifdef CONFIG_FALLOCATE
-if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
+/* Last resort: we are trying to extend the file with zeroed data. This
+ * can be done via fallocate(fd, 0) */
+len = bdrv_getlength(aiocb->bs);
+if (s->has_fallocate && len >= 0 && aiocb->aio_offset >= len) {
 int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
 if (ret == 0 || ret != -ENOTSUP) {
 return ret;
-- 
2.13.4

[Qemu-block] [PULL 09/18] vmdk: Fix error handling/reporting of vmdk_check

[Kevin Wolf]

From: Fam Zheng 

Errors from the callees must be captured and propagated to our caller,
ensure this for both find_extent() and bdrv_getlength().

Reported-by: Markus Armbruster 
Signed-off-by: Fam Zheng 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/vmdk.c | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 0fc97391a6..c665bcc977 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2236,6 +2236,7 @@ static int vmdk_check(BlockDriverState *bs, 
BdrvCheckResult *result,
 fprintf(stderr,
 "ERROR: could not find extent for sector %" PRId64 "\n",
 sector_num);
+ret = -EINVAL;
 break;
 }
 ret = get_cluster_offset(bs, extent, NULL,
@@ -2247,19 +2248,28 @@ static int vmdk_check(BlockDriverState *bs, 
BdrvCheckResult *result,
 PRId64 "\n", sector_num);
 break;
 }
-if (ret == VMDK_OK &&
-cluster_offset >= bdrv_getlength(extent->file->bs))
-{
-fprintf(stderr,
-"ERROR: cluster offset for sector %"
-PRId64 " points after EOF\n", sector_num);
-break;
+if (ret == VMDK_OK) {
+int64_t extent_len = bdrv_getlength(extent->file->bs);
+if (extent_len < 0) {
+fprintf(stderr,
+"ERROR: could not get extent file length for sector %"
+PRId64 "\n", sector_num);
+ret = extent_len;
+break;
+}
+if (cluster_offset >= extent_len) {
+fprintf(stderr,
+"ERROR: cluster offset for sector %"
+PRId64 " points after EOF\n", sector_num);
+ret = -EINVAL;
+break;
+}
 }
 sector_num += extent->cluster_sectors;
 }
 
 result->corruptions++;
-return 0;
+return ret;
 }
 
 static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
-- 
2.13.4

[Qemu-block] [PULL 08/18] block/null: Remove 'filename' option

[Kevin Wolf]

This option was only added to allow 'null-co://' and 'null-aio://' as
filenames, its value never served any actual purpose and was ignored.
Nevertheless it was accepted as '-drive driver=null,filename=foo'.

The correct way to enable the protocol prefixes (and that without adding
a useless -drive option) is implementing .bdrv_parse_filename. This is
what this patch does.

Technically, this is an incompatible change, but the null block driver
is only used for benchmarking, testing and debugging, and an option
without effect isn't likely to be used by anyone anyway, so no bad
effects are to be expected.

Reported-by: Markus Armbruster 
Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: Stefan Hajnoczi 
---
 block/null.c   | 31 ++-
 tests/qemu-iotests/136 |  2 +-
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/block/null.c b/block/null.c
index 876f90965b..dd9c13f9ba 100644
--- a/block/null.c
+++ b/block/null.c
@@ -30,11 +30,6 @@ static QemuOptsList runtime_opts = {
 .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
 .desc = {
 {
-.name = "filename",
-.type = QEMU_OPT_STRING,
-.help = "",
-},
-{
 .name = BLOCK_OPT_SIZE,
 .type = QEMU_OPT_SIZE,
 .help = "size of the null block",
@@ -54,6 +49,30 @@ static QemuOptsList runtime_opts = {
 },
 };
 
+static void null_co_parse_filename(const char *filename, QDict *options,
+   Error **errp)
+{
+/* This functions only exists so that a null-co:// filename is accepted
+ * with the null-co driver. */
+if (strcmp(filename, "null-co://")) {
+error_setg(errp, "The only allowed filename for this driver is "
+ "'null-co://'");
+return;
+}
+}
+
+static void null_aio_parse_filename(const char *filename, QDict *options,
+Error **errp)
+{
+/* This functions only exists so that a null-aio:// filename is accepted
+ * with the null-aio driver. */
+if (strcmp(filename, "null-aio://")) {
+error_setg(errp, "The only allowed filename for this driver is "
+ "'null-aio://'");
+return;
+}
+}
+
 static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
@@ -242,6 +261,7 @@ static BlockDriver bdrv_null_co = {
 .instance_size  = sizeof(BDRVNullState),
 
 .bdrv_file_open = null_file_open,
+.bdrv_parse_filename= null_co_parse_filename,
 .bdrv_close = null_close,
 .bdrv_getlength = null_getlength,
 
@@ -261,6 +281,7 @@ static BlockDriver bdrv_null_aio = {
 .instance_size  = sizeof(BDRVNullState),
 
 .bdrv_file_open = null_file_open,
+.bdrv_parse_filename= null_aio_parse_filename,
 .bdrv_close = null_close,
 .bdrv_getlength = null_getlength,
 
diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index 635b977552..4b994897af 100644
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -75,7 +75,7 @@ sector = "%d"
 drive_args.append("stats-account-failed=%s" %
   (self.account_failed and "on" or "off"))
 self.create_blkdebug_file()
-self.vm = iotests.VM().add_drive('blkdebug:%s:%s ' %
+self.vm = iotests.VM().add_drive('blkdebug:%s:%s' %
  (blkdebug_file, self.test_img),
  ','.join(drive_args))
 self.vm.launch()
-- 
2.13.4

[Qemu-block] [PULL 07/18] block: drop bdrv_set_key from BlockDriver

[Kevin Wolf]

From: Paolo Bonzini 

This is not used anymore since c01c214b69 ("block: remove all encryption
handling APIs", 2017-07-11).

Signed-off-by: Paolo Bonzini 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 include/block/block_int.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index d4f4ea7584..7571c0aaaf 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -127,7 +127,6 @@ struct BlockDriver {
   Error **errp);
 void (*bdrv_close)(BlockDriverState *bs);
 int (*bdrv_create)(const char *filename, QemuOpts *opts, Error **errp);
-int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
 int (*bdrv_make_empty)(BlockDriverState *bs);
 
 void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
-- 
2.13.4

[Qemu-block] [PULL 06/18] block/vhdx: check error return of bdrv_truncate()

[Kevin Wolf]

From: Jeff Cody 

Signed-off-by: Jeff Cody 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/vhdx-log.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index a27dc059cd..14b724ef7b 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -558,7 +558,11 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 ret = -EINVAL;
 goto exit;
 }
-bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, 
NULL);
+ret = bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF,
+NULL);
+if (ret < 0) {
+goto exit;
+}
 }
 }
 qemu_vfree(desc_entries);
-- 
2.13.4

[Qemu-block] [PULL 11/18] parallels: respect error code of bdrv_getlength() in allocate_clusters()

[Kevin Wolf]

From: "Denis V. Lunev" 

If we can not get the file length, the state of BDS is broken completely.
Return error to the caller.

Signed-off-by: Denis V. Lunev 
CC: Markus Armbruster 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/parallels.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 5bbdfabb7a..6794e53c0b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -192,7 +192,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
int64_t sector_num,
  int nb_sectors, int *pnum)
 {
 BDRVParallelsState *s = bs->opaque;
-int64_t pos, space, idx, to_allocate, i;
+int64_t pos, space, idx, to_allocate, i, len;
 
 pos = block_status(s, sector_num, nb_sectors, pnum);
 if (pos > 0) {
@@ -214,7 +214,11 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
int64_t sector_num,
 assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
 
 space = to_allocate * s->tracks;
-if (s->data_end + space > bdrv_getlength(bs->file->bs) >> 
BDRV_SECTOR_BITS) {
+len = bdrv_getlength(bs->file->bs);
+if (len < 0) {
+return len;
+}
+if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
 int ret;
 space += s->prealloc_size;
 if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-- 
2.13.4

[Qemu-block] [PULL 01/18] qemu-iotests/109: Fix lock race condition

[Kevin Wolf]

From: Cleber Rosa 

A race condition is currently present between the clean up attempt of
the QEMU process and the execution of qemu-img.  The actual (bad)
output is:

 -Warning: Image size mismatch!
 -Images are identical.
 +qemu-img: Could not open '/tests/qemu-iotests/scratch/t.raw': 
Failed to get "consistent read" lock
 +Is another process using the image?

A KILL signal is sent to the QEMU process, but qemu-img may begin to
run before the QEMU process is really gone.  qemu-img will then
attempt to open the TEST_IMG file before it can secure a lock on it.

This attempts a more graceful shutdown, and waits for the QEMU process
to exit.

Signed-off-by: Cleber Rosa 
Reviewed-by: Eric Blake 
Reviewed-by: Jeff Cody 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/109 |  3 ++-
 tests/qemu-iotests/109.out | 56 ++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109
index 3b496a3918..d70b574d88 100755
--- a/tests/qemu-iotests/109
+++ b/tests/qemu-iotests/109
@@ -67,7 +67,8 @@ function run_qemu()
 _send_qemu_cmd $QEMU_HANDLE '' "BLOCK_JOB_COMPLETED"
 fi
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"query-block-jobs"}' "return"
-_cleanup_qemu
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"
+wait=1 _cleanup_qemu
 }
 
 for fmt in qcow qcow2 qed vdi vmdk vpc; do
diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out
index dc02f9eefa..c189e2833d 100644
--- a/tests/qemu-iotests/109.out
+++ b/tests/qemu-iotests/109.out
@@ -12,12 +12,17 @@ Specify the 'raw' format explicitly to remove the 
restrictions.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": 
"report"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 0, 
"speed": 0, "type": "mirror", "error": "Operation not permitted"}}
 {"return": []}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, 
"speed": 0, "type": "mirror"}}
 {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, 
"offset": 1024, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, 
"speed": 0, "type": "mirror"}}
 Warning: Image size mismatch!
 Images are identical.
 
@@ -33,12 +38,17 @@ Specify the 'raw' format explicitly to remove the 
restrictions.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": 
"report"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 512, 
"speed": 0, "type": "mirror", "error": "Operation not permitted"}}
 {"return": []}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "src", "len": 197120, "offset": 197120, 
"speed": 0, "type": "mirror"}}
 {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 197120, 
"offset": 197120, "paused": false, "speed": 0, "ready": true, "type": 
"mirror"}]}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 197120, "offset": 
197120, "speed": 0, "type": "mirror"}}
 Warning: Image size mismatch!
 Images are identical.
 
@@ -54,12 +64,17 @@ Specify the 'raw' format explicitly to remove the 
restrictions.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": 
"report"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 

[Qemu-block] [PULL 03/18] block/vhdx: check error return of bdrv_getlength()

[Kevin Wolf]

From: Jeff Cody 

Calls to bdrv_getlength() were not checking for error.  In vhdx.c, this
can lead to truncating an image file, so it is a definite bug.  In
vhdx-log.c, the path for improper behavior is less clear, but it is best
to check in any case.

Some minor code movement of the log_guid intialization, as well.

Reported-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Signed-off-by: Jeff Cody 
Signed-off-by: Kevin Wolf 
---
 block/vhdx-log.c | 23 ++-
 block/vhdx.c |  9 -
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 01278f3fc9..2e26fd46a5 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -491,6 +491,7 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 uint32_t cnt, sectors_read;
 uint64_t new_file_size;
 void *data = NULL;
+int64_t file_length;
 VHDXLogDescEntries *desc_entries = NULL;
 VHDXLogEntryHeader hdr_tmp = { 0 };
 
@@ -510,10 +511,15 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 if (ret < 0) {
 goto exit;
 }
+file_length = bdrv_getlength(bs->file->bs);
+if (file_length < 0) {
+ret = file_length;
+goto exit;
+}
 /* if the log shows a FlushedFileOffset larger than our current file
  * size, then that means the file has been truncated / corrupted, and
  * we must refused to open it / use it */
-if (hdr_tmp.flushed_file_offset > bdrv_getlength(bs->file->bs)) {
+if (hdr_tmp.flushed_file_offset > file_length) {
 ret = -EINVAL;
 goto exit;
 }
@@ -543,7 +549,7 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 goto exit;
 }
 }
-if (bdrv_getlength(bs->file->bs) < desc_entries->hdr.last_file_offset) 
{
+if (file_length < desc_entries->hdr.last_file_offset) {
 new_file_size = desc_entries->hdr.last_file_offset;
 if (new_file_size % (1024*1024)) {
 /* round up to nearest 1MB boundary */
@@ -851,6 +857,7 @@ static int vhdx_log_write(BlockDriverState *bs, 
BDRVVHDXState *s,
 uint32_t partial_sectors = 0;
 uint32_t bytes_written = 0;
 uint64_t file_offset;
+int64_t file_length;
 VHDXHeader *header;
 VHDXLogEntryHeader new_hdr;
 VHDXLogDescriptor *new_desc = NULL;
@@ -904,6 +911,12 @@ static int vhdx_log_write(BlockDriverState *bs, 
BDRVVHDXState *s,
 
 sectors += partial_sectors;
 
+file_length = bdrv_getlength(bs->file->bs);
+if (file_length < 0) {
+ret = file_length;
+goto exit;
+}
+
 /* sectors is now how many sectors the data itself takes, not
  * including the header and descriptor metadata */
 
@@ -913,11 +926,11 @@ static int vhdx_log_write(BlockDriverState *bs, 
BDRVVHDXState *s,
 .sequence_number = s->log.sequence,
 .descriptor_count= sectors,
 .reserved= 0,
-.flushed_file_offset = bdrv_getlength(bs->file->bs),
-.last_file_offset= bdrv_getlength(bs->file->bs),
+.flushed_file_offset = file_length,
+.last_file_offset= file_length,
+.log_guid= header->log_guid,
   };
 
-new_hdr.log_guid = header->log_guid;
 
 desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count);
 
diff --git a/block/vhdx.c b/block/vhdx.c
index a9cecd2773..37224b8858 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1166,7 +1166,14 @@ exit:
 static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s,
 uint64_t *new_offset)
 {
-*new_offset = bdrv_getlength(bs->file->bs);
+int64_t current_len;
+
+current_len = bdrv_getlength(bs->file->bs);
+if (current_len < 0) {
+return current_len;
+}
+
+*new_offset = current_len;
 
 /* per the spec, the address for a block is in units of 1MB */
 *new_offset = ROUND_UP(*new_offset, 1024 * 1024);
-- 
2.13.4

[Qemu-block] [PULL 04/18] block/vhdx: check for offset overflow to bdrv_truncate()

[Kevin Wolf]

From: Jeff Cody 

VHDX uses uint64_t types for most offsets, following the VHDX spec.
However, bdrv_truncate() takes an int64_t value for the truncating
offset.  Check for overflow before calling bdrv_truncate().

While we are here, replace the bit shifting with QEMU_ALIGN_UP as well.

N.B.: For a compliant image this is not an issue, as the maximum VHDX
image size is defined per the spec to be 64TB.

Reviewed-by: Eric Blake 
Reviewed-by: Kevin Wolf 
Signed-off-by: Jeff Cody 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Kevin Wolf 
---
 block/vhdx-log.c | 6 +-
 block/vhdx.c | 3 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 2e26fd46a5..95972230f0 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -553,7 +553,11 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 new_file_size = desc_entries->hdr.last_file_offset;
 if (new_file_size % (1024*1024)) {
 /* round up to nearest 1MB boundary */
-new_file_size = ((new_file_size >> 20) + 1) << 20;
+new_file_size = QEMU_ALIGN_UP(new_file_size, MiB);
+if (new_file_size > INT64_MAX) {
+ret = -EINVAL;
+goto exit;
+}
 bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, 
NULL);
 }
 }
diff --git a/block/vhdx.c b/block/vhdx.c
index 37224b8858..7ae4589879 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1177,6 +1177,9 @@ static int vhdx_allocate_block(BlockDriverState *bs, 
BDRVVHDXState *s,
 
 /* per the spec, the address for a block is in units of 1MB */
 *new_offset = ROUND_UP(*new_offset, 1024 * 1024);
+if (*new_offset > INT64_MAX) {
+return -EINVAL;
+}
 
 return bdrv_truncate(bs->file, *new_offset + s->block_size,
  PREALLOC_MODE_OFF, NULL);
-- 
2.13.4

[Qemu-block] [PULL 05/18] block/vhdx: check error return of bdrv_flush()

[Kevin Wolf]

From: Jeff Cody 

Reported-by: Kevin Wolf 
Signed-off-by: Jeff Cody 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/vhdx-log.c | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 95972230f0..a27dc059cd 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -565,7 +565,10 @@ static int vhdx_log_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 desc_entries = NULL;
 }
 
-bdrv_flush(bs);
+ret = bdrv_flush(bs);
+if (ret < 0) {
+goto exit;
+}
 /* once the log is fully flushed, indicate that we have an empty log
  * now.  This also sets the log guid to 0, to indicate an empty log */
 vhdx_log_reset(bs, s);
@@ -1039,7 +1042,11 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 
 /* Make sure data written (new and/or changed blocks) is stable
  * on disk, before creating log entry */
-bdrv_flush(bs);
+ret = bdrv_flush(bs);
+if (ret < 0) {
+goto exit;
+}
+
 ret = vhdx_log_write(bs, s, data, length, offset);
 if (ret < 0) {
 goto exit;
@@ -1047,7 +1054,11 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 logs.log = s->log;
 
 /* Make sure log is stable on disk */
-bdrv_flush(bs);
+ret = bdrv_flush(bs);
+if (ret < 0) {
+goto exit;
+}
+
 ret = vhdx_log_flush(bs, s, );
 if (ret < 0) {
 goto exit;
-- 
2.13.4

[Qemu-block] [PULL 00/18] Block layer patches for 2.10.0-rc2

[Kevin Wolf]

The following changes since commit b4174c4b08a719e7df7e4f35c29f44b7c2517237:

  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging 
(2017-08-08 10:01:49 +0100)

are available in the git repository at:

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

for you to fetch changes up to 113fe792fd4931dd0538f03859278b8719ee4fa2:

  block/nfs: fix mutex assertion in nfs_file_close() (2017-08-08 15:19:16 +0200)


Block layer patches for 2.10.0-rc2


Alberto Garcia (1):
  quorum: Set sectors-count to 0 when reporting a flush error

Cleber Rosa (1):
  qemu-iotests/109: Fix lock race condition

Denis V. Lunev (3):
  block: respect error code from bdrv_getlength in handle_aiocb_write_zeroes
  parallels: respect error code of bdrv_getlength() in allocate_clusters()
  parallels: drop check that bdrv_truncate() is working

Fam Zheng (1):
  vmdk: Fix error handling/reporting of vmdk_check

Jeff Cody (5):
  block/vhdx: check error return of bdrv_getlength()
  block/vhdx: check for offset overflow to bdrv_truncate()
  block/vhdx: check error return of bdrv_flush()
  block/vhdx: check error return of bdrv_truncate()
  block/nfs: fix mutex assertion in nfs_file_close()

Kevin Wolf (6):
  block/null: Remove 'filename' option
  block: Fix order in bdrv_replace_child()
  block: Allow reopen rw without BDRV_O_ALLOW_RDWR
  block: Set BDRV_O_ALLOW_RDWR during rw reopen
  qemu-io: Allow reopen read-write
  qemu-iotests: Test reopen between read-only and read-write

Paolo Bonzini (1):
  block: drop bdrv_set_key from BlockDriver

 include/block/block.h  |  3 +-
 include/block/block_int.h  |  1 -
 block.c| 20 +-
 block/file-posix.c |  8 +-
 block/nfs.c| 11 ++--
 block/null.c   | 31 +
 block/parallels.c  | 12 
 block/quorum.c |  3 +-
 block/vhdx-log.c   | 52 +++---
 block/vhdx.c   | 12 +++-
 block/vmdk.c   | 26 +++--
 qemu-io-cmds.c | 19 +++--
 tests/qemu-iotests/109 |  3 +-
 tests/qemu-iotests/109.out | 56 +
 tests/qemu-iotests/136 |  2 +-
 tests/qemu-iotests/187 | 69 ++
 tests/qemu-iotests/187.out | 18 
 tests/qemu-iotests/group   |  1 +
 18 files changed, 299 insertions(+), 48 deletions(-)
 create mode 100755 tests/qemu-iotests/187
 create mode 100644 tests/qemu-iotests/187.out

[Qemu-block] [PULL 02/18] quorum: Set sectors-count to 0 when reporting a flush error

[Kevin Wolf]

From: Alberto Garcia 

The QUORUM_REPORT_BAD event has fields to report the sector in which
the error was detected and the number of affected sectors starting
from that one. This is important for read and write errors, but not
for flush errors.

For flush errors the current code reports the total size of the disk
image. That is however not useful information in this case. Moreover,
the bdrv_getlength() call can fail, and there's no good way of
handling that failure.

Since we're reporting useless information and we cannot even guarantee
to do it in a consistent way, this patch changes the code to report 0
instead in all cases.

Reported-by: Markus Armbruster 
Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 block/quorum.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 55ba916655..d04da4f430 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -785,8 +785,7 @@ static coroutine_fn int quorum_co_flush(BlockDriverState 
*bs)
 for (i = 0; i < s->num_children; i++) {
 result = bdrv_co_flush(s->children[i]->bs);
 if (result) {
-quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
-  bdrv_getlength(s->children[i]->bs),
+quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0, 0,
   s->children[i]->bs->node_name, result);
 result_value.l = result;
 quorum_count_vote(_votes, _value, i);
-- 
2.13.4

Re: [Qemu-block] [PATCH v4] tests: Avoid non-portable 'echo -ARG'

[Kevin Wolf]

Am 03.07.2017 um 20:09 hat Eric Blake geschrieben:
> POSIX says that backslashes in the arguments to 'echo', as well as
> any use of 'echo -n' and 'echo -e', are non-portable; it recommends
> people should favor 'printf' instead.  This is definitely true where
> we do not control which shell is running (such as in makefile snippets
> or in documentation examples).  But even for scripts where we
> require bash (and therefore, where echo does what we want by default),
> it is still possible to use 'shopt -s xpg_echo' to change bash's
> behavior of echo.  And setting a good example never hurts when we are
> not sure if a snippet will be copied from a bash-only script to a
> general shell script (although I don't change the use of non-portable
> \e for ESC when we know the running shell is bash).
> 
> Replace 'echo -n "..."' with 'printf %s "..."', and 'echo -e "..."'
> with 'printf %b "...\n"', with the optimization that the %s/%b
> argument can be omitted if the string being printed is a strict
> literal with no '%', '$', or '`' (we could technically also make
> this optimization when there are $ or `` substitutions but where
> we can prove their results will not be problematic, but proving
> that such substitutions are safe makes the patch less trivial
> compared to just being consistent).
> 
> In the qemu-iotests check script, fix unusual shell quoting
> that would result in word-splitting if 'date' outputs a space.
> 
> In test 051, take an opportunity to shorten the line.
> 
> In test 068, get rid of a pointless second invocation of bash.
> 
> CC: qemu-triv...@nongnu.org
> Signed-off-by: Eric Blake 

> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
> index 78d7edf..c8f3da8 100755
> --- a/tests/multiboot/run_test.sh
> +++ b/tests/multiboot/run_test.sh
> @@ -26,7 +26,7 @@ run_qemu() {
>  local kernel=$1
>  shift
> 
> -echo -e "\n\n=== Running test case: $kernel $@ ===\n" >> test.log
> +printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log
> 
>  $QEMU \
>  -kernel $kernel \

Not completely sure why, but this broke the test with whitespace changes
like this:

-=== Running test case: mmap.elf -m 1.1M ===
+=== Running test case: mmap.elf -m1.1M ===

Kevin

Re: [Qemu-block] [PATCH v3 for-2.10 0/4] improved --version/--help tweaks

[Eric Blake]

On 08/07/2017 07:08 PM, John Snow wrote:
> 
> 
> On 08/03/2017 12:33 PM, Eric Blake wrote:
>> Not sure if this should go through Kevin's block tree, Paolo's
>> miscellaneous patches, or if I should just do a pull request
>> myself (since patch 4 includes a change to qemu-nbd)
>>

> 
> Nothing to keep the commands from going out of order again, it looks
> like -- can this be added as a comment or otherwise scripted as a
> ./hey_it_looks_like_you_are_about_to_release_qemu.sh script that makes
> sure we've dotted the 'i's and crossed the 't's?

We don't add subcommands to qemu-img all that frequently; I think a
comment is okay without having to figure out where to script things.

> 
> Lastly, Didn't we fix the certificate issue? (I thought Jeff had) -- and
> even if not, it's still the correct address to send people to IMO. Let
> people report to us if the SSL certificate appears to be broken.

That can be a followup-patch.

> 
> Eh, regardless of shed coloring:
> 
> Reviewed-by: John Snow 
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 5/7] block: add throttle block filter driver

[Manos Pitsidianakis]

On Tue, Aug 08, 2017 at 03:13:36PM +0200, Alberto Garcia wrote:

On Mon 31 Jul 2017 11:54:41 AM CEST, Manos Pitsidianakis wrote:

block/throttle.c uses existing I/O throttle infrastructure inside a
block filter driver. I/O operations are intercepted in the filter's
read/write coroutines, and referred to block/throttle-groups.c

The driver can be used with the syntax
-drive driver=throttle,file.filename=foo.qcow2, \
limits.iops-total=...,throttle-group=bar


Sorry for not having noticed this earlier, but can't you define the
throttling group (and its limits) using -object throttle-group ... as
shown in the previous patch, and simply reference it here? Or would we
have two alternative ways of setting the throttling limits?

What happens if you have many -drive lines each one with a different set
of limits but with the same throttling group?


The limits of the last one to be processed will win. Quoting a reply I 
made to Kevin on the interface test patch:



You're right, I missed this. The test result shows that this command
succeeds. Do we really want to allow other nodes to be affected with a
blockdev-add? Wouldn't it be cleaner to just forbid the combination of
limits and throtte-group?


So basically only anonymous, immutable groups can be created through the 
driver then. All other shared group configurations must be explicitly 
created with an -object / object-add syntax. I think this is a neat 
separation and compromise if we allow anonymous groups. If not, we can 
ignore limits on the throttle driver.


So basically if we have anonymous groups, we accept limits in the driver 
options but only without a group-name. Without anonymous groups, we 
remove limits from the driver options and only use the 
object-add/-object commands to create throttle groups. Does this sound 
like a good idea? It will be more verbose for the human user. One 
advantage: all throttle groups can then be managed through 
qom-set/qom-get since they are owned by the qom tree.


signature.asc
Description: PGP signature

[Qemu-block] [PATCH for-2.11 2/7] block/ssh: make compliant with coding guidelines

[Jeff Cody]

Signed-off-by: Jeff Cody 
---
 block/ssh.c | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index cbb0e34..97f7673 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -241,7 +241,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 goto err;
 }
 
-if(uri->user && strcmp(uri->user, "") != 0) {
+if (uri->user && strcmp(uri->user, "") != 0) {
 qdict_put_str(options, "user", uri->user);
 }
 
@@ -268,7 +268,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 
  err:
 if (uri) {
-  uri_free(uri);
+uri_free(uri);
 }
 return -EINVAL;
 }
@@ -342,7 +342,7 @@ static int check_host_key_knownhosts(BDRVSSHState *s,
 libssh2_knownhost_readfile(knh, knh_file, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
 
 r = libssh2_knownhost_checkp(knh, host, port, hostkey, len,
- LIBSSH2_KNOWNHOST_TYPE_PLAIN|
+ LIBSSH2_KNOWNHOST_TYPE_PLAIN |
  LIBSSH2_KNOWNHOST_KEYENC_RAW,
  );
 switch (r) {
@@ -405,15 +405,18 @@ static int compare_fingerprint(const unsigned char 
*fingerprint, size_t len,
 unsigned c;
 
 while (len > 0) {
-while (*host_key_check == ':')
+while (*host_key_check == ':') {
 host_key_check++;
+}
 if (!qemu_isxdigit(host_key_check[0]) ||
-!qemu_isxdigit(host_key_check[1]))
+!qemu_isxdigit(host_key_check[1])) {
 return 1;
+}
 c = hex2decimal(host_key_check[0]) * 16 +
 hex2decimal(host_key_check[1]);
-if (c - *fingerprint != 0)
+if (c - *fingerprint != 0) {
 return c - *fingerprint;
+}
 fingerprint++;
 len--;
 host_key_check += 2;
@@ -433,8 +436,8 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
 return -EINVAL;
 }
 
-if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
-   hash) != 0) {
+if (compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
+hash) != 0) {
 error_setg(errp, "remote host key does not match host_key_check '%s'",
hash);
 return -EPERM;
@@ -507,7 +510,7 @@ static int authenticate(BDRVSSHState *s, const char *user, 
Error **errp)
 goto out;
 }
 
-for(;;) {
+for (;;) {
 r = libssh2_agent_get_identity(agent, , prev_identity);
 if (r == 1) {   /* end of list */
 break;
@@ -863,8 +866,8 @@ static int ssh_create(const char *filename, QemuOpts *opts, 
Error **errp)
 }
 
 r = connect_to_ssh(, uri_options,
-   LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
-   LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
+   LIBSSH2_FXF_READ  | LIBSSH2_FXF_WRITE |
+   LIBSSH2_FXF_CREAT | LIBSSH2_FXF_TRUNC,
0644, errp);
 if (r < 0) {
 ret = r;
@@ -872,7 +875,7 @@ static int ssh_create(const char *filename, QemuOpts *opts, 
Error **errp)
 }
 
 if (total_size > 0) {
-libssh2_sftp_seek64(s.sftp_handle, total_size-1);
+libssh2_sftp_seek64(s.sftp_handle, total_size - 1);
 r2 = libssh2_sftp_write(s.sftp_handle, c, 1);
 if (r2 < 0) {
 sftp_error_setg(errp, , "truncate failed");
@@ -,7 +1114,7 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState 
*bs,
  * works for me.
  */
 if (r == 0) {
-ssh_seek(s, offset + written, SSH_SEEK_WRITE|SSH_SEEK_FORCE);
+ssh_seek(s, offset + written, SSH_SEEK_WRITE | SSH_SEEK_FORCE);
 co_yield(s, bs);
 goto again;
 }
@@ -1125,8 +1128,9 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState 
*bs,
 end_of_vec = i->iov_base + i->iov_len;
 }
 
-if (offset + written > s->attrs.filesize)
+if (offset + written > s->attrs.filesize) {
 s->attrs.filesize = offset + written;
+}
 }
 
 return 0;
-- 
2.9.4

[Qemu-block] [PATCH for-2.11 3/7] block/sheepdog: remove spurious NULL check

[Jeff Cody]

'tag' is already checked in the lines immediately preceding this check,
and set to non-NULL if NULL.  No need to check again, it hasn't changed.

Signed-off-by: Jeff Cody 
---
 block/sheepdog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index abb2e79..bbbfa72 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1632,7 +1632,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, 
int flags,
 if (!tag) {
 tag = "";
 }
-if (tag && strlen(tag) >= SD_MAX_VDI_TAG_LEN) {
+if (strlen(tag) >= SD_MAX_VDI_TAG_LEN) {
 error_setg(errp, "value of parameter 'tag' is too long");
 ret = -EINVAL;
 goto err_no_fd;
-- 
2.9.4

[Qemu-block] [PATCH for-2.11 1/7] block/ssh: don't call libssh2_init() in block_init()

[Jeff Cody]

We don't need libssh2 failure to be fatal (we could just opt to not
register the driver on failure). But, it is probably a good idea to
avoid external library calls during the block_init(), and call the
libssh2 global init function on the first usage, returning any errors.

Signed-off-by: Jeff Cody 
---
 block/ssh.c | 40 +---
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index e8f0404..cbb0e34 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -83,12 +83,28 @@ typedef struct BDRVSSHState {
 bool unsafe_flush_warning;
 } BDRVSSHState;
 
-static void ssh_state_init(BDRVSSHState *s)
+static bool ssh_libinit_called;
+
+static int ssh_state_init(BDRVSSHState *s, Error **errp)
 {
+int ret;
+
+if (!ssh_libinit_called) {
+ret = libssh2_init(0);
+if (ret) {
+error_setg(errp, "libssh2 initialization failed with %d", ret);
+return ret;
+}
+ssh_libinit_called = true;
+}
+
+
 memset(s, 0, sizeof *s);
 s->sock = -1;
 s->offset = -1;
 qemu_co_mutex_init(>lock);
+
+return 0;
 }
 
 static void ssh_state_free(BDRVSSHState *s)
@@ -772,8 +788,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict 
*options, int bdrv_flags,
 BDRVSSHState *s = bs->opaque;
 int ret;
 int ssh_flags;
+Error *local_err = NULL;
 
-ssh_state_init(s);
+ret = ssh_state_init(s, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return ret;
+}
 
 ssh_flags = LIBSSH2_FXF_READ;
 if (bdrv_flags & BDRV_O_RDWR) {
@@ -821,8 +842,13 @@ static int ssh_create(const char *filename, QemuOpts 
*opts, Error **errp)
 BDRVSSHState s;
 ssize_t r2;
 char c[1] = { '\0' };
+Error *local_err = NULL;
 
-ssh_state_init();
+ret = ssh_state_init(, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return ret;
+}
 
 /* Get desired file size. */
 total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
@@ -1213,14 +1239,6 @@ static BlockDriver bdrv_ssh = {
 
 static void bdrv_ssh_init(void)
 {
-int r;
-
-r = libssh2_init(0);
-if (r != 0) {
-fprintf(stderr, "libssh2 initialization failed, %d\n", r);
-exit(EXIT_FAILURE);
-}
-
 bdrv_register(_ssh);
 }
 
-- 
2.9.4

[Qemu-block] [PATCH for-2.11 5/7] block/curl: check error return of curl_global_init()

[Jeff Cody]

If curl_global_init() fails, per the documentation no other curl
functions may be called, so make sure to check the return value.

Also, some minor changes to the initialization latch variable 'inited':

- Make it static in the file, for clarity
- Change the name for clarity
- Make it a bool

Signed-off-by: Jeff Cody 
---
 block/curl.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 2a244e2..00a9879 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -89,6 +89,8 @@ static CURLMcode __curl_multi_socket_action(CURLM 
*multi_handle,
 
 struct BDRVCURLState;
 
+static bool libcurl_initialized;
+
 typedef struct CURLAIOCB {
 Coroutine *co;
 QEMUIOVector *qiov;
@@ -686,14 +688,23 @@ static int curl_open(BlockDriverState *bs, QDict 
*options, int flags,
 double d;
 const char *secretid;
 const char *protocol_delimiter;
+int ret;
 
-static int inited = 0;
 
 if (flags & BDRV_O_RDWR) {
 error_setg(errp, "curl block device does not support writes");
 return -EROFS;
 }
 
+if (!libcurl_initialized) {
+ret = curl_global_init(CURL_GLOBAL_ALL);
+if (ret) {
+error_setg(errp, "libcurl initialization failed with %d", ret);
+return -EIO;
+}
+libcurl_initialized = true;
+}
+
 qemu_mutex_init(>mutex);
 opts = qemu_opts_create(_opts, NULL, 0, _abort);
 qemu_opts_absorb_qdict(opts, options, _err);
@@ -772,11 +783,6 @@ static int curl_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 }
 
-if (!inited) {
-curl_global_init(CURL_GLOBAL_ALL);
-inited = 1;
-}
-
 DPRINTF("CURL: Opening %s\n", file);
 QSIMPLEQ_INIT(>free_state_waitq);
 s->aio_context = bdrv_get_aio_context(bs);
-- 
2.9.4

[Qemu-block] [PATCH for-2.11 0/7] Code cleanup and minor fixes

[Jeff Cody]

Some minor cleanup for a few network protocols, with a few bug fixes thrown in.
I don't think there is anything in here that needs to be for 2.10, however.

Jeff Cody (7):
  block/ssh: don't call libssh2_init() in block_init()
  block/ssh: make compliant with coding guidelines
  block/sheepdog: remove spurious NULL check
  block/sheepdog: code beautification
  block/curl: check error return of curl_global_init()
  block/curl: fix minor memory leaks
  block/curl: code cleanup to comply with coding style

 block/curl.c | 124 +++--
 block/sheepdog.c | 164 +++
 block/ssh.c  |  72 +++-
 3 files changed, 199 insertions(+), 161 deletions(-)

-- 
2.9.4

[Qemu-block] [PATCH for-2.11 6/7] block/curl: fix minor memory leaks

[Jeff Cody]

Signed-off-by: Jeff Cody 
---
 block/curl.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index 00a9879..35cf417 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -857,6 +857,9 @@ out_noclean:
 qemu_mutex_destroy(>mutex);
 g_free(s->cookie);
 g_free(s->url);
+g_free(s->username);
+g_free(s->proxyusername);
+g_free(s->proxypassword);
 qemu_opts_del(opts);
 return -EINVAL;
 }
@@ -955,6 +958,9 @@ static void curl_close(BlockDriverState *bs)
 
 g_free(s->cookie);
 g_free(s->url);
+g_free(s->username);
+g_free(s->proxyusername);
+g_free(s->proxypassword);
 }
 
 static int64_t curl_getlength(BlockDriverState *bs)
-- 
2.9.4

Re: [Qemu-block] [PATCH v3 4/7] block: convert ThrottleGroup to object with QOM

[Alberto Garcia]

On Wed 02 Aug 2017 12:57:04 PM CEST, Manos Pitsidianakis wrote:
>> At the moment I think throttle_groups_lock isn't strictly needed
>> because incref/decref callers hold the QEMU global mutex anyway.
>>
>> But code accessing throttle_groups still has to be disciplined.
>> Since throttle_groups_lock exists, please use it consistently in all
>> code paths.
>>
>> Alternatively you could remove the lock and document that
>> throttle_groups is protected by the global mutex.  What we can't do
>> is sometimes use throttle_groups_lock and sometimes not use it.
>
> If we use throttle_groups_lock in throttle_group_obj_init() then we
> must give it up in throttle_group_incref() and retake it in
> throttle_group_obj_init(). Maybe indeed it's better to drop
> throttle_groups_lock altogether, since the ThrottleGroup refcounting
> always happens in a QMP or startup/cleanup context.

I checked the code and I also don't see any manipulation of the group
list outside the global mutex, so you can remove throttle_groups_lock,
but please document very clearly that all these calls can only happen
when they are protected by the global mutex.

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Stefan Hajnoczi]

On Tue, Aug 08, 2017 at 10:06:04AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi  writes:
> 
> > On Wed, Jul 26, 2017 at 02:24:02PM -0400, Cleber Rosa wrote:
> >> 
> >> 
> >> On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
> >> > On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
> >> >> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> >> >>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> >>  On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> >> > On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> >> >> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> >>  Without the static capabilities defined, the dynamic check would be
> >>  influenced by the run time environment.  It would really mean "qemu-io
> >>  running on this environment (filesystem?) can do native aio".  Again,
> >>  that's not the best type of information to depend on when writing 
> >>  tests.
> >> >>>
> >> >>> Can you explain this more?
> >> >>>
> >> >>> It seems logical to me that if qemu-io in this environment cannot do
> >> >>> aio=native then we must skip those tests.
> >> >>>
> >> >>> Stefan
> >> >>>
> >> >>
> >> >> OK, let's abstract a bit more.  Let's take this part of your statement:
> >> >>
> >> >>  "if qemu-io in this environment cannot do aio=native"
> >> >>
> >> >> Let's call that a feature check.  Depending on how the *feature check*
> >> >> is written, a negative result may hide a test failure, because it would
> >> >> now be skipped.
> >> > 
> >> > You are saying a pass->skip transition can hide a failure but ./check
> >> > tracks skipped tests.  See tests/qemu-iotests/check.log for a
> >> > pass/fail/skip history.
> >> > 
> >> 
> >> You're not focusing on the problem here.  The problem is that a test
> >> that *was not* supposed to be skipped, would be skipped.
> >
> > As Daniel Berrange mentioned, ./configure has the same problem.  You
> > cannot just run it blindly because it silently disables features.
> >
> > What I'm saying is that in addition to watching ./configure closely, you
> > also need to look at the skipped tests that ./check reports.  If you do
> > that then you can be sure the expected set of tests is passing.
> >
> >> > It is the job of the CI system to flag up pass/fail/skip transitions.
> >> > You're no worse off using feature tests.
> >> > 
> >> > Stefan
> >> > 
> >> 
> >> What I'm trying to help us achieve here is a reliable and predictable
> >> way for the same test job execution to be comparable across
> >> environments.  From the individual developer workstation, CI, QA etc.
> >
> > 1. Use ./configure --enable-foo options for all desired features.
> > 2. Run the ./check command-line and there should be no unexpected skips
> >like this:
> >
> > 087 [not run] missing aio=native support
> >
> > To me this seems to address the problem.
> >
> > I have mentioned the issues with the build flags solution: it creates a
> > dependency on the build environment and forces test feature checks to
> > duplicate build dependency logic.  This is why I think feature tests are
> > a cleaner solution.
> 
> I suspect the actual problem here is that the qemu-iotests harness is
> not integrated in the build process.  For other tests, we specify the
> tests to run in a Makefile, and use the same configuration mechanism as
> for building stuff conditionally.

The ability to run tests against QEMU binaries without a build
environment is useful though.  It would still be possible to symlink to
external binaries but then the build feature information could be
incorrect.

Stefan


signature.asc
Description: PGP signature

[Qemu-block] [PATCH] virtio-blk: handle blk_getlength() errors

[Stefan Hajnoczi]

If blk_getlength() fails in virtio_blk_update_config() consider the disk
image length to be 0 bytes.

Signed-off-by: Stefan Hajnoczi 
---
 hw/block/virtio-blk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b750bd8b53..a16ac75090 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -730,6 +730,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
 BlockConf *conf = >conf.conf;
 struct virtio_blk_config blkcfg;
 uint64_t capacity;
+int64_t length;
 int blk_size = conf->logical_block_size;
 
 blk_get_geometry(s->blk, );
@@ -752,7 +753,8 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
  * divided by 512 - instead it is the amount of blk_size blocks
  * per track (cylinder).
  */
-if (blk_getlength(s->blk) /  conf->heads / conf->secs % blk_size) {
+length = blk_getlength(s->blk);
+if (length > 0 && length / conf->heads / conf->secs % blk_size) {
 blkcfg.geometry.sectors = conf->secs & ~s->sector_mask;
 } else {
 blkcfg.geometry.sectors = conf->secs;
-- 
2.13.3

Re: [Qemu-block] Unchecked blk_getlength() in device models and board code

[Stefan Hajnoczi]

On Fri, Aug 04, 2017 at 04:01:18PM +0200, Markus Armbruster wrote:
> blk_getlength() can fail.  I figure the following need fixing:
> 
> hw/arm/musicpal.c: musicpal_init()

Seems okay:

flash_size = blk_getlength(blk);
if (flash_size != 8*1024*1024 && flash_size != 16*1024*1024 &&
flash_size != 32*1024*1024) {
fprintf(stderr, "Invalid flash image size\n");
exit(1);
}

> hw/block/virtio-blk.c: virtio_blk_update_config()

Will send a fix.


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH 3/3] maint: Include bug-reporting info in --help output.

[Paolo Bonzini]

On 08/08/2017 13:06, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> These days, many programs are including a bug-reporting address,
>> or better yet, a link to the project web site, at the tail of
>> their --help output.  However, we were not very consistent at
>> doing so: only qemu-nbd and qemu-qa mentioned anything, with the
>> latter pointing to an individual person instead of the project.
>>
>> Add a new #define that sets up a uniform string, mentioning both
>> bug reporting instructions and overall project details, and which
>> a downstream vendor could tweak if they want bugs to go to a
>> downstream database.  Then use it in all of our binaries which
>> have --help output.
>>
>> The canned text intentionally references http:// instead of https://
>> because our https website currently causes certificate errors in
>> some browsers.  That can be tweaked later once we have resolved the
>> web site issued.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  include/qemu-common.h | 5 +
>>  vl.c  | 4 +++-
>>  bsd-user/main.c   | 2 ++
>>  linux-user/main.c | 4 +++-
>>  qemu-img.c| 2 +-
>>  qemu-io.c | 5 +++--
>>  qemu-nbd.c| 2 +-
>>  qga/main.c| 2 +-
>>  8 files changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index b5adbfa5e9..e751361458 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -22,6 +22,11 @@
>>  #define QEMU_COPYRIGHT "Copyright (c) 2003-2017 " \
>>  "Fabrice Bellard and the QEMU Project developers"
>>
>> +/* Bug reporting information for --help arguments, About dialogs, etc */
>> +#define QEMU_BUGREPORTS \
>> +"See  for bug reports.\n" \
> 
> "See ... for bug reports" sounds like it's about browsing existing bugs.
> The web page is actually about reporting bugs.  What about "for how to
> report bugs"?
> 
> Since I'm basically bikeshedding already: the macro expands into more
> than just bug reporting.  Call it QEMU_HELP_BOTTOM?  Feel free to ignore
> this one.

Easily squashed:

diff --git a/bsd-user/main.c b/bsd-user/main.c
index 4db10cb376..8a6706a1c8 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -687,7 +687,7 @@ static void usage(void)
"Note that if you provide several changes to single variable\n"
"last change will stay in effect.\n"
"\n"
-   QEMU_BUGREPORTS "\n"
+   QEMU_HELP_BOTTOM "\n"
,
TARGET_NAME,
interp_prefix,
diff --git a/include/qemu-common.h b/include/qemu-common.h
index d29045631f..0456c79df4 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -23,8 +23,8 @@
 "Fabrice Bellard and the QEMU Project developers"
 
 /* Bug reporting information for --help arguments, About dialogs, etc */
-#define QEMU_BUGREPORTS \
-"See  for bug reports.\n" \
+#define QEMU_HELP_BOTTOM \
+"See  for how to report bugs.\n" \
 "More information on the QEMU project at ."
 
 /* main function, renamed */
diff --git a/linux-user/main.c b/linux-user/main.c
index 7d6e481277..03666ef657 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4138,7 +4138,7 @@ static void usage(int exitcode)
"Note that if you provide several changes to a single variable\n"
"the last change will stay in effect.\n"
"\n"
-   QEMU_BUGREPORTS "\n");
+   QEMU_HELP_BOTTOM "\n");
 
 exit(exitcode);
 }
diff --git a/qemu-img.c b/qemu-img.c
index 758719e083..56ef49e214 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -201,7 +201,7 @@ static void QEMU_NORETURN help(void)
 
 printf("%s\nSupported formats:", help_msg);
 bdrv_iterate_format(format_print, NULL);
-printf("\n\n" QEMU_BUGREPORTS "\n");
+printf("\n\n" QEMU_HELP_BOTTOM "\n");
 exit(EXIT_SUCCESS);
 }
 
diff --git a/qemu-io.c b/qemu-io.c
index b93553a603..265445ad89 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -264,7 +264,7 @@ static void usage(const char *name)
 "\n"
 "See '%s -c help' for information on available commands.\n"
 "\n"
-QEMU_BUGREPORTS "\n",
+QEMU_HELP_BOTTOM "\n",
 name, name);
 }
 
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 052eb4d067..27164b8205 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -123,7 +123,7 @@ static void usage(const char *name)
 "  --detect-zeroes=MODE  set detect-zeroes mode (off, on, unmap)\n"
 "  --image-opts  treat FILE as a full set of image options\n"
 "\n"
-QEMU_BUGREPORTS "\n"
+QEMU_HELP_BOTTOM "\n"
 , name, NBD_DEFAULT_PORT, "DEVICE");
 }
 
diff --git a/qga/main.c b/qga/main.c
index 56d5633c13..62a62755bd 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -249,7 +249,7 @@ QEMU_COPYRIGHT "\n"
 "options / command-line parameters to stdout\n"
 "  -h, --helpdisplay 

Re: [Qemu-block] [Qemu-devel] [PATCH 3/3] maint: Include bug-reporting info in --help output.

[Markus Armbruster]

Eric Blake  writes:

> These days, many programs are including a bug-reporting address,
> or better yet, a link to the project web site, at the tail of
> their --help output.  However, we were not very consistent at
> doing so: only qemu-nbd and qemu-qa mentioned anything, with the
> latter pointing to an individual person instead of the project.
>
> Add a new #define that sets up a uniform string, mentioning both
> bug reporting instructions and overall project details, and which
> a downstream vendor could tweak if they want bugs to go to a
> downstream database.  Then use it in all of our binaries which
> have --help output.
>
> The canned text intentionally references http:// instead of https://
> because our https website currently causes certificate errors in
> some browsers.  That can be tweaked later once we have resolved the
> web site issued.
>
> Signed-off-by: Eric Blake 
> ---
>  include/qemu-common.h | 5 +
>  vl.c  | 4 +++-
>  bsd-user/main.c   | 2 ++
>  linux-user/main.c | 4 +++-
>  qemu-img.c| 2 +-
>  qemu-io.c | 5 +++--
>  qemu-nbd.c| 2 +-
>  qga/main.c| 2 +-
>  8 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index b5adbfa5e9..e751361458 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -22,6 +22,11 @@
>  #define QEMU_COPYRIGHT "Copyright (c) 2003-2017 " \
>  "Fabrice Bellard and the QEMU Project developers"
>
> +/* Bug reporting information for --help arguments, About dialogs, etc */
> +#define QEMU_BUGREPORTS \
> +"See  for bug reports.\n" \

"See ... for bug reports" sounds like it's about browsing existing bugs.
The web page is actually about reporting bugs.  What about "for how to
report bugs"?

Since I'm basically bikeshedding already: the macro expands into more
than just bug reporting.  Call it QEMU_HELP_BOTTOM?  Feel free to ignore
this one.

> +"More information on the qemu project at "

"QEMU project"

> +
>  /* main function, renamed */
>  #if defined(CONFIG_COCOA)
>  int qemu_main(int argc, char **argv, char **envp);

Getting late for 2.10, but it's such a lovely little improvement...

Re: [Qemu-block] [PATCH for-2.10] block/nfs: fix mutex assertion in nfs_file_close()

[Peter Lieven]

Am 08.08.2017 um 00:29 schrieb Jeff Cody:

Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion
checks for when qemu_mutex() functions are called without the
corresponding qemu_mutex_init() having initialized the mutex.

This uncovered a latent bug in qemu's nfs driver - in
nfs_client_close(), the NFSClient structure is overwritten with zeros,
prior to the mutex being destroyed.

Go ahead and destroy the mutex in nfs_client_close(), and change where
we call qemu_mutex_init() so that it is correctly balanced.

There are also a couple of memory leaks obscured by the memset, so this
fixes those as well.

Finally, we should be able to get rid of the memset(), as it isn't
necessary.

Signed-off-by: Jeff Cody 
---
  block/nfs.c | 21 +
  1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index d8db419..bec16b7 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -433,19 +433,23 @@ static void nfs_client_close(NFSClient *client)
  if (client->context) {
  if (client->fh) {
  nfs_close(client->context, client->fh);
+client->fh = NULL;
  }
  aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context),
 false, NULL, NULL, NULL, NULL);
  nfs_destroy_context(client->context);
+client->context = NULL;
  }
-memset(client, 0, sizeof(NFSClient));
-}
-
-static void nfs_file_close(BlockDriverState *bs)
-{
-NFSClient *client = bs->opaque;
-nfs_client_close(client);
+g_free(client->path);
  qemu_mutex_destroy(>mutex);
+qapi_free_NFSServer(client->server);
+client->server = NULL;
+}
+
+static void nfs_file_close(BlockDriverState *bs)
+{
+NFSClient *client = bs->opaque;
+nfs_client_close(client);
  }
  
  static NFSServer *nfs_config(QDict *options, Error **errp)

@@ -498,6 +502,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict 
*options,
  struct stat st;
  char *file = NULL, *strp = NULL;
  
+qemu_mutex_init(>mutex);

  opts = qemu_opts_create(_opts, NULL, 0, _abort);
  qemu_opts_absorb_qdict(opts, options, _err);
  if (local_err) {
@@ -660,7 +665,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict 
*options, int flags,
  if (ret < 0) {
  return ret;
  }
-qemu_mutex_init(>mutex);
+
  bs->total_sectors = ret;
  ret = 0;
  return ret;


Reviewed-by: Peter Lieven 

Also CC'ing qemu-stable as this affects 2.9.0 as well.

Peter

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10] throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

[Alberto Garcia]

On Tue 08 Aug 2017 12:17:12 PM CEST, Alberto Garcia wrote:

> I was under the impression that Markus wanted to change the QAPI types
> of the throttling fields in BlockDeviceInfo for 2.10 as well, so this
> patch is relevant.

I just saw that his series is still an RFC, so we can leave this patch
for 2.11. Sorry again!

Berto

Re: [Qemu-block] [PATCH] iotests: fix 185

[Vladimir Sementsov-Ogievskiy]

08.08.2017 12:04, Vladimir Sementsov-Ogievskiy wrote:

08.08.2017 12:04, Vladimir Sementsov-Ogievskiy wrote:

08.08.2017 11:53, Kevin Wolf wrote:

Am 08.08.2017 um 10:42 hat Vladimir Sementsov-Ogievskiy geschrieben:

07.08.2017 18:57, Kevin Wolf wrote:

Am 07.08.2017 um 16:16 hat Vladimir Sementsov-Ogievskiy geschrieben:

185 iotest is broken.

How to test:

i=0; while ./check -qcow2 -nocache 185; do ((i+=1)); echo N = $i; \

done; echo N = $i

finished for me like this:

185 2s ... - output mismatch (see 185.out.bad)
--- /work/src/qemu/master/tests/qemu-iotests/185.out 2017-07-14 \
  15:14:29.520343805 +0300
+++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300
@@ -37,7 +37,7 @@
   {"return": {}}
   {"return": {}}
   {"timestamp": {"seconds":  TIMESTAMP, "microseconds": 
TIMESTAMP}, \

   "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, \
  "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
  "len": 4194304, "offset": 4194304, "speed": 65536, 
"type": \

  "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds": TIMESTAMP}, \
  "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \
  "len": 0, "offset": 0, "speed": 65536, "type": "mirror"}}

   === Start backup job and exit qemu ===

Failures: 185
Failed 1 of 1 tests
N = 8

It doesn't seems logical to expect the constant offset on cancel,
so let filter it out.

Signed-off-by: Vladimir Sementsov-Ogievskiy 


I'm quoting 185:

# Note that the reference output intentionally includes the 
'offset' field in
# BLOCK_JOB_CANCELLED events for all of the following block jobs. 
They are
# predictable and any change in the offsets would hint at a bug in 
the job

# throttling code.
#
# In order to achieve these predictable offsets, all of the 
following tests
# use speed=65536. Each job will perform exactly one iteration 
before it has
# to sleep at least for a second, which is plenty of time for the 
'quit' QMP

# command to be received (after receiving the command, the rest runs
# synchronously, so jobs can arbitrarily continue or complete).
#
# The buffer size for commit and streaming is 512k (waiting for 8 
seconds after
# the first request), for active commit and mirror it's large 
enough to cover
# the full 4M, and for backup it's the qcow2 cluster size, which 
we know is
# 64k. As all of these are at least as large as the speed, we are 
sure that the

# offset doesn't advance after the first iteration before qemu exits.

So before we change the expected output, can we explain why the 
offsets
aren't predictable, even if throttling is used and contrary to 
what the

comment says? Should we sleep a little before issuing 'quit'?
Throttling "guaranties" that there will not be more than one 
request. But

what prevent less than one, i.e. zero, like in my reproduction?

Yes, I understand. Can we somehow make sure that at least one iteration
is made? I'd really like to keep the functional test for block job
throttling. I suppose a simple 'sleep 0.1' would do the trick, though
it's not very clean.

Kevin



I've started with 'sleep 0.5', now there are >100 successful 
iterations... The other way is to check in test that there was 0 or 1 
requests, but for this it looks better to rewrite it in python.





is sleep for ms portable?




>1500 successful iterations, so, 'sleep 0.5' is OK for me.


--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10] throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

[Alberto Garcia]

On Tue 08 Aug 2017 12:00:30 PM CEST, Stefan Hajnoczi wrote:
> On Mon, Aug 07, 2017 at 07:15:29PM +0300, Alberto Garcia wrote:
>> Both the throttling limits set with the throttling.iops-* and
>> throttling.bps-* options and their QMP equivalents defined in the
>> BlockIOThrottle struct are integer values.
>> 
>> Those limits are also reported in the BlockDeviceInfo struct and they
>> are integers there as well.
>> 
>> Therefore there's no reason to store them internally as double and do
>> the conversion everytime we're setting or querying them, so this patch
>> uses int64_t for those types.
>> 
> Why is this marked for-2.10?  Does it fix a bug?

I was under the impression that Markus wanted to change the QAPI types
of the throttling fields in BlockDeviceInfo for 2.10 as well, so this
patch is relevant.

If that's not the case we can ignore it for now and leave it for the
next cycle. Sorry for the noise!

Berto

Re: [Qemu-block] [PATCH v3] block: document semanatics of bdrv_co_preadv|pwritev

[Stefan Hajnoczi]

On Fri, Aug 04, 2017 at 03:08:26PM +0100, Daniel P. Berrange wrote:
> Signed-off-by: Daniel P. Berrange 
> ---
> 
>  - Clarify that @bytes matches @qiov total size (Kevin)
> 
>  include/block/block_int.h | 31 +++
>  1 file changed, 31 insertions(+)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature