Re: [Qemu-block] [Qemu-trivial] [PATCH 1/1] virtio-blk: fix comment for virtio_blk_rw_complete as nalloc is initially -1

[Laurent Vivier]

On 06/11/2018 05:52, Dongli Zhang wrote:
> The initial value of nalloc is -1, but not 1.
> 
> Signed-off-by: Dongli Zhang 
> ---
> This is based on git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git 
> tags/for_upstream
> 
>  hw/block/virtio-blk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 83cf5c0..30999c3 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -96,7 +96,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
>  trace_virtio_blk_rw_complete(vdev, req, ret);
>  
>  if (req->qiov.nalloc != -1) {
> -/* If nalloc is != 1 req->qiov is a local copy of the original
> +/* If nalloc is != -1 req->qiov is a local copy of the original
>   * external iovec. It was allocated in submit_requests to be
>   * able to merge requests. */
>  qemu_iovec_destroy(&req->qiov);
> 

Reviewed-by: Laurent Vivier 

[Qemu-block] [PATCH 1/1] virtio-blk: fix comment for virtio_blk_rw_complete as nalloc is initially -1

[Dongli Zhang]

The initial value of nalloc is -1, but not 1.

Signed-off-by: Dongli Zhang 
---
This is based on git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git 
tags/for_upstream

 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 83cf5c0..30999c3 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -96,7 +96,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
 trace_virtio_blk_rw_complete(vdev, req, ret);
 
 if (req->qiov.nalloc != -1) {
-/* If nalloc is != 1 req->qiov is a local copy of the original
+/* If nalloc is != -1 req->qiov is a local copy of the original
  * external iovec. It was allocated in submit_requests to be
  * able to merge requests. */
 qemu_iovec_destroy(&req->qiov);
-- 
2.7.4

Re: [Qemu-block] [Qemu-devel] [PULL 05/33] virtio-blk: fix comment for virtio_blk_rw_complete

[Michael S. Tsirkin]

On Tue, Nov 06, 2018 at 11:17:03AM +0800, Dongli Zhang wrote:
> 
> 
> On 11/06/2018 02:15 AM, Michael S. Tsirkin wrote:
> > From: Yaowei Bai 
> > 
> > Here should be submit_requests, there is no submit_merged_requests
> > function.
> > 
> > Signed-off-by: Yaowei Bai 
> > Reviewed-by: Michael S. Tsirkin 
> > Signed-off-by: Michael S. Tsirkin 
> > ---
> >  hw/block/virtio-blk.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 225fe44b7a..83cf5c01f9 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -97,8 +97,8 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
> >  
> >  if (req->qiov.nalloc != -1) {
> >  /* If nalloc is != 1 req->qiov is a local copy of the original
> 
> Should it be "If nalloc is != -1" in the comment? Seems the initial state is 
> -1.

Makes sense. Patch?

> > - * external iovec. It was allocated in submit_merged_requests
> > - * to be able to merge requests. */
> > + * external iovec. It was allocated in submit_requests to be
> > + * able to merge requests. */
> >  qemu_iovec_destroy(&req->qiov);
> >  }
> >  
> > 
> 
> Dongli Zhang

Re: [Qemu-block] [Qemu-devel] [PULL 05/33] virtio-blk: fix comment for virtio_blk_rw_complete

[Dongli Zhang]

On 11/06/2018 02:15 AM, Michael S. Tsirkin wrote:
> From: Yaowei Bai 
> 
> Here should be submit_requests, there is no submit_merged_requests
> function.
> 
> Signed-off-by: Yaowei Bai 
> Reviewed-by: Michael S. Tsirkin 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  hw/block/virtio-blk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 225fe44b7a..83cf5c01f9 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -97,8 +97,8 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
>  
>  if (req->qiov.nalloc != -1) {
>  /* If nalloc is != 1 req->qiov is a local copy of the original

Should it be "If nalloc is != -1" in the comment? Seems the initial state is -1.

> - * external iovec. It was allocated in submit_merged_requests
> - * to be able to merge requests. */
> + * external iovec. It was allocated in submit_requests to be
> + * able to merge requests. */
>  qemu_iovec_destroy(&req->qiov);
>  }
>  
> 

Dongli Zhang

Re: [Qemu-block] [Qemu-devel] [PATCH] block: Make more block drivers compile-time configurable

[Max Reitz]

On 05.11.18 16:25, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> On 19.10.18 13:34, Markus Armbruster wrote:
>>> From: Jeff Cody 
>>>
>>> This adds configure options to control the following block drivers:
>>>
>>> * Bochs
>>> * Cloop
>>> * Dmg
>>> * Qcow (V1)
>>> * Vdi
>>> * Vvfat
>>> * qed
>>> * parallels
>>> * sheepdog
>>>
>>> Each of these defaults to being enabled.
>>>
>>> Signed-off-by: Jeff Cody 
>>> Signed-off-by: Markus Armbruster 
>>> ---
>>>
>>> Hmm, we got quite a few --enable-BLOCK-DRIVER now.  Perhaps a single
>>> list-valued option similar --target-list would be better.  Could be
>>> done on top.
>>>
>>>  block/Makefile.objs | 22 ---
>>>  configure   | 91 +
>>>  2 files changed, 107 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>>> index c8337bf186..1cad9fc4f1 100644
>>> --- a/block/Makefile.objs
>>> +++ b/block/Makefile.objs
>@@ -1,10 +1,18 @@
>-block-obj-y += raw-format.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o 
> vvfat.o dmg.o
>+block-obj-y += raw-format.o vmdk.o vpc.o
>+block-obj-$(CONFIG_QCOW1) += qcow.o
>+block-obj-$(CONFIG_VDI) += vdi.o
>+block-obj-$(CONFIG_CLOOP) += cloop.o
>+block-obj-$(CONFIG_BOCHS) += bochs.o
>+block-obj-$(CONFIG_VVFAT) += vvfat.o
>+block-obj-$(CONFIG_DMG) += dmg.o
>+
> block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
> qcow2-cache.o qcow2-bitmap.o
>>
>> [...]
>>
>>> @@ -45,7 +54,8 @@ gluster.o-libs := $(GLUSTERFS_LIBS)
>>>  vxhs.o-libs:= $(VXHS_LIBS)
>>>  ssh.o-cflags   := $(LIBSSH2_CFLAGS)
>>>  ssh.o-libs := $(LIBSSH2_LIBS)
>>> -block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
>>> +block-obj-dmg-bz2$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
>>> +block-obj-$(CONFIG_DMG) += $(block-obj-dmg-bz2-y)
>>
>> This defines "block-obj-dmg-bz2m" or "block-obj-dmg-bz2n", so
>> "block-obj-dmg-bz2-y" is never defined (note both the missing hyphen and
>> the "m" vs. "y").
>>
>> How about:
>>
>> block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
> 
> As far as I can tell, CONFIG_BZIP2 is either undefined or "y".  Thus,
> block-obj-dmg-bz2-y is either left undefined or set to dmg-bz2.o.

Yes.

> Perhaps the '+=' be ':=', but we seem to use '+=' pretty
> indiscriminately.

Yep.  I don't know.  Whatever works, and both do, so...

>> block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
> 
> As far as I can tell, CONFIG_DMG is also either undefined or "y".  So,
> this adds dmg-bz2.o to block-obj-m if both CONFIG_BZIP2 and CONFIG_DMG
> are enabled.

Yes.

> Shouldn't it be added to block-obj-y, like dmg.o, or am I confused?

The behavior before this patch was to add it to block-obj-m.
27685a8dd08c051fa6d641fe46106fc0dfa51073 has the explanation: We want
the bz2 part to be a module so you can launch qemu even without libbz2
around.  Only when you use dmg will it load that module.

(And if you dig deeper, it was 88d88798b7efe that (intentionally) broke
 that intended behavior, until it was restored by the above commit.)

>> Bonus point: The "+=" are naturally aligned!
> 
> Woot!

:-)

Max

>> (Fun fact on the side: I tried downloading some dmg image, but qemu
>> refused to open that.  ("sector count 409600 for chunk 4 is larger than
>> max (131072)" -- yeah, yeah, I know that I'm not the largest guy) -- but
>> you can test it just by replacing "dmg-bz2.o" by "does-not-exist.o", and
>> then make complains normally, but stops complaining with --disable-dmg
>> or --disable-bzip2.)
> 
> Thanks!
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 0/5] Various option help readability improvement suggestions

[Max Reitz]

On 05.11.18 15:18, Kevin Wolf wrote:
> Am 19.10.2018 um 18:49 hat Max Reitz geschrieben:
>> I noticed that with the (more or less) recent series from Marc-André the
>> output of qemu-img amend -f qcow2 -o help changed to this:
>>
>> $ ./qemu-img amend -f qcow2 -o help
>> Creation options for 'qcow2':
>> qcow2-create-opts.backing_file=str - File name of a base image
>> qcow2-create-opts.backing_fmt=str - Image format of the base image
>> qcow2-create-opts.cluster_size=size - qcow2 cluster size
>> qcow2-create-opts.compat=str - Compatibility level (0.10 or 1.1)
>> [...]
>>
>> The types are a nice addition, but I didn't like having the list name
>> printed in every single line (in fact, the list name does not make any
>> sense here at all, because there already is a caption which reads
>> "Creation options for 'qcow2'"), and I did not like the use of '=' for
>> types.
>>
>> In general, I don't like the robot-y appearance, which is even worse in
>> things like -device virtio-blk,help, which gives you this (among
>> other lines):
>>
>>> virtio-blk-pci.iothread=link
>>
>> Sadly, there isn't much we can do about the "link", so this
>> series doesn't improve on that point.
>>
>> What this series does do, however, is it changes these lists not to
>> print the list name on every single line, but only as a caption (and for
>> option lists, this caption is option, because the caller may want to
>> print a custom caption that is more expressive -- as is the case for
>> qemu-img amend -o help).
>>
>> Consequentially, all list items are indented by two spaces to make clear
>> they belong to the caption.  I can already see that some people might
>> disagree on having this indentation, but I like it, so I have it in this
>> series.
>>
>> Furthermore, types are now enclosed by angle brackets, and the alignment
>> we originally had for descriptions is restored (although now after 24
>> instead of 16 characters, because every option name is now accompanied
>> by indentation and a type).
>>
>>
>> Thus, after this series, the amend output looks like this:
>>
>> $ ./qemu-img amend -f qcow2 -o help
>> Creation options for 'qcow2':
>>   backing_file= - File name of a base image
>>   backing_fmt=  - Image format of the base image
>>   cluster_size=- qcow2 cluster size
>>   compat=   - Compatibility level (0.10 or 1.1)
>> [...]
>>
>>
>> virtio-blk's list presents itself like so:
>>
>> $ x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help
>> virtio-blk-pci options:
>>   iothread=>
>>   request-merging= - on/off
>>   secs=
>> [...]
>>
>>
>> And now we even print something when there are no options:
>>
>> $ x86_64-softmmu/qemu-system-x86_64 -object can-bus,help
>> There are no options for can-bus.
>>
>> (Before this series, there just is no output.)
>>
>>
>> As a side effect, patch 1 fixes iotest 082.
> 
> Thanks, applied to the block branch.

Thank you both for the quick pong. :-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] include: Add a comment to explain the origin of sizes' lookup table

[no-reply]

Hi,

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

Type: series
Message-id: 20181103015821.30074-1-lbl...@janustech.com
Subject: [Qemu-devel] [PATCH] include: Add a comment to explain the origin of 
sizes' lookup table

=== 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
git config --local diff.algorithm histogram

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'
4b786d4cd6 include: Add a comment to explain the origin of sizes' lookup table

=== OUTPUT BEGIN ===
Checking PATCH 1/1: include: Add a comment to explain the origin of sizes' 
lookup table...
ERROR: code indent should never use tabs
#43: FILE: include/qemu/units.h:27:
+ *  ^Isuffix="KMGTPE";$

ERROR: code indent should never use tabs
#44: FILE: include/qemu/units.h:28:
+ *  ^Ifor(i=10; i<64; i++) {$

ERROR: code indent should never use tabs
#45: FILE: include/qemu/units.h:29:
+ *  ^I^Ival=2**i;$

ERROR: code indent should never use tabs
#46: FILE: include/qemu/units.h:30:
+ *  ^I^Is=substr(suffix, int(i/10), 1);$

ERROR: code indent should never use tabs
#47: FILE: include/qemu/units.h:31:
+ *  ^I^In=2**(i%10);$

ERROR: code indent should never use tabs
#48: FILE: include/qemu/units.h:32:
+ *  ^I^Ipad=21-int(log(n)/log(10));$

ERROR: code indent should never use tabs
#49: FILE: include/qemu/units.h:33:
+ *  ^I^Iprintf("#define S_%d%siB %*d\n", n, s, pad, val);$

ERROR: code indent should never use tabs
#50: FILE: include/qemu/units.h:34:
+ *  ^I}$

total: 8 errors, 0 warnings, 24 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.

=== OUTPUT END ===

Test command exited with code: 1


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

Re: [Qemu-block] [PATCH v3 1/6] qemu-nbd: add support for authorization of TLS clients

[Eric Blake]

On 10/9/18 8:23 AM, Daniel P. Berrangé wrote:

From: "Daniel P. Berrange" 

Currently any client which can complete the TLS handshake is able to use
the NBD server. The server admin can turn on the 'verify-peer' option
for the x509 creds to require the client to provide a x509 certificate.
This means the client will have to acquire a certificate from the CA
before they are permitted to use the NBD server. This is still a fairly
low bar to cross.

This adds a '--tls-authz OBJECT-ID' option to the qemu-nbd command which
takes the ID of a previously added 'QAuthZ' object instance. This will
be used to validate the client's x509 distinguished name. Clients
failing the authorization check will not be permitted to use the NBD
server.

For example to setup authorization that only allows connection from a client
whose x509 certificate distinguished name is

CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB

use:

   qemu-nbd --object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
 endpoint=server,verify-peer=yes \
--object authz-simple,id=auth0,identity=CN=laptop.example.com,,\
 O=Example Org,,L=London,,ST=London,,C=GB \


Missing shell quoting around the space in 'Example Org'. It's also 
fairly obvious that actual shell commands can't have leading space 
between \-newline line continuations.



--tls-creds tls0 \
--tls-authz authz0
   other qemu-nbd args...

Signed-off-by: Daniel P. Berrange 
---
  include/block/nbd.h |  2 +-
  nbd/server.c| 10 +-
  qemu-nbd.c  | 13 -
  qemu-nbd.texi   |  4 
  4 files changed, 22 insertions(+), 7 deletions(-)




+++ b/qemu-nbd.c
@@ -52,6 +52,7 @@
  #define QEMU_NBD_OPT_TLSCREDS  261
  #define QEMU_NBD_OPT_IMAGE_OPTS262
  #define QEMU_NBD_OPT_FORK  263
+#define QEMU_NBD_OPT_TLSAUTHZ  264
  



@@ -532,6 +534,7 @@ int main(int argc, char **argv)
  { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
  { "trace", required_argument, NULL, 'T' },
  { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
+{ "tls-authz", no_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
  { NULL, 0, NULL, 0 }
  };


Missing a change to qemu-nbd --help to describe the new option.

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

[Qemu-block] [PATCH v5 0/5] off-by-one and NULL pointer accesses detected by static analysis

[Liam Merwick]

Below are a number of fixes to some off-by-one, read outside array bounds, and
NULL pointer accesses detected by an internal Oracle static analysis tool 
(Parfait).
https://labs.oracle.com/pls/apex/f?p=labs:49:P49_PROJECT_ID:13

v1 -> v2
Based on feedback from Eric Blake:
patch2: reworded commit message to clarify issue
patch6: Reverted common qlist routines and added assert to qlist_dump instead
patch7: Fixed incorrect logic
patch8: Added QEMU_BUILD_BUG_ON to catch future іnstance at compile-time

v2 -> v3
Based on feedback from Eric Blake:
patch6: removed double space from commit message
patch8: removed unnecessary comment and updated QEMU_BUILD_BUG_ON to use 
ARRAY_SIZE
Added Eric's R-b to patches 6,7,8

v3 -> v4
Based on feedback from Max Reitz:
patch2: Added R-b from John Snow
patch3: fixed blk_get_attached_dev_id() instead of checking return value
patch4: switched to assert()
patch5: numerous changes based on feedback from Max
patch6: updated commit message
patch7: (was patch8): Added Max's R-b
patch8: (new): patch fixing NULL pointer dereference in kvm_arch_init_vcpu()

v4 -> v5
Based on further feedback from Max Reitz:
Dropped v4 patch1 (configure --disable-avx2) as Thomas Huth already pulled it. 
Dropped v4 patch6 (dump_qlist) as it was just an unnecessary assert
Dropped v4 patch8 'patch fixing NULL pointer dereference in 
kvm_arch_init_vcpu()'
  so as to limit this seies to block changes (will send in a separate series).
patch1: no change (v4 patch2)
patch2: Switched to using ?: in return (v4 patch3)
patch3: Added Max's R-b (v4 patch4)
patch4: couple of changes based on feedback from Max (v4 patch5)
patch5: no change (v4 patch7)

Liam Merwick (5):
  job: Fix off-by-one assert checks for JobSTT and JobVerbTable
  block: Null pointer dereference in blk_root_get_parent_desc()
  qemu-img: assert block_job_get() does not return NULL in img_commit()
  block: Fix potential Null pointer dereferences in vvfat.c
  qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

 block/block-backend.c  |  3 ++-
 block/qcow2-refcount.c | 18 ++
 block/vvfat.c  | 49 +
 job.c  |  4 ++--
 qemu-img.c |  1 +
 5 files changed, 48 insertions(+), 27 deletions(-)

-- 
1.8.3.1

[Qemu-block] [PATCH v5 5/5] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

[Liam Merwick]

The commit for 0e4e4318eaa5 increments QCOW2_OL_MAX_BITNR but does not
add an array entry for QCOW2_OL_BITMAP_DIRECTORY_BITNR to metadata_ol_names[].
As a result, an array dereference of metadata_ol_names[8] in
qcow2_pre_write_overlap_check() could result in a read outside of the array 
bounds.

Fixes: 0e4e4318eaa5 ('qcow2: add overlap check for bitmap directory')

Cc: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Liam Merwick 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
---
 block/qcow2-refcount.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c539f02e5ec..46082aeac1d6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2719,15 +2719,17 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, 
int ign, int64_t offset,
 }
 
 static const char *metadata_ol_names[] = {
-[QCOW2_OL_MAIN_HEADER_BITNR]= "qcow2_header",
-[QCOW2_OL_ACTIVE_L1_BITNR]  = "active L1 table",
-[QCOW2_OL_ACTIVE_L2_BITNR]  = "active L2 table",
-[QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
-[QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
-[QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
-[QCOW2_OL_INACTIVE_L1_BITNR]= "inactive L1 table",
-[QCOW2_OL_INACTIVE_L2_BITNR]= "inactive L2 table",
+[QCOW2_OL_MAIN_HEADER_BITNR]= "qcow2_header",
+[QCOW2_OL_ACTIVE_L1_BITNR]  = "active L1 table",
+[QCOW2_OL_ACTIVE_L2_BITNR]  = "active L2 table",
+[QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
+[QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
+[QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
+[QCOW2_OL_INACTIVE_L1_BITNR]= "inactive L1 table",
+[QCOW2_OL_INACTIVE_L2_BITNR]= "inactive L2 table",
+[QCOW2_OL_BITMAP_DIRECTORY_BITNR]   = "bitmap directory",
 };
+QEMU_BUILD_BUG_ON(QCOW2_OL_MAX_BITNR != ARRAY_SIZE(metadata_ol_names));
 
 /*
  * First performs a check for metadata overlaps (through
-- 
1.8.3.1

[Qemu-block] [PATCH v5 4/5] block: Fix potential Null pointer dereferences in vvfat.c

[Liam Merwick]

The calls to find_mapping_for_cluster() may return NULL but it
isn't always checked for before dereferencing the value returned.
Additionally, add some asserts to cover cases where NULL can't
be returned but which might not be obvious at first glance.

Signed-off-by: Liam Merwick 
---
 block/vvfat.c | 50 ++
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c3c..263274d9739a 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -100,30 +100,26 @@ static inline void array_free(array_t* array)
 /* does not automatically grow */
 static inline void* array_get(array_t* array,unsigned int index) {
 assert(index < array->next);
+assert(array->pointer);
 return array->pointer + index * array->item_size;
 }
 
-static inline int array_ensure_allocated(array_t* array, int index)
+static inline void array_ensure_allocated(array_t *array, int index)
 {
 if((index + 1) * array->item_size > array->size) {
 int new_size = (index + 32) * array->item_size;
 array->pointer = g_realloc(array->pointer, new_size);
-if (!array->pointer)
-return -1;
+assert(array->pointer);
 memset(array->pointer + array->size, 0, new_size - array->size);
 array->size = new_size;
 array->next = index + 1;
 }
-
-return 0;
 }
 
 static inline void* array_get_next(array_t* array) {
 unsigned int next = array->next;
 
-if (array_ensure_allocated(array, next) < 0)
-return NULL;
-
+array_ensure_allocated(array, next);
 array->next = next + 1;
 return array_get(array, next);
 }
@@ -2428,16 +2424,13 @@ static int commit_direntries(BDRVVVFATState* s,
 direntry_t* direntry = array_get(&(s->directory), dir_index);
 uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
 mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
-
 int factor = 0x10 * s->sectors_per_cluster;
 int old_cluster_count, new_cluster_count;
-int current_dir_index = mapping->info.dir.first_dir_index;
-int first_dir_index = current_dir_index;
+int current_dir_index;
+int first_dir_index;
 int ret, i;
 uint32_t c;
 
-DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", 
mapping->path, parent_mapping_index));
-
 assert(direntry);
 assert(mapping);
 assert(mapping->begin == first_cluster);
@@ -2445,6 +2438,15 @@ DLOG(fprintf(stderr, "commit_direntries for %s, 
parent_mapping_index %d\n", mapp
 assert(mapping->mode & MODE_DIRECTORY);
 assert(dir_index == 0 || is_directory(direntry));
 
+if (mapping == NULL) {
+return -1;
+}
+
+DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n",
+mapping->path, parent_mapping_index));
+
+current_dir_index = mapping->info.dir.first_dir_index;
+first_dir_index = current_dir_index;
 mapping->info.dir.parent_mapping_index = parent_mapping_index;
 
 if (first_cluster == 0) {
@@ -2494,6 +2496,9 @@ DLOG(fprintf(stderr, "commit_direntries for %s, 
parent_mapping_index %d\n", mapp
 direntry = array_get(&(s->directory), first_dir_index + i);
 if (is_directory(direntry) && !is_dot(direntry)) {
 mapping = find_mapping_for_cluster(s, first_cluster);
+if (mapping == NULL) {
+return -1;
+}
 assert(mapping->mode & MODE_DIRECTORY);
 ret = commit_direntries(s, first_dir_index + i,
 array_index(&(s->mapping), mapping));
@@ -2522,6 +2527,10 @@ static int commit_one_file(BDRVVVFATState* s,
 assert(offset < size);
 assert((offset % s->cluster_size) == 0);
 
+if (mapping == NULL) {
+return -1;
+}
+
 for (i = s->cluster_size; i < offset; i += s->cluster_size)
 c = modified_fat_get(s, c);
 
@@ -2668,8 +2677,12 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
 if (commit->action == ACTION_RENAME) {
 mapping_t* mapping = find_mapping_for_cluster(s,
 commit->param.rename.cluster);
-char* old_path = mapping->path;
+char *old_path;
 
+if (mapping == NULL) {
+return -1;
+}
+old_path = mapping->path;
 assert(commit->path);
 mapping->path = commit->path;
 if (rename(old_path, mapping->path))
@@ -2690,10 +2703,15 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
 direntry_t* d = direntry + i;
 
 if (is_file(d) || (is_directory(d) && !is_dot(d))) {
+int l;
+char *new_path;
 mapping_t* m = find_mapping_for_cluster(s,
 begin_of_direntry(d));
-int l = strlen(m->path);
-c

[Qemu-block] [PATCH v5 3/5] qemu-img: assert block_job_get() does not return NULL in img_commit()

[Liam Merwick]

Although the function block_job_get() can return NULL, it would be a
serious bug if it did so (because the job yields before executing anything
(if it started successfully); but otherwise, commit_active_start() would
have returned an error).  However, as a precaution, before dereferencing
the 'job' pointer in img_commit() assert it is not NULL.

Signed-off-by: Liam Merwick 
Reviewed-by: Max Reitz 
---
 qemu-img.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b0a..457aa152296b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1029,6 +1029,7 @@ static int img_commit(int argc, char **argv)
 }
 
 job = block_job_get("commit");
+assert(job);
 run_block_job(job, &local_err);
 if (local_err) {
 goto unref_backing;
-- 
1.8.3.1

[Qemu-block] [PATCH v5 2/5] block: Null pointer dereference in blk_root_get_parent_desc()

[Liam Merwick]

The dev_id returned by the call to blk_get_attached_dev_id() in
blk_root_get_parent_desc() can be NULL (an internal call to
object_get_canonical_path may have returned NULL).

Instead of just checking this case before before dereferencing,
adjust blk_get_attached_dev_id() to return the empty string if no
object path can be found (similar to the case when blk->dev is NULL
and an empty string is returned).

Signed-off-by: Liam Merwick 
---
 block/block-backend.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index dc0cd5772413..a2061a565024 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -918,7 +918,8 @@ char *blk_get_attached_dev_id(BlockBackend *blk)
 } else if (dev->id) {
 return g_strdup(dev->id);
 }
-return object_get_canonical_path(OBJECT(dev));
+
+return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");
 }
 
 /*
-- 
1.8.3.1

[Qemu-block] [PATCH v5 1/5] job: Fix off-by-one assert checks for JobSTT and JobVerbTable

[Liam Merwick]

In the assert checking the array dereference of JobVerbTable[verb]
in job_apply_verb() the check of the index, verb, allows an overrun
because an index equal to the array size is permitted.

Similarly, in the assert check of JobSTT[s0][s1] with index s1
in job_state_transition(), an off-by-one overrun is not flagged
either.

This is not a run-time issue as there are no callers actually
passing in the max value.

Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny 
Reviewed-by: Mark Kanda 
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
---
 job.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/job.c b/job.c
index c65e01bbfa34..da8e4b7bf2f3 100644
--- a/job.c
+++ b/job.c
@@ -159,7 +159,7 @@ bool job_is_internal(Job *job)
 static void job_state_transition(Job *job, JobStatus s1)
 {
 JobStatus s0 = job->status;
-assert(s1 >= 0 && s1 <= JOB_STATUS__MAX);
+assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
 trace_job_state_transition(job, job->ret,
JobSTT[s0][s1] ? "allowed" : "disallowed",
JobStatus_str(s0), JobStatus_str(s1));
@@ -174,7 +174,7 @@ static void job_state_transition(Job *job, JobStatus s1)
 int job_apply_verb(Job *job, JobVerb verb, Error **errp)
 {
 JobStatus s0 = job->status;
-assert(verb >= 0 && verb <= JOB_VERB__MAX);
+assert(verb >= 0 && verb < JOB_VERB__MAX);
 trace_job_apply_verb(job, JobStatus_str(s0), JobVerb_str(verb),
  JobVerbTable[verb][s0] ? "allowed" : "prohibited");
 if (JobVerbTable[verb][s0]) {
-- 
1.8.3.1

Re: [Qemu-block] [PATCH v4 6/8] block: dump_qlist() may dereference a Null pointer

[Liam Merwick]

On 05/11/18 00:07, Max Reitz wrote:

On 19.10.18 22:39, Liam Merwick wrote:

A NULL 'list' passed into function dump_qlist() isn't correctly
validated and can be passed to qlist_first() where it is dereferenced.

Given that dump_qlist() is static, and callers already do the right
thing, just add an assert to catch future potential bugs (plus the
added benefit of suppressing a warning from a static analysis tool
and removing this noise will help us better find real issues).


But can't you fix the tool? 


I don't have access to the tool source but have been filing bugs against 
it as I run it on the QEMU codebase and discover false positives.



My opinion is still that large parts of our
code do not assert that some parameter is not NULL, and I think it isn't
a good idea to make them assert that.  


Yeah, that can be a slippery slope


I don't know what makes this
function special, and I wonder why it is special to your tool -- as I've
said in the last version, dump_qdict() is basically the same in this
regard.  I wonder why your tool doesn't mind that.



I had gone though the code paths to try to see how the tool was happy 
with one and not the other - the implementation differed slightly w.r.t 
macro usage but I couldn't see any obvious reason.



Can you not whitelist something as false positives?  I know we have a
lot of those in Coverity, and we just mark them as such, and that's it.


Yeah, I can flag this as a FP and have it fall off my list.

I'll will drop this patch in v5

Regards,
Liam



Finally, one could argue that the nonnull GCC function attribute would
be a better fit, actually.

But overall, I just don't think it's a good idea to start changing the
code to accommodate for false positives in static analyzers, because in
my experience the number of false positives only rises with time.

Max


Signed-off-by: Liam Merwick 
Reviewed-by: Eric Blake 
---
  block/qapi.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/block/qapi.c b/block/qapi.c
index c66f949db839..e81be604217c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void 
*f, int indentation,
  const QListEntry *entry;
  int i = 0;
  
+assert(list);

+
  for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
  QType type = qobject_type(entry->value);
  bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);

Re: [Qemu-block] [PATCH v4 5/8] block: Fix potential Null pointer dereferences in vvfat.c

[Liam Merwick]

On 05/11/18 00:19, Max Reitz wrote:

On 19.10.18 22:39, Liam Merwick wrote:

The calls to find_mapping_for_cluster() may return NULL but it
isn't always checked for before dereferencing the value returned.
Additionally, add some asserts to cover cases where NULL can't
be returned but which might not be obvious at first glance.

Signed-off-by: Liam Merwick 
---
  block/vvfat.c | 33 -
  1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c3c..19f6725054a0 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -100,6 +100,7 @@ static inline void array_free(array_t* array)
  /* does not automatically grow */
  static inline void* array_get(array_t* array,unsigned int index) {
  assert(index < array->next);
+assert(array->pointer);
  return array->pointer + index * array->item_size;
  }
  
@@ -108,8 +109,7 @@ static inline int array_ensure_allocated(array_t* array, int index)

  if((index + 1) * array->item_size > array->size) {
  int new_size = (index + 32) * array->item_size;
  array->pointer = g_realloc(array->pointer, new_size);
-if (!array->pointer)
-return -1;
+assert(array->pointer);


It would make sense to make this function not return any value (because
it just always returns 0 now), but I fully understand if you don't want
to mess around with vvfat more than you have to.  (Neither do I.)


It had occurred to me too but wasn't sure if it'd be preferred to roll 
that change in. 3 of the 4 callers ignored the return value already, so 
I bit the bullet and made the change.






  memset(array->pointer + array->size, 0, new_size - array->size);
  array->size = new_size;
  array->next = index + 1;
@@ -2261,6 +2261,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s,
  }
  if (index >= s->mapping.next || mapping->begin > begin) {
  mapping = array_insert(&(s->mapping), index, 1);
+if (mapping == NULL) {
+return NULL;
+}


array_insert() will never return NULL.



Removed.




  mapping->path = NULL;
  adjust_mapping_indices(s, index, +1);
  }
@@ -2428,6 +2431,9 @@ static int commit_direntries(BDRVVVFATState* s,
  direntry_t* direntry = array_get(&(s->directory), dir_index);
  uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
  mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
+if (mapping == NULL) {
+return -1;
+}


This should be moved below the declarations that still follow here.


Done. (It resulted in a bit more code rearranging and I had to fix two 
checkpatch warnings in existing code)




  
  int factor = 0x10 * s->sectors_per_cluster;

  int old_cluster_count, new_cluster_count;


[...]


@@ -3193,6 +3215,7 @@ static int enable_write_target(BlockDriverState *bs, 
Error **errp)
  
  backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,

 &error_abort);
+assert(backing);
  *(void**) backing->opaque = s;


I personally wouldn't use an assert() here because it's clear that the
value is dereferenced immediately, so that is the assertion that it is
non-NULL, but I won't give too much of a fight.

The thing is, I believe we should write code for humans, not machines.
Fixing machines to understand what we produce is possible -- fixing
humans is more difficult.

On top of that, it would be a bug if NULL is returned and it would be
good if a static analyzer could catch that case.  Just fully silencing
it with assert() is not ideal.  Ideal would be if it would know that
setting &error_abort to any value crashes the program, and could thus
infer whether this function will actually ever get to return NULL when
&error_abort has been passed to it.



I'm investigating if the tool's config file syntax can describe that 
error_handle_fatal() exits when particular error_xxx parameters are passed.


I'll drop that assert in any case.

Regards,
Liam



Max

  
  bdrv_set_backing_hd(s->bs, backing, &error_abort);

Re: [Qemu-block] [PATCH v4 3/8] block: Null pointer dereference in blk_root_get_parent_desc()

[Liam Merwick]

On 04/11/18 23:57, Max Reitz wrote:

On 19.10.18 22:39, Liam Merwick wrote:

The dev_id returned by the call to blk_get_attached_dev_id() in
blk_root_get_parent_desc() can be NULL (an internal call to
object_get_canonical_path may have returned NULL).

Instead of just checking this case before before dereferencing,
adjust blk_get_attached_dev_id() to return the empty string if no
object path can be found (similar to the case when blk->dev is NULL
and an empty string is returned).

Signed-off-by: Liam Merwick 
---
  block/block-backend.c | 6 +-
  dtc   | 2 +-
  2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index dc0cd5772413..e628920f3cd8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -909,6 +909,7 @@ void *blk_get_attached_dev(BlockBackend *blk)
  char *blk_get_attached_dev_id(BlockBackend *blk)
  {
  DeviceState *dev;
+char *dev_id;
  
  assert(!blk->legacy_dev);

  dev = blk->dev;
@@ -918,7 +919,10 @@ char *blk_get_attached_dev_id(BlockBackend *blk)
  } else if (dev->id) {
  return g_strdup(dev->id);
  }
-return object_get_canonical_path(OBJECT(dev));
+
+dev_id = object_get_canonical_path(OBJECT(dev));
+
+return dev_id ? dev_id : g_strdup("");
  }
  
  /*


Looks good, but since you'll have to respin anyway because of the hunk
below, you may want to consider

 return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");

instead.  (We have several instances of binary "?:" in the code already,
so it's fine to use it.  Of course you don't have to, though, if you
don't like it.)



I like it. Will make that change in v5



diff --git a/dtc b/dtc
index 88f18909db73..e54388015af1 16
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit 88f18909db731a627456f26d779445f84e449536
+Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42


I don't think this hunk belongs here.



Indeed.

Regards,
Liam

[Qemu-block] [PATCH 0/1 V2] Add vhost-pci-blk driver

[Vitaly Mayatskikh]

V2 changes:
- checkpatch style fixes
- correct size detection of disk image placed on a file system

This driver moves virtio-blk host-side processing to kernel (via new
vhost_blk kernel driver). It accelerates virtual disk performance
close to the bare metal levels, especially for parellel loads.

For example, fio numjobs=16 gets 101k randread IOPS using virtio-blk
and 1202k IOPS using vhost-blk, close to 1480k of raw disk performance.

See the IOPS numbers below.

The kernel part if you want to try:
- vhost_blk: https://lkml.org/lkml/2018/11/2/648
- vhost num-queues scalability fix: https://lkml.org/lkml/2018/11/2/550

# fio num-jobs
# A: bare metal over block
# B: bare metal over file
# C: virtio-blk over block
# D: virtio-blk over file
# E: vhost-blk over block
# F: vhost-blk over file
#
#  A B CDE F

1  171k  151k  148k 151k 187k  175k
2  328k  302k  249k 241k 334k  296k
3  479k  437k  179k 174k 464k  404k
4  622k  568k  143k 183k 580k  492k
5  755k  697k  136k 128k 693k  579k
6  887k  808k  131k 120k 782k  640k
7  1004k 926k  126k 131k 863k  693k
8  1099k 1015k 117k 115k 931k  712k
9  1194k 1119k 115k 111k 991k  711k
10 1278k 1207k 109k 114k 1046k 695k
11 1345k 1280k 110k 108k 1091k 663k
12 1411k 1356k 104k 106k 1142k 629k
13 1466k 1423k 106k 106k 1170k 607k
14 1517k 1486k 103k 106k 1179k 589k
15 1552k 1543k 102k 102k 1191k 571k
16 1480k 1506k 101k 102k 1202k 566k

Vitaly Mayatskikh (1):
  Add vhost-pci-blk driver

 configure |  10 +
 default-configs/virtio.mak|   1 +
 hw/block/Makefile.objs|   1 +
 hw/block/vhost-blk.c  | 429 ++
 hw/virtio/virtio-pci.c|  60 +
 hw/virtio/virtio-pci.h|  19 ++
 include/hw/virtio/vhost-blk.h |  43 
 7 files changed, 563 insertions(+)
 create mode 100644 hw/block/vhost-blk.c
 create mode 100644 include/hw/virtio/vhost-blk.h

-- 
2.17.1

[Qemu-block] [PATCH 1/1 V2] Add vhost-pci-blk driver

[Vitaly Mayatskikh]

This driver uses the kernel-mode acceleration for virtio-blk and
allows to get a near bare metal disk performance inside a VM.

Signed-off-by: Vitaly Mayatskikh 
---
 configure |  10 +
 default-configs/virtio.mak|   1 +
 hw/block/Makefile.objs|   1 +
 hw/block/vhost-blk.c  | 429 ++
 hw/virtio/virtio-pci.c|  60 +
 hw/virtio/virtio-pci.h|  19 ++
 include/hw/virtio/vhost-blk.h |  43 
 7 files changed, 563 insertions(+)
 create mode 100644 hw/block/vhost-blk.c
 create mode 100644 include/hw/virtio/vhost-blk.h

diff --git a/configure b/configure
index 46ae1e8c76..787bc780da 100755
--- a/configure
+++ b/configure
@@ -371,6 +371,7 @@ vhost_crypto="no"
 vhost_scsi="no"
 vhost_vsock="no"
 vhost_user=""
+vhost_blk=""
 kvm="no"
 hax="no"
 hvf="no"
@@ -869,6 +870,7 @@ Linux)
   vhost_crypto="yes"
   vhost_scsi="yes"
   vhost_vsock="yes"
+  vhost_blk="yes"
   QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers 
$QEMU_INCLUDES"
   supported_os="yes"
   libudev="yes"
@@ -1263,6 +1265,10 @@ for opt do
   ;;
   --enable-vhost-vsock) vhost_vsock="yes"
   ;;
+  --disable-vhost-blk) vhost_blk="no"
+  ;;
+  --enable-vhost-blk) vhost_blk="yes"
+  ;;
   --disable-opengl) opengl="no"
   ;;
   --enable-opengl) opengl="yes"
@@ -6000,6 +6006,7 @@ echo "vhost-crypto support $vhost_crypto"
 echo "vhost-scsi support $vhost_scsi"
 echo "vhost-vsock support $vhost_vsock"
 echo "vhost-user support $vhost_user"
+echo "vhost-blk support $vhost_blk"
 echo "Trace backends$trace_backends"
 if have_backend "simple"; then
 echo "Trace output file $trace_file-"
@@ -6461,6 +6468,9 @@ fi
 if test "$vhost_user" = "yes" ; then
   echo "CONFIG_VHOST_USER=y" >> $config_host_mak
 fi
+if test "$vhost_blk" = "yes" ; then
+  echo "CONFIG_VHOST_BLK=y" >> $config_host_mak
+fi
 if test "$blobs" = "yes" ; then
   echo "INSTALL_BLOBS=yes" >> $config_host_mak
 fi
diff --git a/default-configs/virtio.mak b/default-configs/virtio.mak
index 1304849018..765c0a2a04 100644
--- a/default-configs/virtio.mak
+++ b/default-configs/virtio.mak
@@ -1,5 +1,6 @@
 CONFIG_VHOST_USER_SCSI=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX))
 CONFIG_VHOST_USER_BLK=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX))
+CONFIG_VHOST_BLK=$(CONFIG_LINUX)
 CONFIG_VIRTIO=y
 CONFIG_VIRTIO_9P=y
 CONFIG_VIRTIO_BALLOON=y
diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index 53ce5751ae..857ce823fc 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -14,3 +14,4 @@ obj-$(CONFIG_SH4) += tc58128.o
 obj-$(CONFIG_VIRTIO_BLK) += virtio-blk.o
 obj-$(CONFIG_VIRTIO_BLK) += dataplane/
 obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o
+obj-$(CONFIG_VHOST_BLK) += vhost-blk.o
diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
new file mode 100644
index 00..4ca8040ee7
--- /dev/null
+++ b/hw/block/vhost-blk.c
@@ -0,0 +1,429 @@
+/*
+ * vhost-blk host device
+ *
+ * Copyright(C) 2018 IBM Corporation
+ *
+ * Authors:
+ *  Vitaly Mayatskikh 
+ *
+ * Largely based on the "vhost-user-blk.c" implemented by:
+ * Changpeng Liu 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/cutils.h"
+#include "qom/object.h"
+#include "hw/qdev-core.h"
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-blk.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
+#include 
+#include 
+
+static const int feature_bits[] = {
+VIRTIO_BLK_F_SIZE_MAX,
+VIRTIO_BLK_F_SEG_MAX,
+VIRTIO_BLK_F_BLK_SIZE,
+VIRTIO_BLK_F_TOPOLOGY,
+VIRTIO_BLK_F_MQ,
+VIRTIO_BLK_F_RO,
+VIRTIO_BLK_F_FLUSH,
+VIRTIO_BLK_F_CONFIG_WCE,
+VIRTIO_F_VERSION_1,
+VIRTIO_RING_F_INDIRECT_DESC,
+VIRTIO_RING_F_EVENT_IDX,
+VIRTIO_F_NOTIFY_ON_EMPTY,
+VHOST_INVALID_FEATURE_BIT
+};
+
+static void vhost_blk_get_config(VirtIODevice *vdev, uint8_t *config)
+{
+VHostBlk *s = VHOST_BLK(vdev);
+memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));
+}
+
+static void vhost_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
+{
+VHostBlk *s = VHOST_BLK(vdev);
+struct virtio_blk_config *blkcfg = (struct virtio_blk_config *)config;
+int ret;
+
+if (blkcfg->wce == s->blkcfg.wce) {
+return;
+}
+
+ret = vhost_dev_set_config(&s->dev, &blkcfg->wce,
+   offsetof(struct virtio_blk_config, wce),
+   sizeof(blkcfg->wce),
+   VHOST_SET_CONFIG_TYPE_MASTER);
+if (ret) {
+error_report("set device config space failed");
+return;
+}
+
+s->blkcfg.wce = blkcfg->wce;
+}
+
+static int vhost_blk_handle_config_change(struct vhost_dev *dev)
+{
+int ret;
+struct virtio_blk_config blkcfg;
+VHost

Re: [Qemu-block] [PULL 00/36] Block layer patches

[Peter Maydell]

On 5 November 2018 at 16:37, Kevin Wolf  wrote:
> The following changes since commit b2f7a038bb4c4fc5ce6b8486e8513dfd97665e2a:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-softfloat-20181104' 
> into staging (2018-11-05 10:32:49 +)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 1240ac558d348f6c7a5752b1a57c1da58e4efe3e:
>
>   include: Add a comment to explain the origin of sizes' lookup table 
> (2018-11-05 15:29:59 +0100)
>
> 
> Block layer patches:
>
> - auto-read-only option to fix commit job when used with -blockdev
> - Fix help text related qemu-iotests failure (by improving the help text
>   and updating the reference output)
> - quorum: Add missing checks when adding/removing child nodes
> - Don't take address of fields in packed structs
> - vvfat: Fix crash when reporting error about too many files in directory
>
> 

Applied, thanks.

-- PMM

[Qemu-block] [PULL 10/33] vhost-user-blk: start vhost when guest kicks

[Michael S. Tsirkin]

From: Yongji Xie 

Some old guests (before commit 7a11370e5: "virtio_blk: enable VQs early")
kick virtqueue before setting VIRTIO_CONFIG_S_DRIVER_OK. This violates
the virtio spec. But virtio 1.0 transitional devices support this behaviour.
So we should start vhost when guest kicks in this case.

Signed-off-by: Yongji Xie 
Signed-off-by: Chai Wen 
Signed-off-by: Ni Xun 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 hw/block/vhost-user-blk.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index d755223643..1451940845 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -217,7 +217,32 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice 
*vdev,
 
 static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
+VHostUserBlk *s = VHOST_USER_BLK(vdev);
+int i;
 
+if (!(virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
+!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1))) {
+return;
+}
+
+if (s->dev.started) {
+return;
+}
+
+/* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
+ * vhost here instead of waiting for .set_status().
+ */
+vhost_user_blk_start(vdev);
+
+/* Kick right away to begin processing requests already in vring */
+for (i = 0; i < s->dev.nvqs; i++) {
+VirtQueue *kick_vq = virtio_get_queue(vdev, i);
+
+if (!virtio_queue_get_desc_addr(vdev, i)) {
+continue;
+}
+event_notifier_set(virtio_queue_get_host_notifier(kick_vq));
+}
 }
 
 static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
-- 
MST

[Qemu-block] [PULL 05/33] virtio-blk: fix comment for virtio_blk_rw_complete

[Michael S. Tsirkin]

From: Yaowei Bai 

Here should be submit_requests, there is no submit_merged_requests
function.

Signed-off-by: Yaowei Bai 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 hw/block/virtio-blk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 225fe44b7a..83cf5c01f9 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -97,8 +97,8 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
 
 if (req->qiov.nalloc != -1) {
 /* If nalloc is != 1 req->qiov is a local copy of the original
- * external iovec. It was allocated in submit_merged_requests
- * to be able to merge requests. */
+ * external iovec. It was allocated in submit_requests to be
+ * able to merge requests. */
 qemu_iovec_destroy(&req->qiov);
 }
 
-- 
MST

Re: [Qemu-block] [Qemu-devel] How to emulate block I/O timeout on qemu side?

[John Snow]

On 11/03/2018 01:24 PM, Dongli Zhang wrote:
> Hi all,
> 

Hi, please reply below the quoted text when writing to qemu-devel in the
future; my reply is below.

> I tried with the patch at:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg00394.html
> 
> The patch is applied to qemu-3.0.0.
> 
> 
> Below configuration is used to test the feature for guest VM nvme.
> 
> # qemu-system-x86_64 \
> -smp 4 -m 2000M -enable-kvm -vnc :0 -monitor stdio \
> -net nic -net user,hostfwd=tcp::5022-:22 \
> -drive file=virtio-disk.img,format=raw,if=none,id=disk0 \
> -device virtio-blk-pci,drive=disk0,id=disk0-dev,num-queues=2,iothread=io1 \
> -object iothread,id=io1 \
> -device nvme,drive=nvme1,serial=deadbeaf1 \
> -drive file=blkdebug:blkdebug.config:nvme.img,if=none,id=nvme1
> 
> # cat blkdebug.config
> [delay]
> event = "write_aio"
> latency = "99"
> sector = "40960"
> 
> 
> The 'write' latency of sector=40960 is set to a very large value. When the I/O
> is stalled in guest due to that sector=40960 is accessed, I do see below
> messages in guest log:
> 
> [   80.807755] nvme nvme0: I/O 11 QID 2 timeout, aborting
> [   80.808095] nvme nvme0: Abort status: 0x4001
> 
> 
> However, then nothing happens further. nvme I/O hangs in guest. I am not able 
> to
> kill the qemu process with Ctrl+C. Both vnc and qemu user net do not work. I
> need to kill qemu with "kill -9"
> >
> The same result for virtio-scsi and qemu is stuck as well.
> 

OK, sounds like a bug in the delay implementation here, then; or
something I've not considered with the locking/drain specifics. Thanks
for the report.

> 
> About blkdebug, I can only trigger the error by the config file. Is there a 
> way
> to inject error or latency via qemu monior? For instance, I would like to 
> inject
> error not for a specific sector or state, but for the entire disk when I input
> some command via qemu monitor.
> 

I don't recall.

There are some tricks you can play with set-state and rules that only
apply when in a certain state. I don't remember if there are monitor or
QMP commands to set the state explicitly.

I'm looking at docs/devel/blkdebug.txt and don't see anything immediately.

There's maybe a way you can use blockdev-add to create the blkdebug node
and insert it live into the graph when you want it, and live-remove it
when you don't, but I'm not sure of the syntax right away.

(maybe that's not possible?)

--js

> Dongli Zhang
> 
> 
> On 11/03/2018 02:17 AM, John Snow wrote:
>>
>>
>> On 11/02/2018 01:55 PM, Marc Olson wrote:
>>> On 11/2/18 10:49 AM, John Snow wrote:
 On 11/02/2018 04:11 AM, Dongli Zhang wrote:
> Hi,
>
> Is there any way to emulate I/O timeout on qemu side (not fault
> injection in VM
> kernel) without modifying qemu source code?
>
> For instance, I would like to observe/study/debug the I/O timeout
> handling of
> nvme, scsi, virtio-blk (not supported) of VM kernel.
>
> Is there a way to trigger this on purpose on qemu side?
>
> Thank you very much!
>
> Dongli Zhang
>
 I don't think the blkdebug driver supports arbitrary delays right now.
 Maybe we could augment it to do so?

 (I thought someone already had, but maybe it wasn't merged?)

 Aha, here:

 https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg05297.html
 V2: https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg00394.html

 Let's work from there.
>>>
>>> I've got updates to that patch series that fell on the floor due to
>>> other competing things. I'll get some screen time this weekend to work
>>> on them and submit v3.
>>>
>>> /marc
>>>
>>
>> Great! Please CC the usual maintainers, but also include me.
>>
>> In the meantime, Dongli Zhang, why don't you try the v2 patch and see if
>> that helps you out for your use case? Report back if it works for you or
>> not.
>>
>> --js
>>

[Qemu-block] [PULL 13/36] block/vdi: Don't take address of fields in packed structs

[Kevin Wolf]

From: Peter Maydell 

Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.

There are a few places where the in-place swap function is
used on something other than a packed struct field; we convert
those anyway, for consistency.

Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.

There are other places where we take the address of a packed member
in this file for other purposes than passing it to a byteswap
function (all the calls to qemu_uuid_*()); we leave those for now.

Signed-off-by: Peter Maydell 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
---
 block/vdi.c | 64 ++---
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 6555cffb88..0ff1ead736 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -187,22 +187,22 @@ typedef struct {
 
 static void vdi_header_to_cpu(VdiHeader *header)
 {
-le32_to_cpus(&header->signature);
-le32_to_cpus(&header->version);
-le32_to_cpus(&header->header_size);
-le32_to_cpus(&header->image_type);
-le32_to_cpus(&header->image_flags);
-le32_to_cpus(&header->offset_bmap);
-le32_to_cpus(&header->offset_data);
-le32_to_cpus(&header->cylinders);
-le32_to_cpus(&header->heads);
-le32_to_cpus(&header->sectors);
-le32_to_cpus(&header->sector_size);
-le64_to_cpus(&header->disk_size);
-le32_to_cpus(&header->block_size);
-le32_to_cpus(&header->block_extra);
-le32_to_cpus(&header->blocks_in_image);
-le32_to_cpus(&header->blocks_allocated);
+header->signature = le32_to_cpu(header->signature);
+header->version = le32_to_cpu(header->version);
+header->header_size = le32_to_cpu(header->header_size);
+header->image_type = le32_to_cpu(header->image_type);
+header->image_flags = le32_to_cpu(header->image_flags);
+header->offset_bmap = le32_to_cpu(header->offset_bmap);
+header->offset_data = le32_to_cpu(header->offset_data);
+header->cylinders = le32_to_cpu(header->cylinders);
+header->heads = le32_to_cpu(header->heads);
+header->sectors = le32_to_cpu(header->sectors);
+header->sector_size = le32_to_cpu(header->sector_size);
+header->disk_size = le64_to_cpu(header->disk_size);
+header->block_size = le32_to_cpu(header->block_size);
+header->block_extra = le32_to_cpu(header->block_extra);
+header->blocks_in_image = le32_to_cpu(header->blocks_in_image);
+header->blocks_allocated = le32_to_cpu(header->blocks_allocated);
 qemu_uuid_bswap(&header->uuid_image);
 qemu_uuid_bswap(&header->uuid_last_snap);
 qemu_uuid_bswap(&header->uuid_link);
@@ -211,22 +211,22 @@ static void vdi_header_to_cpu(VdiHeader *header)
 
 static void vdi_header_to_le(VdiHeader *header)
 {
-cpu_to_le32s(&header->signature);
-cpu_to_le32s(&header->version);
-cpu_to_le32s(&header->header_size);
-cpu_to_le32s(&header->image_type);
-cpu_to_le32s(&header->image_flags);
-cpu_to_le32s(&header->offset_bmap);
-cpu_to_le32s(&header->offset_data);
-cpu_to_le32s(&header->cylinders);
-cpu_to_le32s(&header->heads);
-cpu_to_le32s(&header->sectors);
-cpu_to_le32s(&header->sector_size);
-cpu_to_le64s(&header->disk_size);
-cpu_to_le32s(&header->block_size);
-cpu_to_le32s(&header->block_extra);
-cpu_to_le32s(&header->blocks_in_image);
-cpu_to_le32s(&header->blocks_allocated);
+header->signature = cpu_to_le32(header->signature);
+header->version = cpu_to_le32(header->version);
+header->header_size = cpu_to_le32(header->header_size);
+header->image_type = cpu_to_le32(header->image_type);
+header->image_flags = cpu_to_le32(header->image_flags);
+header->offset_bmap = cpu_to_le32(header->offset_bmap);
+header->offset_data = cpu_to_le32(header->offset_data);
+header->cylinders = cpu_to_le32(header->cylinders);
+header->heads = cpu_to_le32(header->heads);
+header->sectors = cpu_to_le32(header->sectors);
+header->sector_size = cpu_to_le32(header->sector_size);
+header->disk_size = cpu_to_le64(header->disk_size);
+header->block_size = cpu_to_le32(header->block_size);
+header->block_extra = cpu_to_le32(header->block_extra);
+header->blocks_in_image = cpu_to_le32(header->blocks_in_image);
+header->blocks_allocated = cpu_to_le32(header->blocks_allocated);
 qemu_uuid_bswap(&header->uuid_image);
 qemu_uuid_bswap(&header->uuid_last_snap);
 qemu_uuid_bswap(&header->uuid_link);
-- 
2.19.1

[Qemu-block] [PULL 27/36] iscsi: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, open the volume
read-write if we have the permissions, but instead of erroring out for
read-only volumes, just degrade to read-only.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/iscsi.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 73998c2860..727dee50bf 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1878,9 +1878,11 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 /* Check the write protect flag of the LUN if we want to write */
 if (iscsilun->type == TYPE_DISK && (flags & BDRV_O_RDWR) &&
 iscsilun->write_protected) {
-error_setg(errp, "Cannot open a write protected LUN as read-write");
-ret = -EACCES;
-goto out;
+ret = bdrv_apply_auto_read_only(bs, "LUN is write protected", errp);
+if (ret < 0) {
+goto out;
+}
+flags &= ~BDRV_O_RDWR;
 }
 
 iscsi_readcapacity_sync(iscsilun, &local_err);
-- 
2.19.1

[Qemu-block] [PULL 24/36] file-posix: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, open the file
read-write if we have the permissions, but instead of erroring out for
read-only files, just degrade to read-only.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/file-posix.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2da3a76355..0c1b81ce4b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -527,9 +527,22 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 
 s->fd = -1;
 fd = qemu_open(filename, s->open_flags, 0644);
-if (fd < 0) {
-ret = -errno;
-error_setg_errno(errp, errno, "Could not open '%s'", filename);
+ret = fd < 0 ? -errno : 0;
+
+if (ret == -EACCES || ret == -EROFS) {
+/* Try to degrade to read-only, but if it doesn't work, still use the
+ * normal error message. */
+if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
+bdrv_flags &= ~BDRV_O_RDWR;
+raw_parse_flags(bdrv_flags, &s->open_flags);
+assert(!(s->open_flags & O_CREAT));
+fd = qemu_open(filename, s->open_flags);
+ret = fd < 0 ? -errno : 0;
+}
+}
+
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not open '%s'", filename);
 if (ret == -EROFS) {
 ret = -EACCES;
 }
-- 
2.19.1

[Qemu-block] [PULL 29/36] qemu-iotests: Test auto-read-only with -drive and -blockdev

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/232 | 147 +
 tests/qemu-iotests/232.out |  59 +++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 207 insertions(+)
 create mode 100755 tests/qemu-iotests/232
 create mode 100644 tests/qemu-iotests/232.out

diff --git a/tests/qemu-iotests/232 b/tests/qemu-iotests/232
new file mode 100755
index 00..bc2972d124
--- /dev/null
+++ b/tests/qemu-iotests/232
@@ -0,0 +1,147 @@
+#!/bin/bash
+#
+# Test for auto-read-only
+#
+# Copyright (C) 2018 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.snap
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@"
+(
+if ! test -t 0; then
+while read cmd; do
+echo $cmd
+done
+fi
+echo quit
+) | $QEMU -nographic -monitor stdio -nodefaults "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_hmp |
+_filter_generated_node_ids | _filter_imgfmt
+}
+
+function run_qemu_info_block()
+{
+echo "info block -n" | run_qemu "$@" | grep -e "(file" -e "QEMU_PROG"
+}
+
+size=128M
+
+_make_test_img $size
+
+echo
+echo "=== -drive with read-write image: read-only/auto-read-only combinations 
==="
+echo
+
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=off
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on
+echo
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=off
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off
+echo
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,auto-read-only=off
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none
+
+echo
+echo "=== -drive with read-only image: read-only/auto-read-only combinations 
==="
+echo
+
+chmod a-w $TEST_IMG
+
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=off
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on
+echo
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=off
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off
+echo
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,auto-read-only=off
+run_qemu_info_block -drive 
driver=file,file="$TEST_IMG",if=none,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none
+
+echo
+echo "=== -blockdev with read-write image: read-only/auto-read-only 
combinations ==="
+echo
+
+chmod a+w $TEST_IMG
+
+run_qemu_info_block -blockdev 
driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=off
+run_qemu_info_block -blockdev 
driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=on
+run_qemu_info_block -blockdev 
driver=file,filename="$TEST_IMG",node-name=node0,read-only=on
+echo
+run_qemu_info_block -blockdev 
driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=off
+run_qemu_info_block -blockdev 
driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=on
+run_qemu_info_block -blockdev 
driver=file,filename="$TEST_IMG",node-name=node0,read-only=off
+echo
+run_qemu_info_block -blockdev 
driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=off
+run_qemu_info_block -blockdev 
driver=file,filename="$TEST_IMG",

[Qemu-block] [PULL 33/36] object: Make option help nicer to read

[Kevin Wolf]

From: Max Reitz 

Just like in qemu_opts_print_help(), print the object name as a caption
instead of on every single line, indent all options, add angle brackets
around types, and align the descriptions after 24 characters.

Also, indent every object name in the list of available objects.

Signed-off-by: Max Reitz 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Kevin Wolf 
---
 vl.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index 1fcacc5caa..bed97b92ac 100644
--- a/vl.c
+++ b/vl.c
@@ -2743,7 +2743,7 @@ static bool object_create_initial(const char *type, 
QemuOpts *opts)
 list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
 for (l = list; l != NULL; l = l->next) {
 ObjectClass *oc = OBJECT_CLASS(l->data);
-printf("%s\n", object_class_get_name(oc));
+printf("  %s\n", object_class_get_name(oc));
 }
 g_slist_free(list);
 exit(0);
@@ -2765,14 +2765,21 @@ static bool object_create_initial(const char *type, 
QemuOpts *opts)
 }
 
 str = g_string_new(NULL);
-g_string_append_printf(str, "%s.%s=%s", type,
-   prop->name, prop->type);
+g_string_append_printf(str, "  %s=<%s>", prop->name, prop->type);
 if (prop->description) {
+if (str->len < 24) {
+g_string_append_printf(str, "%*s", 24 - (int)str->len, "");
+}
 g_string_append_printf(str, " - %s", prop->description);
 }
 g_ptr_array_add(array, g_string_free(str, false));
 }
 g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
+if (array->len > 0) {
+printf("%s options:\n", type);
+} else {
+printf("There are no options for %s.\n", type);
+}
 for (i = 0; i < array->len; i++) {
 printf("%s\n", (char *)array->pdata[i]);
 }
-- 
2.19.1

[Qemu-block] [PULL 25/36] curl: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, just degrade to
read-only.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/curl.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index fabb2b4da7..db5d2bd8ef 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -684,10 +684,10 @@ static int curl_open(BlockDriverState *bs, QDict 
*options, int flags,
 const char *protocol_delimiter;
 int ret;
 
-
-if (flags & BDRV_O_RDWR) {
-error_setg(errp, "curl block device does not support writes");
-return -EROFS;
+ret = bdrv_apply_auto_read_only(bs, "curl driver does not support writes",
+errp);
+if (ret < 0) {
+return ret;
 }
 
 if (!libcurl_initialized) {
-- 
2.19.1

[Qemu-block] [PULL 32/36] qdev-monitor: Make device options help nicer

[Kevin Wolf]

From: Max Reitz 

Just like in qemu_opts_print_help(), print the device name as a caption
instead of on every single line, indent all options, add angle brackets
around types, and align the descriptions after 24 characters.  Also,
separate the descriptions with " - " instead of putting them in
parentheses, because that is what we do everywhere else.  This does look
a bit funny here because basically all bits have the description
"on/off", but funny does not mean it is less readable.

Signed-off-by: Max Reitz 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Kevin Wolf 
---
 qdev-monitor.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 802c18a74e..07147c63bf 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -285,10 +285,19 @@ int qdev_device_help(QemuOpts *opts)
 goto error;
 }
 
+if (prop_list) {
+out_printf("%s options:\n", driver);
+} else {
+out_printf("There are no options for %s.\n", driver);
+}
 for (prop = prop_list; prop; prop = prop->next) {
-out_printf("%s.%s=%s", driver, prop->value->name, prop->value->type);
+int len;
+out_printf("  %s=<%s>%n", prop->value->name, prop->value->type, &len);
 if (prop->value->has_description) {
-out_printf(" (%s)\n", prop->value->description);
+if (len < 24) {
+out_printf("%*s", 24 - len, "");
+}
+out_printf(" - %s\n", prop->value->description);
 } else {
 out_printf("\n");
 }
-- 
2.19.1

[Qemu-block] [PULL 26/36] gluster: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, open the file
read-write if we have the permissions, but instead of erroring out for
read-only files, just degrade to read-only.

Signed-off-by: Kevin Wolf 
Reviewed-by: Niels de Vos 
---
 block/gluster.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 4fd55a9cc5..5e300c96c8 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -849,8 +849,16 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict 
*options,
 qemu_gluster_parse_flags(bdrv_flags, &open_flags);
 
 s->fd = glfs_open(s->glfs, gconf->path, open_flags);
-if (!s->fd) {
-ret = -errno;
+ret = s->fd ? 0 : -errno;
+
+if (ret == -EACCES || ret == -EROFS) {
+/* Try to degrade to read-only, but if it doesn't work, still use the
+ * normal error message. */
+if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
+open_flags = (open_flags & ~O_RDWR) | O_RDONLY;
+s->fd = glfs_open(s->glfs, gconf->path, open_flags);
+ret = s->fd ? 0 : -errno;
+}
 }
 
 s->supports_seek_data = qemu_gluster_test_seek(s->fd);
-- 
2.19.1

[Qemu-block] [PULL 36/36] include: Add a comment to explain the origin of sizes' lookup table

[Kevin Wolf]

From: Leonid Bloch 

The lookup table for power-of-two sizes was added in commit 540b8492618eb
for the purpose of having convenient shortcuts for these sizes in cases
when the literal number has to be present at compile time, and
expressions as '(1 * KiB)' can not be used. One such case is the
stringification of sizes. Beyond that, it is convenient to use these
shortcuts for all power-of-two sizes, even if they don't have to be
literal numbers.

Despite its convenience, this table introduced 55 lines of "dumb" code,
the purpose and origin of which are obscure without reading the message
of the commit which introduced it. This patch fixes that by adding a
comment to the code itself with a brief explanation for the reasoning
behind this table. This comment includes the short AWK script that
generated the table, so that anyone who's interested could make sure
that the values in it are correct (otherwise these values look as if
they were typed manually).

Signed-off-by: Leonid Bloch 
Signed-off-by: Kevin Wolf 
---
 include/qemu/units.h | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/include/qemu/units.h b/include/qemu/units.h
index 68a7758650..1c959d182e 100644
--- a/include/qemu/units.h
+++ b/include/qemu/units.h
@@ -17,6 +17,24 @@
 #define PiB (INT64_C(1) << 50)
 #define EiB (INT64_C(1) << 60)
 
+/*
+ * The following lookup table is intended to be used when a literal string of
+ * the number of bytes is required (for example if it needs to be stringified).
+ * It can also be used for generic shortcuts of power-of-two sizes.
+ * This table is generated using the AWK script below:
+ *
+ *  BEGIN {
+ *  suffix="KMGTPE";
+ *  for(i=10; i<64; i++) {
+ *  val=2**i;
+ *  s=substr(suffix, int(i/10), 1);
+ *  n=2**(i%10);
+ *  pad=21-int(log(n)/log(10));
+ *  printf("#define S_%d%siB %*d\n", n, s, pad, val);
+ *  }
+ *  }
+ */
+
 #define S_1KiB  1024
 #define S_2KiB  2048
 #define S_4KiB  4096
-- 
2.19.1

[Qemu-block] [PULL 11/36] vpc: Don't leak opts in vpc_open()

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
Reviewed-by: Alberto Garcia 
---
 block/vpc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/vpc.c b/block/vpc.c
index 984187cadd..80c5b2b197 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -456,10 +456,12 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 
 qemu_co_mutex_init(&s->lock);
+qemu_opts_del(opts);
 
 return 0;
 
 fail:
+qemu_opts_del(opts);
 qemu_vfree(s->pagetable);
 #ifdef CACHE
 g_free(s->pageentry_u8);
-- 
2.19.1

[Qemu-block] [PULL 21/36] rbd: Close image in qemu_rbd_open() error path

[Kevin Wolf]

Commit e2b8247a322 introduced an error path in qemu_rbd_open() after
calling rbd_open(), but neglected to close the image again in this error
path. The error path should contain everything that the regular close
function qemu_rbd_close() contains.

This adds the missing rbd_close() call.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/rbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/rbd.c b/block/rbd.c
index e5bf5a146f..1e9819a50f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -787,6 +787,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
  "automatically marking the image read-only.");
 r = bdrv_set_read_only(bs, true, &local_err);
 if (r < 0) {
+rbd_close(s->image);
 error_propagate(errp, local_err);
 goto failed_open;
 }
-- 
2.19.1

[Qemu-block] [PULL 19/36] block: Update flags in bdrv_set_read_only()

[Kevin Wolf]

To fully change the read-only state of a node, we must not only change
bs->read_only, but also update bs->open_flags.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
 block.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index 9d2adf7962..3132c78f01 100644
--- a/block.c
+++ b/block.c
@@ -281,6 +281,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool 
read_only, Error **errp)
 }
 
 bs->read_only = read_only;
+
+if (read_only) {
+bs->open_flags &= ~BDRV_O_RDWR;
+} else {
+bs->open_flags |= BDRV_O_RDWR;
+}
+
 return 0;
 }
 
-- 
2.19.1

[Qemu-block] [PULL 31/36] chardev: Indent list of chardevs

[Kevin Wolf]

From: Max Reitz 

Following the example of qemu_opts_print_help(), indent all entries in
the list of character devices.

Signed-off-by: Max Reitz 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Kevin Wolf 
---
 chardev/char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char.c b/chardev/char.c
index 79b05fb7b7..152dde5327 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -572,7 +572,7 @@ help_string_append(const char *name, void *opaque)
 {
 GString *str = opaque;
 
-g_string_append_printf(str, "\n%s", name);
+g_string_append_printf(str, "\n  %s", name);
 }
 
 static const char *chardev_alias_translate(const char *name)
-- 
2.19.1

[Qemu-block] [PULL 34/36] fw_cfg: Drop newline in @file description

[Kevin Wolf]

From: Max Reitz 

There is no good reason why there should be a newline in this
description, so remove it.

Signed-off-by: Max Reitz 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Kevin Wolf 
---
 vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index bed97b92ac..03ed215d7b 100644
--- a/vl.c
+++ b/vl.c
@@ -537,7 +537,7 @@ static QemuOptsList qemu_fw_cfg_opts = {
 }, {
 .name = "file",
 .type = QEMU_OPT_STRING,
-.help = "Sets the name of the file from which\n"
+.help = "Sets the name of the file from which "
 "the fw_cfg blob will be loaded",
 }, {
 .name = "string",
-- 
2.19.1

[Qemu-block] [PULL 20/36] block: Add auto-read-only option

[Kevin Wolf]

If a management application builds the block graph node by node, the
protocol layer doesn't inherit its read-only option from the format
layer any more, so it must be set explicitly.

Backing files should work on read-only storage, but at the same time, a
block job like commit should be able to reopen them read-write if they
are on read-write storage. However, without option inheritance, reopen
only changes the read-only option for the root node (typically the
format layer), but not the protocol layer, so reopening fails (the
format layer wants to get write permissions, but the protocol layer is
still read-only).

A simple workaround for the problem in the management tool would be to
open the protocol layer always read-write and to make only the format
layer read-only for backing files. However, sometimes the file is
actually stored on read-only storage and we don't know whether the image
can be opened read-write (for example, for NBD it depends on the server
we're trying to connect to). This adds an option that makes QEMU try to
open the image read-write, but allows it to degrade to a read-only mode
without returning an error.

The documentation for this option is consciously phrased in a way that
allows QEMU to switch to a better model eventually: Instead of trying
when the image is first opened, making the read-only flag dynamic and
changing it automatically whenever the first BLK_PERM_WRITE user is
attached or the last one is detached would be much more useful
behaviour.

Unfortunately, this more useful behaviour is also a lot harder to
implement, and libvirt needs a solution now before it can switch to
-blockdev, so let's start with this easier approach for now.

Instead of adding a new auto-read-only option, turning the existing
read-only into an enum (with a bool alternate for compatibility) was
considered, but it complicated the implementation to the point that it
didn't seem to be worth it.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 qapi/block-core.json  |  7 +++
 include/block/block.h |  2 ++
 block.c   | 17 +
 block/vvfat.c |  1 +
 blockdev.c|  2 +-
 5 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0fc1590c1b..d4fe710836 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3656,6 +3656,12 @@
 # either generally or in certain configurations. In this case,
 # the default value does not work and the option must be
 # specified explicitly.
+# @auto-read-only: if true and @read-only is false, QEMU may automatically
+#  decide not to open the image read-write as requested, but
+#  fall back to read-only instead (and switch between the modes
+#  later), e.g. depending on whether the image file is writable
+#  or whether a writing user is attached to the node
+#  (default: false, since 3.1)
 # @detect-zeroes: detect and optimize zero writes (Since 2.1)
 # (default: off)
 # @force-share:   force share all permission on added nodes.
@@ -3671,6 +3677,7 @@
 '*discard': 'BlockdevDiscardOptions',
 '*cache': 'BlockdevCacheOptions',
 '*read-only': 'bool',
+'*auto-read-only': 'bool',
 '*force-share': 'bool',
 '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
   'discriminator': 'driver',
diff --git a/include/block/block.h b/include/block/block.h
index b189cf422e..580b3716c3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -115,6 +115,7 @@ typedef struct HDGeometry {
   select an appropriate protocol driver,
   ignoring the format layer */
 #define BDRV_O_NO_IO   0x1 /* don't initialize for I/O */
+#define BDRV_O_AUTO_RDONLY 0x2 /* degrade to read-only if opening 
read-write fails */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
@@ -125,6 +126,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_DIRECT   "cache.direct"
 #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"
 #define BDRV_OPT_READ_ONLY  "read-only"
+#define BDRV_OPT_AUTO_READ_ONLY "auto-read-only"
 #define BDRV_OPT_DISCARD"discard"
 #define BDRV_OPT_FORCE_SHARE"force-share"
 
diff --git a/block.c b/block.c
index 3132c78f01..96090c3b9a 100644
--- a/block.c
+++ b/block.c
@@ -930,6 +930,7 @@ static void bdrv_inherited_options(int *child_flags, QDict 
*child_options,
 
 /* Inherit the read-only option from the parent if it's not set */
 qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
+qdict_copy_default(child_options, parent_options, BDRV_OPT_AUTO_READ_ONLY);
 
 /* Our block drivers take care to send flushes and respect unmap policy,
  * so we can default to enable both on lower layers regardless of the
@@ -1053,6 +1054,7 

[Qemu-block] [PULL 35/36] vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE

[Kevin Wolf]

From: Leonid Bloch 

If an expression is used to define DEFAULT_CLUSTER_SIZE, when compiled,
it will be embedded as a literal expression in the binary (as the
default value) because it is stringified to mark the size of the default
value. Now this is fixed by using a defined number to define this value.

Signed-off-by: Leonid Bloch 
Reviewed-by: Stefan Weil 
Signed-off-by: Kevin Wolf 
---
 block/vdi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 0ff1ead736..2380daa583 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -85,7 +85,7 @@
 #define BLOCK_OPT_STATIC "static"
 
 #define SECTOR_SIZE 512
-#define DEFAULT_CLUSTER_SIZE (1 * MiB)
+#define DEFAULT_CLUSTER_SIZE S_1MiB
 
 #if defined(CONFIG_VDI_DEBUG)
 #define VDI_DEBUG 1
@@ -432,7 +432,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, 
int flags,
 goto fail;
 } else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
 error_setg(errp, "unsupported VDI image (block size %" PRIu32
- " is not %" PRIu64 ")",
+ " is not %" PRIu32 ")",
header.block_size, DEFAULT_CLUSTER_SIZE);
 ret = -ENOTSUP;
 goto fail;
-- 
2.19.1

[Qemu-block] [PULL 28/36] block: Make auto-read-only=on default for -drive

[Kevin Wolf]

While we want machine interfaces like -blockdev and QMP blockdev-add to
add as little auto-detection as possible so that management tools are
explicit about their needs, -drive is a convenience option for human
users. Enabling auto-read-only=on by default there enables users to use
read-only images for read-only guest devices without having to specify
read-only=on explicitly. If they try to attach the image to a read-write
device, they will still get an error message.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 blockdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockdev.c b/blockdev.c
index fc8794862f..e5b5eb46e2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -590,6 +590,7 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
 qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
 qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY,
   read_only ? "on" : "off");
+qdict_set_default_str(bs_opts, BDRV_OPT_AUTO_READ_ONLY, "on");
 assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0);
 
 if (runstate_check(RUN_STATE_INMIGRATE)) {
-- 
2.19.1

[Qemu-block] [PULL 14/36] quorum: Remove quorum_err()

[Kevin Wolf]

From: Alberto Garcia 

This is a static function with only one caller, so there's no need to
keep it. Inlining the code in quorum_compare() makes it much simpler.

Signed-off-by: Alberto Garcia 
Reported-by: Markus Armbruster 
Signed-off-by: Kevin Wolf 
---
 block/quorum.c | 24 +---
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index eb526cc0f1..b1b777baef 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -437,23 +437,7 @@ static bool quorum_iovec_compare(QEMUIOVector *a, 
QEMUIOVector *b)
 return true;
 }
 
-static void GCC_FMT_ATTR(2, 3) quorum_err(QuorumAIOCB *acb,
-  const char *fmt, ...)
-{
-va_list ap;
-
-va_start(ap, fmt);
-fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64 " ",
-acb->offset, acb->bytes);
-vfprintf(stderr, fmt, ap);
-fprintf(stderr, "\n");
-va_end(ap);
-exit(1);
-}
-
-static bool quorum_compare(QuorumAIOCB *acb,
-   QEMUIOVector *a,
-   QEMUIOVector *b)
+static bool quorum_compare(QuorumAIOCB *acb, QEMUIOVector *a, QEMUIOVector *b)
 {
 BDRVQuorumState *s = acb->bs->opaque;
 ssize_t offset;
@@ -462,8 +446,10 @@ static bool quorum_compare(QuorumAIOCB *acb,
 if (s->is_blkverify) {
 offset = qemu_iovec_compare(a, b);
 if (offset != -1) {
-quorum_err(acb, "contents mismatch at offset %" PRIu64,
-   acb->offset + offset);
+fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64
+" contents mismatch at offset %" PRIu64 "\n",
+acb->offset, acb->bytes, acb->offset + offset);
+exit(1);
 }
 return true;
 }
-- 
2.19.1

[Qemu-block] [PULL 23/36] nbd: Support auto-read-only option

[Kevin Wolf]

If read-only=off, but auto-read-only=on is given, open a read-write NBD
connection if the server provides a read-write export, but instead of
erroring out for read-only exports, just degrade to read-only.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/nbd-client.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 9686ecbd5e..76e9ca3abe 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -992,11 +992,11 @@ int nbd_client_init(BlockDriverState *bs,
 logout("Failed to negotiate with the NBD server\n");
 return ret;
 }
-if (client->info.flags & NBD_FLAG_READ_ONLY &&
-!bdrv_is_read_only(bs)) {
-error_setg(errp,
-   "request for write access conflicts with read-only export");
-return -EACCES;
+if (client->info.flags & NBD_FLAG_READ_ONLY) {
+ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
+if (ret < 0) {
+return ret;
+}
 }
 if (client->info.flags & NBD_FLAG_SEND_FUA) {
 bs->supported_write_flags = BDRV_REQ_FUA;
-- 
2.19.1

[Qemu-block] [PULL 16/36] iotest: Test the blkverify mode of the Quorum driver

[Kevin Wolf]

From: Alberto Garcia 

Signed-off-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/081 | 30 ++
 tests/qemu-iotests/081.out | 16 
 2 files changed, 46 insertions(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index da3fb0984b..0ea010afbf 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -168,6 +168,36 @@ echo "== checking that quorum is broken =="
 
 $QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
 
+echo
+echo "== checking the blkverify mode with broken content =="
+
+quorum="driver=raw,file.driver=quorum,file.vote-threshold=2,file.blkverify=on"
+quorum="$quorum,file.children.0.file.filename=$TEST_DIR/1.raw"
+quorum="$quorum,file.children.1.file.filename=$TEST_DIR/2.raw"
+quorum="$quorum,file.children.0.driver=raw"
+quorum="$quorum,file.children.1.driver=raw"
+
+$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
+
+echo
+echo "== writing the same data to both files =="
+
+$QEMU_IO -c "write -P 0x32 0 $size" "$TEST_DIR/1.raw" | _filter_qemu_io
+$QEMU_IO -c "write -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io
+
+echo
+echo "== checking the blkverify mode with valid content =="
+
+$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
+
+echo
+echo "== checking the blkverify mode with invalid settings =="
+
+quorum="$quorum,file.children.2.file.filename=$TEST_DIR/3.raw"
+quorum="$quorum,file.children.2.driver=raw"
+
+$QEMU_IO -c "open -o $quorum" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out
index 2533c31c78..2f12c890e9 100644
--- a/tests/qemu-iotests/081.out
+++ b/tests/qemu-iotests/081.out
@@ -55,4 +55,20 @@ wrote 10485760/10485760 bytes at offset 0
 
 == checking that quorum is broken ==
 read failed: Input/output error
+
+== checking the blkverify mode with broken content ==
+quorum: offset=0 bytes=10485760 contents mismatch at offset 0
+
+== writing the same data to both files ==
+wrote 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== checking the blkverify mode with valid content ==
+read 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== checking the blkverify mode with invalid settings ==
+can't open: blkverify=on can only be set if there are exactly two files and 
vote-threshold is 2
 *** done
-- 
2.19.1

[Qemu-block] [PULL 15/36] quorum: Return an error if the blkverify mode has invalid settings

[Kevin Wolf]

From: Alberto Garcia 

The blkverify mode of Quorum can only be enabled if the number of
children is exactly two and the value of vote-threshold is also two.

If the user tries to enable it but the other settings are incorrect
then QEMU simply prints an error message to stderr and carries on
disabling the blkverify setting.

This patch makes quorum_open() fail and return an error in this case.

Signed-off-by: Alberto Garcia 
Reported-by: Markus Armbruster 
Signed-off-by: Kevin Wolf 
---
 block/quorum.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index b1b777baef..6188ff 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -912,13 +912,12 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->read_pattern = ret;
 
 if (s->read_pattern == QUORUM_READ_PATTERN_QUORUM) {
-/* is the driver in blkverify mode */
-if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false) &&
-s->num_children == 2 && s->threshold == 2) {
-s->is_blkverify = true;
-} else if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false)) {
-fprintf(stderr, "blkverify mode is set by setting blkverify=on "
-"and using two files with vote_threshold=2\n");
+s->is_blkverify = qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false);
+if (s->is_blkverify && (s->num_children != 2 || s->threshold != 2)) {
+error_setg(&local_err, "blkverify=on can only be set if there are "
+   "exactly two files and vote-threshold is 2");
+ret = -EINVAL;
+goto exit;
 }
 
 s->rewrite_corrupted = qemu_opt_get_bool(opts, QUORUM_OPT_REWRITE,
-- 
2.19.1

[Qemu-block] [PULL 22/36] block: Require auto-read-only for existing fallbacks

[Kevin Wolf]

Some block drivers have traditionally changed their node to read-only
mode without asking the user. This behaviour has been marked deprecated
since 2.11, expecting users to provide an explicit read-only=on option.

Now that we have auto-read-only=on, enable these drivers to make use of
the option.

This is the only use of bdrv_set_read_only(), so we can make it a bit
more specific and turn it into a bdrv_apply_auto_read_only() that is
more convenient for drivers to use.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 include/block/block.h |  3 ++-
 block.c   | 42 +++---
 block/bochs.c | 17 ++---
 block/cloop.c | 16 +---
 block/dmg.c   | 16 +---
 block/rbd.c   | 15 ---
 block/vvfat.c | 10 ++
 7 files changed, 51 insertions(+), 68 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 580b3716c3..7f5453b45b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -438,7 +438,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, 
BlockDriverState *base,
 bool bdrv_is_read_only(BlockDriverState *bs);
 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);
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+  Error **errp);
 bool bdrv_is_writable(BlockDriverState *bs);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index 96090c3b9a..fd67e14dfa 100644
--- a/block.c
+++ b/block.c
@@ -266,29 +266,41 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool 
read_only,
 return 0;
 }
 
-/* TODO Remove (deprecated since 2.11)
- * Block drivers are not supposed to automatically change bs->read_only.
- * Instead, they should just check whether they can provide what the user
- * explicitly requested and error out if read-write is requested, but they can
- * only provide read-only access. */
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+/*
+ * Called by a driver that can only provide a read-only image.
+ *
+ * Returns 0 if the node is already read-only or it could switch the node to
+ * read-only because BDRV_O_AUTO_RDONLY is set.
+ *
+ * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not set
+ * or bdrv_can_set_read_only() forbids making the node read-only. If @errmsg
+ * is not NULL, it is used as the error message for the Error object.
+ */
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+  Error **errp)
 {
 int ret = 0;
 
-ret = bdrv_can_set_read_only(bs, read_only, false, errp);
-if (ret < 0) {
-return ret;
+if (!(bs->open_flags & BDRV_O_RDWR)) {
+return 0;
+}
+if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
+goto fail;
 }
 
-bs->read_only = read_only;
-
-if (read_only) {
-bs->open_flags &= ~BDRV_O_RDWR;
-} else {
-bs->open_flags |= BDRV_O_RDWR;
+ret = bdrv_can_set_read_only(bs, true, false, NULL);
+if (ret < 0) {
+goto fail;
 }
 
+bs->read_only = true;
+bs->open_flags &= ~BDRV_O_RDWR;
+
 return 0;
+
+fail:
+error_setg(errp, "%s", errmsg ?: "Image is read-only");
+return -EACCES;
 }
 
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
diff --git a/block/bochs.c b/block/bochs.c
index 50c630047b..22e7d44211 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -105,23 +105,18 @@ static int bochs_open(BlockDriverState *bs, QDict 
*options, int flags,
 struct bochs_header bochs;
 int ret;
 
+/* No write support yet */
+ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+if (ret < 0) {
+return ret;
+}
+
 bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
false, errp);
 if (!bs->file) {
 return -EINVAL;
 }
 
-if (!bdrv_is_read_only(bs)) {
-error_report("Opening bochs images without an explicit read-only=on "
- "option is deprecated. Future versions will refuse to "
- "open the image instead of automatically marking the "
- "image read-only.");
-ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
-if (ret < 0) {
-return ret;
-}
-}
-
 ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
 if (ret < 0) {
 return ret;
diff --git a/block/cloop.c b/block/cloop.c
index 2be68987bd..df2b85f723 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -67,23 +67,17 @@ static int cloop_open(BlockDriverState *bs, QDict *options, 
int flags,
 uint32_t offsets_size, max_compressed_block_size = 1, i;
 int re

[Qemu-block] [PULL 12/36] block/vhdx: Don't take address of fields in packed structs

[Kevin Wolf]

From: Peter Maydell 

Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.

There are a few places where the in-place swap function is
used on something other than a packed struct field; we convert
those anyway, for consistency.

Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.

Signed-off-by: Peter Maydell 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
---
 block/vhdx.h|  12 ++---
 block/vhdx-endian.c | 118 ++--
 block/vhdx-log.c|   4 +-
 block/vhdx.c|  18 +++
 4 files changed, 76 insertions(+), 76 deletions(-)

diff --git a/block/vhdx.h b/block/vhdx.h
index 7003ab7a79..3a5f5293ad 100644
--- a/block/vhdx.h
+++ b/block/vhdx.h
@@ -420,16 +420,16 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, 
BDRVVHDXState *s,
 
 static inline void leguid_to_cpus(MSGUID *guid)
 {
-le32_to_cpus(&guid->data1);
-le16_to_cpus(&guid->data2);
-le16_to_cpus(&guid->data3);
+guid->data1 = le32_to_cpu(guid->data1);
+guid->data2 = le16_to_cpu(guid->data2);
+guid->data3 = le16_to_cpu(guid->data3);
 }
 
 static inline void cpu_to_leguids(MSGUID *guid)
 {
-cpu_to_le32s(&guid->data1);
-cpu_to_le16s(&guid->data2);
-cpu_to_le16s(&guid->data3);
+guid->data1 = cpu_to_le32(guid->data1);
+guid->data2 = cpu_to_le16(guid->data2);
+guid->data3 = cpu_to_le16(guid->data3);
 }
 
 void vhdx_header_le_import(VHDXHeader *h);
diff --git a/block/vhdx-endian.c b/block/vhdx-endian.c
index 41fbdd2b8f..ebfa33cb8a 100644
--- a/block/vhdx-endian.c
+++ b/block/vhdx-endian.c
@@ -35,18 +35,18 @@ void vhdx_header_le_import(VHDXHeader *h)
 {
 assert(h != NULL);
 
-le32_to_cpus(&h->signature);
-le32_to_cpus(&h->checksum);
-le64_to_cpus(&h->sequence_number);
+h->signature = le32_to_cpu(h->signature);
+h->checksum = le32_to_cpu(h->checksum);
+h->sequence_number = le64_to_cpu(h->sequence_number);
 
 leguid_to_cpus(&h->file_write_guid);
 leguid_to_cpus(&h->data_write_guid);
 leguid_to_cpus(&h->log_guid);
 
-le16_to_cpus(&h->log_version);
-le16_to_cpus(&h->version);
-le32_to_cpus(&h->log_length);
-le64_to_cpus(&h->log_offset);
+h->log_version = le16_to_cpu(h->log_version);
+h->version = le16_to_cpu(h->version);
+h->log_length = le32_to_cpu(h->log_length);
+h->log_offset = le64_to_cpu(h->log_offset);
 }
 
 void vhdx_header_le_export(VHDXHeader *orig_h, VHDXHeader *new_h)
@@ -80,68 +80,68 @@ void vhdx_log_desc_le_import(VHDXLogDescriptor *d)
 {
 assert(d != NULL);
 
-le32_to_cpus(&d->signature);
-le64_to_cpus(&d->file_offset);
-le64_to_cpus(&d->sequence_number);
+d->signature = le32_to_cpu(d->signature);
+d->file_offset = le64_to_cpu(d->file_offset);
+d->sequence_number = le64_to_cpu(d->sequence_number);
 }
 
 void vhdx_log_desc_le_export(VHDXLogDescriptor *d)
 {
 assert(d != NULL);
 
-cpu_to_le32s(&d->signature);
-cpu_to_le32s(&d->trailing_bytes);
-cpu_to_le64s(&d->leading_bytes);
-cpu_to_le64s(&d->file_offset);
-cpu_to_le64s(&d->sequence_number);
+d->signature = cpu_to_le32(d->signature);
+d->trailing_bytes = cpu_to_le32(d->trailing_bytes);
+d->leading_bytes = cpu_to_le64(d->leading_bytes);
+d->file_offset = cpu_to_le64(d->file_offset);
+d->sequence_number = cpu_to_le64(d->sequence_number);
 }
 
 void vhdx_log_data_le_import(VHDXLogDataSector *d)
 {
 assert(d != NULL);
 
-le32_to_cpus(&d->data_signature);
-le32_to_cpus(&d->sequence_high);
-le32_to_cpus(&d->sequence_low);
+d->data_signature = le32_to_cpu(d->data_signature);
+d->sequence_high = le32_to_cpu(d->sequence_high);
+d->sequence_low = le32_to_cpu(d->sequence_low);
 }
 
 void vhdx_log_data_le_export(VHDXLogDataSector *d)
 {
 assert(d != NULL);
 
-cpu_to_le32s(&d->data_signature);
-cpu_to_le32s(&d->sequence_high);
-cpu_to_le32s(&d->sequence_low);
+d->data_signature = cpu_to_le32(d->data_signature);
+d->sequence_high = cpu_to_le32(d->sequence_high);
+d->sequence_low = cpu_to_le32(d->sequence_low);
 }
 
 void vhdx_log_entry_hdr_le_import(VHDXLogEntryHeader *hdr)
 {
 assert(hdr != NULL);
 
-le32_to_cpus(&hdr->signature);
-le32_to_cpus(&hdr->checksum);
-le32_to_cpus(&hdr->entry_length);
-le32_to_cpus(&hdr->tail);
-le64_to_cpus(&hdr->sequence_number);
-le32_to_cpus(&hdr->descriptor_count);
+hdr->signature = le32_to_cpu(hdr->signature);
+hdr->checksum = le32_to_cpu(hdr->checksum);
+hdr->entry_length = le32_to_cpu(hdr->entry_length);
+hdr->tail = le32_to_cpu(hdr->tail);
+hdr->sequence_number = le64_to_cpu(hdr->sequence_number);
+hdr->descriptor_count = le32_to_cpu(hdr->descri

[Qemu-block] [PULL 18/36] iotest: Test x-blockdev-change on a Quorum

[Kevin Wolf]

From: Alberto Garcia 

This patch tests that you can add and remove drives from a Quorum
using the x-blockdev-change command.

Signed-off-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/081 | 86 ++
 tests/qemu-iotests/081.out | 54 
 2 files changed, 140 insertions(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index 0ea010afbf..9f1dece271 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -198,6 +198,92 @@ quorum="$quorum,file.children.2.driver=raw"
 
 $QEMU_IO -c "open -o $quorum" | _filter_qemu_io
 
+echo
+echo "== dynamically adding a child to a quorum =="
+
+for verify in false true; do
+run_qemu <

[Qemu-block] [PULL 17/36] quorum: Forbid adding children in blkverify mode

[Kevin Wolf]

From: Alberto Garcia 

The blkverify mode of Quorum only works when the number of children is
exactly two, so any attempt to add a new one must return an error.

quorum_del_child() on the other hand doesn't need any additional check
because decreasing the number of children would make it go under the
vote threshold.

Signed-off-by: Alberto Garcia 
Reported-by: Kevin Wolf 
Signed-off-by: Kevin Wolf 
---
 block/quorum.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/block/quorum.c b/block/quorum.c
index 6188ff..16b3c8067c 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -992,6 +992,11 @@ static void quorum_add_child(BlockDriverState *bs, 
BlockDriverState *child_bs,
 char indexstr[32];
 int ret;
 
+if (s->is_blkverify) {
+error_setg(errp, "Cannot add a child to a quorum in blkverify mode");
+return;
+}
+
 assert(s->num_children <= INT_MAX / sizeof(BdrvChild *));
 if (s->num_children == INT_MAX / sizeof(BdrvChild *) ||
 s->next_child_index == UINT_MAX) {
@@ -1046,6 +1051,9 @@ static void quorum_del_child(BlockDriverState *bs, 
BdrvChild *child,
 return;
 }
 
+/* We know now that num_children > threshold, so blkverify must be false */
+assert(!s->is_blkverify);
+
 bdrv_drained_begin(bs);
 
 /* We can safely remove this child now */
-- 
2.19.1

[Qemu-block] [PULL 30/36] option: Make option help nicer to read

[Kevin Wolf]

From: Max Reitz 

This adds some whitespace into the option help (including indentation)
and puts angle brackets around the type names.  Furthermore, the list
name is no longer printed as part of every line, but only once in
advance, and only if the caller did not print a caption already.

This patch also restores the description alignment we had before commit
9cbef9d68ee1d8d0, just at 24 instead of 16 characters like we used to.
This increase is because now we have the type and two spaces of
indentation before the description, and with a usual type name length of
three chracters, this sums up to eight additional characters -- which
means that we now need 24 characters to get the same amount of padding
for most options.  Also, 24 is a third of 80, which makes it kind of a
round number in terminal terms.

Finally, this patch amends the reference output of iotest 082 to match
the changes (and thus makes it pass again).

Signed-off-by: Max Reitz 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Kevin Wolf 
---
 include/qemu/option.h  |   2 +-
 qemu-img.c |   4 +-
 util/qemu-option.c |  32 +-
 tests/qemu-iotests/082.out | 956 ++---
 4 files changed, 507 insertions(+), 487 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 3dfb4493cc..844587cab3 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -132,7 +132,7 @@ typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts 
*opts, Error **errp);
 int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
   void *opaque, Error **errp);
 void qemu_opts_print(QemuOpts *opts, const char *sep);
-void qemu_opts_print_help(QemuOptsList *list);
+void qemu_opts_print_help(QemuOptsList *list, bool print_caption);
 void qemu_opts_free(QemuOptsList *list);
 QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
 
diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b..4c96db7ba4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -269,7 +269,7 @@ static int print_block_option_help(const char *filename, 
const char *fmt)
 }
 
 printf("Supported options:\n");
-qemu_opts_print_help(create_opts);
+qemu_opts_print_help(create_opts, false);
 qemu_opts_free(create_opts);
 return 0;
 }
@@ -3773,7 +3773,7 @@ static int print_amend_option_help(const char *format)
 assert(drv->create_opts);
 
 printf("Creation options for '%s':\n", format);
-qemu_opts_print_help(drv->create_opts);
+qemu_opts_print_help(drv->create_opts, false);
 printf("\nNote that not all of these options may be amendable.\n");
 return 0;
 }
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 9a5f263294..de42e2a406 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -224,7 +224,14 @@ static const char *opt_type_to_string(enum QemuOptType 
type)
 g_assert_not_reached();
 }
 
-void qemu_opts_print_help(QemuOptsList *list)
+/**
+ * Print the list of options available in the given list.  If
+ * @print_caption is true, a caption (including the list name, if it
+ * exists) is printed.  The options itself will be indented, so
+ * @print_caption should only be set to false if the caller prints its
+ * own custom caption (so that the indentation makes sense).
+ */
+void qemu_opts_print_help(QemuOptsList *list, bool print_caption)
 {
 QemuOptDesc *desc;
 int i;
@@ -234,12 +241,12 @@ void qemu_opts_print_help(QemuOptsList *list)
 desc = list->desc;
 while (desc && desc->name) {
 GString *str = g_string_new(NULL);
-if (list->name) {
-g_string_append_printf(str, "%s.", list->name);
-}
-g_string_append_printf(str, "%s=%s", desc->name,
+g_string_append_printf(str, "  %s=<%s>", desc->name,
opt_type_to_string(desc->type));
 if (desc->help) {
+if (str->len < 24) {
+g_string_append_printf(str, "%*s", 24 - (int)str->len, "");
+}
 g_string_append_printf(str, " - %s", desc->help);
 }
 g_ptr_array_add(array, g_string_free(str, false));
@@ -247,6 +254,19 @@ void qemu_opts_print_help(QemuOptsList *list)
 }
 
 g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
+if (print_caption && array->len > 0) {
+if (list->name) {
+printf("%s options:\n", list->name);
+} else {
+printf("Options:\n");
+}
+} else if (array->len == 0) {
+if (list->name) {
+printf("There are no options for %s.\n", list->name);
+} else {
+printf("No options available.\n");
+}
+}
 for (i = 0; i < array->len; i++) {
 printf("%s\n", (char *)array->pdata[i]);
 }
@@ -930,7 +950,7 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const 
char *params,
 opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
 if (err) {
 if (invalidp && has_help

[Qemu-block] [PULL 10/36] iotests: make 083 specific to raw

[Kevin Wolf]

From: Cleber Rosa 

While testing the Python 3 changes which touch the 083 test, I noticed
that it would fail with qcow2.  Expanding the testing, I noticed it
had nothing to do with the Python 3 changes, and in fact, it would not
pass on anything but raw:

 raw: pass
 bochs: not generic
 cloop: not generic
 parallels: fail
 qcow: fail
 qcow2: fail
 qed: fail
 vdi: fail
 vhdx: fail
 vmdk: fail
 vpc: fail
 luks: fail

The errors are a mixture I/O and "image not in xxx format", such as:

  === Check disconnect before data ===

  Unexpected end-of-file before all bytes were read
 -read failed: Input/output error
 +can't open device nbd+tcp://127.0.0.1:PORT/foo: Could not open 
'nbd://127.0.0.1:PORT/foo': Input/output error

  === Check disconnect after data ===

 -read 512/512 bytes at offset 0
 -512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +can't open device nbd+tcp://127.0.0.1:PORT/foo: Image not in qcow format

I'm not aware if there's a quick fix, so, for the time being, it looks
like the honest approach is to make the test known to work on raw
only.

Signed-off-by: Cleber Rosa 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/083 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 3c1adbf0fb..9f92317b0a 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-_supported_fmt generic
+_supported_fmt raw
 _supported_proto nbd
 _supported_os Linux
 
-- 
2.19.1

[Qemu-block] [PULL 07/36] crypto: initialize sector size even when opening with no IO flag

[Kevin Wolf]

From: Daniel P. Berrangé 

The qcow2 block driver expects to see a valid sector size even when it
has opened the crypto layer with QCRYPTO_BLOCK_OPEN_NO_IO.

Signed-off-by: Daniel P. Berrangé 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 crypto/block-qcow.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 4284e05167..7606231e79 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -102,6 +102,8 @@ qcrypto_block_qcow_open(QCryptoBlock *block,
 Error **errp)
 {
 if (flags & QCRYPTO_BLOCK_OPEN_NO_IO) {
+block->sector_size = QCRYPTO_BLOCK_QCOW_SECTOR_SIZE;
+block->payload_offset = 0;
 return 0;
 } else {
 if (!options->u.qcow.key_secret) {
-- 
2.19.1

[Qemu-block] [PULL 09/36] block: change some function return type to bool

[Kevin Wolf]

From: Li Qiang 

Signed-off-by: Li Qiang 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 include/sysemu/block-backend.h | 6 +++---
 block/block-backend.c  | 8 
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 830d873f24..c96bcdee14 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -166,9 +166,9 @@ BlockErrorAction blk_get_error_action(BlockBackend *blk, 
bool is_read,
   int error);
 void blk_error_action(BlockBackend *blk, BlockErrorAction action,
   bool is_read, int error);
-int blk_is_read_only(BlockBackend *blk);
-int blk_is_sg(BlockBackend *blk);
-int blk_enable_write_cache(BlockBackend *blk);
+bool blk_is_read_only(BlockBackend *blk);
+bool blk_is_sg(BlockBackend *blk);
+bool blk_enable_write_cache(BlockBackend *blk);
 void blk_set_enable_write_cache(BlockBackend *blk, bool wce);
 void blk_invalidate_cache(BlockBackend *blk, Error **errp);
 bool blk_is_inserted(BlockBackend *blk);
diff --git a/block/block-backend.c b/block/block-backend.c
index dc0cd57724..2a8f3b55f8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1708,7 +1708,7 @@ void blk_error_action(BlockBackend *blk, BlockErrorAction 
action,
 }
 }
 
-int blk_is_read_only(BlockBackend *blk)
+bool blk_is_read_only(BlockBackend *blk)
 {
 BlockDriverState *bs = blk_bs(blk);
 
@@ -1719,18 +1719,18 @@ int blk_is_read_only(BlockBackend *blk)
 }
 }
 
-int blk_is_sg(BlockBackend *blk)
+bool blk_is_sg(BlockBackend *blk)
 {
 BlockDriverState *bs = blk_bs(blk);
 
 if (!bs) {
-return 0;
+return false;
 }
 
 return bdrv_is_sg(bs);
 }
 
-int blk_enable_write_cache(BlockBackend *blk)
+bool blk_enable_write_cache(BlockBackend *blk)
 {
 return blk->enable_write_cache;
 }
-- 
2.19.1

[Qemu-block] [PULL 08/36] qcow2: Get the request alignment for encrypted images from QCryptoBlock

[Kevin Wolf]

From: Alberto Garcia 

This doesn't have any practical effect at the moment because the
values of BDRV_SECTOR_SIZE, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE and
QCRYPTO_BLOCK_QCOW_SECTOR_SIZE are all the same (512 bytes), but
future encryption methods could have different requirements.

Signed-off-by: Alberto Garcia 
Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Kevin Wolf 
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index de94b290e6..991d6ac91b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1677,7 +1677,7 @@ static void qcow2_refresh_limits(BlockDriverState *bs, 
Error **errp)
 
 if (bs->encrypted) {
 /* Encryption works on a sector granularity */
-bs->bl.request_alignment = BDRV_SECTOR_SIZE;
+bs->bl.request_alignment = qcrypto_block_get_sector_size(s->crypto);
 }
 bs->bl.pwrite_zeroes_alignment = s->cluster_size;
 bs->bl.pdiscard_alignment = s->cluster_size;
-- 
2.19.1

[Qemu-block] [PULL 05/36] block/qcow: Don't take address of fields in packed structs

[Kevin Wolf]

From: Peter Maydell 

Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.

There are a few places where the in-place swap function is
used on something other than a packed struct field; we convert
those anyway, for consistency.

This patch was produced with the following spatch script:

@@
expression E;
@@
-be16_to_cpus(&E);
+E = be16_to_cpu(E);
@@
expression E;
@@
-be32_to_cpus(&E);
+E = be32_to_cpu(E);
@@
expression E;
@@
-be64_to_cpus(&E);
+E = be64_to_cpu(E);
@@
expression E;
@@
-cpu_to_be16s(&E);
+E = cpu_to_be16(E);
@@
expression E;
@@
-cpu_to_be32s(&E);
+E = cpu_to_be32(E);
@@
expression E;
@@
-cpu_to_be64s(&E);
+E = cpu_to_be64(E);

Signed-off-by: Peter Maydell 
Reviewed-by: Richard Henderson 
Tested-by: John Snow 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 block/qcow.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 385d935258..4518cb4c35 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -140,14 +140,14 @@ static int qcow_open(BlockDriverState *bs, QDict 
*options, int flags,
 if (ret < 0) {
 goto fail;
 }
-be32_to_cpus(&header.magic);
-be32_to_cpus(&header.version);
-be64_to_cpus(&header.backing_file_offset);
-be32_to_cpus(&header.backing_file_size);
-be32_to_cpus(&header.mtime);
-be64_to_cpus(&header.size);
-be32_to_cpus(&header.crypt_method);
-be64_to_cpus(&header.l1_table_offset);
+header.magic = be32_to_cpu(header.magic);
+header.version = be32_to_cpu(header.version);
+header.backing_file_offset = be64_to_cpu(header.backing_file_offset);
+header.backing_file_size = be32_to_cpu(header.backing_file_size);
+header.mtime = be32_to_cpu(header.mtime);
+header.size = be64_to_cpu(header.size);
+header.crypt_method = be32_to_cpu(header.crypt_method);
+header.l1_table_offset = be64_to_cpu(header.l1_table_offset);
 
 if (header.magic != QCOW_MAGIC) {
 error_setg(errp, "Image not in qcow format");
@@ -270,7 +270,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 
 for(i = 0;i < s->l1_size; i++) {
-be64_to_cpus(&s->l1_table[i]);
+s->l1_table[i] = be64_to_cpu(s->l1_table[i]);
 }
 
 /* alloc L2 cache (max. 64k * 16 * 8 = 8 MB) */
-- 
2.19.1

[Qemu-block] [PULL 04/36] block/qcow2: Don't take address of fields in packed structs

[Kevin Wolf]

From: Peter Maydell 

Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.

There are a few places where the in-place swap function is
used on something other than a packed struct field; we convert
those anyway, for consistency.

This patch was produced with the following spatch script
(and hand-editing to fold a few resulting overlength lines):

@@
expression E;
@@
-be16_to_cpus(&E);
+E = be16_to_cpu(E);
@@
expression E;
@@
-be32_to_cpus(&E);
+E = be32_to_cpu(E);
@@
expression E;
@@
-be64_to_cpus(&E);
+E = be64_to_cpu(E);
@@
expression E;
@@
-cpu_to_be16s(&E);
+E = cpu_to_be16(E);
@@
expression E;
@@
-cpu_to_be32s(&E);
+E = cpu_to_be32(E);
@@
expression E;
@@
-cpu_to_be64s(&E);
+E = cpu_to_be64(E);

Signed-off-by: Peter Maydell 
Reviewed-by: Richard Henderson 
Tested-by: John Snow 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 block/qcow2.c | 64 +++
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 30689b7688..de94b290e6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -210,8 +210,8 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
  "pread fail from offset %" PRIu64, offset);
 return 1;
 }
-be32_to_cpus(&ext.magic);
-be32_to_cpus(&ext.len);
+ext.magic = be32_to_cpu(ext.magic);
+ext.len = be32_to_cpu(ext.len);
 offset += sizeof(ext);
 #ifdef DEBUG_EXT
 printf("ext.magic = 0x%x\n", ext.magic);
@@ -279,8 +279,8 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
  "Unable to read CRYPTO header extension");
 return ret;
 }
-be64_to_cpus(&s->crypto_header.offset);
-be64_to_cpus(&s->crypto_header.length);
+s->crypto_header.offset = be64_to_cpu(s->crypto_header.offset);
+s->crypto_header.length = be64_to_cpu(s->crypto_header.length);
 
 if ((s->crypto_header.offset % s->cluster_size) != 0) {
 error_setg(errp, "Encryption header offset '%" PRIu64 "' is "
@@ -342,9 +342,11 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
 return -EINVAL;
 }
 
-be32_to_cpus(&bitmaps_ext.nb_bitmaps);
-be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
-be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
+bitmaps_ext.nb_bitmaps = be32_to_cpu(bitmaps_ext.nb_bitmaps);
+bitmaps_ext.bitmap_directory_size =
+be64_to_cpu(bitmaps_ext.bitmap_directory_size);
+bitmaps_ext.bitmap_directory_offset =
+be64_to_cpu(bitmaps_ext.bitmap_directory_offset);
 
 if (bitmaps_ext.nb_bitmaps > QCOW2_MAX_BITMAPS) {
 error_setg(errp,
@@ -1159,19 +1161,20 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 error_setg_errno(errp, -ret, "Could not read qcow2 header");
 goto fail;
 }
-be32_to_cpus(&header.magic);
-be32_to_cpus(&header.version);
-be64_to_cpus(&header.backing_file_offset);
-be32_to_cpus(&header.backing_file_size);
-be64_to_cpus(&header.size);
-be32_to_cpus(&header.cluster_bits);
-be32_to_cpus(&header.crypt_method);
-be64_to_cpus(&header.l1_table_offset);
-be32_to_cpus(&header.l1_size);
-be64_to_cpus(&header.refcount_table_offset);
-be32_to_cpus(&header.refcount_table_clusters);
-be64_to_cpus(&header.snapshots_offset);
-be32_to_cpus(&header.nb_snapshots);
+header.magic = be32_to_cpu(header.magic);
+header.version = be32_to_cpu(header.version);
+header.backing_file_offset = be64_to_cpu(header.backing_file_offset);
+header.backing_file_size = be32_to_cpu(header.backing_file_size);
+header.size = be64_to_cpu(header.size);
+header.cluster_bits = be32_to_cpu(header.cluster_bits);
+header.crypt_method = be32_to_cpu(header.crypt_method);
+header.l1_table_offset = be64_to_cpu(header.l1_table_offset);
+header.l1_size = be32_to_cpu(header.l1_size);
+header.refcount_table_offset = be64_to_cpu(header.refcount_table_offset);
+header.refcount_table_clusters =
+be32_to_cpu(header.refcount_table_clusters);
+header.snapshots_offset = be64_to_cpu(header.snapshots_offset);
+header.nb_snapshots = be32_to_cpu(header.nb_snapshots);
 
 if (header.magic != QCOW_MAGIC) {
 error_setg(errp, "Image is not in qcow2 format");
@@ -1207,11 +1210,12 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 header.refcount_order  

[Qemu-block] [PULL 01/36] block/vvfat: Fix crash when reporting error about too many files in directory

[Kevin Wolf]

From: Thomas Huth 

When using the vvfat driver with a directory that contains too many files,
QEMU currently crashes. This can be triggered like this for example:

 mkdir /tmp/vvfattest
 cd /tmp/vvfattest
 for ((x=0;x<=513;x++)); do mkdir $x; done
 qemu-system-x86_64 -drive \
   file.driver=vvfat,file.dir=.,read-only=on,media=cdrom

Seems like read_directory() is changing the mapping->path variable. Make
sure we use the right pointer instead.

Signed-off-by: Thomas Huth 
Signed-off-by: Kevin Wolf 
---
 block/vvfat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c..f2e7d501cf 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -973,10 +973,10 @@ static int init_directories(BDRVVVFATState* s,
 mapping = array_get(&(s->mapping), i);
 
 if (mapping->mode & MODE_DIRECTORY) {
+char *path = mapping->path;
 mapping->begin = cluster;
 if(read_directory(s, i)) {
-error_setg(errp, "Could not read directory %s",
-   mapping->path);
+error_setg(errp, "Could not read directory %s", path);
 return -1;
 }
 mapping = array_get(&(s->mapping), i);
-- 
2.19.1

[Qemu-block] [PULL 00/36] Block layer patches

[Kevin Wolf]

The following changes since commit b2f7a038bb4c4fc5ce6b8486e8513dfd97665e2a:

  Merge remote-tracking branch 'remotes/rth/tags/pull-softfloat-20181104' into 
staging (2018-11-05 10:32:49 +)

are available in the Git repository at:

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

for you to fetch changes up to 1240ac558d348f6c7a5752b1a57c1da58e4efe3e:

  include: Add a comment to explain the origin of sizes' lookup table 
(2018-11-05 15:29:59 +0100)


Block layer patches:

- auto-read-only option to fix commit job when used with -blockdev
- Fix help text related qemu-iotests failure (by improving the help text
  and updating the reference output)
- quorum: Add missing checks when adding/removing child nodes
- Don't take address of fields in packed structs
- vvfat: Fix crash when reporting error about too many files in directory


Alberto Garcia (7):
  block: replace "discard" literal with BDRV_OPT_DISCARD macro
  qcow2: Get the request alignment for encrypted images from QCryptoBlock
  quorum: Remove quorum_err()
  quorum: Return an error if the blkverify mode has invalid settings
  iotest: Test the blkverify mode of the Quorum driver
  quorum: Forbid adding children in blkverify mode
  iotest: Test x-blockdev-change on a Quorum

Cleber Rosa (1):
  iotests: make 083 specific to raw

Daniel P. Berrangé (1):
  crypto: initialize sector size even when opening with no IO flag

Kevin Wolf (12):
  vpc: Don't leak opts in vpc_open()
  block: Update flags in bdrv_set_read_only()
  block: Add auto-read-only option
  rbd: Close image in qemu_rbd_open() error path
  block: Require auto-read-only for existing fallbacks
  nbd: Support auto-read-only option
  file-posix: Support auto-read-only option
  curl: Support auto-read-only option
  gluster: Support auto-read-only option
  iscsi: Support auto-read-only option
  block: Make auto-read-only=on default for -drive
  qemu-iotests: Test auto-read-only with -drive and -blockdev

Leonid Bloch (2):
  vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE
  include: Add a comment to explain the origin of sizes' lookup table

Li Qiang (1):
  block: change some function return type to bool

Max Reitz (5):
  option: Make option help nicer to read
  chardev: Indent list of chardevs
  qdev-monitor: Make device options help nicer
  object: Make option help nicer to read
  fw_cfg: Drop newline in @file description

Peter Maydell (5):
  block/qcow2: Don't take address of fields in packed structs
  block/qcow: Don't take address of fields in packed structs
  block/qcow2-bitmap: Don't take address of fields in packed structs
  block/vhdx: Don't take address of fields in packed structs
  block/vdi: Don't take address of fields in packed structs

Stefan Weil (1):
  qemu-io-cmds: Fix two format strings

Thomas Huth (1):
  block/vvfat: Fix crash when reporting error about too many files in 
directory

 qapi/block-core.json   |   7 +
 block/vhdx.h   |  12 +-
 include/block/block.h  |   5 +-
 include/qemu/option.h  |   2 +-
 include/qemu/units.h   |  18 +
 include/sysemu/block-backend.h |   6 +-
 block.c|  60 ++-
 block/block-backend.c  |   8 +-
 block/bochs.c  |  17 +-
 block/cloop.c  |  16 +-
 block/curl.c   |   8 +-
 block/dmg.c|  16 +-
 block/file-posix.c |  19 +-
 block/gluster.c|  12 +-
 block/iscsi.c  |   8 +-
 block/nbd-client.c |  10 +-
 block/qcow.c   |  18 +-
 block/qcow2-bitmap.c   |  24 +-
 block/qcow2.c  |  66 +--
 block/quorum.c |  45 +-
 block/rbd.c|  14 +-
 block/vdi.c|  68 +--
 block/vhdx-endian.c| 118 ++---
 block/vhdx-log.c   |   4 +-
 block/vhdx.c   |  18 +-
 block/vpc.c|   2 +
 block/vvfat.c  |  15 +-
 blockdev.c |   3 +-
 chardev/char.c |   2 +-
 crypto/block-qcow.c|   2 +
 qdev-monitor.c |  13 +-
 qemu-img.c |   4 +-
 qemu-io-cmds.c |   4 +-
 util/qemu-option.c |  32 +-
 vl.c   |  15 +-
 tests/qemu-iotests/081 | 116 +
 tests/qemu-iotests/081.out |  70 +++
 tests/qemu-iotests/082.out | 956 -
 tests/qemu-iotests/083 |   2 +-
 tests/qemu-iotests/232 | 147 +++
 tests/qemu-iotests/232.out |  59 +++
 tests/qemu-iotests/group   |   1 +
 42 files changed, 1266 insertions(+), 776 deletions(-)
 create mode 1

[Qemu-block] [PULL 06/36] block/qcow2-bitmap: Don't take address of fields in packed structs

[Kevin Wolf]

From: Peter Maydell 

Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.

There are a few places where the in-place swap function is
used on something other than a packed struct field; we convert
those anyway, for consistency.

This patch was produced with the following spatch script:

@@
expression E;
@@
-be16_to_cpus(&E);
+E = be16_to_cpu(E);
@@
expression E;
@@
-be32_to_cpus(&E);
+E = be32_to_cpu(E);
@@
expression E;
@@
-be64_to_cpus(&E);
+E = be64_to_cpu(E);
@@
expression E;
@@
-cpu_to_be16s(&E);
+E = cpu_to_be16(E);
@@
expression E;
@@
-cpu_to_be32s(&E);
+E = cpu_to_be32(E);
@@
expression E;
@@
-cpu_to_be64s(&E);
+E = cpu_to_be64(E);

Signed-off-by: Peter Maydell 
Reviewed-by: Richard Henderson 
Tested-by: John Snow 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 block/qcow2-bitmap.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b5f1b3563d..accebef4cf 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -118,7 +118,7 @@ static inline void bitmap_table_to_be(uint64_t 
*bitmap_table, size_t size)
 size_t i;
 
 for (i = 0; i < size; ++i) {
-cpu_to_be64s(&bitmap_table[i]);
+bitmap_table[i] = cpu_to_be64(bitmap_table[i]);
 }
 }
 
@@ -231,7 +231,7 @@ static int bitmap_table_load(BlockDriverState *bs, 
Qcow2BitmapTable *tb,
 }
 
 for (i = 0; i < tb->size; ++i) {
-be64_to_cpus(&table[i]);
+table[i] = be64_to_cpu(table[i]);
 ret = check_table_entry(table[i], s->cluster_size);
 if (ret < 0) {
 goto fail;
@@ -394,20 +394,20 @@ fail:
 
 static inline void bitmap_dir_entry_to_cpu(Qcow2BitmapDirEntry *entry)
 {
-be64_to_cpus(&entry->bitmap_table_offset);
-be32_to_cpus(&entry->bitmap_table_size);
-be32_to_cpus(&entry->flags);
-be16_to_cpus(&entry->name_size);
-be32_to_cpus(&entry->extra_data_size);
+entry->bitmap_table_offset = be64_to_cpu(entry->bitmap_table_offset);
+entry->bitmap_table_size = be32_to_cpu(entry->bitmap_table_size);
+entry->flags = be32_to_cpu(entry->flags);
+entry->name_size = be16_to_cpu(entry->name_size);
+entry->extra_data_size = be32_to_cpu(entry->extra_data_size);
 }
 
 static inline void bitmap_dir_entry_to_be(Qcow2BitmapDirEntry *entry)
 {
-cpu_to_be64s(&entry->bitmap_table_offset);
-cpu_to_be32s(&entry->bitmap_table_size);
-cpu_to_be32s(&entry->flags);
-cpu_to_be16s(&entry->name_size);
-cpu_to_be32s(&entry->extra_data_size);
+entry->bitmap_table_offset = cpu_to_be64(entry->bitmap_table_offset);
+entry->bitmap_table_size = cpu_to_be32(entry->bitmap_table_size);
+entry->flags = cpu_to_be32(entry->flags);
+entry->name_size = cpu_to_be16(entry->name_size);
+entry->extra_data_size = cpu_to_be32(entry->extra_data_size);
 }
 
 static inline int calc_dir_entry_size(size_t name_size, size_t extra_data_size)
-- 
2.19.1

[Qemu-block] [PULL 03/36] qemu-io-cmds: Fix two format strings

[Kevin Wolf]

From: Stefan Weil 

Use %zu instead of %zd for unsigned numbers.

This fixes two error messages from the LSTM static code analyzer:

This argument should be of type 'ssize_t' but is of type 'unsigned long'

Signed-off-by: Stefan Weil 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Kevin Wolf 
---
 qemu-io-cmds.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index db0b3ee5ef..5363482213 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -907,7 +907,7 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
 memset(cmp_buf, pattern, qiov.size);
 if (memcmp(buf, cmp_buf, qiov.size)) {
 printf("Pattern verification failed at offset %"
-   PRId64 ", %zd bytes\n", offset, qiov.size);
+   PRId64 ", %zu bytes\n", offset, qiov.size);
 ret = -EINVAL;
 }
 g_free(cmp_buf);
@@ -1294,7 +1294,7 @@ static void aio_read_done(void *opaque, int ret)
 memset(cmp_buf, ctx->pattern, ctx->qiov.size);
 if (memcmp(ctx->buf, cmp_buf, ctx->qiov.size)) {
 printf("Pattern verification failed at offset %"
-   PRId64 ", %zd bytes\n", ctx->offset, ctx->qiov.size);
+   PRId64 ", %zu bytes\n", ctx->offset, ctx->qiov.size);
 }
 g_free(cmp_buf);
 }
-- 
2.19.1

[Qemu-block] [PULL 02/36] block: replace "discard" literal with BDRV_OPT_DISCARD macro

[Kevin Wolf]

From: Alberto Garcia 

Signed-off-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 95d8635aa1..9d2adf7962 100644
--- a/block.c
+++ b/block.c
@@ -1327,7 +1327,7 @@ QemuOptsList bdrv_runtime_opts = {
 .help = "try to optimize zero writes (off, on, unmap)",
 },
 {
-.name = "discard",
+.name = BDRV_OPT_DISCARD,
 .type = QEMU_OPT_STRING,
 .help = "discard operation (ignore/off, unmap/on)",
 },
@@ -1432,7 +1432,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockBackend *file,
 }
 }
 
-discard = qemu_opt_get(opts, "discard");
+discard = qemu_opt_get(opts, BDRV_OPT_DISCARD);
 if (discard != NULL) {
 if (bdrv_parse_discard_flags(discard, &bs->open_flags) != 0) {
 error_setg(errp, "Invalid discard option");
@@ -3186,7 +3186,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 
 update_flags_from_options(&reopen_state->flags, opts);
 
-discard = qemu_opt_get_del(opts, "discard");
+discard = qemu_opt_get_del(opts, BDRV_OPT_DISCARD);
 if (discard != NULL) {
 if (bdrv_parse_discard_flags(discard, &reopen_state->flags) != 0) {
 error_setg(errp, "Invalid discard option");
-- 
2.19.1

Re: [Qemu-block] [Qemu-devel] xen_disk qdevification

[Paul Durrant]

> -Original Message-
> From: Markus Armbruster [mailto:arm...@redhat.com]
> Sent: 05 November 2018 15:58
> To: Paul Durrant 
> Cc: 'Kevin Wolf' ; Tim Smith ;
> Stefano Stabellini ; qemu-block@nongnu.org; qemu-
> de...@nongnu.org; Max Reitz ; Anthony Perard
> ; xen-de...@lists.xenproject.org
> Subject: Re: [Qemu-devel] xen_disk qdevification
> 
> Paul Durrant  writes:
> 
> >> -Original Message-
> >> From: Kevin Wolf [mailto:kw...@redhat.com]
> >> Sent: 02 November 2018 11:04
> >> To: Tim Smith 
> >> Cc: xen-de...@lists.xenproject.org; qemu-de...@nongnu.org; qemu-
> >> bl...@nongnu.org; Anthony Perard ; Paul
> Durrant
> >> ; Stefano Stabellini ;
> >> Max Reitz ; arm...@redhat.com
> >> Subject: xen_disk qdevification (was: [PATCH 0/3] Performance
> improvements
> >> for xen_disk v2)
> >>
> >> Am 02.11.2018 um 11:00 hat Tim Smith geschrieben:
> >> > A series of performance improvements for disks using the Xen PV ring.
> >> >
> >> > These have had fairly extensive testing.
> >> >
> >> > The batching and latency improvements together boost the throughput
> >> > of small reads and writes by two to six percent (measured using fio
> >> > in the guest)
> >> >
> >> > Avoiding repeated calls to posix_memalign() reduced the dirty heap
> >> > from 25MB to 5MB in the case of a single datapath process while also
> >> > improving performance.
> >> >
> >> > v2 removes some checkpatch complaints and fixes the CCs
> >>
> >> Completely unrelated, but since you're the first person touching
> >> xen_disk in a while, you're my victim:
> >>
> >> At KVM Forum we discussed sending a patch to deprecate xen_disk because
> >> after all those years, it still hasn't been converted to qdev. Markus
> is
> >> currently fixing some other not yet qdevified block device, but after
> >> that xen_disk will be the only one left.
> >>
> >> A while ago, a downstream patch review found out that there are some
> QMP
> >> commands that would immediately crash if a xen_disk device were present
> >> because of the lacking qdevification. This is not the code quality
> >> standard I envision for QEMU. It's time for non-qdev devices to go.
> >>
> >> So if you guys are still interested in the device, could someone please
> >> finally look into converting it?
> >>
> >
> > I have a patch series to do exactly this. It's somewhat involved as I
> > need to convert the whole PV backend infrastructure. I will try to
> > rebase and clean up my series a.s.a.p.
> 
> Awesome!  Please coordinate with Anthony Prerard to avoid duplicating
> work if you haven't done so already.

Sure. I have already spoken to Anthony.

  Thanks,

   Paul

Re: [Qemu-block] [PATCH 1/2] The discard flag for block stream operation

[Alberto Garcia]

On Wed 31 Oct 2018 05:47:19 PM CET, Andrey Shinkevich 
 wrote:
> Adding a parameter to QMP block-stream command to allow discarding
> blocks in the backing chain while blocks are being copied to the
> active layer.
>
> Signed-off-by: Andrey Shinkevich 

I haven't checked the other patch yet, but if you're going to add new
API (this new 'discard' parameter) I suggest you to do it _after_ the
implementation.

> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2329,6 +2329,9 @@
>  #
>  # @speed:  the maximum speed, in bytes per second
>  #
> +# @discard: true to delete blocks duplicated in old backing files.
> +#   (default: false). Since 3.1.
> +#
>  # @on-error: the action to take on an error (default report).
>  #'stop' and 'enospc' can only be used if the block device
>  #supports io-status (see BlockInfo).  Since 1.3.
> @@ -2361,7 +2364,7 @@
>  { 'command': 'block-stream',
>'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
>  '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
> -'*on-error': 'BlockdevOnError',
> +'*discard': 'bool', '*on-error': 'BlockdevOnError',
>  '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }

Berto

Re: [Qemu-block] [Qemu-devel] xen_disk qdevification

[Markus Armbruster]

Paul Durrant  writes:

>> -Original Message-
>> From: Kevin Wolf [mailto:kw...@redhat.com]
>> Sent: 02 November 2018 11:04
>> To: Tim Smith 
>> Cc: xen-de...@lists.xenproject.org; qemu-de...@nongnu.org; qemu-
>> bl...@nongnu.org; Anthony Perard ; Paul Durrant
>> ; Stefano Stabellini ;
>> Max Reitz ; arm...@redhat.com
>> Subject: xen_disk qdevification (was: [PATCH 0/3] Performance improvements
>> for xen_disk v2)
>> 
>> Am 02.11.2018 um 11:00 hat Tim Smith geschrieben:
>> > A series of performance improvements for disks using the Xen PV ring.
>> >
>> > These have had fairly extensive testing.
>> >
>> > The batching and latency improvements together boost the throughput
>> > of small reads and writes by two to six percent (measured using fio
>> > in the guest)
>> >
>> > Avoiding repeated calls to posix_memalign() reduced the dirty heap
>> > from 25MB to 5MB in the case of a single datapath process while also
>> > improving performance.
>> >
>> > v2 removes some checkpatch complaints and fixes the CCs
>> 
>> Completely unrelated, but since you're the first person touching
>> xen_disk in a while, you're my victim:
>> 
>> At KVM Forum we discussed sending a patch to deprecate xen_disk because
>> after all those years, it still hasn't been converted to qdev. Markus is
>> currently fixing some other not yet qdevified block device, but after
>> that xen_disk will be the only one left.
>> 
>> A while ago, a downstream patch review found out that there are some QMP
>> commands that would immediately crash if a xen_disk device were present
>> because of the lacking qdevification. This is not the code quality
>> standard I envision for QEMU. It's time for non-qdev devices to go.
>> 
>> So if you guys are still interested in the device, could someone please
>> finally look into converting it?
>> 
>
> I have a patch series to do exactly this. It's somewhat involved as I
> need to convert the whole PV backend infrastructure. I will try to
> rebase and clean up my series a.s.a.p.

Awesome!  Please coordinate with Anthony Prerard to avoid duplicating
work if you haven't done so already.

Re: [Qemu-block] [PATCH v2 1/1] include: Add a comment to explain the origin of sizes' lookup table

[Philippe Mathieu-Daudé]

Hi Leonid,

On 4/11/18 19:07, Leonid Bloch wrote:

The lookup table for power-of-two sizes was added in commit 540b8492618eb
for the purpose of having convenient shortcuts for these sizes in cases
when the literal number has to be present at compile time, and
expressions as '(1 * KiB)' can not be used. One such case is the
stringification of sizes. Beyond that, it is convenient to use these
shortcuts for all power-of-two sizes, even if they don't have to be
literal numbers.

Despite its convenience, this table introduced 55 lines of "dumb" code,
the purpose and origin of which are obscure without reading the message
of the commit which introduced it. This patch fixes that by adding a
comment to the code itself with a brief explanation for the reasoning
behind this table. This comment includes the short AWK script that
generated the table, so that anyone who's interested could make sure
that the values in it are correct (otherwise these values look as if
they were typed manually).

Signed-off-by: Leonid Bloch 
---
  include/qemu/units.h | 18 ++
  1 file changed, 18 insertions(+)

diff --git a/include/qemu/units.h b/include/qemu/units.h
index 68a7758650..1c959d182e 100644
--- a/include/qemu/units.h
+++ b/include/qemu/units.h
@@ -17,6 +17,24 @@
  #define PiB (INT64_C(1) << 50)
  #define EiB (INT64_C(1) << 60)
  
+/*

+ * The following lookup table is intended to be used when a literal string of
+ * the number of bytes is required (for example if it needs to be stringified).
+ * It can also be used for generic shortcuts of power-of-two sizes.


^ ok

v this part is not useful in the tree IMHO.


+ * This table is generated using the AWK script below:
+ *
+ *  BEGIN {
+ *  suffix="KMGTPE";
+ *  for(i=10; i<64; i++) {
+ *  val=2**i;
+ *  s=substr(suffix, int(i/10), 1);
+ *  n=2**(i%10);
+ *  pad=21-int(log(n)/log(10));
+ *  printf("#define S_%d%siB %*d\n", n, s, pad, val);
+ *  }
+ *  }
+ */


If it is generated and the stringified are also generated, why not 
generate this once via the ./configure, since it is used by machines and 
not by humans?



+
  #define S_1KiB  1024
  #define S_2KiB  2048
  #define S_4KiB  4096



Thanks,

Phil.

Re: [Qemu-block] [PATCH 2/7] qcow2: make more generic interface for qcow2_compress

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:33 PM CET, Vladimir Sementsov-Ogievskiy 
 wrote:
> Give explicit size both for source and destination buffers, to make it
> similar with decompression path and than cleanly reuse parameter
> structure for decompression threads.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH] include: Add a comment to explain the origin of sizes' lookup table

[Eric Blake]

On 11/2/18 8:58 PM, Leonid Bloch wrote:

The lookup table for power-of-two sizes was added in commit 540b8492618eb
for the purpose of having convenient shortcuts for these sizes in cases
when the literal number has to be present at compile time, and
expressions as '(1 * KiB)' can not be used. One such case is the
stringification of sizes. Beyond that, it is convenient to use these
shortcuts for all power-of-two sizes, even if they don't have to be
literal numbers.

Despite its convenience, this table introduced 55 lines of "dumb" code,
the purpose and origin of which are obscure without reading the message
of the commit which introduced it. This patch fixes that by adding a
comment to the code itself with a brief explanation for the reasoning
behind this table. This comment includes the short AWK script that
generated the table, so that anyone who's interested could make sure
that the values in it are correct (otherwise these values look as if
they were typed manually).

Signed-off-by: Leonid Bloch 
---
  include/qemu/units.h | 18 ++
  1 file changed, 18 insertions(+)


I'm still not completely sold that we can't come up with a more elegant 
runtime solution that avoids the need for hard-coding stringified 
defaults at compile time; but if we keep these S_* constants, this 
comment definitely helps.


Reviewed-by: Eric Blake 

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

Re: [Qemu-block] [PATCH 3/7] qcow2: move decompression from qcow2-cluster.c to qcow2.c

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:34 PM CET, Vladimir Sementsov-Ogievskiy 
 wrote:
> Compression is done in threads in qcow2.c. We want to do decompression
> in the same way, so, firstly, move it to the same file.
>
> The only change is braces around if-body in decompress_buffer, to
> satisfy checkpatch.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu/units: Move out QCow2 specific definitions

[Eric Blake]

On 11/2/18 7:29 PM, Leonid Bloch wrote:

Agreed. I didn't want it in the first place, arguing that if we want
stringification of defaults, it would be better to have a runtime function
do that, rather than adding a set of near-duplicate macro names.


A runtime function will not help here, as these are used in compile
time. These result in strings that are actually compiled into the binaries.


Well, my point is that right now, QemuOpts outputs a hard-coded string 
(with no alternative), which does mean that things are compiled in.  Is 
it worth exploring an enhancement to QemuOpts that lets it decide 
between either a const char * hardcoded string, or a runtime formatter 
callback function, and convert all existing hardcoded strings with 
awkward contents to instead use a new runtime formatter?


Or is that just putting lipstick on a pig, since we are already trying 
to move away from QemuOpts into a more structured command line 
introspection?


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

Re: [Qemu-block] [PATCH 1/7] qcow2: use Z_OK instead of 0 for deflateInit2 return code check

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:32 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Use appropriate macro, corresponding to deflateInit2 spec.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH] block: Make more block drivers compile-time configurable

[Markus Armbruster]

Max Reitz  writes:

> On 19.10.18 13:34, Markus Armbruster wrote:
>> From: Jeff Cody 
>> 
>> This adds configure options to control the following block drivers:
>> 
>> * Bochs
>> * Cloop
>> * Dmg
>> * Qcow (V1)
>> * Vdi
>> * Vvfat
>> * qed
>> * parallels
>> * sheepdog
>> 
>> Each of these defaults to being enabled.
>> 
>> Signed-off-by: Jeff Cody 
>> Signed-off-by: Markus Armbruster 
>> ---
>> 
>> Hmm, we got quite a few --enable-BLOCK-DRIVER now.  Perhaps a single
>> list-valued option similar --target-list would be better.  Could be
>> done on top.
>> 
>>  block/Makefile.objs | 22 ---
>>  configure   | 91 +
>>  2 files changed, 107 insertions(+), 6 deletions(-)
>> 
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index c8337bf186..1cad9fc4f1 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
   @@ -1,10 +1,18 @@
   -block-obj-y += raw-format.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o 
vvfat.o dmg.o
   +block-obj-y += raw-format.o vmdk.o vpc.o
   +block-obj-$(CONFIG_QCOW1) += qcow.o
   +block-obj-$(CONFIG_VDI) += vdi.o
   +block-obj-$(CONFIG_CLOOP) += cloop.o
   +block-obj-$(CONFIG_BOCHS) += bochs.o
   +block-obj-$(CONFIG_VVFAT) += vvfat.o
   +block-obj-$(CONFIG_DMG) += dmg.o
   +
block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
qcow2-cache.o qcow2-bitmap.o
>
> [...]
>
>> @@ -45,7 +54,8 @@ gluster.o-libs := $(GLUSTERFS_LIBS)
>>  vxhs.o-libs:= $(VXHS_LIBS)
>>  ssh.o-cflags   := $(LIBSSH2_CFLAGS)
>>  ssh.o-libs := $(LIBSSH2_LIBS)
>> -block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
>> +block-obj-dmg-bz2$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
>> +block-obj-$(CONFIG_DMG) += $(block-obj-dmg-bz2-y)
>
> This defines "block-obj-dmg-bz2m" or "block-obj-dmg-bz2n", so
> "block-obj-dmg-bz2-y" is never defined (note both the missing hyphen and
> the "m" vs. "y").
>
> How about:
>
> block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o

As far as I can tell, CONFIG_BZIP2 is either undefined or "y".  Thus,
block-obj-dmg-bz2-y is either left undefined or set to dmg-bz2.o.

Perhaps the '+=' be ':=', but we seem to use '+=' pretty
indiscriminately.

> block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)

As far as I can tell, CONFIG_DMG is also either undefined or "y".  So,
this adds dmg-bz2.o to block-obj-m if both CONFIG_BZIP2 and CONFIG_DMG
are enabled.

Shouldn't it be added to block-obj-y, like dmg.o, or am I confused?

> Bonus point: The "+=" are naturally aligned!

Woot!

> (Fun fact on the side: I tried downloading some dmg image, but qemu
> refused to open that.  ("sector count 409600 for chunk 4 is larger than
> max (131072)" -- yeah, yeah, I know that I'm not the largest guy) -- but
> you can test it just by replacing "dmg-bz2.o" by "does-not-exist.o", and
> then make complains normally, but stops complaining with --disable-dmg
> or --disable-bzip2.)

Thanks!

[Qemu-block] [PATCH v3 1/4] block: adding lzfse decompressing support as a module.

[Julio Faracco]

QEMU dmg support includes zlib and bzip2, but it does not contains lzfse
support. This commit adds the source file to extend compression support
for new DMGs.

Signed-off-by: Julio Faracco 
---
 block/dmg-lzfse.c | 49 +++
 1 file changed, 49 insertions(+)
 create mode 100644 block/dmg-lzfse.c

diff --git a/block/dmg-lzfse.c b/block/dmg-lzfse.c
new file mode 100644
index 00..19d25bc646
--- /dev/null
+++ b/block/dmg-lzfse.c
@@ -0,0 +1,49 @@
+/*
+ * DMG lzfse uncompression
+ *
+ * Copyright (c) 2018 Julio Cesar Faracco
+ *
+ * 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 "qemu-common.h"
+#include "dmg.h"
+#include 
+
+static int dmg_uncompress_lzfse_do(char *next_in, unsigned int avail_in,
+   char *next_out, unsigned int avail_out)
+{
+size_t out_size = lzfse_decode_buffer((uint8_t *) next_out, avail_out,
+  (uint8_t *) next_in, avail_in,
+  NULL);
+
+/* We need to decode the single chunk only. */
+/* So, out_size == avail_out is not an error here. */
+if (out_size > 0) {
+return out_size;
+}
+return -1;
+}
+
+__attribute__((constructor))
+static void dmg_lzfse_init(void)
+{
+assert(!dmg_uncompress_lzfse);
+dmg_uncompress_lzfse = dmg_uncompress_lzfse_do;
+}
-- 
2.17.1

[Qemu-block] [PATCH v3 0/4] Adding LZFSE compression support for DMG block driver.

[Julio Faracco]

Since Mac OS X El Capitain (v10.11), Apple uses LZFSE compression to 
generate compressed DMGs as an alternative to BZIP2. Possible, Apple
want to keep this algorithm as default in long term. Some years ago, 
Apple opened the LZFSE algorithm to opensource and the main source (or 
the most active repo), can be found at: https://github.com/lzfse/lzfse

v1-v2: Fixing some error handlings from dmg-lzfse.c
v2-v3: Master rebasing suggestion from Stefan.

Julio Faracco (4):
  block: adding lzfse decompressing support as a module.
  configure: adding support to lzfse library.
  dmg: including dmg-lzfse module inside dmg block driver.
  dmg: exchanging hardcoded dmg UDIF block types to enum.

 block/Makefile.objs |  2 ++
 block/dmg-lzfse.c   | 49 ++
 block/dmg.c | 65 -
 block/dmg.h |  3 +++
 configure   | 31 +
 5 files changed, 138 insertions(+), 12 deletions(-)
 create mode 100644 block/dmg-lzfse.c

-- 
2.17.1

[Qemu-block] [PATCH v3 4/4] dmg: exchanging hardcoded dmg UDIF block types to enum.

[Julio Faracco]

This change is better to understand what kind of block type is being
handled by the code. Using a syntax similar to the DMG documentation is
easier than tracking all hex values assigned to a block type.

Signed-off-by: Julio Faracco 
---
 block/dmg.c | 43 ---
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 615f818c5a..6d055594df 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -44,6 +44,19 @@ enum {
 DMG_SECTORCOUNTS_MAX = DMG_LENGTHS_MAX / 512,
 };
 
+enum {
+/* DMG Block Type */
+UDZE = 0, /* Zeroes */
+UDRW, /* RAW type */
+UDIG, /* Ignore */
+UDCO = 0x8004,
+UDZO,
+UDBZ,
+ULFO,
+UDCM = 0x7ffe, /* Comments */
+UDLE   /* Last Entry */
+};
+
 static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
 int len;
@@ -108,16 +121,16 @@ static void update_max_chunk_size(BDRVDMGState *s, 
uint32_t chunk,
 uint32_t uncompressed_sectors = 0;
 
 switch (s->types[chunk]) {
-case 0x8005: /* zlib compressed */
-case 0x8006: /* bzip2 compressed */
-case 0x8007: /* lzfse compressed */
+case UDZO: /* zlib compressed */
+case UDBZ: /* bzip2 compressed */
+case ULFO: /* lzfse compressed */
 compressed_size = s->lengths[chunk];
 uncompressed_sectors = s->sectorcounts[chunk];
 break;
-case 1: /* copy */
+case UDRW: /* copy */
 uncompressed_sectors = DIV_ROUND_UP(s->lengths[chunk], 512);
 break;
-case 2: /* zero */
+case UDIG: /* zero */
 /* as the all-zeroes block may be large, it is treated specially: the
  * sector is not copied from a large buffer, a simple memset is used
  * instead. Therefore uncompressed_sectors does not need to be set. */
@@ -186,13 +199,13 @@ typedef struct DmgHeaderState {
 static bool dmg_is_known_block_type(uint32_t entry_type)
 {
 switch (entry_type) {
-case 0x0001:/* uncompressed */
-case 0x0002:/* zeroes */
-case 0x8005:/* zlib */
+case UDRW:/* uncompressed */
+case UDIG:/* zeroes */
+case UDZO:/* zlib */
 return true;
-case 0x8006:/* bzip2 */
+case UDBZ:/* bzip2 */
 return !!dmg_uncompress_bz2;
-case 0x8007:/* lzfse */
+case ULFO:/* lzfse */
 return !!dmg_uncompress_lzfse;
 default:
 return false;
@@ -592,7 +605,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs, 
uint64_t sector_num)
 
 s->current_chunk = s->n_chunks;
 switch (s->types[chunk]) { /* block entry type */
-case 0x8005: { /* zlib compressed */
+case UDZO: { /* zlib compressed */
 /* we need to buffer, because only the chunk as whole can be
  * inflated. */
 ret = bdrv_pread(bs->file, s->offsets[chunk],
@@ -615,7 +628,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs, 
uint64_t sector_num)
 return -1;
 }
 break; }
-case 0x8006: /* bzip2 compressed */
+case UDBZ: /* bzip2 compressed */
 if (!dmg_uncompress_bz2) {
 break;
 }
@@ -636,7 +649,7 @@ static inline int dmg_read_chunk(BlockDriverState *bs, 
uint64_t sector_num)
 return ret;
 }
 break;
-case 0x8007:
+case ULFO:
 if (!dmg_uncompress_lzfse) {
 break;
 }
@@ -657,14 +670,14 @@ static inline int dmg_read_chunk(BlockDriverState *bs, 
uint64_t sector_num)
 return ret;
 }
 break;
-case 1: /* copy */
+case UDRW: /* copy */
 ret = bdrv_pread(bs->file, s->offsets[chunk],
  s->uncompressed_chunk, s->lengths[chunk]);
 if (ret != s->lengths[chunk]) {
 return -1;
 }
 break;
-case 2: /* zero */
+case UDIG: /* zero */
 /* see dmg_read, it is treated specially. No buffer needs to be
  * pre-filled, the zeroes can be set directly. */
 break;
-- 
2.17.1

[Qemu-block] [PATCH v3 3/4] dmg: including dmg-lzfse module inside dmg block driver.

[Julio Faracco]

This commit includes the support to new module dmg-lzfse into dmg block
driver. It includes the support for block type ULFO (0x8007).

Signed-off-by: Julio Faracco 
---
 block/dmg.c | 28 
 block/dmg.h |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/block/dmg.c b/block/dmg.c
index c9b3c519c4..615f818c5a 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -33,6 +33,9 @@
 int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
   char *next_out, unsigned int avail_out);
 
+int (*dmg_uncompress_lzfse)(char *next_in, unsigned int avail_in,
+char *next_out, unsigned int avail_out);
+
 enum {
 /* Limit chunk sizes to prevent unreasonable amounts of memory being used
  * or truncating when converting to 32-bit types
@@ -107,6 +110,7 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t 
chunk,
 switch (s->types[chunk]) {
 case 0x8005: /* zlib compressed */
 case 0x8006: /* bzip2 compressed */
+case 0x8007: /* lzfse compressed */
 compressed_size = s->lengths[chunk];
 uncompressed_sectors = s->sectorcounts[chunk];
 break;
@@ -188,6 +192,8 @@ static bool dmg_is_known_block_type(uint32_t entry_type)
 return true;
 case 0x8006:/* bzip2 */
 return !!dmg_uncompress_bz2;
+case 0x8007:/* lzfse */
+return !!dmg_uncompress_lzfse;
 default:
 return false;
 }
@@ -431,6 +437,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 
 block_module_load_one("dmg-bz2");
+block_module_load_one("dmg-lzfse");
 
 s->n_chunks = 0;
 s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
@@ -629,6 +636,27 @@ static inline int dmg_read_chunk(BlockDriverState *bs, 
uint64_t sector_num)
 return ret;
 }
 break;
+case 0x8007:
+if (!dmg_uncompress_lzfse) {
+break;
+}
+/* we need to buffer, because only the chunk as whole can be
+ * inflated. */
+ret = bdrv_pread(bs->file, s->offsets[chunk],
+ s->compressed_chunk, s->lengths[chunk]);
+if (ret != s->lengths[chunk]) {
+return -1;
+}
+
+ret = dmg_uncompress_lzfse((char *)s->compressed_chunk,
+   (unsigned int) s->lengths[chunk],
+   (char *)s->uncompressed_chunk,
+   (unsigned int)
+   (512 * s->sectorcounts[chunk]));
+if (ret < 0) {
+return ret;
+}
+break;
 case 1: /* copy */
 ret = bdrv_pread(bs->file, s->offsets[chunk],
  s->uncompressed_chunk, s->lengths[chunk]);
diff --git a/block/dmg.h b/block/dmg.h
index 2ecf239ba5..f28929998f 100644
--- a/block/dmg.h
+++ b/block/dmg.h
@@ -55,4 +55,7 @@ typedef struct BDRVDMGState {
 extern int (*dmg_uncompress_bz2)(char *next_in, unsigned int avail_in,
  char *next_out, unsigned int avail_out);
 
+extern int (*dmg_uncompress_lzfse)(char *next_in, unsigned int avail_in,
+   char *next_out, unsigned int avail_out);
+
 #endif
-- 
2.17.1

[Qemu-block] [PATCH v3 2/4] configure: adding support to lzfse library.

[Julio Faracco]

This commit includes the support to lzfse opensource library. With this
library dmg block driver can decompress images with this type of
compression inside.

Signed-off-by: Julio Faracco 
---
 block/Makefile.objs |  2 ++
 configure   | 31 +++
 2 files changed, 33 insertions(+)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index c8337bf186..f4ddbb9c7b 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -47,6 +47,8 @@ ssh.o-cflags   := $(LIBSSH2_CFLAGS)
 ssh.o-libs := $(LIBSSH2_LIBS)
 block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
 dmg-bz2.o-libs := $(BZIP2_LIBS)
+block-obj-$(if $(CONFIG_LZFSE),m,n) += dmg-lzfse.o
+dmg-lzfse.o-libs   := $(LZFSE_LIBS)
 qcow.o-libs:= -lz
 linux-aio.o-libs   := -laio
 parallels.o-cflags := $(LIBXML2_CFLAGS)
diff --git a/configure b/configure
index 46ae1e8c76..56b26f0579 100755
--- a/configure
+++ b/configure
@@ -434,6 +434,7 @@ capstone=""
 lzo=""
 snappy=""
 bzip2=""
+lzfse=""
 guest_agent=""
 guest_agent_with_vss="no"
 guest_agent_ntddscsi="no"
@@ -1301,6 +1302,10 @@ for opt do
   ;;
   --enable-bzip2) bzip2="yes"
   ;;
+  --enable-lzfse) lzfse="yes"
+  ;;
+  --disable-lzfse) lzfse="no"
+  ;;
   --enable-guest-agent) guest_agent="yes"
   ;;
   --disable-guest-agent) guest_agent="no"
@@ -1700,6 +1705,8 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   snappy  support of snappy compression library
   bzip2   support of bzip2 compression library
   (for reading bzip2-compressed dmg images)
+  lzfse   support of lzfse compression library
+  (for reading lzfse-compressed dmg images)
   seccomp seccomp support
   coroutine-pool  coroutine freelist (better performance)
   glusterfs   GlusterFS backend
@@ -2209,6 +2216,24 @@ EOF
 fi
 fi
 
+##
+# lzfse check
+
+if test "$lzfse" != "no" ; then
+cat > $TMPC << EOF
+#include 
+int main(void) { lzfse_decode_scratch_size(); return 0; }
+EOF
+if compile_prog "" "-llzfse" ; then
+lzfse="yes"
+else
+if test "$lzfse" = "yes"; then
+feature_not_found "lzfse" "Install lzfse devel"
+fi
+lzfse="no"
+fi
+fi
+
 ##
 # libseccomp check
 
@@ -6036,6 +6061,7 @@ echo "Live block migration $live_block_migration"
 echo "lzo support   $lzo"
 echo "snappy support$snappy"
 echo "bzip2 support $bzip2"
+echo "lzfse support $lzfse"
 echo "NUMA host support $numa"
 echo "libxml2   $libxml2"
 echo "tcmalloc support  $tcmalloc"
@@ -6549,6 +6575,11 @@ if test "$bzip2" = "yes" ; then
   echo "BZIP2_LIBS=-lbz2" >> $config_host_mak
 fi
 
+if test "$lzfse" = "yes" ; then
+  echo "CONFIG_LZFSE=y" >> $config_host_mak
+  echo "LZFSE_LIBS=-llzfse" >> $config_host_mak
+fi
+
 if test "$libiscsi" = "yes" ; then
   echo "CONFIG_LIBISCSI=m" >> $config_host_mak
   echo "LIBISCSI_CFLAGS=$libiscsi_cflags" >> $config_host_mak
-- 
2.17.1

Re: [Qemu-block] [PATCH 1/2] nvme: don't unref ctrl_mem when device unrealized

[Li Qiang]

Ping...

I think this is a serious issue, can go 3.1

Thanks,
Li Qiang

Li Qiang  于2018年10月29日周一 下午2:29写道：

> Currently, when hotplug/unhotplug nvme device, it will cause an
> assert in object.c. Following is the backtrack:
>
> ERROR:qom/object.c:981:object_unref: assertion failed: (obj->ref > 0)
>
> Thread 2 "qemu-system-x86" received signal SIGABRT, Aborted.
> [Switching to Thread 0x7fffcbd32700 (LWP 18844)]
> 0x7fffdb9e4fff in raise () from /lib/x86_64-linux-gnu/libc.so.6
> (gdb) bt
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> qom/object.c:981
> /home/liqiang02/qemu-upstream/qemu/memory.c:1732
> /home/liqiang02/qemu-upstream/qemu/memory.c:285
> util/qemu-thread-posix.c:504
> /lib/x86_64-linux-gnu/libpthread.so.0
>
> This is caused by memory_region_unref in nvme_exit.
>
> Remove it to make the PCIdevice refcount correct.
>
> Signed-off-by: Li Qiang 
> ---
>  hw/block/nvme.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..359a06d0ad 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1331,9 +1331,6 @@ static void nvme_exit(PCIDevice *pci_dev)
>  g_free(n->namespaces);
>  g_free(n->cq);
>  g_free(n->sq);
> -if (n->cmbsz) {
> -memory_region_unref(&n->ctrl_mem);
> -}
>
>  msix_uninit_exclusive_bar(pci_dev);
>  }
> --
> 2.11.0
>
>

Re: [Qemu-block] [PATCH] block/vdi: Don't take address of fields in packed structs

[Peter Maydell]

On 17 October 2018 at 15:55, Kevin Wolf  wrote:
> Am 16.10.2018 um 19:25 hat Peter Maydell geschrieben:
>> Taking the address of a field in a packed struct is a bad idea, because
>> it might not be actually aligned enough for that pointer type (and
>> thus cause a crash on dereference on some host architectures). Newer
>> versions of clang warn about this. Avoid the bug by not using the
>> "modify in place" byte swapping functions.
>>
>> There are a few places where the in-place swap function is
>> used on something other than a packed struct field; we convert
>> those anyway, for consistency.
>>
>> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
>>
>> There are other places where we take the address of a packed member
>> in this file for other purposes than passing it to a byteswap
>> function (all the calls to qemu_uuid_*()); we leave those for now.
>>
>> Signed-off-by: Peter Maydell 
>
> Thanks, applied to the block branch.

This also hasn't made it into master.

thanks
-- PMM

Re: [Qemu-block] [PATCH] block/vhdx: Don't take address of fields in packed structs

[Peter Maydell]

On 17 October 2018 at 15:36, Kevin Wolf  wrote:
> Am 16.10.2018 um 19:09 hat Peter Maydell geschrieben:
>> Taking the address of a field in a packed struct is a bad idea, because
>> it might not be actually aligned enough for that pointer type (and
>> thus cause a crash on dereference on some host architectures). Newer
>> versions of clang warn about this. Avoid the bug by not using the
>> "modify in place" byte swapping functions.
>>
>> There are a few places where the in-place swap function is
>> used on something other than a packed struct field; we convert
>> those anyway, for consistency.
>>
>> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
>>
>> Signed-off-by: Peter Maydell 
>> ---
>> Usual disclaimer: produced with "make check" only, but purely
>> automated conversion should be safe.
>
> More relevant, qemu-iotests for vhdx passes, too. I don't think "make
> check" would run any of the modified code.
>
> Thanks, applied to the block branch

Ping? This doesn't seem to have reached master.

thanks
-- PMM

Re: [Qemu-block] [PATCH 0/3] block/qcow*: Don't take address of fields in packed structs

[Peter Maydell]

On 10 October 2018 at 11:55, Kevin Wolf  wrote:
> Am 09.10.2018 um 19:24 hat Peter Maydell geschrieben:
>> Taking the address of a field in a packed struct is a bad idea, because
>> it might not be actually aligned enough for that pointer type (and
>> thus cause a crash on dereference on some host architectures). Newer
>> versions of clang warn about this. Avoid the bug by not using the
>> "modify in place" byte swapping functions.
>>
>> There are a few places in the affected files where the in-place swap
>> function is used on something other than a packed struct field; we
>> convert those anyway, for consistency.
>>
>> Patches produced mechanically using spatch; in one case I also
>> did a little hand-editing to wrap overlong lines that checkpatch
>> would otherwise complain about.
>>
>> (clang also complains about other files in block: vdi.c, vpc.c,
>> vhdx.h, vhdx.c, vhdx-endian.c, vhdx-log.c -- I may produce patches
>> for those later if nobody else gets there first.)
>>
>> thanks
>> -- PMM
>>
>> Peter Maydell (3):
>>   block/qcow2: Don't take address of fields in packed structs
>>   block/qcow: Don't take address of fields in packed structs
>>   block/qcow2-bitmap: Don't take address of fields in packed structs
>
> Thanks, applied to the block branch.

Ping? This doesn't seem to have made it into master, unless
I've missed it...

thanks
-- PMM

Re: [Qemu-block] [PATCH v2 0/1] include: Add a comment to explain the origin of

[Kevin Wolf]

Am 04.11.2018 um 19:07 hat Leonid Bloch geschrieben:
> Please see the commit message for the rationale.
> 
> Difference from v1:
> * Tabs removed from the code indentation of the sample code in the
>   comment, in order to pass checkpatch.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v2 0/1] vdi: Use a literal number of bytes for

[Kevin Wolf]

Am 04.11.2018 um 19:09 hat Leonid Bloch geschrieben:
> Please see the commit message for the rationale.
> 
> Difference from v1:
> * Format string is fixed.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v3 6/6] monitor: deprecate acl_show, acl_reset, acl_policy, acl_add, acl_remove

[Juan Quintela]

Daniel P. Berrangé  wrote:
> The various ACL related commands are obsolete now that the QAuthZ
> framework for authorization is fully integrated throughout QEMU network
> services. Mark it as deprecated with no replacement to be provided.
>
> Authorization is now provided by using 'object_add' together with
> the 'tls-authz' or 'sasl-authz' parameters to the VNC server, and
> equivalent for other network services.
>
> Signed-off-by: Daniel P. Berrangé 

Reviewed-by: Juan Quintela 

Re: [Qemu-block] [PATCH v3 5/6] vnc: allow specifying a custom authorization object name

[Juan Quintela]

Daniel P. Berrangé  wrote:
> From: "Daniel P. Berrange" 
>
> The VNC server has historically had support for ACLs to check both the
> SASL username and the TLS x509 distinguished name. The VNC server was
> responsible for creating the initial ACL, and the client app was then
> responsible for populating it with rules using the HMP 'acl_add' command.
>
> This is not satisfactory for a variety of reasons. There is no way to
> populate the ACLs from the command line, users are forced to use the
> HMP. With multiple network services all supporting TLS and ACLs now, it
> is desirable to be able to define a single ACL that is referenced by all
> services.
>
> To address these limitations, two new options are added to the VNC
> server CLI. The 'tls-authz' option takes the ID of a QAuthZ object to
> use for checking TLS x509 distinguished names, and the 'sasl-authz'
> option takes the ID of another object to use for checking SASL usernames.
>
> In this example, we setup two authorization rules. The first allows any
> client with a certificate issued by the 'RedHat' organization in the
> 'London' locality. The second ACL allows clients with either the
> 'j...@redhat.com' or  'f...@redhat.com' kerberos usernames. Both checks
> must pass for the user to be allowed.
>
> $QEMU -object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
>   endpoint=server,verify-peer=yes \
>   -object authz-simple,id=authz0,policy=deny,\
>   rules.0.match=O=RedHat,,L=London,rules.0.policy=allow \
>   -object authz-simple,id=authz1,policy=deny,\
>   rules.0.match=f...@redhat.com,rules.0.policy=allow \
>   rules.0.match=j...@redhat.com,rules.0.policy=allow \
>   -vnc 0.0.0.0:1,tls-creds=tls0,tls-authz=authz0,
>  sasl,sasl-authz=authz1 \
>   ...other QEMU args...
>
> Signed-off-by: Daniel P. Berrange 

Reviewed-by: Juan Quintela 

Re: [Qemu-block] [PATCH v2 0/5] Various option help readability improvement suggestions

[Kevin Wolf]

Am 19.10.2018 um 18:49 hat Max Reitz geschrieben:
> I noticed that with the (more or less) recent series from Marc-André the
> output of qemu-img amend -f qcow2 -o help changed to this:
> 
> $ ./qemu-img amend -f qcow2 -o help
> Creation options for 'qcow2':
> qcow2-create-opts.backing_file=str - File name of a base image
> qcow2-create-opts.backing_fmt=str - Image format of the base image
> qcow2-create-opts.cluster_size=size - qcow2 cluster size
> qcow2-create-opts.compat=str - Compatibility level (0.10 or 1.1)
> [...]
> 
> The types are a nice addition, but I didn't like having the list name
> printed in every single line (in fact, the list name does not make any
> sense here at all, because there already is a caption which reads
> "Creation options for 'qcow2'"), and I did not like the use of '=' for
> types.
> 
> In general, I don't like the robot-y appearance, which is even worse in
> things like -device virtio-blk,help, which gives you this (among
> other lines):
> 
> > virtio-blk-pci.iothread=link
> 
> Sadly, there isn't much we can do about the "link", so this
> series doesn't improve on that point.
> 
> What this series does do, however, is it changes these lists not to
> print the list name on every single line, but only as a caption (and for
> option lists, this caption is option, because the caller may want to
> print a custom caption that is more expressive -- as is the case for
> qemu-img amend -o help).
> 
> Consequentially, all list items are indented by two spaces to make clear
> they belong to the caption.  I can already see that some people might
> disagree on having this indentation, but I like it, so I have it in this
> series.
> 
> Furthermore, types are now enclosed by angle brackets, and the alignment
> we originally had for descriptions is restored (although now after 24
> instead of 16 characters, because every option name is now accompanied
> by indentation and a type).
> 
> 
> Thus, after this series, the amend output looks like this:
> 
> $ ./qemu-img amend -f qcow2 -o help
> Creation options for 'qcow2':
>   backing_file= - File name of a base image
>   backing_fmt=  - Image format of the base image
>   cluster_size=- qcow2 cluster size
>   compat=   - Compatibility level (0.10 or 1.1)
> [...]
> 
> 
> virtio-blk's list presents itself like so:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help
> virtio-blk-pci options:
>   iothread=>
>   request-merging= - on/off
>   secs=
> [...]
> 
> 
> And now we even print something when there are no options:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -object can-bus,help
> There are no options for can-bus.
> 
> (Before this series, there just is no output.)
> 
> 
> As a side effect, patch 1 fixes iotest 082.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE

[no-reply]

Hi,

This series failed docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

Type: series
Message-id: 20181103014831.29889-1-lbl...@janustech.com
Subject: [Qemu-devel] [PATCH] vdi: Use a literal number of bytes for 
DEFAULT_CLUSTER_SIZE

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
95535d6957 vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE

=== OUTPUT BEGIN ===
  BUILD   fedora
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-_s4dc72k/src'
  GEN 
/var/tmp/patchew-tester-tmp-_s4dc72k/src/docker-src.2018-11-04-06.59.49.14368/qemu.tar
Cloning into 
'/var/tmp/patchew-tester-tmp-_s4dc72k/src/docker-src.2018-11-04-06.59.49.14368/qemu.tar.vroot'...
done.
Checking out files:  45% (2954/6451)   
Checking out files:  46% (2968/6451)   
Checking out files:  47% (3032/6451)   
Checking out files:  48% (3097/6451)   
Checking out files:  49% (3161/6451)   
Checking out files:  50% (3226/6451)   
Checking out files:  51% (3291/6451)   
Checking out files:  52% (3355/6451)   
Checking out files:  53% (3420/6451)   
Checking out files:  54% (3484/6451)   
Checking out files:  55% (3549/6451)   
Checking out files:  56% (3613/6451)   
Checking out files:  57% (3678/6451)   
Checking out files:  58% (3742/6451)   
Checking out files:  59% (3807/6451)   
Checking out files:  60% (3871/6451)   
Checking out files:  61% (3936/6451)   
Checking out files:  62% (4000/6451)   
Checking out files:  63% (4065/6451)   
Checking out files:  64% (4129/6451)   
Checking out files:  64% (4152/6451)   
Checking out files:  65% (4194/6451)   
Checking out files:  66% (4258/6451)   
Checking out files:  67% (4323/6451)   
Checking out files:  68% (4387/6451)   
Checking out files:  69% (4452/6451)   
Checking out files:  70% (4516/6451)   
Checking out files:  71% (4581/6451)   
Checking out files:  72% (4645/6451)   
Checking out files:  73% (4710/6451)   
Checking out files:  74% (4774/6451)   
Checking out files:  75% (4839/6451)   
Checking out files:  76% (4903/6451)   
Checking out files:  77% (4968/6451)   
Checking out files:  78% (5032/6451)   
Checking out files:  79% (5097/6451)   
Checking out files:  80% (5161/6451)   
Checking out files:  81% (5226/6451)   
Checking out files:  82% (5290/6451)   
Checking out files:  83% (5355/6451)   
Checking out files:  84% (5419/6451)   
Checking out files:  85% (5484/6451)   
Checking out files:  86% (5548/6451)   
Checking out files:  87% (5613/6451)   
Checking out files:  88% (5677/6451)   
Checking out files:  89% (5742/6451)   
Checking out files:  90% (5806/6451)   
Checking out files:  91% (5871/6451)   
Checking out files:  92% (5935/6451)   
Checking out files:  93% (6000/6451)   
Checking out files:  94% (6064/6451)   
Checking out files:  95% (6129/6451)   
Checking out files:  96% (6193/6451)   
Checking out files:  97% (6258/6451)   
Checking out files:  98% (6322/6451)   
Checking out files:  99% (6387/6451)   
Checking out files: 100% (6451/6451)   
Checking out files: 100% (6451/6451), done.
Your branch is up-to-date with 'origin/test'.
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 
'/var/tmp/patchew-tester-tmp-_s4dc72k/src/docker-src.2018-11-04-06.59.49.14368/qemu.tar.vroot/dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered 
for path 'ui/keycodemapdb'
Cloning into 
'/var/tmp/patchew-tester-tmp-_s4dc72k/src/docker-src.2018-11-04-06.59.49.14368/qemu.tar.vroot/ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out 
'6b3d716e2b6472eb7189d3220552280ef3d832ce'
  COPYRUNNER
RUN test-mingw in qemu:fedora 
Packages installed:
SDL2-devel-2.0.8-5.fc28.x86_64
bc-1.07.1-5.fc28.x86_64
bison-3.0.4-9.fc28.x86_64
bluez-libs-devel-5.50-1.fc28.x86_64
brlapi-devel-0.6.7-19.fc28.x86_64
bzip2-1.0.6-26.fc28.x86_64
bzip2-devel-1.0.6-26.fc28.x86_64
ccache-3.4.2-2.fc28.x86_64
clang-6.0.1-1.fc28.x86_64
device-mapper-multipath-devel-0.7.4-3.git07e7bd5.fc28.x86_64
findutils-4.6.0-19.fc28.x86_64
flex-2.6.1-7.fc28.x86_64
gcc-8.1.1-5.fc28.x86_64
gcc-c++-8.1.1-5.fc28.x86_64
gettext-0.19.8.1-14.fc28.x86_64
git-2.17.1-3.fc28.x86_64
glib2-devel-2.56.1-4.fc28.x86_64
glusterfs-api-devel-4.1.2-2.fc28.x86_64
gnutls-devel-3.6.3-3.fc28.x86_64
gtk3-devel-3.22.30-1.fc28.x86_64
hostname-3.20-3.fc28.x86_64
libaio-devel-0.3.110-11.fc28.x86_64
libasan-8.1.1-5.fc28.x86_64
libattr-devel-2.4.48-3.fc28.x86_64
libcap-devel-2.25-9.fc28.x86_64
libcap-ng-devel-0.7.9-4.fc28.x86_64
libcurl-devel-7.59.0-6.fc28.x86_64
libfdt-devel-1.4.6-5.fc28.x86_64
libpng-devel-1.6.34-6.fc28.x86_64
librbd-devel-12.2.7-1.fc28.x86_64
libssh2-devel-1.8.0-7.fc28.x86_64
libubsan-8.1.1-5.fc28.x86_64
li

Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Disable test-bdrv-drain

[Peter Maydell]

On 4 November 2018 at 22:03, Paolo Bonzini  wrote:
> On 02/11/2018 14:33, Peter Maydell wrote:
>> On 9 October 2018 at 12:16, Paolo Bonzini  wrote:
>>> Yup, we have to stop using pthread_key_create.  Luckily, these days
>>> there is always qemu_thread_start that wraps the thread, so we can call
>>> qemu_thread_atexit_run from there, and change exit_key to a thread-local
>>> NotifierList.
>>
>> We would also need to catch exits via qemu_thread_exit(), right?
>> We probably also need to handle the main thread specially, via
>> atexit(). This seems to be pretty much what we already do in
>> util/qemu-thread-win32.c...
>
> There's only one caller of qemu_thread_exit and it can be removed easily.
>
> However, an improvement on the idea is to use pthread_cleanup_push/pop
> in qemu_thread_start.  This does handle qemu_thread_exit.

Interesting. (I have a patch I put together on Friday and haven't
tested yet that just uses the win32-style "explicitly run notifiers
in exit and at end of thread_start".)

thanks
-- PMM

Re: [Qemu-block] [PATCH v2 4/5] object: Make option help nicer to read

[Marc-André Lureau]

On Fri, Oct 19, 2018 at 8:49 PM Max Reitz  wrote:
>
> Just like in qemu_opts_print_help(), print the object name as a caption
> instead of on every single line, indent all options, add angle brackets
> around types, and align the descriptions after 24 characters.
>
> Also, indent every object name in the list of available objects.
>
> Signed-off-by: Max Reitz 

Reviewed-by: Marc-André Lureau 

> ---
>  vl.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index ac3ed17de4..ecddbb6b8e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2707,7 +2707,7 @@ static bool object_create_initial(const char *type, 
> QemuOpts *opts)
>  list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
>  for (l = list; l != NULL; l = l->next) {
>  ObjectClass *oc = OBJECT_CLASS(l->data);
> -printf("%s\n", object_class_get_name(oc));
> +printf("  %s\n", object_class_get_name(oc));
>  }
>  g_slist_free(list);
>  exit(0);
> @@ -2729,14 +2729,21 @@ static bool object_create_initial(const char *type, 
> QemuOpts *opts)
>  }
>
>  str = g_string_new(NULL);
> -g_string_append_printf(str, "%s.%s=%s", type,
> -   prop->name, prop->type);
> +g_string_append_printf(str, "  %s=<%s>", prop->name, prop->type);
>  if (prop->description) {
> +if (str->len < 24) {
> +g_string_append_printf(str, "%*s", 24 - (int)str->len, 
> "");
> +}
>  g_string_append_printf(str, " - %s", prop->description);
>  }
>  g_ptr_array_add(array, g_string_free(str, false));
>  }
>  g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
> +if (array->len > 0) {
> +printf("%s options:\n", type);
> +} else {
> +printf("There are no options for %s.\n", type);
> +}
>  for (i = 0; i < array->len; i++) {
>  printf("%s\n", (char *)array->pdata[i]);
>  }
> --
> 2.17.1
>

Re: [Qemu-block] [PATCH v2 2/5] chardev: Indent list of chardevs

[Marc-André Lureau]

On Fri, Oct 19, 2018 at 8:49 PM Max Reitz  wrote:
>
> Following the example of qemu_opts_print_help(), indent all entries in
> the list of character devices.
>
> Signed-off-by: Max Reitz 

Reviewed-by: Marc-André Lureau 

> ---
>  chardev/char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index e115166995..10d44aaefc 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -569,7 +569,7 @@ help_string_append(const char *name, void *opaque)
>  {
>  GString *str = opaque;
>
> -g_string_append_printf(str, "\n%s", name);
> +g_string_append_printf(str, "\n  %s", name);
>  }
>
>  static const char *chardev_alias_translate(const char *name)
> --
> 2.17.1
>

Re: [Qemu-block] [PATCH v2 5/5] fw_cfg: Drop newline in @file description

[Marc-André Lureau]

On Fri, Oct 19, 2018 at 8:50 PM Max Reitz  wrote:
>
> There is no good reason why there should be a newline in this
> description, so remove it.
>
> Signed-off-by: Max Reitz 
> Reviewed-by: Philippe Mathieu-Daudé 

Reviewed-by: Marc-André Lureau 

> ---
>  vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index ecddbb6b8e..6604648570 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -529,7 +529,7 @@ static QemuOptsList qemu_fw_cfg_opts = {
>  }, {
>  .name = "file",
>  .type = QEMU_OPT_STRING,
> -.help = "Sets the name of the file from which\n"
> +.help = "Sets the name of the file from which "
>  "the fw_cfg blob will be loaded",
>  }, {
>  .name = "string",
> --
> 2.17.1
>

Re: [Qemu-block] [PATCH v2 1/5] option: Make option help nicer to read

[Marc-André Lureau]

On Fri, Oct 19, 2018 at 8:49 PM Max Reitz  wrote:
>
> This adds some whitespace into the option help (including indentation)
> and puts angle brackets around the type names.  Furthermore, the list
> name is no longer printed as part of every line, but only once in
> advance, and only if the caller did not print a caption already.
>
> This patch also restores the description alignment we had before commit
> 9cbef9d68ee1d8d0, just at 24 instead of 16 characters like we used to.
> This increase is because now we have the type and two spaces of
> indentation before the description, and with a usual type name length of
> three chracters, this sums up to eight additional characters -- which
> means that we now need 24 characters to get the same amount of padding
> for most options.  Also, 24 is a third of 80, which makes it kind of a
> round number in terminal terms.
>
> Finally, this patch amends the reference output of iotest 082 to match
> the changes (and thus makes it pass again).
>
> Signed-off-by: Max Reitz 

Reviewed-by: Marc-André Lureau 

> ---
>  include/qemu/option.h  |   2 +-
>  qemu-img.c |   4 +-
>  util/qemu-option.c |  32 +-
>  tests/qemu-iotests/082.out | 956 ++---
>  4 files changed, 507 insertions(+), 487 deletions(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 3dfb4493cc..844587cab3 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -132,7 +132,7 @@ typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts 
> *opts, Error **errp);
>  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
>void *opaque, Error **errp);
>  void qemu_opts_print(QemuOpts *opts, const char *sep);
> -void qemu_opts_print_help(QemuOptsList *list);
> +void qemu_opts_print_help(QemuOptsList *list, bool print_caption);
>  void qemu_opts_free(QemuOptsList *list);
>  QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
>
> diff --git a/qemu-img.c b/qemu-img.c
> index b12f4cd19b..4c96db7ba4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -269,7 +269,7 @@ static int print_block_option_help(const char *filename, 
> const char *fmt)
>  }
>
>  printf("Supported options:\n");
> -qemu_opts_print_help(create_opts);
> +qemu_opts_print_help(create_opts, false);
>  qemu_opts_free(create_opts);
>  return 0;
>  }
> @@ -3773,7 +3773,7 @@ static int print_amend_option_help(const char *format)
>  assert(drv->create_opts);
>
>  printf("Creation options for '%s':\n", format);
> -qemu_opts_print_help(drv->create_opts);
> +qemu_opts_print_help(drv->create_opts, false);
>  printf("\nNote that not all of these options may be amendable.\n");
>  return 0;
>  }
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 9a5f263294..de42e2a406 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -224,7 +224,14 @@ static const char *opt_type_to_string(enum QemuOptType 
> type)
>  g_assert_not_reached();
>  }
>
> -void qemu_opts_print_help(QemuOptsList *list)
> +/**
> + * Print the list of options available in the given list.  If
> + * @print_caption is true, a caption (including the list name, if it
> + * exists) is printed.  The options itself will be indented, so
> + * @print_caption should only be set to false if the caller prints its
> + * own custom caption (so that the indentation makes sense).
> + */
> +void qemu_opts_print_help(QemuOptsList *list, bool print_caption)
>  {
>  QemuOptDesc *desc;
>  int i;
> @@ -234,12 +241,12 @@ void qemu_opts_print_help(QemuOptsList *list)
>  desc = list->desc;
>  while (desc && desc->name) {
>  GString *str = g_string_new(NULL);
> -if (list->name) {
> -g_string_append_printf(str, "%s.", list->name);
> -}
> -g_string_append_printf(str, "%s=%s", desc->name,
> +g_string_append_printf(str, "  %s=<%s>", desc->name,
> opt_type_to_string(desc->type));
>  if (desc->help) {
> +if (str->len < 24) {
> +g_string_append_printf(str, "%*s", 24 - (int)str->len, "");
> +}
>  g_string_append_printf(str, " - %s", desc->help);
>  }
>  g_ptr_array_add(array, g_string_free(str, false));
> @@ -247,6 +254,19 @@ void qemu_opts_print_help(QemuOptsList *list)
>  }
>
>  g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
> +if (print_caption && array->len > 0) {
> +if (list->name) {
> +printf("%s options:\n", list->name);
> +} else {
> +printf("Options:\n");
> +}
> +} else if (array->len == 0) {
> +if (list->name) {
> +printf("There are no options for %s.\n", list->name);
> +} else {
> +printf("No options available.\n");
> +}
> +}
>  for (i = 0; i < array->len; i++) {
>  printf("%s\n", (char *)array->pdata[

Re: [Qemu-block] [PATCH v2 3/5] qdev-monitor: Make device options help nicer

[Marc-André Lureau]

On Fri, Oct 19, 2018 at 8:49 PM Max Reitz  wrote:
>
> Just like in qemu_opts_print_help(), print the device name as a caption
> instead of on every single line, indent all options, add angle brackets
> around types, and align the descriptions after 24 characters.  Also,
> separate the descriptions with " - " instead of putting them in
> parentheses, because that is what we do everywhere else.  This does look
> a bit funny here because basically all bits have the description
> "on/off", but funny does not mean it is less readable.
>
> Signed-off-by: Max Reitz 

Reviewed-by: Marc-André Lureau 

> ---
>  qdev-monitor.c | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 802c18a74e..07147c63bf 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -285,10 +285,19 @@ int qdev_device_help(QemuOpts *opts)
>  goto error;
>  }
>
> +if (prop_list) {
> +out_printf("%s options:\n", driver);
> +} else {
> +out_printf("There are no options for %s.\n", driver);
> +}
>  for (prop = prop_list; prop; prop = prop->next) {
> -out_printf("%s.%s=%s", driver, prop->value->name, prop->value->type);
> +int len;
> +out_printf("  %s=<%s>%n", prop->value->name, prop->value->type, 
> &len);
>  if (prop->value->has_description) {
> -out_printf(" (%s)\n", prop->value->description);
> +if (len < 24) {
> +out_printf("%*s", 24 - len, "");
> +}
> +out_printf(" - %s\n", prop->value->description);
>  } else {
>  out_printf("\n");
>  }
> --
> 2.17.1
>