Re: [Qemu-block] [PATCH 1/2] block/qapi: make two printf() formats literal

[Peter Xu]

On Mon, Mar 21, 2016 at 03:14:48PM -0600, Eric Blake wrote:
> On 03/09/2016 06:46 PM, Peter Xu wrote:
> > 
> > Is this a grammar btw?
> 
> Yes, C has an ugly grammar, because [] is just syntactic sugar for
> deferencing pointer addition with nicer operator precedence.  Quoting
> C99 6.5.2.1:
> 
> "The definition of the subscript operator [] is that E1[E2] is identical
> to (*((E1)+(E2))).  Because of the conversion rules that apply to the
> binary + operator, if E1 is an array object (equivalently, a pointer to
> the initial element of an array object) and E2 is an integer, E1[E2]
> designates the E2-th element of E1 (counting from zero)."
> 
> And a string literal is just a fancy way of writing the address of an
> array of characters (where the address is chosen by the compiler).
> 
> Thus, it IS valid to dereference the addition of an integer offset with
> the address implied by a string literal in order to obtain a character
> within the string.  And since the [] operator is commutative (even
> though no one in their right mind commutes the operands), you can also
> write the even-uglier:
> 
> composite["\n "]
> 
> But now we've gone far astray from the original patch review :)

Interesting thing to know.  Thanks. :)

-- peterx

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Mon, 03/21 15:19, Cornelia Huck wrote:
> On Mon, 21 Mar 2016 14:54:04 +0100
> Paolo Bonzini  wrote:
> 
> > On 21/03/2016 14:47, TU BO wrote:
> > >> I'll see if I can produce something based on Conny's patches, which are
> > >> already a start.  Today I had a short day so I couldn't play with the
> > >> bug; out of curiosity, does the bug reproduce with her work + patch 4
> > >> from this series + the reentrancy assertion?
> > > I did NOT see crash with qemu master + "[PATCH RFC 0/6] virtio: refactor
> > > host notifiers" from Conny + patch 4 + assertion.  thx
> > 
> > That's unexpected, but I guess it only says that I didn't review her
> > patches well enough. :)
> 
> I'm also a bit surprised, the only thing that should really be
> different is passing the 'assign' argument in stop_ioeventfd(). Any
> other fixes are purely accidental :)
> 
> Would be interesting to see how this setup fares with virtio-pci.
> 

Seems to fix the assertion I'm hitting too.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Mon, 03/21 14:02, Cornelia Huck wrote:
> On Mon, 21 Mar 2016 20:45:27 +0800
> Fam Zheng  wrote:
> 
> > On Mon, 03/21 12:15, Cornelia Huck wrote:
> > > On Mon, 21 Mar 2016 18:57:18 +0800
> > > Fam Zheng  wrote:
> > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index 08275a9..47f8043 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > > > 
> > > >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > > >  {
> > > > -virtio_queue_notify_vq(>vq[n]);
> > > > +VirtQueue *vq = >vq[n];
> > > > +EventNotifier *n;
> > > > +n = virtio_queue_get_host_notifier(vq);
> > > > +if (n) {
> > > 
> > > Isn't that always true, even if the notifier has not been setup?
> > 
> > You are right, this doesn't make a correct fix. But we can still do a quick
> > test with this as the else branch should never be used with ioeventfd=on. 
> > Am I
> > right?
> > 
> > Fam
> 
> Won't we come through here for the very first kick, when we haven't
> registered the ioeventfd with the kernel yet?
> 

The ioeventfd in virtio-ccw is registered in the main loop when
VIRTIO_CONFIG_S_DRIVER_OK is set, so I think the first kick is okay.

Fam

Re: [Qemu-block] [PATCH v3 03/10] qom: support arbitrary non-scalar properties with -object

[Eric Blake]

On 03/10/2016 11:59 AM, Daniel P. Berrange wrote:
> The current -object command line syntax only allows for
> creation of objects with scalar properties, or a list
> with a fixed scalar element type. Objects which have
> properties that are represented as structs in the QAPI
> schema cannot be created using -object.
> 
> This is a design limitation of the way the OptsVisitor
> is written. It simply iterates over the QemuOpts values
> as a flat list. The support for lists is enabled by
> allowing the same key to be repeated in the opts string.
> 
> It is not practical to extend the OptsVisitor to support
> more complex data structures while also maintaining
> the existing list handling behaviour that is relied upon
> by other areas of QEMU.

Zoltán Kővágó tried earlier with his GSoC patches for the audio
subsystem last year, but those got stalled waiting for qapi enhancements
to go in.  But I think your approach is indeed a bit nicer (rather than
making the warty OptsVisitor even wartier, just avoid it).

> 
> Fortunately there is no existing object that implements
> the UserCreatable interface that relies on the list
> handling behaviour, so it is possible to swap out the
> OptsVisitor for a different visitor implementation, so
> -object supports non-scalar properties, thus leaving
> other users of OptsVisitor unaffected.
> 
> The previously added qdict_crumple() method is able to
> take a qdict containing a flat set of properties and
> turn that into a arbitrarily nested set of dicts and
> lists. By combining qemu_opts_to_qdict and qdict_crumple()
> together, we can turn the opt string into a data structure
> that is practically identical to that passed over QMP
> when defining an object. The only difference is that all
> the scalar values are represented as strings, rather than
> strings, ints and bools. This is sufficient to let us
> replace the OptsVisitor with the QMPInputVisitor for
> use with -object.

Indeed, nice replacement.

> 
> Thus -object can now support non-scalar properties,
> for example the QMP object
> 
>   {
> "execute": "object-add",
> "arguments": {
>   "qom-type": "demo",
>   "id": "demo0",
>   "parameters": {
> "foo": [
> { "bar": "one", "wizz": "1" },
> { "bar": "two", "wizz": "2" }
> ]
>   }
> }
>   }
> 
> Would be creatable via the CLI now using
> 
> $QEMU \
>   -object demo,id=demo0,\
>   foo.0.bar=one,foo.0.wizz=1,\
>   foo.1.bar=two,foo.1.wizz=2
> 
> This is also wired up to work for the 'object_add' command
> in the HMP monitor with the same syntax.
> 
>   (hmp) object_add demo,id=demo0,\
>foo.0.bar=one,foo.0.wizz=1,\
>  foo.1.bar=two,foo.1.wizz=2

Maybe mention that the indentation is not actually present in the real
command lines typed.

> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  hmp.c  |  18 +--
>  qom/object_interfaces.c|  20 ++-
>  tests/check-qom-proplist.c | 295 
> -
>  3 files changed, 313 insertions(+), 20 deletions(-)
> 

> @@ -120,6 +120,7 @@ Object *user_creatable_add_type(const char *type, const 
> char *id,
>  obj = object_new(type);
>  if (qdict) {
>  for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> +
>  object_property_set(obj, v, e->key, _err);
>  if (local_err) {
>  goto out;

Spurious hunk?


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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 01/10] qdict: implement a qdict_crumple method for un-flattening a dict

[Eric Blake]

On 03/10/2016 11:59 AM, Daniel P. Berrange wrote:
> The qdict_flatten() method will take a dict whose elements are
> further nested dicts/lists and flatten them by concatenating
> keys.
> 
> The qdict_crumple() method aims to do the reverse, taking a flat
> qdict, and turning it into a set of nested dicts/lists. It will
> apply nesting based on the key name, with a '.' indicating a
> new level in the hierarchy. If the keys in the nested structure
> are all numeric, it will create a list, otherwise it will create
> a dict.
> 

> 
> will get turned into a dict with one element 'foo' whose
> value is a list. The list elements will each in turn be
> dicts.
> 
>  {
>'foo' => [

s/=>/:/

>  { 'bar': 'one', 'wizz': '1' }

s/$/,/

>  { 'bar': 'two', 'wizz': '2' }
>],
>  }
> 

> The intent of this function is that it allows a set of QemuOpts
> to be turned into a nested data structure that mirrors the nested

s/the nested/the nesting/

> used when the same object is defined over QMP.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qmp/qdict.h |   1 +
>  qobject/qdict.c  | 267 
> +++
>  tests/check-qdict.c  | 143 +
>  3 files changed, 411 insertions(+)
> 
> +
> +/**
> + * qdict_split_flat_key:
> + *
> + * Given a flattened key such as 'foo.0.bar', split it
> + * into two parts at the first '.' separator. Allows
> + * double dot ('..') to escape the normal separator.
> + *
> + * eg
> + *'foo.0.bar' -> prefix='foo' and suffix='0.bar'
> + *'foo..0.bar' -> prefix='foo.0' and suffix='bar'
> + *
> + * The '..' sequence will be unescaped in the returned
> + * 'prefix' string. The 'suffix' string will be left
> + * in escaped format, so it can be fed back into the
> + * qdict_split_flat_key() key as the input later.
> + */

Might be worth mentioning that prefix and suffix must both be non-NULL,
and that the caller must g_free() the two resulting strings.

> +static void qdict_split_flat_key(const char *key, char **prefix, char 
> **suffix)
> +{
> +const char *separator;
> +size_t i, j;
> +
> +/* Find first '.' separator, but if there is a pair '..'
> + * that acts as an escape, so skip over '..' */
> +separator = NULL;
> +do {
> +if (separator) {
> +separator += 2;
> +} else {
> +separator = key;
> +}
> +separator = strchr(separator, '.');
> +} while (separator && *(separator + 1) == '.');

I'd probably have written separator[1] == '.', but your approach is
synonymous.

> +
> +if (separator) {
> +*prefix = g_strndup(key,
> +separator - key);
> +*suffix = g_strdup(separator + 1);
> +} else {
> +*prefix = g_strdup(key);
> +*suffix = NULL;
> +}
> +
> +/* Unescape the '..' sequence into '.' */
> +for (i = 0, j = 0; (*prefix)[i] != '\0'; i++, j++) {
> +if ((*prefix)[i] == '.' &&
> +(*prefix)[i + 1] == '.') {

Technically, if (*prefix)[i] == '.', we could assert((*prefix)[i + 1] ==
'.'), since the only way to get a '.' in prefix is via escaping.  For
that matter, you could short-circuit (part of) the loop by doing a
strchr for '.' (if not found, the loop is not needed; if found, start
the reduction at that point rather on the bytes leading up to that point).

> +i++;
> +}
> +(*prefix)[j] = (*prefix)[i];
> +}
> +(*prefix)[j] = '\0';
> +}
> +
> +
> +/**
> + * qdict_list_size:
> + * @maybe_List: dict that may be only list elements

s/List/list/

> + *
> + * Determine whether all keys in @maybe_list are
> + * valid list elements. They they are all valid,

s/They they/If they/

> + * then this returns the number of elements. If
> + * they all look like non-numeric keys, then returns
> + * zero. If there is a mix of numeric and non-numeric
> + * keys, then an error is set as it is both a list
> + * and a dict at once.
> + *
> + * Returns: number of list elemets, 0 if a dict, -1 on error

s/elemets/elements/

> + */
> +static ssize_t qdict_list_size(QDict *maybe_list, Error **errp)
> +{
> +const QDictEntry *entry, *next;
> +ssize_t len = 0;
> +ssize_t max = -1;
> +int is_list = -1;
> +int64_t val;
> +
> +entry = qdict_first(maybe_list);
> +while (entry != NULL) {
> +next = qdict_next(maybe_list, entry);
> +
> +if (qemu_strtoll(entry->key, NULL, 10, ) == 0) {
> +if (is_list == -1) {
> +is_list = 1;
> +} else if (!is_list) {
> +error_setg(errp,
> +   "Key '%s' is for a list, but previous key is "
> +   "for a dict", entry->key);

Keys are unsorted, so it's a bit hard to call it "previous key".  Maybe
a better error message would be along the lines of "cannot crumple
dictionary because of a mix of list and non-list keys"?  I dunno...

> +

Re: [Qemu-block] [Qemu-devel] [PATCH 06/22] hbitmap: load/store

[John Snow]

On 03/15/2016 04:04 PM, Vladimir Sementsov-Ogievskiy wrote:
> Add functions for load/store HBitmap to BDS, using clusters table:
> Last level of the bitmap is splitted into chunks of 'cluster_size'
> size. Each cell of the table contains offset in bds, to load/store
> corresponding chunk.
> 
> Also,
> 0 in cell means all-zeroes-chunk (should not be saved)
> 1 in cell means all-ones-chunk (should not be saved)
> hbitmap_prepare_store() fills table with
>   0 for all-zeroes chunks
>   1 for all-ones chunks
>   2 for others
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/dirty-bitmap.c |  23 +
>  include/block/dirty-bitmap.h |  11 +++
>  include/qemu/hbitmap.h   |  12 +++
>  util/hbitmap.c   | 209 
> +++
>  4 files changed, 255 insertions(+)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index e68c177..816c6ee 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -396,3 +396,26 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
>  {
>  return hbitmap_count(bitmap->bitmap);
>  }
> +
> +int bdrv_dirty_bitmap_load(BdrvDirtyBitmap *bitmap, BlockDriverState *bs,
> +   const uint64_t *table, uint32_t table_size,
> +   uint32_t cluster_size)
> +{
> +return hbitmap_load(bitmap->bitmap, bs, table, table_size, cluster_size);
> +}
> +
> +int bdrv_dirty_bitmap_prepare_store(const BdrvDirtyBitmap *bitmap,
> +uint32_t cluster_size,
> +uint64_t *table,
> +uint32_t *table_size)
> +{
> +return hbitmap_prepare_store(bitmap->bitmap, cluster_size,
> + table, table_size);
> +}
> +
> +int bdrv_dirty_bitmap_store(const BdrvDirtyBitmap *bitmap, BlockDriverState 
> *bs,
> +const uint64_t *table, uint32_t table_size,
> +uint32_t cluster_size)
> +{
> +return hbitmap_store(bitmap->bitmap, bs, table, table_size, 
> cluster_size);
> +}
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 27515af..20cb540 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -43,4 +43,15 @@ void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t 
> offset);
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>  
> +int bdrv_dirty_bitmap_load(BdrvDirtyBitmap *bitmap, BlockDriverState *bs,
> +   const uint64_t *table, uint32_t table_size,
> +   uint32_t cluster_size);
> +int bdrv_dirty_bitmap_prepare_store(const BdrvDirtyBitmap *bitmap,
> +uint32_t cluster_size,
> +uint64_t *table,
> +uint32_t *table_size);
> +int bdrv_dirty_bitmap_store(const BdrvDirtyBitmap *bitmap, BlockDriverState 
> *bs,
> +const uint64_t *table, uint32_t table_size,
> +uint32_t cluster_size);
> +
>  #endif
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index 6d1da4d..d83bb79 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -241,5 +241,17 @@ static inline size_t hbitmap_iter_next_word(HBitmapIter 
> *hbi, unsigned long *p_c
>  return hbi->pos;
>  }
>  
> +typedef struct BlockDriverState BlockDriverState;
> +
> +int hbitmap_load(HBitmap *bitmap, BlockDriverState *bs,
> + const uint64_t *table, uint32_t table_size,
> + uint32_t cluster_size);
> +
> +int hbitmap_prepare_store(const HBitmap *bitmap, uint32_t cluster_size,
> +  uint64_t *table, uint32_t *table_size);
> +
> +int hbitmap_store(HBitmap *bitmap, BlockDriverState *bs,
> +  const uint64_t *table, uint32_t table_size,
> +  uint32_t cluster_size);
>  
>  #endif
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 28595fb..1960e4f 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -15,6 +15,8 @@
>  #include "qemu/host-utils.h"
>  #include "trace.h"
>  
> +#include "block/block.h"
> +

This is a bit of a red flag -- we shouldn't need block layer specifics
in the subcomponent-agnostic HBitmap utility.

Further, by relying on these facilities here in hbitmap.c, "make check"
no longer can compile the relevant hbitmap tests.

Make sure that each intermediate commit here passes these necessary
tests, test-hbitmap in particular for each, and a "make check" overall
at the end of your series.

--js

>  /* HBitmaps provides an array of bits.  The bits are stored as usual in an
>   * array of unsigned longs, but HBitmap is also optimized to provide fast
>   * iteration over set bits; going from one bit to the next is O(logB n)
> @@ -499,3 

Re: [Qemu-block] [Qemu-devel] [PATCH] block: Remove bdrv_make_anon()

[Eric Blake]

On 03/18/2016 04:31 AM, Kevin Wolf wrote:
> The call in hmp_drive_del() is dead code because blk_remove_bs() is
> called a few lines above. The only other remaining user is
> bdrv_delete(), which only abuses bdrv_make_anon() to remove it from the
> named nodes list. This path inlines the list entry removal into
> bdrv_delete() and removes bdrv_make_anon().
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 15 +++
>  blockdev.c|  3 ---
>  include/block/block.h |  1 -
>  3 files changed, 3 insertions(+), 16 deletions(-)

Nice diffstat, as well.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v6 09/11] block: add support for encryption secrets in block I/O tests

[Eric Blake]

On 03/21/2016 08:11 AM, Daniel P. Berrange wrote:
> The LUKS block driver tests will require the ability to specify
> encryption secrets with block devices. This requires using the
> --object argument to qemu-img/qemu-io to create a 'secret'
> object.
> 
> When the IMGKEYSECRET env variable is set, it provides the
> password to be associated with a secret called 'keysec0'
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  tests/qemu-iotests/common|  1 +
>  tests/qemu-iotests/common.config |  6 ++
>  tests/qemu-iotests/common.filter |  3 ++-
>  tests/qemu-iotests/common.rc | 16 +---
>  4 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
> index 05c9df2..75ca4a7 100644
> --- a/tests/qemu-iotests/common
> +++ b/tests/qemu-iotests/common
> @@ -53,6 +53,7 @@ export QEMU_IO_OPTIONS=""
>  export CACHEMODE_IS_DEFAULT=true
>  export QEMU_OPTIONS="-nodefaults"
>  export VALGRIND_QEMU=
> +export IMGKEYSECRET=

As with the previous patch, maybe IMG_KEY_SECRET is more legible, and
fits in with other variable names.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v6 07/11] block: move encryption deprecation warning into qcow code

[Eric Blake]

On 03/21/2016 08:11 AM, Daniel P. Berrange wrote:
> or a couple of releases we have been warning

s/^or/For/

> 
>   Encrypted images are deprecated
>   Support for them will be removed in a future release.
>   You can use 'qemu-img convert' to convert your image to an unencrypted one.
> 
> This warning was issued by system emulators, qemu-img, qemu-nbd
> and qemu-io. Such a broad warning was issued because the original
> intention was to rip out all the code for dealing with encryption
> inside the QEMU block layer APIs.
> 
> The new block encryption framework used for the LUKS driver does
> not rely on the unloved block layer API for encryption keys,
> instead using the QOM 'secret' object type. It is thus no longer
> appropriate to warn about encryption unconditionally.
> 
> When the qcow/qcow2 drivers are converted to use the new encryption
> framework too, it will be practical to keep AES-CBC support present
> for use in qemu-img, qemu-io & qemu-nbd to allow for interoperability
> with older QEMU versions and liberation of data from existing encrypted
> qcow2 files.
> 
> This change moves the warning out of the generic block code and
> into the qcow/qcow2 drivers. Further, the warning is set to only
> appear when running the system emulators, since qemu-img, qemu-io,
> qemu-nbd are expected to support qcow2 encryption long term now that
> the maint burden has been eliminated.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  block.c| 12 +---
>  block/qcow.c   |  9 +
>  block/qcow2.c  |  8 
>  include/block/block.h  |  1 +
>  tests/qemu-iotests/049.out |  6 --
>  tests/qemu-iotests/087 |  3 ++-
>  tests/qemu-iotests/087.out | 26 --
>  tests/qemu-iotests/134.out | 18 --
>  8 files changed, 33 insertions(+), 50 deletions(-)
> 

With the typo fix,
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] vpc: use current_size field for XenServer VHD images

[Spencer Baugh]

Stefan Hajnoczi  writes:
> What output did you get from "qemu-img info /dev/dm-1"?

After the patching:

root@storage-4:~# hexdump -C -n 512 /dev/vgstorage/tartanfsbackup
  63 6f 6e 65 63 74 69 78  00 00 00 02 00 01 00 00 |conectix|
0010  00 00 00 00 00 00 02 00  14 10 4b 1f 71 65 6d 32 |..K.qem2|
0020  00 01 00 03 00 00 00 00  00 00 01 c4 4f 40 00 00 |O@..|
0030  00 00 01 f4 00 00 00 00  ff ff 10 ff 00 00 00 03 ||
0040  ff ff ed 81 6d 0d 68 bd  9c fd 4d 65 a3 17 c7 6c |m.h...Me...l|
0050  06 a6 aa bf 00 00 00 00  00 00 00 00 00 00 00 00 ||
0060  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 ||
*
0200
root@storage-4:~# qemu-img info /dev/vgstorage/tartanfsbackup
block-vpc: The header checksum of '/dev/vgstorage/tartanfsbackup' is incorrect.
qemu-img: Could not open '/dev/vgstorage/tartanfsbackup': Could not open 
'/dev/vgstorage/tartanfsbackup': Invalid argument

> Did qemu-nbd work?

root@storage-4:~# qemu-nbd -c /dev/nbd1 /dev/vgstorage/tartanfsbackup
block-vpc: The header checksum of '/dev/vgstorage/tartanfsbackup' is incorrect.
qemu-nbd: Failed to blk_new_open '/dev/vgstorage/tartanfsbackup': Could not 
open '/dev/vgstorage/tartanfsbackup': Invalid argument

Re: [Qemu-block] [PATCH v6 00/11] Add new LUKS block driver (for 2.6)

[Daniel P. Berrange]

On Mon, Mar 21, 2016 at 05:50:13PM +0100, Kevin Wolf wrote:
> Am 21.03.2016 um 15:11 hat Daniel P. Berrange geschrieben:
> > This series is just the block layer parts needed to add
> > a LUKS driver to QEMU. It was previously posted as part
> > of the larger series
> > 
> >   v1: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04748.html
> >   v2: https://lists.gnu.org/archive/html/qemu-block/2016-01/msg00534.html
> >   v3: https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03176.html
> >   v4: https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06552.html
> >   v5: https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04397.html
> > 
> > The crypto subsystem pieces that this series depends on have now
> > all been merged.
> > 
> > In this posting I am only proposing merge of the generic LUKS
> > driver. This can be added as a layer above any of the existing
> > drivers for accessing LUKS formatted volumes.
> > 
> > When creating new volumes, however, only the file backend can
> > be used, since the block driver API doesn't allow for arbitrary
> > stacking of protocols when creating images.
> > 
> > The integration with the qcow2 driver to replace the existing
> > built-in AES-CBC code is dropped from this series, postponed
> > until the 2.7 development cycle.
> > 
> > There is a QEMU I/O test 149 that exercises the LUKS driver
> > and checks for compatibility with the dm-crypt/cryptsetup
> > impl.
> 
> Reviewed-by: Kevin Wolf 
> 
> I can't seem to test this, however, as long as git master doesn't
> compile on RHEL 7 without --disable-nettle. Any news on that?

Apply this pending PULL request and you should be sorted

https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04874.html

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-block] [PATCH v6 00/11] Add new LUKS block driver (for 2.6)

[Kevin Wolf]

Am 21.03.2016 um 15:11 hat Daniel P. Berrange geschrieben:
> This series is just the block layer parts needed to add
> a LUKS driver to QEMU. It was previously posted as part
> of the larger series
> 
>   v1: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04748.html
>   v2: https://lists.gnu.org/archive/html/qemu-block/2016-01/msg00534.html
>   v3: https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03176.html
>   v4: https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06552.html
>   v5: https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04397.html
> 
> The crypto subsystem pieces that this series depends on have now
> all been merged.
> 
> In this posting I am only proposing merge of the generic LUKS
> driver. This can be added as a layer above any of the existing
> drivers for accessing LUKS formatted volumes.
> 
> When creating new volumes, however, only the file backend can
> be used, since the block driver API doesn't allow for arbitrary
> stacking of protocols when creating images.
> 
> The integration with the qcow2 driver to replace the existing
> built-in AES-CBC code is dropped from this series, postponed
> until the 2.7 development cycle.
> 
> There is a QEMU I/O test 149 that exercises the LUKS driver
> and checks for compatibility with the dm-crypt/cryptsetup
> impl.

Reviewed-by: Kevin Wolf 

I can't seem to test this, however, as long as git master doesn't
compile on RHEL 7 without --disable-nettle. Any news on that?

Kevin

Re: [Qemu-block] [PATCH v2] Replaced get_tick_per_sec() by NANOSECONDS_PER_SECOND

[Paolo Bonzini]

On 21/03/2016 17:02, rutu.shah...@gmail.com wrote:
> 
> Hi,
> This patch replaces get_ticks_per_sec() calls to NANOSECONDS_PER_SECOND. 
> Also, as there are no callers, get_ticks_per_sec() has been removed. 
> Replacement imporves readability and understandability of code.
> 
> Example given by Paolo Bonzini,
> 
> timer_mod(fdctrl->result_timer, 
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + (get_ticks_per_sec() / 50));
> 
> NANOSECONDS_PER_SECOND makes it obvious that the timer will expire in 1/50th 
> of a second.

Looks good.  I've edited the commit message as follows:


This patch replaces get_ticks_per_sec() calls with the macro
NANOSECONDS_PER_SECOND. Also, as there are no callers, get_ticks_per_sec()
is then removed.  This replacement improves the readability and
understandability of code.

For example,

timer_mod(fdctrl->result_timer,
  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + (get_ticks_per_sec() / 
50));

NANOSECONDS_PER_SECOND makes it obvious that qemu_clock_get_ns
matches the unit of the expression on the right side of the plus.

[Qemu-block] [PATCH v2] Replaced get_tick_per_sec() by NANOSECONDS_PER_SECOND

[rutu . shah . 26]

From: Rutuja Shah 

Hi,
This patch replaces get_ticks_per_sec() calls to NANOSECONDS_PER_SECOND. Also, 
as there are no callers, get_ticks_per_sec() has been removed. Replacement 
imporves readability and understandability of code.

Example given by Paolo Bonzini,

timer_mod(fdctrl->result_timer, 
  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + (get_ticks_per_sec() / 50));

NANOSECONDS_PER_SECOND makes it obvious that the timer will expire in 1/50th of 
a second.

Signed-off-by: Rutuja Shah 

---
 audio/audio.c |  3 +--
 audio/noaudio.c   |  8 
 audio/spiceaudio.c|  4 ++--
 audio/wavaudio.c  |  2 +-
 backends/baum.c   |  2 +-
 block/qed.c   |  2 +-
 cpus.c|  6 +++---
 hw/acpi/core.c|  4 ++--
 hw/arm/omap1.c| 17 +
 hw/arm/spitz.c|  2 +-
 hw/arm/stellaris.c|  2 +-
 hw/arm/strongarm.c|  2 +-
 hw/audio/adlib.c  |  2 +-
 hw/audio/sb16.c   |  4 ++--
 hw/block/fdc.c|  4 ++--
 hw/block/pflash_cfi02.c   |  8 
 hw/bt/hci-csr.c   |  4 ++--
 hw/char/cadence_uart.c|  4 ++--
 hw/char/serial.c  | 10 ++
 hw/display/vga.c  |  6 +++---
 hw/dma/rc4030.c   |  2 +-
 hw/ide/core.c |  4 ++--
 hw/input/hid.c|  2 +-
 hw/input/tsc2005.c|  3 ++-
 hw/input/tsc210x.c|  3 ++-
 hw/intc/i8259.c   |  2 +-
 hw/misc/arm_sysctl.c  |  3 ++-
 hw/misc/macio/cuda.c  | 16 
 hw/misc/macio/macio.c |  2 +-
 hw/net/dp8393x.c  |  2 +-
 hw/ppc/ppc.c  | 21 -
 hw/ppc/ppc405_uc.c|  4 ++--
 hw/ppc/ppc_booke.c|  2 +-
 hw/sd/sdhci-internal.h|  2 +-
 hw/sparc64/sun4u.c|  4 ++--
 hw/timer/i8254.c  |  4 ++--
 hw/timer/i8254_common.c   |  6 +++---
 hw/timer/mc146818rtc.c| 14 --
 hw/timer/omap_gptimer.c   |  2 +-
 hw/timer/omap_synctimer.c |  3 ++-
 hw/timer/pl031.c  | 10 +-
 hw/timer/pxa2xx_timer.c   | 18 ++
 hw/usb/hcd-ehci.c |  5 +++--
 hw/usb/hcd-musb.c |  2 +-
 hw/usb/hcd-ohci.c | 10 +-
 hw/usb/hcd-uhci.c |  6 +++---
 hw/usb/tusb6010.c |  6 +++---
 hw/watchdog/wdt_diag288.c |  2 +-
 hw/watchdog/wdt_ib700.c   |  2 +-
 include/hw/acpi/acpi.h|  2 +-
 include/qemu/timer.h  |  9 ++---
 monitor.c |  4 ++--
 target-ppc/kvm.c  |  4 ++--
 53 files changed, 143 insertions(+), 134 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index e841532..ab0ade8 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1869,8 +1869,7 @@ static void audio_init (void)
 }
 conf.period.ticks = 1;
 } else {
-conf.period.ticks =
-muldiv64 (1, get_ticks_per_sec (), conf.period.hertz);
+conf.period.ticks = NANOSECONDS_PER_SECOND / conf.period.hertz;
 }
 
 e = qemu_add_vm_change_state_handler (audio_vm_change_state_handler, s);
diff --git a/audio/noaudio.c b/audio/noaudio.c
index 09588b9..b360c19 100644
--- a/audio/noaudio.c
+++ b/audio/noaudio.c
@@ -49,8 +49,8 @@ static int no_run_out (HWVoiceOut *hw, int live)
 
 now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 ticks = now - no->old_ticks;
-bytes = muldiv64 (ticks, hw->info.bytes_per_second, get_ticks_per_sec ());
-bytes = audio_MIN (bytes, INT_MAX);
+bytes = muldiv64(ticks, hw->info.bytes_per_second, NANOSECONDS_PER_SECOND);
+bytes = audio_MIN(bytes, INT_MAX);
 samples = bytes >> hw->info.shift;
 
 no->old_ticks = now;
@@ -61,7 +61,7 @@ static int no_run_out (HWVoiceOut *hw, int live)
 
 static int no_write (SWVoiceOut *sw, void *buf, int len)
 {
-return audio_pcm_sw_write (sw, buf, len);
+return audio_pcm_sw_write(sw, buf, len);
 }
 
 static int no_init_out(HWVoiceOut *hw, struct audsettings *as, void 
*drv_opaque)
@@ -106,7 +106,7 @@ static int no_run_in (HWVoiceIn *hw)
 int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 int64_t ticks = now - no->old_ticks;
 int64_t bytes =
-muldiv64 (ticks, hw->info.bytes_per_second, get_ticks_per_sec ());
+muldiv64(ticks, hw->info.bytes_per_second, NANOSECONDS_PER_SECOND);
 
 no->old_ticks = now;
 bytes = audio_MIN (bytes, INT_MAX);
diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
index 297fd41..dea71d3 100644
--- a/audio/spiceaudio.c
+++ b/audio/spiceaudio.c
@@ -104,11 +104,11 @@ static int rate_get_samples (struct audio_pcm_info *info, 
SpiceRateCtl *rate)
 
 now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 ticks = now - rate->start_ticks;
-bytes = muldiv64 (ticks, info->bytes_per_second, get_ticks_per_sec ());
+bytes = muldiv64(ticks, info->bytes_per_second, NANOSECONDS_PER_SECOND);
 samples = (bytes - rate->bytes_sent) >> info->shift;
 if (samples < 0 || 

Re: [Qemu-block] ping [PATCH v15] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Kevin Wolf]

Am 21.03.2016 um 16:41 hat Programmingkid geschrieben:
> http://patchwork.ozlabs.org/patch/591171/
> 
> Mac OS X can be picky when it comes to allowing the user
> to use physical devices in QEMU. Most mounted volumes
> appear to be off limits to QEMU. If an issue is detected,
> a message is displayed showing the user how to unmount a
> volume. Now QEMU uses both CD and DVD media.
> 
> Signed-off-by: John Arbuckle 

Thanks, applied to the block branch.

Kevin

[Qemu-block] ping [PATCH v15] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

http://patchwork.ozlabs.org/patch/591171/

Mac OS X can be picky when it comes to allowing the user
to use physical devices in QEMU. Most mounted volumes
appear to be off limits to QEMU. If an issue is detected,
a message is displayed showing the user how to unmount a
volume. Now QEMU uses both CD and DVD media.

Signed-off-by: John Arbuckle 

---
Replaced one "return -1" with "return -ENOENT".
Replaced another "return -1" with "return ret".
Removed trailing whitespace.

 block/raw-posix.c | 165 +-
 1 file changed, 126 insertions(+), 39 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 8866121..eb50c02 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -44,6 +44,7 @@
 #include 
 #include 
 //#include 
+#include 
 #include 
 #endif
 
@@ -1965,33 +1966,47 @@ BlockDriver bdrv_file = {
 /* host device */
 
 #if defined(__APPLE__) && defined(__MACH__)
-static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
 static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
 CFIndex maxPathSize, int flags);
-kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
+static char *FindEjectableOpticalMedia(io_iterator_t *mediaIterator)
 {
-kern_return_t   kernResult;
+kern_return_t kernResult = KERN_FAILURE;
 mach_port_t masterPort;
 CFMutableDictionaryRef  classesToMatch;
+const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
+char *mediaType = NULL;
 
 kernResult = IOMasterPort( MACH_PORT_NULL,  );
 if ( KERN_SUCCESS != kernResult ) {
 printf( "IOMasterPort returned %d\n", kernResult );
 }
 
-classesToMatch = IOServiceMatching( kIOCDMediaClass );
-if ( classesToMatch == NULL ) {
-printf( "IOServiceMatching returned a NULL dictionary.\n" );
-} else {
-CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ), 
kCFBooleanTrue );
-}
-kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch, 
mediaIterator );
-if ( KERN_SUCCESS != kernResult )
-{
-printf( "IOServiceGetMatchingServices returned %d\n", kernResult );
-}
+int index;
+for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
+classesToMatch = IOServiceMatching(matching_array[index]);
+if (classesToMatch == NULL) {
+error_report("IOServiceMatching returned NULL for %s",
+ matching_array[index]);
+continue;
+}
+CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
+ kCFBooleanTrue);
+kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
+  mediaIterator);
+if (kernResult != KERN_SUCCESS) {
+error_report("Note: IOServiceGetMatchingServices returned %d",
+ kernResult);
+continue;
+}
 
-return kernResult;
+/* If a match was found, leave the loop */
+if (*mediaIterator != 0) {
+DPRINTF("Matching using %s\n", matching_array[index]);
+mediaType = g_strdup(matching_array[index]);
+break;
+}
+}
+return mediaType;
 }
 
 kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
@@ -2023,7 +2038,46 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator, 
char *bsdPath,
 return kernResult;
 }
 
-#endif
+/* Sets up a real cdrom for use in QEMU */
+static bool setup_cdrom(char *bsd_path, Error **errp)
+{
+int index, num_of_test_partitions = 2, fd;
+char test_partition[MAXPATHLEN];
+bool partition_found = false;
+
+/* look for a working partition */
+for (index = 0; index < num_of_test_partitions; index++) {
+snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
+ index);
+fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
+if (fd >= 0) {
+partition_found = true;
+qemu_close(fd);
+break;
+}
+}
+
+/* if a working partition on the device was not found */
+if (partition_found == false) {
+error_setg(errp, "Failed to find a working partition on disc");
+} else {
+DPRINTF("Using %s as optical disc\n", test_partition);
+pstrcpy(bsd_path, MAXPATHLEN, test_partition);
+}
+return partition_found;
+}
+
+/* Prints directions on mounting and unmounting a device */
+static void print_unmounting_directions(const char *file_name)
+{
+error_report("If device %s is mounted on the desktop, unmount"
+ " it first before using it in QEMU", file_name);
+error_report("Command to unmount device: diskutil unmountDisk %s",
+ file_name);
+error_report("Command to mount device: diskutil mountDisk %s", file_name);

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Mon, 21 Mar 2016 14:54:04 +0100
Paolo Bonzini  wrote:

> On 21/03/2016 14:47, TU BO wrote:
> >> I'll see if I can produce something based on Conny's patches, which are
> >> already a start.  Today I had a short day so I couldn't play with the
> >> bug; out of curiosity, does the bug reproduce with her work + patch 4
> >> from this series + the reentrancy assertion?
> > I did NOT see crash with qemu master + "[PATCH RFC 0/6] virtio: refactor
> > host notifiers" from Conny + patch 4 + assertion.  thx
> 
> That's unexpected, but I guess it only says that I didn't review her
> patches well enough. :)

I'm also a bit surprised, the only thing that should really be
different is passing the 'assign' argument in stop_ioeventfd(). Any
other fixes are purely accidental :)

Would be interesting to see how this setup fares with virtio-pci.

[Qemu-block] [PATCH v6 11/11] block: an interoperability test for luks vs dm-crypt/cryptsetup

[Daniel P. Berrange]

It is important that the QEMU luks implementation retains 100%
compatibility with the reference implementation provided by
the combination of the linux kernel dm-crypt module and cryptsetup
userspace tools.

There is a matrix of tests to be performed with different sets
of encryption settings. For each matrix entry, two tests will
be performed. One will create a LUKS image with the cryptsetup
tool and then do I/O with both cryptsetup & qemu-io. The other
will create the image with qemu-img and then again do I/O with
both cryptsetup and qemu-io.

The new I/O test 149 performs interoperability testing between
QEMU and the reference implementation. Such testing inherantly
requires elevated privileges, so to this this the user must have
configured passwordless sudo access. The test will automatically
skip if sudo is not available.

The test has to be run explicitly thus:

cd tests/qemu-iotests
./check -luks 149

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/149 |  519 
 tests/qemu-iotests/149.out | 1880 
 tests/qemu-iotests/common  |1 +
 tests/qemu-iotests/group   |1 +
 4 files changed, 2401 insertions(+)
 create mode 100755 tests/qemu-iotests/149
 create mode 100644 tests/qemu-iotests/149.out

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
new file mode 100755
index 000..bb5811d
--- /dev/null
+++ b/tests/qemu-iotests/149
@@ -0,0 +1,519 @@
+#!/usr/bin/python
+#
+# Copyright (C) 2016 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: Daniel P. Berrange 
+#
+# Exercise the QEMU 'luks' block driver to validate interoperability
+# with the Linux dm-crypt + cryptsetup implementation
+
+import subprocess
+import os
+import os.path
+
+import base64
+
+import iotests
+
+
+class LUKSConfig(object):
+"""Represent configuration parameters for a single LUKS
+   setup to be tested"""
+
+def __init__(self, name, cipher, keylen, mode, ivgen,
+ ivgen_hash, hash, password=None, passwords=None):
+
+self.name = name
+self.cipher = cipher
+self.keylen = keylen
+self.mode = mode
+self.ivgen = ivgen
+self.ivgen_hash = ivgen_hash
+self.hash = hash
+
+if passwords is not None:
+self.passwords = passwords
+else:
+self.passwords = {}
+
+if password is None:
+self.passwords["0"] = "123456"
+else:
+self.passwords["0"] = password
+
+def __repr__(self):
+return self.name
+
+def image_name(self):
+return "luks-%s.img" % self.name
+
+def image_path(self):
+return os.path.join(iotests.test_dir, self.image_name())
+
+def device_name(self):
+return "qiotest-145-%s" % self.name
+
+def device_path(self):
+return "/dev/mapper/" + self.device_name()
+
+def first_password(self):
+for i in range(8):
+slot = str(i)
+if slot in self.passwords:
+return (self.passwords[slot], slot)
+raise Exception("No password found")
+
+def first_password_base64(self):
+(pw, slot) = self.first_password()
+return base64.b64encode(pw)
+
+def active_slots(self):
+slots = []
+for i in range(8):
+slot = str(i)
+if slot in self.passwords:
+slots.append(slot)
+return slots
+
+def verify_passwordless_sudo():
+"""Check whether sudo is configured to allow
+   password-less access to commands"""
+
+args = ["sudo", "-n", "/bin/true"]
+
+proc = subprocess.Popen(args,
+stdin=subprocess.PIPE,
+stdout=subprocess.PIPE,
+stderr=subprocess.STDOUT)
+
+msg = proc.communicate()[0]
+
+if proc.returncode != 0:
+iotests.notrun('requires password-less sudo access: %s' % msg)
+
+
+def cryptsetup(args, password=None):
+"""Run the cryptsetup command in batch mode"""
+
+fullargs = ["sudo", "cryptsetup", "-q", "-v"]
+fullargs.extend(args)
+
+iotests.log(" ".join(fullargs), filters=[iotests.filter_test_dir])
+proc = subprocess.Popen(fullargs,
+

[Qemu-block] [PATCH v6 03/11] tests: redirect stderr to stdout for iotests

[Daniel P. Berrange]

The python I/O tests helper for running qemu-img/qemu-io
setup stdout to be captured to a pipe, but left stderr
untouched. As a result, if something failed in qemu-img/
qemu-io, data written to stderr would get output directly
and not line up with data on the test stdout due to
buffering.  If we explicitly redirect stderr to the same
pipe as stdout, things are much clearer when they go
wrong.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/iotests.py | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0a238ec..5f82bbe 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -71,7 +71,9 @@ def qemu_img_verbose(*args):
 
 def qemu_img_pipe(*args):
 '''Run qemu-img and return its output'''
-subp = subprocess.Popen(qemu_img_args + list(args), stdout=subprocess.PIPE)
+subp = subprocess.Popen(qemu_img_args + list(args),
+stdout=subprocess.PIPE,
+stderr=subprocess.STDOUT)
 exitcode = subp.wait()
 if exitcode < 0:
 sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
@@ -80,7 +82,8 @@ def qemu_img_pipe(*args):
 def qemu_io(*args):
 '''Run qemu-io and return the stdout data'''
 args = qemu_io_args + list(args)
-subp = subprocess.Popen(args, stdout=subprocess.PIPE)
+subp = subprocess.Popen(args, stdout=subprocess.PIPE,
+stderr=subprocess.STDOUT)
 exitcode = subp.wait()
 if exitcode < 0:
 sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' 
'.join(args)))
-- 
2.5.0

[Qemu-block] [PATCH v6 06/11] block: add generic full disk encryption driver

[Daniel P. Berrange]

Add a block driver that is capable of supporting any full disk
encryption format. This utilizes the previously added block
encryption code, and at this time supports the LUKS format.

The driver code is capable of supporting any format supported
by the QCryptoBlock module, so it registers one block driver
for each format. This patch only registers the "luks" driver
since the "qcow" driver is there only for back-compatibility
with existing qcow built-in encryption.

New LUKS compatible volumes can be formatted using qemu-img
with defaults for all settings.

$ qemu-img create --object secret,data=123456,id=sec0 \
  -f luks -o key-secret=sec0 demo.luks 10G

Alternatively the cryptographic settings can be explicitly
set

$ qemu-img create --object secret,data=123456,id=sec0 \
  -f luks -o key-secret=sec0,cipher-alg=aes-256,\
 cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha256 \
  demo.luks 10G

And query its size

$ qemu-img info demo.img
image: demo.img
file format: luks
virtual size: 10G (10737418240 bytes)
disk size: 132K
encrypted: yes

Note that it was not necessary to provide the password
when querying info for the volume. The password is only
required when performing I/O on the volume

All volumes created by this new 'luks' driver should be
capable of being opened by the kernel dm-crypt driver.

The only algorithms listed in the LUKS spec that are
not currently supported by this impl are sha512 and
ripemd160 hashes and cast6 cipher.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 block/Makefile.objs  |   2 +
 block/crypto.c   | 586 +++
 qapi/block-core.json |  22 +-
 3 files changed, 608 insertions(+), 2 deletions(-)
 create mode 100644 block/crypto.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index cdd8655..3426a15 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -23,6 +23,8 @@ block-obj-$(CONFIG_LIBSSH2) += ssh.o
 block-obj-y += accounting.o dirty-bitmap.o
 block-obj-y += write-threshold.o
 
+block-obj-y += crypto.o
+
 common-obj-y += stream.o
 common-obj-y += commit.o
 common-obj-y += backup.o
diff --git a/block/crypto.c b/block/crypto.c
new file mode 100644
index 000..e3467d5
--- /dev/null
+++ b/block/crypto.c
@@ -0,0 +1,586 @@
+/*
+ * QEMU block full disk encryption
+ *
+ * Copyright (c) 2015-2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see .
+ *
+ */
+
+#include "qemu/osdep.h"
+
+#include "block/block_int.h"
+#include "sysemu/block-backend.h"
+#include "crypto/block.h"
+#include "qapi/opts-visitor.h"
+#include "qapi-visit.h"
+
+#define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret"
+#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg"
+#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode"
+#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG "ivgen-alg"
+#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg"
+#define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"
+
+typedef struct BlockCrypto BlockCrypto;
+
+struct BlockCrypto {
+QCryptoBlock *block;
+};
+
+
+static int block_crypto_probe_generic(QCryptoBlockFormat format,
+  const uint8_t *buf,
+  int buf_size,
+  const char *filename)
+{
+if (qcrypto_block_has_format(format, buf, buf_size)) {
+return 100;
+} else {
+return 0;
+}
+}
+
+
+static ssize_t block_crypto_read_func(QCryptoBlock *block,
+  size_t offset,
+  uint8_t *buf,
+  size_t buflen,
+  Error **errp,
+  void *opaque)
+{
+BlockDriverState *bs = opaque;
+ssize_t ret;
+
+ret = bdrv_pread(bs->file->bs, offset, buf, buflen);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Could not read encryption header");
+return ret;
+}
+return ret;
+}
+
+
+struct BlockCryptoCreateData {
+const char *filename;
+QemuOpts *opts;
+BlockBackend *blk;
+uint64_t size;
+};
+
+
+static ssize_t block_crypto_write_func(QCryptoBlock *block,
+   size_t offset,
+   const uint8_t 

[Qemu-block] [PATCH v6 07/11] block: move encryption deprecation warning into qcow code

[Daniel P. Berrange]

or a couple of releases we have been warning

  Encrypted images are deprecated
  Support for them will be removed in a future release.
  You can use 'qemu-img convert' to convert your image to an unencrypted one.

This warning was issued by system emulators, qemu-img, qemu-nbd
and qemu-io. Such a broad warning was issued because the original
intention was to rip out all the code for dealing with encryption
inside the QEMU block layer APIs.

The new block encryption framework used for the LUKS driver does
not rely on the unloved block layer API for encryption keys,
instead using the QOM 'secret' object type. It is thus no longer
appropriate to warn about encryption unconditionally.

When the qcow/qcow2 drivers are converted to use the new encryption
framework too, it will be practical to keep AES-CBC support present
for use in qemu-img, qemu-io & qemu-nbd to allow for interoperability
with older QEMU versions and liberation of data from existing encrypted
qcow2 files.

This change moves the warning out of the generic block code and
into the qcow/qcow2 drivers. Further, the warning is set to only
appear when running the system emulators, since qemu-img, qemu-io,
qemu-nbd are expected to support qcow2 encryption long term now that
the maint burden has been eliminated.

Signed-off-by: Daniel P. Berrange 
---
 block.c| 12 +---
 block/qcow.c   |  9 +
 block/qcow2.c  |  8 
 include/block/block.h  |  1 +
 tests/qemu-iotests/049.out |  6 --
 tests/qemu-iotests/087 |  3 ++-
 tests/qemu-iotests/087.out | 26 --
 tests/qemu-iotests/134.out | 18 --
 8 files changed, 33 insertions(+), 50 deletions(-)

diff --git a/block.c b/block.c
index 97787b3..217c708 100644
--- a/block.c
+++ b/block.c
@@ -288,6 +288,11 @@ static int bdrv_is_whitelisted(BlockDriver *drv, bool 
read_only)
 return 0;
 }
 
+bool bdrv_uses_whitelist(void)
+{
+return use_bdrv_whitelist;
+}
+
 typedef struct CreateCo {
 BlockDriver *drv;
 char *filename;
@@ -1004,13 +1009,6 @@ static int bdrv_open_common(BlockDriverState *bs, 
BdrvChild *file,
 goto free_and_fail;
 }
 
-if (bs->encrypted) {
-error_report("Encrypted images are deprecated");
-error_printf("Support for them will be removed in a future release.\n"
- "You can use 'qemu-img convert' to convert your image"
- " to an unencrypted one.\n");
-}
-
 ret = refresh_total_sectors(bs, bs->total_sectors);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "Could not refresh total sector count");
diff --git a/block/qcow.c b/block/qcow.c
index 73cf8a7..a98d819 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -23,6 +23,7 @@
  */
 #include "qemu/osdep.h"
 #include "qemu-common.h"
+#include "qemu/error-report.h"
 #include "block/block_int.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
@@ -157,6 +158,14 @@ static int qcow_open(BlockDriverState *bs, QDict *options, 
int flags,
 }
 s->crypt_method_header = header.crypt_method;
 if (s->crypt_method_header) {
+if (bdrv_uses_whitelist() &&
+s->crypt_method_header == QCOW_CRYPT_AES) {
+error_report("qcow built-in AES encryption is deprecated");
+error_printf("Support for it will be removed in a future 
release.\n"
+ "You can use 'qemu-img convert' to switch to an\n"
+ "unencrypted qcow image, or a LUKS raw image.\n");
+}
+
 bs->encrypted = 1;
 }
 s->cluster_bits = header.cluster_bits;
diff --git a/block/qcow2.c b/block/qcow2.c
index cec5bd0..dad7322 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -965,6 +965,14 @@ static int qcow2_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 s->crypt_method_header = header.crypt_method;
 if (s->crypt_method_header) {
+if (bdrv_uses_whitelist() &&
+s->crypt_method_header == QCOW_CRYPT_AES) {
+error_report("qcow2 built-in AES encryption is deprecated");
+error_printf("Support for it will be removed in a future 
release.\n"
+ "You can use 'qemu-img convert' to switch to an\n"
+ "unencrypted qcow2 image, or a LUKS raw image.\n");
+}
+
 bs->encrypted = 1;
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index d8945ff..72934fb 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -193,6 +193,7 @@ void bdrv_io_limits_update_group(BlockDriverState *bs, 
const char *group);
 
 void bdrv_init(void);
 void bdrv_init_with_whitelist(void);
+bool bdrv_uses_whitelist(void);
 BlockDriver *bdrv_find_protocol(const char *filename,
 bool allow_protocol_prefix,
 Error **errp);
diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out

[Qemu-block] [PATCH v6 08/11] block: add support for --image-opts in block I/O tests

[Daniel P. Berrange]

Currently all block tests use the traditional syntax for images
just specifying a filename. To support the LUKS driver without
resorting to JSON, the tests need to be able to use the new
--image-opts argument to qemu-img and qemu-io.

This introduces a new env variable IMGOPTSSYNTAX. If this is
set to 'true', then qemu-img/qemu-io should use --image-opts.

Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/common|  7 -
 tests/qemu-iotests/common.config | 15 +--
 tests/qemu-iotests/common.rc | 58 +---
 3 files changed, 62 insertions(+), 18 deletions(-)

diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index ff84f4b..05c9df2 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -53,6 +53,7 @@ export QEMU_IO_OPTIONS=""
 export CACHEMODE_IS_DEFAULT=true
 export QEMU_OPTIONS="-nodefaults"
 export VALGRIND_QEMU=
+export IMGOPTSSYNTAX=false
 
 for r
 do
@@ -398,7 +399,11 @@ BEGIN{ for (t='$start'; t<='$end'; t++) printf 
"%03d\n",t }' \
 done
 
 # Set qemu-io cache mode with $CACHEMODE we have
-QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS -f $IMGFMT --cache $CACHEMODE"
+if [ "$IMGOPTSSYNTAX" = "true" ]; then
+QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS --cache $CACHEMODE"
+else
+QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS -f $IMGFMT --cache $CACHEMODE"
+fi
 
 # Set default options for qemu-img create -o if they were not specified
 _set_default_imgopts
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 60bfabf..6d4c829 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -123,12 +123,16 @@ _qemu_img_wrapper()
 _qemu_io_wrapper()
 {
 local VALGRIND_LOGFILE=/tmp/$$.valgrind
+local QEMU_IO_ARGS="$QEMU_IO_OPTIONS"
+if [ "$IMGOPTSSYNTAX" = "true" ]; then
+QEMU_IO_ARGS="--image-opts $QEMU_IO_ARGS"
+fi
 local RETVAL
 (
 if [ "${VALGRIND_QEMU}" == "y" ]; then
-exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
"$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@"
+exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
"$QEMU_IO_PROG" $QEMU_IO_ARGS "$@"
 else
-exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@"
+exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@"
 fi
 )
 RETVAL=$?
@@ -154,6 +158,13 @@ export QEMU_IMG=_qemu_img_wrapper
 export QEMU_IO=_qemu_io_wrapper
 export QEMU_NBD=_qemu_nbd_wrapper
 
+QEMU_IMG_EXTRA_ARGS=
+if [ "$IMGOPTSSYNTAX" = "true" ]; then
+QEMU_IMG_EXTRA_ARGS="--image-opts $QEMU_IMG_EXTRA_ARGS"
+fi
+export QEMU_IMG_EXTRA_ARGS
+
+
 default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p')
 default_alias_machine=$($QEMU -machine help | \
sed -n "/(alias of $default_machine)/ { s/ .*//p; q; }")
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index d9913f8..5eb654b 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,21 +53,43 @@ fi
 # make sure we have a standard umask
 umask 022
 
-if [ "$IMGPROTO" = "file" ]; then
-TEST_IMG=$TEST_DIR/t.$IMGFMT
-elif [ "$IMGPROTO" = "nbd" ]; then
-TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
-TEST_IMG="nbd:127.0.0.1:10810"
-elif [ "$IMGPROTO" = "ssh" ]; then
-TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
-TEST_IMG="ssh://127.0.0.1$TEST_IMG_FILE"
-elif [ "$IMGPROTO" = "nfs" ]; then
-TEST_DIR="nfs://127.0.0.1/$TEST_DIR"
-TEST_IMG=$TEST_DIR/t.$IMGFMT
-elif [ "$IMGPROTO" = "archipelago" ]; then
-TEST_IMG="archipelago:at.$IMGFMT"
+if [ "$IMGOPTSSYNTAX" = "true" ]; then
+DRIVER="driver=$IMGFMT"
+if [ "$IMGPROTO" = "file" ]; then
+TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
+TEST_IMG="$DRIVER,file.filename=$TEST_DIR/t.$IMGFMT"
+elif [ "$IMGPROTO" = "nbd" ]; then
+TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
+TEST_IMG="$DRIVER,file.driver=nbd,file.host=127.0.0.1,file.port=10810"
+elif [ "$IMGPROTO" = "ssh" ]; then
+TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
+
TEST_IMG="$DRIVER,file.driver=ssh,file.host=127.0.0.1,file.path=$TEST_IMG_FILE"
+elif [ "$IMGPROTO" = "nfs" ]; then
+
TEST_DIR="$DRIVER,file.driver=nfs,file.filename=nfs://127.0.0.1/$TEST_DIR"
+TEST_IMG=$TEST_DIR_OPTS/t.$IMGFMT
+elif [ "$IMGPROTO" = "archipelago" ]; then
+TEST_IMG="$DRIVER,file.driver=archipelago,file.volume=:at.$IMGFMT"
+else
+
TEST_IMG="$DRIVER,file.driver=$IMGPROTO,file.filename=$TEST_DIR/t.$IMGFMT"
+fi
 else
-TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
+if [ "$IMGPROTO" = "file" ]; then
+TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
+TEST_IMG=$TEST_DIR/t.$IMGFMT
+elif [ "$IMGPROTO" = "nbd" ]; then
+TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
+TEST_IMG="nbd:127.0.0.1:10810"
+elif [ "$IMGPROTO" = "ssh" ]; then
+TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
+TEST_IMG="ssh://127.0.0.1$TEST_IMG_FILE"
+elif [ "$IMGPROTO" = 

[Qemu-block] [PATCH v6 01/11] block: add flag to indicate that no I/O will be performed

[Daniel P. Berrange]

When opening an image it is useful to know whether the caller
intends to perform I/O on the image or not. In the case of
encrypted images this will allow the block driver to avoid
having to prompt for decryption keys when we merely want to
query header metadata about the image. eg qemu-img info

This flag is enforced at the top level only, since even if
we don't want todo I/O on the 'qcow2' file payload, the
underlying 'file' driver will still need todo I/O to read
the qcow2 header, for example.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 block.c   |  5 +++--
 block/io.c|  2 ++
 include/block/block.h |  1 +
 qemu-img.c| 44 ++--
 4 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/block.c b/block.c
index b4107fc..97787b3 100644
--- a/block.c
+++ b/block.c
@@ -701,7 +701,8 @@ static void bdrv_inherited_options(int *child_flags, QDict 
*child_options,
 flags |= BDRV_O_UNMAP;
 
 /* Clear flags that only apply to the top layer */
-flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ);
+flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ |
+   BDRV_O_NO_IO);
 
 *child_flags = flags;
 }
@@ -721,7 +722,7 @@ static void bdrv_inherited_fmt_options(int *child_flags, 
QDict *child_options,
 child_file.inherit_options(child_flags, child_options,
parent_flags, parent_options);
 
-*child_flags &= ~BDRV_O_PROTOCOL;
+*child_flags &= ~(BDRV_O_PROTOCOL | BDRV_O_NO_IO);
 }
 
 const BdrvChildRole child_format = {
diff --git a/block/io.c b/block/io.c
index 41d954ca..bfc8592 100644
--- a/block/io.c
+++ b/block/io.c
@@ -842,6 +842,7 @@ static int coroutine_fn 
bdrv_aligned_preadv(BlockDriverState *bs,
 assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
 assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
 assert(!qiov || bytes == qiov->size);
+assert((bs->open_flags & BDRV_O_NO_IO) == 0);
 
 /* Handle Copy on Read and associated serialisation */
 if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -1128,6 +1129,7 @@ static int coroutine_fn 
bdrv_aligned_pwritev(BlockDriverState *bs,
 assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
 assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
 assert(!qiov || bytes == qiov->size);
+assert((bs->open_flags & BDRV_O_NO_IO) == 0);
 
 waited = wait_serialising_requests(req);
 assert(!waited || !req->serialising);
diff --git a/include/block/block.h b/include/block/block.h
index 01349ef..d8945ff 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -93,6 +93,7 @@ typedef struct HDGeometry {
 #define BDRV_O_PROTOCOL0x8000  /* if no block driver is explicitly given:
   select an appropriate protocol driver,
   ignoring the format layer */
+#define BDRV_O_NO_IO   0x1 /* don't initialize for I/O */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
 
diff --git a/qemu-img.c b/qemu-img.c
index 29eae2a..7fa2a13 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -224,13 +224,13 @@ static int print_block_option_help(const char *filename, 
const char *fmt)
 
 
 static int img_open_password(BlockBackend *blk, const char *filename,
- bool require_io, bool quiet)
+ int flags, bool quiet)
 {
 BlockDriverState *bs;
 char password[256];
 
 bs = blk_bs(blk);
-if (bdrv_is_encrypted(bs) && require_io) {
+if (bdrv_is_encrypted(bs) && !(flags & BDRV_O_NO_IO)) {
 qprintf(quiet, "Disk image '%s' is encrypted.\n", filename);
 if (qemu_read_password(password, sizeof(password)) < 0) {
 error_report("No password given");
@@ -247,7 +247,7 @@ static int img_open_password(BlockBackend *blk, const char 
*filename,
 
 static BlockBackend *img_open_opts(const char *optstr,
QemuOpts *opts, int flags,
-   bool require_io, bool quiet)
+   bool quiet)
 {
 QDict *options;
 Error *local_err = NULL;
@@ -259,7 +259,7 @@ static BlockBackend *img_open_opts(const char *optstr,
 return NULL;
 }
 
-if (img_open_password(blk, optstr, require_io, quiet) < 0) {
+if (img_open_password(blk, optstr, flags, quiet) < 0) {
 blk_unref(blk);
 return NULL;
 }
@@ -268,7 +268,7 @@ static BlockBackend *img_open_opts(const char *optstr,
 
 static BlockBackend *img_open_file(const char *filename,
const char *fmt, int flags,
-   bool require_io, bool quiet)
+   bool quiet)
 {
 BlockBackend *blk;
 Error *local_err = NULL;
@@ -285,7 +285,7 @@ static BlockBackend *img_open_file(const char *filename,
 

[Qemu-block] [PATCH v6 02/11] qemu-img/qemu-io: don't prompt for passwords if not required

[Daniel P. Berrange]

The qemu-img/qemu-io tools prompt for disk encryption passwords
regardless of whether any are actually required. Adding a check
on bdrv_key_required() avoids this prompt for disk formats which
have been converted to the QCryptoSecret APIs.

This is just a temporary hack to ensure the block I/O tests
continue to work after each patch, since the last patch will
completely delete all the password prompting code.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 qemu-img.c | 3 ++-
 qemu-io.c  | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7fa2a13..681cb70 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -230,7 +230,8 @@ static int img_open_password(BlockBackend *blk, const char 
*filename,
 char password[256];
 
 bs = blk_bs(blk);
-if (bdrv_is_encrypted(bs) && !(flags & BDRV_O_NO_IO)) {
+if (bdrv_is_encrypted(bs) && bdrv_key_required(bs) &&
+!(flags & BDRV_O_NO_IO)) {
 qprintf(quiet, "Disk image '%s' is encrypted.\n", filename);
 if (qemu_read_password(password, sizeof(password)) < 0) {
 error_report("No password given");
diff --git a/qemu-io.c b/qemu-io.c
index d7c2f26..1bd1158 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -69,7 +69,7 @@ static int openfile(char *name, int flags, QDict *opts)
 }
 
 bs = blk_bs(qemuio_blk);
-if (bdrv_is_encrypted(bs)) {
+if (bdrv_is_encrypted(bs) && bdrv_key_required(bs)) {
 char password[256];
 printf("Disk image '%s' is encrypted.\n", name);
 if (qemu_read_password(password, sizeof(password)) < 0) {
-- 
2.5.0

[Qemu-block] [PATCH v6 00/11] Add new LUKS block driver (for 2.6)

[Daniel P. Berrange]

This series is just the block layer parts needed to add
a LUKS driver to QEMU. It was previously posted as part
of the larger series

  v1: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04748.html
  v2: https://lists.gnu.org/archive/html/qemu-block/2016-01/msg00534.html
  v3: https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg03176.html
  v4: https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06552.html
  v5: https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04397.html

The crypto subsystem pieces that this series depends on have now
all been merged.

In this posting I am only proposing merge of the generic LUKS
driver. This can be added as a layer above any of the existing
drivers for accessing LUKS formatted volumes.

When creating new volumes, however, only the file backend can
be used, since the block driver API doesn't allow for arbitrary
stacking of protocols when creating images.

The integration with the qcow2 driver to replace the existing
built-in AES-CBC code is dropped from this series, postponed
until the 2.7 development cycle.

There is a QEMU I/O test 149 that exercises the LUKS driver
and checks for compatibility with the dm-crypt/cryptsetup
impl.

Changed in v6:

 - Remove redundant mutex locking from I/O aths
 - Rebased to resolve conflicts in qemu-img
 - Switch to use blk_open where appropriate
 - Add BDRV_O_CACHE_WB flag
 - Avoid creating bounce buffer larger than iov size
 - Add drv_truncate implementation
 - Extend I/O test framework to support running with
   luks driver for existing tests
 - Split dm-crypt interop test into separate patch
 - Don't prevent use of qcow2 AES encryption entirely,
   just move location of warning message out of generic
   block layer

Daniel P. Berrange (11):
  block: add flag to indicate that no I/O will be performed
  qemu-img/qemu-io: don't prompt for passwords if not required
  tests: redirect stderr to stdout for iotests
  tests: refactor python I/O tests helper main method
  tests: add output filter to python I/O tests helper
  block: add generic full disk encryption driver
  block: move encryption deprecation warning into qcow code
  block: add support for --image-opts in block I/O tests
  block: add support for encryption secrets in block I/O tests
  block: enable testing of LUKS driver with block I/O tests
  block: an interoperability test for luks vs dm-crypt/cryptsetup

 block.c  |   17 +-
 block/Makefile.objs  |2 +
 block/crypto.c   |  586 
 block/io.c   |2 +
 block/qcow.c |9 +
 block/qcow2.c|8 +
 include/block/block.h|2 +
 qapi/block-core.json |   22 +-
 qemu-img.c   |   45 +-
 qemu-io.c|2 +-
 tests/qemu-iotests/004   |2 +-
 tests/qemu-iotests/012   |2 +-
 tests/qemu-iotests/048   |   22 +-
 tests/qemu-iotests/048.out   |6 +-
 tests/qemu-iotests/049.out   |6 -
 tests/qemu-iotests/052   |4 +
 tests/qemu-iotests/052.out   |4 +
 tests/qemu-iotests/087   |3 +-
 tests/qemu-iotests/087.out   |   26 +-
 tests/qemu-iotests/100   |7 +
 tests/qemu-iotests/100.out   |   14 +
 tests/qemu-iotests/134.out   |   18 -
 tests/qemu-iotests/149   |  519 +++
 tests/qemu-iotests/149.out   | 1880 ++
 tests/qemu-iotests/common|   16 +-
 tests/qemu-iotests/common.config |   21 +-
 tests/qemu-iotests/common.filter |3 +-
 tests/qemu-iotests/common.rc |   75 +-
 tests/qemu-iotests/group |1 +
 tests/qemu-iotests/iotests.py|   48 +-
 30 files changed, 3255 insertions(+), 117 deletions(-)
 create mode 100644 block/crypto.c
 create mode 100755 tests/qemu-iotests/149
 create mode 100644 tests/qemu-iotests/149.out

-- 
2.5.0

[Qemu-block] [PATCH v6 04/11] tests: refactor python I/O tests helper main method

[Daniel P. Berrange]

The iotests.py helper provides a main() method for running
tests via the python unit test framework. Not all tests
will want to use this, so refactor it to split the testing
of compatible formats and platforms into separate helper
methods

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/iotests.py | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5f82bbe..51e53bb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -29,7 +29,8 @@ import qtest
 import struct
 
 __all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 'qemu_io',
-   'VM', 'QMPTestCase', 'notrun', 'main']
+   'VM', 'QMPTestCase', 'notrun', 'main', 'verify_image_format',
+   'verify_platform']
 
 # This will not work if arguments contain spaces but is necessary if we
 # want to support the override options that ./check supports.
@@ -394,17 +395,22 @@ def notrun(reason):
 print '%s not run: %s' % (seq, reason)
 sys.exit(0)
 
-def main(supported_fmts=[], supported_oses=['linux']):
-'''Run tests'''
-
-debug = '-d' in sys.argv
-verbosity = 1
+def verify_image_format(supported_fmts=[]):
 if supported_fmts and (imgfmt not in supported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
+def verify_platform(supported_oses=['linux']):
 if True not in [sys.platform.startswith(x) for x in supported_oses]:
 notrun('not suitable for this OS: %s' % sys.platform)
 
+def main(supported_fmts=[], supported_oses=['linux']):
+'''Run tests'''
+
+debug = '-d' in sys.argv
+verbosity = 1
+verify_image_format(supported_fmts)
+verify_platform(supported_oses)
+
 # We need to filter out the time taken from the output so that qemu-iotest
 # can reliably diff the results against master output.
 import StringIO
-- 
2.5.0

[Qemu-block] [PATCH v6 10/11] block: enable testing of LUKS driver with block I/O tests

[Daniel P. Berrange]

This adds support for testing the LUKS driver with the block
I/O test framework.

   cd tests/qemu-io-tests
   ./check -luks

A handful of test cases are modified to work with luks

 - 004 - whitelist luks format
 - 012 - use TEST_IMG_FILE instead of TEST_IMG for file ops
 - 048 - use TEST_IMG_FILE instead of TEST_IMG for file ops.
 don't assume extended image contents is all zeros,
 explicitly initialize with zeros
 Make file size smaller to avoid having to decrypt
 1 GB of data.
 - 052 - don't assume initial image contents is all zeros,
 explicitly initialize with zeros
 - 100 - don't assume initial image contents is all zeros,
 explicitly initialize with zeros

With this patch applied, the results are as follows:

  Passed: 001 002 003 004 005 008 009 010 011 012 021 032 043
  047 048 049 052 087 100 134 143
  Failed: 033 120 140 145
 Skipped: 007 013 014 015 017 018 019 020 022 023 024 025 026
  027 028 029 030 031 034 035 036 037 038 039 040 041
  042 043 044 045 046 047 049 050 051 053 054 055 056
  057 058 059 060 061 062 063 064 065 066 067 068 069
  070 071 072 073 074 075 076 077 078 079 080 081 082
  083 084 085 086 087 088 089 090 091 092 093 094 095
  096 097 098 099 101 102 103 104 105 107 108 109 110
  111 112 113 114 115 116 117 118 119 121 122 123 124
  128 129 130 131 132 133 134 135 136 137 138 139 141
  142 144 146 148

The reasons for the failed tests are:

 - 033 - needs adapting to use image opts syntax with blkdebug
 and test image in order to correctly set align property
 - 120 - needs adapting to use correct -drive syntax for luks
 - 140 - needs adapting to use correct -drive syntax for luks
 - 145 - needs adapting to use correct -drive syntax for luks

The vast majority of skipped tests are exercising code that is
qcow2 specific, though a couple could probably be usefully
enabled for luks too.

Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/004   |  2 +-
 tests/qemu-iotests/012   |  2 +-
 tests/qemu-iotests/048   | 22 +++---
 tests/qemu-iotests/048.out   |  6 --
 tests/qemu-iotests/052   |  4 
 tests/qemu-iotests/052.out   |  4 
 tests/qemu-iotests/100   |  7 +++
 tests/qemu-iotests/100.out   | 14 ++
 tests/qemu-iotests/common|  7 +++
 tests/qemu-iotests/common.rc |  3 +++
 10 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/004 b/tests/qemu-iotests/004
index 2ad77ed..bd09437 100755
--- a/tests/qemu-iotests/004
+++ b/tests/qemu-iotests/004
@@ -38,7 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-_supported_fmt raw qcow qcow2 qed vdi vmdk vhdx
+_supported_fmt raw qcow qcow2 qed vdi vmdk vhdx luks
 _supported_proto generic
 _supported_os Linux
 
diff --git a/tests/qemu-iotests/012 b/tests/qemu-iotests/012
index 7c5b689..85ed20a 100755
--- a/tests/qemu-iotests/012
+++ b/tests/qemu-iotests/012
@@ -50,7 +50,7 @@ _make_test_img $size
 
 echo
 echo "== mark image read-only"
-chmod a-w "$TEST_IMG"
+chmod a-w "$TEST_IMG_FILE"
 
 echo
 echo "== read from read-only image"
diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
index e1eeac2..0e58364 100755
--- a/tests/qemu-iotests/048
+++ b/tests/qemu-iotests/048
@@ -31,13 +31,13 @@ _cleanup()
 {
 echo "Cleanup"
 _cleanup_test_img
-rm "${TEST_IMG2}"
+rm "${TEST_IMG_FILE2}"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _compare()
 {
-$QEMU_IMG compare "$@" "$TEST_IMG" "${TEST_IMG2}"
+$QEMU_IMG compare $QEMU_IMG_EXTRA_ARGS "$@" "$TEST_IMG" "${TEST_IMG2}"
 echo $?
 }
 
@@ -46,25 +46,33 @@ _compare()
 . ./common.filter
 . ./common.pattern
 
-_supported_fmt raw qcow qcow2 qed
+_supported_fmt raw qcow qcow2 qed luks
 _supported_proto file
 _supported_os Linux
 
 # Setup test basic parameters
 TEST_IMG2=$TEST_IMG.2
+TEST_IMG_FILE2=$TEST_IMG_FILE.2
 CLUSTER_SIZE=4096
-size=1024M
+size=128M
 
 _make_test_img $size
 io_pattern write 524288 $CLUSTER_SIZE $CLUSTER_SIZE 4 45
 
 # Compare identical images
-cp "$TEST_IMG" "${TEST_IMG2}"
+cp "$TEST_IMG_FILE" "${TEST_IMG_FILE2}"
 _compare
 _compare -q
 
 # Compare images with different size
-$QEMU_IMG resize -f $IMGFMT "$TEST_IMG" +512M
+if [ "$IMGOPTSSYNTAX" = "true" ]; then
+$QEMU_IMG resize $QEMU_IMG_EXTRA_ARGS "$TEST_IMG" +32M
+else
+$QEMU_IMG resize -f $IMGFMT "$TEST_IMG" +32M
+fi
+# Ensure extended space is zero-initialized
+$QEMU_IO "$TEST_IMG" -c "write -P 0 $size 32M" | _filter_qemu_io
+
 _compare
 _compare -s
 
@@ -77,7 +85,7 @@ _compare
 # Test unaligned case of mismatch offsets in allocated clusters
 _make_test_img $size
 io_pattern write 0 512 0 1 100
-cp "$TEST_IMG" "$TEST_IMG2"
+cp "$TEST_IMG_FILE" "$TEST_IMG_FILE2"
 io_pattern write 512 512 0 1 101
 _compare
 
diff --git a/tests/qemu-iotests/048.out b/tests/qemu-iotests/048.out
index 

[Qemu-block] [PATCH v6 05/11] tests: add output filter to python I/O tests helper

[Daniel P. Berrange]

Add a 'log' method to iotests.py which prints messages to
stdout, with optional filtering of data. Port over some
standard filters already present in the shell common.filter
code to be usable in python too.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/iotests.py | 25 -
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 51e53bb..8499e1b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,7 +30,8 @@ import struct
 
 __all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 'qemu_io',
'VM', 'QMPTestCase', 'notrun', 'main', 'verify_image_format',
-   'verify_platform']
+   'verify_platform', 'filter_test_dir', 'filter_win32',
+   'filter_qemu_io', 'filter_chown', 'log']
 
 # This will not work if arguments contain spaces but is necessary if we
 # want to support the override options that ./check supports.
@@ -105,6 +106,28 @@ def create_image(name, size):
 i = i + 512
 file.close()
 
+test_dir_re = re.compile(r"%s" % test_dir)
+def filter_test_dir(msg):
+return test_dir_re.sub("TEST_DIR", msg)
+
+win32_re = re.compile(r"\r")
+def filter_win32(msg):
+return win32_re.sub("", msg)
+
+qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* 
[EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)")
+def filter_qemu_io(msg):
+msg = filter_win32(msg)
+return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", 
msg)
+
+chown_re = re.compile(r"chown [0-9]+:[0-9]+")
+def filter_chown(msg):
+return chown_re.sub("chown UID:GID", msg)
+
+def log(msg, filters=[]):
+for flt in filters:
+msg = flt(msg)
+print msg
+
 # Test if 'match' is a recursive subset of 'event'
 def event_match(event, match=None):
 if match is None:
-- 
2.5.0

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 21/03/2016 14:47, TU BO wrote:
>> I'll see if I can produce something based on Conny's patches, which are
>> already a start.  Today I had a short day so I couldn't play with the
>> bug; out of curiosity, does the bug reproduce with her work + patch 4
>> from this series + the reentrancy assertion?
> I did NOT see crash with qemu master + "[PATCH RFC 0/6] virtio: refactor
> host notifiers" from Conny + patch 4 + assertion.  thx

That's unexpected, but I guess it only says that I didn't review her
patches well enough. :)

Paolo

[Qemu-block] [PATCH v2 2/3] qemu-iotests: fix test_stream_partial()

[Alberto Garcia]

This test is streaming to the top layer using the intermediate image
as the base. This is a mistake since block-stream never copies data
from the base image and its backing chain, so this is effectively a
no-op.

In addition to fixing the base parameter, this patch also writes some
data to the intermediate image before the test, so there's something
to copy and the test is meaningful.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/030 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 32469ef..48a924c 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -35,6 +35,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
backing_img, mid_img)
 qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
mid_img, test_img)
 qemu_io('-f', 'raw', '-c', 'write -P 0x1 0 512', backing_img)
+qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 524288 512', mid_img)
 self.vm = iotests.VM().add_drive("blkdebug::" + test_img)
 self.vm.launch()
 
@@ -93,7 +94,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 def test_stream_partial(self):
 self.assert_no_active_block_jobs()
 
-result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
+result = self.vm.qmp('block-stream', device='drive0', base=backing_img)
 self.assert_qmp(result, 'return', {})
 
 self.wait_until_completed()
-- 
2.8.0.rc3

[Qemu-block] [PATCH v2 3/3] qemu-iotests: add no-op streaming test

[Alberto Garcia]

This patch tests that in a partial block-stream operation, no data is
ever copied from the base image.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/030 | 18 ++
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 48a924c..3ac2443 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -91,6 +91,24 @@ class TestSingleDrive(iotests.QMPTestCase):
  qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
  'image file map does not match backing file after 
streaming')
 
+def test_stream_no_op(self):
+self.assert_no_active_block_jobs()
+
+# The image map is empty before the operation
+empty_map = qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img)
+
+# This is a no-op: no data should ever be copied from the base image
+result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
+self.assert_qmp(result, 'return', {})
+
+self.wait_until_completed()
+
+self.assert_no_active_block_jobs()
+self.vm.shutdown()
+
+self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
+ empty_map, 'image file map changed after a no-op')
+
 def test_stream_partial(self):
 self.assert_no_active_block_jobs()
 
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index fa16b5c..6323079 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-.
+..
 --
-Ran 13 tests
+Ran 14 tests
 
 OK
-- 
2.8.0.rc3

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[TU BO]

On 16/3/18 下午11:03, Paolo Bonzini wrote:


On 17/03/2016 17:08, Christian Borntraeger wrote:

Good (or bad?) news is the assert also triggers on F23, it just seems
to take longer.

I guess good news, because we can rule out the kernel (not that I
believed it was a kernel problem, but the thought is always there in
the background...).

The interaction between ioeventfd and dataplane is too complicated.  I
think if we get rid of the start/stop ioeventfd calls (just set up the
ioeventfd as soon as possible and then only set/clear the handlers)
things would be much simpler.

I'll see if I can produce something based on Conny's patches, which are
already a start.  Today I had a short day so I couldn't play with the
bug; out of curiosity, does the bug reproduce with her work + patch 4
from this series + the reentrancy assertion?
I did NOT see crash with qemu master + "[PATCH RFC 0/6] virtio: refactor 
host notifiers" from Conny + patch 4 + assertion.  thx


Paolo

[Qemu-block] [PATCH v2 0/3] Various block-stream fixes

[Alberto Garcia]

This series is a follow-up to the patch I sent last week[1]. It
contains a few fixes from my intermediate block streaming branch, but
are independent from that feature, so they can be applied now without
problems.

Berto

[1] https://lists.gnu.org/archive/html/qemu-block/2016-03/msg00551.html

Alberto Garcia (3):
  block: never cancel a streaming job without running stream_complete()
  qemu-iotests: fix test_stream_partial()
  qemu-iotests: add no-op streaming test

 block/stream.c | 11 ++-
 tests/qemu-iotests/030 | 21 -
 tests/qemu-iotests/030.out |  4 ++--
 3 files changed, 28 insertions(+), 8 deletions(-)

-- 
2.8.0.rc3

[Qemu-block] [PATCH v2 1/3] block: never cancel a streaming job without running stream_complete()

[Alberto Garcia]

We need to call stream_complete() in order to do all the necessary
clean-ups, even if there's an early failure. At the moment it's only
useful to make sure that s->backing_file_str is not leaked, but it
will become more important if we introduce support for streaming to
any intermediate node.

Signed-off-by: Alberto Garcia 
Reviewed-by: Max Reitz 
Reviewed-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
---
 block/stream.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index cafaa07..eea3938 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -89,21 +89,21 @@ static void coroutine_fn stream_run(void *opaque)
 StreamCompleteData *data;
 BlockDriverState *bs = s->common.bs;
 BlockDriverState *base = s->base;
-int64_t sector_num, end;
+int64_t sector_num = 0;
+int64_t end = -1;
 int error = 0;
 int ret = 0;
 int n = 0;
 void *buf;
 
 if (!bs->backing) {
-block_job_completed(>common, 0);
-return;
+goto out;
 }
 
 s->common.len = bdrv_getlength(bs);
 if (s->common.len < 0) {
-block_job_completed(>common, s->common.len);
-return;
+ret = s->common.len;
+goto out;
 }
 
 end = s->common.len >> BDRV_SECTOR_BITS;
@@ -190,6 +190,7 @@ wait:
 
 qemu_vfree(buf);
 
+out:
 /* Modify backing chain and close BDSes in main loop */
 data = g_malloc(sizeof(*data));
 data->ret = ret;
-- 
2.8.0.rc3

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Mon, 21 Mar 2016 20:45:27 +0800
Fam Zheng  wrote:

> On Mon, 03/21 12:15, Cornelia Huck wrote:
> > On Mon, 21 Mar 2016 18:57:18 +0800
> > Fam Zheng  wrote:
> > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 08275a9..47f8043 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > > 
> > >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > >  {
> > > -virtio_queue_notify_vq(>vq[n]);
> > > +VirtQueue *vq = >vq[n];
> > > +EventNotifier *n;
> > > +n = virtio_queue_get_host_notifier(vq);
> > > +if (n) {
> > 
> > Isn't that always true, even if the notifier has not been setup?
> 
> You are right, this doesn't make a correct fix. But we can still do a quick
> test with this as the else branch should never be used with ioeventfd=on. Am I
> right?
> 
> Fam

Won't we come through here for the very first kick, when we haven't
registered the ioeventfd with the kernel yet?

> 
> > 
> > > +event_notifier_set(n);
> > > +} else {
> > > +virtio_queue_notify_vq(vq);
> > > +}
> > >  }
> > > 
> > >  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> > > 
> > > 
> > 
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Mon, 03/21 12:15, Cornelia Huck wrote:
> On Mon, 21 Mar 2016 18:57:18 +0800
> Fam Zheng  wrote:
> 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 08275a9..47f8043 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > 
> >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> >  {
> > -virtio_queue_notify_vq(>vq[n]);
> > +VirtQueue *vq = >vq[n];
> > +EventNotifier *n;
> > +n = virtio_queue_get_host_notifier(vq);
> > +if (n) {
> 
> Isn't that always true, even if the notifier has not been setup?

You are right, this doesn't make a correct fix. But we can still do a quick
test with this as the else branch should never be used with ioeventfd=on. Am I
right?

Fam

> 
> > +event_notifier_set(n);
> > +} else {
> > +virtio_queue_notify_vq(vq);
> > +}
> >  }
> > 
> >  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> > 
> > 
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Mon, 21 Mar 2016 17:42:06 +0800
Fam Zheng  wrote:

> On Fri, 03/18 16:03, Paolo Bonzini wrote:
> >
> >
> > On 17/03/2016 17:08, Christian Borntraeger wrote:
> > > Good (or bad?) news is the assert also triggers on F23, it just seems
> > > to take longer.
> >
> > I guess good news, because we can rule out the kernel (not that I
> > believed it was a kernel problem, but the thought is always there in
> > the background...).
> >
> > The interaction between ioeventfd and dataplane is too complicated.  I
> > think if we get rid of the start/stop ioeventfd calls (just set up the
> > ioeventfd as soon as possible and then only set/clear the handlers)
> > things would be much simpler.
> >
> > I'll see if I can produce something based on Conny's patches, which are
> > already a start.  Today I had a short day so I couldn't play with the
> > bug; out of curiosity, does the bug reproduce with her work + patch 4
> > from this series + the reentrancy assertion?
> 
> The other half of the race condition is from ioport write in the vcpu thread. 
> I
> hit this by adding an extra assert(is_in_iothread()) in
> virtio_blk_handle_request(), at the same place with Paolo's atomic read of
> variable "test".
> 
> I haven't tried to find where this ioport write is from, but that is indeed an
> issue in virtio-pci.

Might this be a slow-path notification from before the ioeventfd got
registered on the io bus (IOW before qemu got control)? We have the
first notification that triggers dataplane setup (including registering
the ioeventfd). The second notification was already making its way to
qemu, and gets only handled when ->started had already been set.

Now I'm totally disregarding any possible locking between threads etc.
(perhaps somebody on cc: knows better?), but would the following logic
in handle_output make sense?

if (dataplane) {
   if (!started) {
  dataplane_start();
   }
   if (!disabled) {
  return;
   }
}
/* slow path processing */

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Mon, 21 Mar 2016 18:57:18 +0800
Fam Zheng  wrote:

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 08275a9..47f8043 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> 
>  void virtio_queue_notify(VirtIODevice *vdev, int n)
>  {
> -virtio_queue_notify_vq(>vq[n]);
> +VirtQueue *vq = >vq[n];
> +EventNotifier *n;
> +n = virtio_queue_get_host_notifier(vq);
> +if (n) {

Isn't that always true, even if the notifier has not been setup?

> +event_notifier_set(n);
> +} else {
> +virtio_queue_notify_vq(vq);
> +}
>  }
> 
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> 
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Thu, 03/17 19:03, tu bo wrote:
> 
> On 03/17/2016 08:39 AM, Fam Zheng wrote:
> >On Wed, 03/16 14:45, Paolo Bonzini wrote:
> >>
> >>
> >>On 16/03/2016 14:38, Christian Borntraeger wrote:
> If you just remove the calls to virtio_queue_host_notifier_read, here
> and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
> (keeping patches 2-4 in)?
> >>>
> >>>With these changes and patch 2-4 it does no longer locks up.
> >>>I keep it running some hour to check if a crash happens.
> >>>
> >>>Tu Bo, your setup is currently better suited for reproducing. Can you also 
> >>>check?
> >>
> >>Great, I'll prepare a patch to virtio then sketching the solution that
> >>Conny agreed with.
> >>
> >>While Fam and I agreed that patch 1 is not required, I'm not sure if the
> >>mutex is necessary in the end.
> >
> >If we can fix this from the virtio_queue_host_notifier_read side, the 
> >mutex/BH
> >are not necessary; but OTOH the mutex does catch such bugs, so maybe it's 
> >good
> >to have it. I'm not sure about the BH.
> >
> >And on a hindsight I realize we don't want patches 2-3 too. Actually the
> >begin/end pair won't work as expected because of the blk_set_aio_context.
> >
> >Let's hold on this series.
> >
> >>
> >>So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
> >>and both with/without Fam's patches, it would be great.
> >
> >Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the noise.
> >
> 1. without the virtio_queue_host_notifier_read calls,  without patch 4
> 
> crash happens very often,
> 
> (gdb) bt
> #0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> #1  0x02aa165da37e in coroutine_trampoline (i0=,
> i1=1812051552) at util/coroutine-ucontext.c:79
> #2  0x03ff7dd5150a in __makecontext_ret () from /lib64/libc.so.6
> 
> 
> 2. without the virtio_queue_host_notifier_read calls, with patch 4
> 
> crash happens very often,
> 
> (gdb) bt
> #0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> #1  0x02aa39dda43e in coroutine_trampoline (i0=,
> i1=-1677715600) at util/coroutine-ucontext.c:79
> #2  0x03ffab6d150a in __makecontext_ret () from /lib64/libc.so.6
> 
> 

Tu Bo,

Could you help test this patch (on top of master, without patch 4)?

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 08275a9..47f8043 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
 
 void virtio_queue_notify(VirtIODevice *vdev, int n)
 {
-virtio_queue_notify_vq(>vq[n]);
+VirtQueue *vq = >vq[n];
+EventNotifier *n;
+n = virtio_queue_get_host_notifier(vq);
+if (n) {
+event_notifier_set(n);
+} else {
+virtio_queue_notify_vq(vq);
+}
 }
 
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Fri, 03/18 16:03, Paolo Bonzini wrote:
>
>
> On 17/03/2016 17:08, Christian Borntraeger wrote:
> > Good (or bad?) news is the assert also triggers on F23, it just seems
> > to take longer.
>
> I guess good news, because we can rule out the kernel (not that I
> believed it was a kernel problem, but the thought is always there in
> the background...).
>
> The interaction between ioeventfd and dataplane is too complicated.  I
> think if we get rid of the start/stop ioeventfd calls (just set up the
> ioeventfd as soon as possible and then only set/clear the handlers)
> things would be much simpler.
>
> I'll see if I can produce something based on Conny's patches, which are
> already a start.  Today I had a short day so I couldn't play with the
> bug; out of curiosity, does the bug reproduce with her work + patch 4
> from this series + the reentrancy assertion?

The other half of the race condition is from ioport write in the vcpu thread. I
hit this by adding an extra assert(is_in_iothread()) in
virtio_blk_handle_request(), at the same place with Paolo's atomic read of
variable "test".

I haven't tried to find where this ioport write is from, but that is indeed an
issue in virtio-pci.

(gdb) thread apply all bt

<...>

Thread 3 (Thread 0x7f9e8928b700 (LWP 30671)):
#0  0x7f9e8bac65d7 in __GI_raise (sig=sig@entry=6) at 
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7f9e8bac7cc8 in __GI_abort () at abort.c:90
#2  0x7f9e8babf546 in __assert_fail_base (fmt=0x7f9e8bc0f128 "%s%s%s:%u: 
%s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7f9e8e04e9d1 
"is_in_iothread()",
file=file@entry=0x7f9e8e04e8e0 "/home/fam/work/qemu/hw/block/virtio-blk.c", 
line=line@entry=597,
function=function@entry=0x7f9e8e04ec30 <__PRETTY_FUNCTION__.37148> 
"virtio_blk_handle_output") at assert.c:92
#3  0x7f9e8babf5f2 in __GI___assert_fail (assertion=0x7f9e8e04e9d1 
"is_in_iothread()", file=0x7f9e8e04e8e0 
"/home/fam/work/qemu/hw/block/virtio-blk.c", line=597,
function=0x7f9e8e04ec30 <__PRETTY_FUNCTION__.37148> 
"virtio_blk_handle_output") at assert.c:101
#4  0x7f9e8dc9f414 in virtio_blk_handle_output (vdev=0x7f9e929d7b68, 
vq=0x7f9e92a762f0) at /home/fam/work/qemu/hw/block/virtio-blk.c:597
#5  0x7f9e8dcd6f53 in virtio_queue_notify_vq (vq=0x7f9e92a762f0) at 
/home/fam/work/qemu/hw/virtio/virtio.c:1095
#6  0x7f9e8dcd6f91 in virtio_queue_notify (vdev=0x7f9e929d7b68, n=0) at 
/home/fam/work/qemu/hw/virtio/virtio.c:1101
#7  0x7f9e8df03d2f in virtio_ioport_write (opaque=0x7f9e929cf840, addr=16, 
val=0) at /home/fam/work/qemu/hw/virtio/virtio-pci.c:419
#8  0x7f9e8df041be in virtio_pci_config_write (opaque=0x7f9e929cf840, 
addr=16, val=0, size=2) at /home/fam/work/qemu/hw/virtio/virtio-pci.c:552
#9  0x7f9e8dc7c8c9 in memory_region_write_accessor (mr=0x7f9e929d00c0, 
addr=16, value=0x7f9e8928a988, size=2, shift=0, mask=65535, attrs=...)
at /home/fam/work/qemu/memory.c:524
#10 0x7f9e8dc7cad4 in access_with_adjusted_size (addr=16, 
value=0x7f9e8928a988, size=2, access_size_min=1, access_size_max=4, access=
0x7f9e8dc7c7e8 , mr=0x7f9e929d00c0, 
attrs=...) at /home/fam/work/qemu/memory.c:590
#11 0x7f9e8dc7f71b in memory_region_dispatch_write (mr=0x7f9e929d00c0, 
addr=16, data=0, size=2, attrs=...) at /home/fam/work/qemu/memory.c:1272
#12 0x7f9e8dc32815 in address_space_write_continue (as=0x7f9e8e5834a0 
, addr=49232, attrs=..., buf=0x7f9e8daa9000 ,
len=2, addr1=16, l=2, mr=0x7f9e929d00c0) at /home/fam/work/qemu/exec.c:2607
#13 0x7f9e8dc329c1 in address_space_write (as=0x7f9e8e5834a0 
, addr=49232, attrs=..., buf=0x7f9e8daa9000 , len=2)
at /home/fam/work/qemu/exec.c:2659
#14 0x7f9e8dc32d78 in address_space_rw (as=0x7f9e8e5834a0 
, addr=49232, attrs=..., buf=0x7f9e8daa9000 , len=2,
is_write=true) at /home/fam/work/qemu/exec.c:2762
#15 0x7f9e8dc79358 in kvm_handle_io (port=49232, attrs=..., 
data=0x7f9e8daa9000, direction=1, size=2, count=1) at 
/home/fam/work/qemu/kvm-all.c:1699
#16 0x7f9e8dc79858 in kvm_cpu_exec (cpu=0x7f9e905a5250) at 
/home/fam/work/qemu/kvm-all.c:1863
#17 0x7f9e8dc619a3 in qemu_kvm_cpu_thread_fn (arg=0x7f9e905a5250) at 
/home/fam/work/qemu/cpus.c:1056
#18 0x7f9e8be59df5 in start_thread (arg=0x7f9e8928b700) at 
pthread_create.c:308
#19 0x7f9e8bb871ad in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:113

<...>

Thread 1 (Thread 0x7f9e8b28f700 (LWP 30667)):
#0  0x7f9e8bac65d7 in __GI_raise (sig=sig@entry=6) at 
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7f9e8bac7cc8 in __GI_abort () at abort.c:90
#2  0x7f9e8babf546 in __assert_fail_base (fmt=0x7f9e8bc0f128 "%s%s%s:%u: 
%s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7f9e8e04e9e2 "x == 
0",
file=file@entry=0x7f9e8e04e8e0 "/home/fam/work/qemu/hw/block/virtio-blk.c", 
line=line@entry=598,
function=function@entry=0x7f9e8e04ec30 <__PRETTY_FUNCTION__.37148> 
"virtio_blk_handle_output") at assert.c:92
#3  0x7f9e8babf5f2 

Re: [Qemu-block] [PATCH v2] Replaced get_tick_per_sec() by NANOSECONDS_PER_SECOND

[Christian Borntraeger]

On 03/20/2016 07:31 PM, rutu.shah...@gmail.com wrote:
> From: Rutuja Shah 

Can you add the patch description (with examples of why this is the right thing
to do) here and not in the cover letter?

> 
> Signed-off-by: Rutuja Shah 
> 
> ---
>  audio/audio.c |  3 +--
>  audio/noaudio.c   |  8 
>  audio/spiceaudio.c|  4 ++--
>  audio/wavaudio.c  |  2 +-
>  backends/baum.c   |  2 +-
>  block/qed.c   |  2 +-
>  cpus.c|  6 +++---
>  hw/acpi/core.c|  4 ++--
>  hw/arm/omap1.c| 17 +
>  hw/arm/spitz.c|  2 +-
>  hw/arm/stellaris.c|  2 +-
>  hw/arm/strongarm.c|  2 +-
>  hw/audio/adlib.c  |  2 +-
>  hw/audio/sb16.c   |  4 ++--
>  hw/block/fdc.c|  4 ++--
>  hw/block/pflash_cfi02.c   |  8 
>  hw/bt/hci-csr.c   |  4 ++--
>  hw/char/cadence_uart.c|  4 ++--
>  hw/char/serial.c  | 10 ++
>  hw/display/vga.c  |  6 +++---
>  hw/dma/rc4030.c   |  2 +-
>  hw/ide/core.c |  4 ++--
>  hw/input/hid.c|  2 +-
>  hw/input/tsc2005.c|  3 ++-
>  hw/input/tsc210x.c|  3 ++-
>  hw/intc/i8259.c   |  2 +-
>  hw/misc/arm_sysctl.c  |  3 ++-
>  hw/misc/macio/cuda.c  | 16 
>  hw/misc/macio/macio.c |  2 +-
>  hw/net/dp8393x.c  |  2 +-
>  hw/ppc/ppc.c  | 21 -
>  hw/ppc/ppc405_uc.c|  4 ++--
>  hw/ppc/ppc_booke.c|  2 +-
>  hw/sd/sdhci-internal.h|  2 +-
>  hw/sparc64/sun4u.c|  4 ++--
>  hw/timer/i8254.c  |  4 ++--
>  hw/timer/i8254_common.c   |  6 +++---
>  hw/timer/mc146818rtc.c| 14 --
>  hw/timer/omap_gptimer.c   |  2 +-
>  hw/timer/omap_synctimer.c |  3 ++-
>  hw/timer/pl031.c  | 10 +-
>  hw/timer/pxa2xx_timer.c   | 18 ++
>  hw/usb/hcd-ehci.c |  5 +++--
>  hw/usb/hcd-musb.c |  2 +-
>  hw/usb/hcd-ohci.c | 10 +-
>  hw/usb/hcd-uhci.c |  6 +++---
>  hw/usb/tusb6010.c |  6 +++---
>  hw/watchdog/wdt_diag288.c |  2 +-
>  hw/watchdog/wdt_ib700.c   |  2 +-
>  include/hw/acpi/acpi.h|  2 +-
>  include/qemu/timer.h  |  9 ++---
>  monitor.c |  4 ++--
>  target-ppc/kvm.c  |  4 ++--
>  53 files changed, 143 insertions(+), 134 deletions(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index e841532..ab0ade8 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1869,8 +1869,7 @@ static void audio_init (void)
>  }
>  conf.period.ticks = 1;
>  } else {
> -conf.period.ticks =
> -muldiv64 (1, get_ticks_per_sec (), conf.period.hertz);
> +conf.period.ticks = NANOSECONDS_PER_SECOND / conf.period.hertz;
>  }
> 
>  e = qemu_add_vm_change_state_handler (audio_vm_change_state_handler, s);
> diff --git a/audio/noaudio.c b/audio/noaudio.c
> index 09588b9..b360c19 100644
> --- a/audio/noaudio.c
> +++ b/audio/noaudio.c
> @@ -49,8 +49,8 @@ static int no_run_out (HWVoiceOut *hw, int live)
> 
>  now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  ticks = now - no->old_ticks;
> -bytes = muldiv64 (ticks, hw->info.bytes_per_second, get_ticks_per_sec 
> ());
> -bytes = audio_MIN (bytes, INT_MAX);
> +bytes = muldiv64(ticks, hw->info.bytes_per_second, 
> NANOSECONDS_PER_SECOND);
> +bytes = audio_MIN(bytes, INT_MAX);
>  samples = bytes >> hw->info.shift;
> 
>  no->old_ticks = now;
> @@ -61,7 +61,7 @@ static int no_run_out (HWVoiceOut *hw, int live)
> 
>  static int no_write (SWVoiceOut *sw, void *buf, int len)
>  {
> -return audio_pcm_sw_write (sw, buf, len);
> +return audio_pcm_sw_write(sw, buf, len);
>  }
> 
>  static int no_init_out(HWVoiceOut *hw, struct audsettings *as, void 
> *drv_opaque)
> @@ -106,7 +106,7 @@ static int no_run_in (HWVoiceIn *hw)
>  int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  int64_t ticks = now - no->old_ticks;
>  int64_t bytes =
> -muldiv64 (ticks, hw->info.bytes_per_second, get_ticks_per_sec 
> ());
> +muldiv64(ticks, hw->info.bytes_per_second, 
> NANOSECONDS_PER_SECOND);
> 
>  no->old_ticks = now;
>  bytes = audio_MIN (bytes, INT_MAX);
> diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
> index 297fd41..dea71d3 100644
> --- a/audio/spiceaudio.c
> +++ b/audio/spiceaudio.c
> @@ -104,11 +104,11 @@ static int rate_get_samples (struct audio_pcm_info 
> *info, SpiceRateCtl *rate)
> 
>  now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  ticks = now - rate->start_ticks;
> -bytes = muldiv64 (ticks, info->bytes_per_second, get_ticks_per_sec ());
> +bytes = muldiv64(ticks, info->bytes_per_second, NANOSECONDS_PER_SECOND);
>  samples = (bytes - rate->bytes_sent) >> info->shift;
>  if (samples < 0 || samples > 65536) {
>