Re: [Qemu-block] [PATCH v2 17/21] block: Move cache options into options QDict

[Kevin Wolf]

Am 27.11.2015 um 20:57 hat Max Reitz geschrieben:
> On 23.11.2015 16:59, Kevin Wolf wrote:
> > This adds the cache mode options to the QDict, so that they can be
> > specified for child nodes (e.g. backing.cache.direct=off).
> > 
> > The cache modes are not removed from the flags at this point; instead,
> > options and flags are kept in sync. If the user specifies both flags and
> > options, the options take precedence.
> > 
> > Child node inherit cache modes as options now, they don't use flags any
> > more.
> > 
> > Note that this forbids specifying the cache mode for empty drives. It
> > didn't make sense anyway to specify it there, because it didn't have any
> > effect. blockdev_init() considers the cache options now bdrv_open()
> > options and therefore doesn't create an empty drive any more but calls
> > into bdrv_open(). This in turn will fail with no driver and filename
> > specified.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block.c| 100 
> > ++---
> >  blockdev.c |  52 ++--
> >  2 files changed, 111 insertions(+), 41 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index eff0c19..397014a 100644
> > --- a/block.c
> > +++ b/block.c
> 
> [...]
> 
> > @@ -1747,12 +1816,22 @@ static BlockReopenQueue 
> > *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
> >  /*
> >   * Precedence of options:
> >   * 1. Explicitly passed in options (highest)
> > - * 2. TODO Set in flags (only for top level)
> > + * 2. Set in flags (only for top level)
> >   * 3. Retained from explicitly set options of bs
> >   * 4. Inherited from parent node
> >   * 5. Retained from effective options of bs
> >   */
> >  
> > +if (!parent_options) {
> > +/*
> > + * Any setting represented by flags is always updated. If the
> > + * corresponding QDict option is set, it takes precedence. 
> > Otherwise
> > + * the flag is translated into a QDict option. The old setting of 
> > bs is
> > + * not considered.
> > + */
> > +update_options_from_flags(options, flags);
> > +}
> > +
> >  /* Old explicitly set values (don't overwrite by inherited value) */
> >  old_options = qdict_clone_shallow(bs->explicit_options);
> >  bdrv_join_options(bs, options, old_options);
> > @@ -1923,6 +2002,19 @@ int bdrv_reopen_prepare(BDRVReopenState 
> > *reopen_state, BlockReopenQueue *queue,
> >  goto error;
> >  }
> >  
> > +update_flags_from_options(_state->flags, opts);
> > +
> > +/* If a guest device is attached, it owns WCE */
> > +if (reopen_state->bs->blk && 
> > blk_get_attached_dev(reopen_state->bs->blk)) {
> > +bool old_wce = bdrv_enable_write_cache(reopen_state->bs);
> > +bool new_wce = (reopen_state->flags & BDRV_O_CACHE_WB);
> > +if (old_wce != new_wce) {
> > +error_setg(errp, "Cannot change cache.writeback: Device 
> > attached");
> > +ret = -EINVAL;
> > +goto error;
> > +}
> > +}
> > +
> 
> Time to get back to my question regarding bdrv_set_enable_write_cache():

Alles kaputt. :-)

> 1. bdrv_set_enable_write_cache() sets/unsets BDRV_O_CACHE_WB in
>bs->open_flags so that it is preserved through a reopen.
> 2. On reopen, bdrv_reopen_queue_child() calls
>update_options_from_flags() unless @parent_options is set. If that
>function is called, it will set the appropriate caching options in
>@options as dictated by bdrv_set_enable_write_cache().
> 3. If @parent_options was NULL, the update_flags_from_options() call
>here in bdrv_reopen_prepare() will set BDRV_O_CACHE_WB just as
>dictated by bdrv_set_enable_write_cache() (unless overwritten).
>That is what we want.
> 4. If @parent_options was not NULL, the caching options in
>bs->open_flags are completely overwritten, discarding everything that
>had been set by bdrv_set_enable_write_cache(). That's not so good.
> 
> @parent_options is tested so that update_options_from_flags() is called
> only for root BDSs. It is possible to call bdrv_set_enable_write_cache()
> on non-root BDSs (e.g. commit_active_start() does it on the commit base
> via mirror_start_job()), so I do think overriding the setting made by
> bdrv_set_enable_write_cache() on reopen for any non-root BDSs is not
> correct.

I think you didn't interpret @parent_options correctly. It is NULL for
the node that bdrv_reopen() is called for. It is non-NULL for
(recursive) child nodes of that node that inherit options from it. Not
sure if it matters for this case, though.

In my opinion, what these block jobs are doing is insane. They should
never touch the cache mode of a node as long as this cache mode can be
shared with other users. It doesn't matter much with block jobs that can
only work on a BDS that they just created, but with blockdev-backup I'd
consider this a real bug.

> Maybe 

Re: [Qemu-block] [Qemu-devel] [PATCH 05/40] virtio: read/write the VirtQueueElement a field at a time

[Fam Zheng]

On Tue, 11/24 19:00, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/virtio/virtio.c | 95 
> --
>  1 file changed, 93 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index fd63206..f5f8108 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -578,14 +578,105 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>  void *qemu_get_virtqueue_element(QEMUFile *f, size_t sz)
>  {
>  VirtQueueElement *elem = g_malloc(sz);
> -qemu_get_buffer(f, (uint8_t *)elem, sizeof(VirtQueueElement));
> +bool swap;
> +hwaddr addr[VIRTQUEUE_MAX_SIZE];
> +struct iovec iov[VIRTQUEUE_MAX_SIZE];
> +uint64_t scratch;
> +int i;
> +
> +qemu_get_be32s(f, >index);
> +qemu_get_be32s(f, >out_num);
> +qemu_get_be32s(f, >in_num);
> +
> +swap = (elem->out_num & 0x) || (elem->in_num & 0x);

This is interesting, out_num and in_num are 32 bit numbers but there max values
are both VIRTQUEUE_MAX_SIZE (thanks for explaining this on IRC), so it can be a
clue for the source using a different endianness.

Probably worth a few comments here?

It's a great patch!  Thanks!

Fam

> +if (swap) {
> +bswap32s(>index);
> +bswap32s(>out_num);
> +bswap32s(>in_num);
> +}
> +
> +for (i = 0; i < elem->in_num; i++) {
> +qemu_get_be64s(f, >in_addr[i]);
> +if (swap) {
> +bswap64s(>in_addr[i]);
> +}
> +}
> +if (i < ARRAY_SIZE(addr)) {
> +qemu_get_buffer(f, (uint8_t *)addr, sizeof(addr) - i * 
> sizeof(addr[0]));
> +}
> +
> +for (i = 0; i < elem->out_num; i++) {
> +qemu_get_be64s(f, >out_addr[i]);
> +if (swap) {
> +bswap64s(>out_addr[i]);
> +}
> +}
> +if (i < ARRAY_SIZE(addr)) {
> +qemu_get_buffer(f, (uint8_t *)addr, sizeof(addr) - i * 
> sizeof(addr[0]));
> +}
> +
> +for (i = 0; i < elem->in_num; i++) {
> +(void) qemu_get_be64(f); /* base */
> + qemu_get_be64s(f, ); /* length */
> +if (swap) {
> +bswap64s();
> +}
> + elem->in_sg[i].iov_len = scratch;
> +}
> +if (i < ARRAY_SIZE(iov)) {
> +qemu_get_buffer(f, (uint8_t *)iov, sizeof(iov) - i * sizeof(iov[0]));
> +}
> +
> +for (i = 0; i < elem->out_num; i++) {
> +(void) qemu_get_be64(f); /* base */
> +qemu_get_be64s(f, ); /* length */
> +if (swap) {
> +bswap64s();
> +}
> + elem->out_sg[i].iov_len = scratch;
> +}
> +if (i < ARRAY_SIZE(iov)) {
> +qemu_get_buffer(f, (uint8_t *)iov, sizeof(iov) - i * sizeof(iov[0]));
> +}
> +
>  virtqueue_map(elem);
>  return elem;
>  }
>  
>  void qemu_put_virtqueue_element(QEMUFile *f, VirtQueueElement *elem)
>  {
> -qemu_put_buffer(f, (uint8_t *)elem, sizeof(VirtQueueElement));
> +hwaddr addr[VIRTQUEUE_MAX_SIZE];
> +struct iovec iov[VIRTQUEUE_MAX_SIZE];
> +int i;
> +
> +memset(addr, 0, sizeof(addr));
> +memset(iov, 0, sizeof(iov));
> +
> +qemu_put_be32s(f, >index);
> +qemu_put_be32s(f, >out_num);
> +qemu_put_be32s(f, >in_num);
> +
> +for (i = 0; i < elem->in_num; i++) {
> +qemu_put_be64s(f, >in_addr[i]);
> +}
> +qemu_put_buffer(f, (uint8_t *)addr, sizeof(addr) - i * sizeof(addr[0]));
> +
> +for (i = 0; i < elem->out_num; i++) {
> +qemu_put_be64s(f, >out_addr[i]);
> +}
> +qemu_put_buffer(f, (uint8_t *)addr, sizeof(addr) - i * sizeof(addr[0]));
> +
> +for (i = 0; i < elem->in_num; i++) {
> +qemu_put_be64(f, 0);
> +qemu_put_be64(f, elem->in_sg[i].iov_len);
> +}
> +qemu_put_buffer(f, (uint8_t *)iov, sizeof(iov) - i * sizeof(iov[0]));
> +
> +for (i = 0; i < elem->out_num; i++) {
> +qemu_put_be64(f, 0);
> +qemu_put_be64(f, elem->out_sg[i].iov_len);
> +}
> +qemu_put_buffer(f, (uint8_t *)iov, sizeof(iov) - i * sizeof(iov[0]));
>  }
>  
>  /* virtio device */
> -- 
> 1.8.3.1
> 
> 
> 

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

[Kevin Wolf]

Am 30.11.2015 um 17:19 hat Eric Blake geschrieben:
> On 11/27/2015 12:35 PM, Programmingkid wrote:
> 
> >> Unusual indentation; more typical is:
> >>
> >> | static kern_return_t FindEjectableOpticalMedia(io_iterator_t
> >> *mediaIterator,
> >> | char *mediatType)
> > 
> > I agree. I wanted the second long to be right justified with the 80 
> > character line count.
> 
> No.  We don't right-justify code to 80 columns.  That's not how it is
> done.  Trying to do it just makes you look like the proverbial 'kid' in
> your pseudonym, rather than an adult to be taken seriously.
> 
> Really, PLEASE follow the indentation patterns of the rest of the code
> base - where continued lines are left-justified to be underneath the
> character after (, and NOT right-justified to 80 columns.  Violating
> style doesn't make your code invalid, but does make your patches less
> likely to be applied.
> 
> 
> >>> +/* If you found a match, leave the loop */
> >>> +if (*mediaIterator != 0) {
> >>> +DPRINTF("Matching using %s\n", matching_array[index]);
> >>> +snprintf(mediaType, strlen(matching_array[index])+1, "%s",
> >>
> >> Spaces around binary '+'.
> > 
> > What's wrong with no spaces around the plus sign?
> 
> Again, the prevailing conventions in the code base is that you put
> spaces around every binary operator.  Yes, there is existing old code
> that does not meet the conventions, but it is not an excuse to add new
> code that is gratuitously different.
> 
> > 
> >>
> >>> +/* if a working partition on the device was not found */
> >>> +if (partition_found == false) {
> >>> +error_setg(errp, "Error: Failed to find a working partition on "
> >>> + 
> >>> "disc!\n");
> >>
> >> and I already pointed out on v8 that this is not the correct usage of
> >> error_setg().  So, here's hoping v10 addresses the comments here and
> >> elsewhere.
> > 
> > Kevin Wolf wanted it this way. What would you do instead?
> 
> Keven and I both want you to use error_setg(), but to use it correctly -
> and the correct way is to NOT supply a trailing \n.

Nor leading "Error:", for that matter.

Kevin


pgpNR6FGhum4z.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH v7 24/24] iotests: Add test for block jobs and BDS ejection

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Suggested-by: Paolo Bonzini 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/141 | 166 
> +
>  tests/qemu-iotests/141.out |  47 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 214 insertions(+)
>  create mode 100755 tests/qemu-iotests/141
>  create mode 100644 tests/qemu-iotests/141.out
> 
> diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
> new file mode 100755
> index 000..6a32d56
> --- /dev/null
> +++ b/tests/qemu-iotests/141
> @@ -0,0 +1,166 @@
> +#!/bin/bash
> +#
> +# Test case for ejecting BDSs with block jobs still running on them
> +#
> +# Copyright (C) 2015 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=mre...@redhat.com
> +
> +seq="$(basename $0)"
> +echo "QA output created by $seq"
> +
> +here="$PWD"
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +
> +_cleanup()
> +{
> +_cleanup_test_img
> +rm -f "$TEST_DIR/{b,o}.$IMGFMT"
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +
> +# Needs backing file support
> +_supported_fmt qcow qcow2 qed

The test doesn't work for me on qcow1.

> +echo
> +echo '=== Testing block-commit ==='
> +echo
> +
> +# block-commit will send BLOCK_JOB_READY basically immediately, and 
> cancelling
> +# the job will consequently result in BLOCK_JOB_COMPLETED being emitted.
> +
> +test_blockjob \
> +"{'execute': 'block-commit',
> +  'arguments': {'device': 'drv0'}}" \
> +'BLOCK_JOB_READY' \
> +'BLOCK_JOB_COMPLETED'

This is commit of the active layer, i.e. just a mirror in disguise.
Should we test a "real" commit block job as well?

Anyway, with qcow1 removed from the list:
Reviewed-by: Kevin Wolf 

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

[Eric Blake]

On 11/27/2015 02:49 PM, Programmingkid wrote:
> 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.
> 
> Signed-off-by: John Arbuckle 
> 
> ---
> Fixed some spacing issues. 
> Removed else condition in FindEjectableOpticalMedia.
> Added continue statement to FindEjectableOpticalMedia.
> Replaced printf() with error_report() in FindEjectableOpticalMedia.
> Altered comment in FindEjectableOpticalMedia.
> If the spacing in this patch looks off, try changing the font to something
> that is mono-spaced.

Patches are best read in monospaced fonts, anyways; it's better to make
that part of your workflow, and assume that everyone else has already
done likewise, than to advertise that you are only making life harder
for yourself.

> 
>  block/raw-posix.c |  140 ++--
>  1 files changed, 102 insertions(+), 38 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index ccfec1c..9e7de11 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -42,9 +42,9 @@
>  #include 
>  #include 
>  #include 
> -//#include 
> +#include 
>  #include 
> -#endif
> +#endif /* (__APPLE__) && (__MACH__) */
>  

I have now mentioned in both v8 and v9 that this hunk should be its own
patch (and is simple enough to cc qemu-trivial).  Disregarding reviewers
suggestions is not a good idea - it only serves to waste time (both
yours and reviewers) and earn you black marks, such that it will be even
less likely that anyone wants to review your patches in the first place.
 I'm trying to help you be a better contributor, but it feels like you
are ignoring advice, and so my natural reaction is to ignore you.

>  #ifdef __sun__
>  #define _POSIX_PTHREAD_SEMANTICS 1
> @@ -1975,32 +1975,46 @@ 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 kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
> +char 
> *mediaType)

No, your indentation is still wrong.  I tried to point out on your v8
that we don't right-justify to 80 columns, but rather left-justify to
the point just after the (.


> +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.\n",

No. Don't use trailing '.' or trailing '\n' in error_report() calls.
I've already mentioned this, and feel like I'm becoming a broken record.
 When you disregard my review comments, I become disinclined to review
your patches any further.

> + 
> matching_array[index]);

Indentation is still wrong.

> +continue;
> +}
> +CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
> +
> kCFBooleanTrue);
> +kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
> + 
> mediaIterator);
> +if (kernResult != KERN_SUCCESS) {
> +error_report("Note: IOServiceGetMatchingServices returned %d\n",
> +
> kernResult);

No trailing \n in error_report(), indentation is wrong.

> +}
>  
> +/* If a match was found, leave the loop */
> +if (*mediaIterator != 0) {
> +DPRINTF("Matching using %s\n", matching_array[index]);
> +snprintf(mediaType, strlen(matching_array[index])+1, "%s",
> + 
> matching_array[index]);

Spaces around binary '+', and indentation is wrong.


> +/* 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);

Indentation is wrong.


> +/* if a working partition on the device was not found */
> +if (partition_found == false) {
> +error_setg(errp, "Error: Failed to find a working partition on "
> + 
> "disc!\n");

Indentation is wrong, no trailing '!' or '\n' in error_setg().

I'm so 

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

[Eric Blake]

On 11/30/2015 09:38 AM, Programmingkid wrote:

>> +/* if a working partition on the device was not found */
>> +if (partition_found == false) {
>> +error_setg(errp, "Error: Failed to find a working partition on "
>> + 
>> "disc!\n");
>
> and I already pointed out on v8 that this is not the correct usage of
> error_setg().  So, here's hoping v10 addresses the comments here and
> elsewhere.

 Kevin Wolf wanted it this way. What would you do instead?
>>>
>>> Keven and I both want you to use error_setg(), but to use it correctly -
>>> and the correct way is to NOT supply a trailing \n.
>>
>> Nor leading "Error:", for that matter.
> 
> I just think that using "Error" does communicate the fact that something is 
> wrong
> a lot better than just printing the message. 

But error_setg() _already_ provides the context that an error message is
being printed.  The whole point of using wrapper functions is that the
common functionality (like an 'error:' prefix, or '\n' suffix) is done
in the wrapper, not at every call site.  If you were using raw printf(),
then yes, using your own 'Error:' prefix would be appropriate.  But we
aren't using raw printf().  Your use of an 'Error:' prefix is therefore
redundant, and we are trying to convince you that you are using
error_setg() incorrectly.

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



signature.asc
Description: OpenPGP digital signature

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

[Eric Blake]

On 11/30/2015 09:51 AM, Programmingkid wrote:

>>> +++ b/block/raw-posix.c
>>> @@ -42,9 +42,9 @@
>>> #include 
>>> #include 
>>> #include 
>>> -//#include 
>>> +#include 
>>> #include 
>>> -#endif
>>> +#endif /* (__APPLE__) && (__MACH__) */
>>>
>>
>> I have now mentioned in both v8 and v9 that this hunk should be its own
>> patch (and is simple enough to cc qemu-trivial).  Disregarding reviewers
>> suggestions is not a good idea - it only serves to waste time (both
>> yours and reviewers) and earn you black marks, such that it will be even
>> less likely that anyone wants to review your patches in the first place.
>> I'm trying to help you be a better contributor, but it feels like you
>> are ignoring advice, and so my natural reaction is to ignore you.
> 
> I assure you that this change is *required* for my patch. Without it the 
> patch would
> not even compile. I need a macro from IODVDMedia.h. If removing the 
> IOCDTypes.h
> is what is bothering you, it is a very small change that no one is going to 
> miss. That
> header file was commented out but not removed for some reason. 

And I assure you that splitting the change into a separate patch, and
making this a 2-patch series, is essential for you getting your patch
applied.  It's okay for one patch to depend on another; it is not okay
to shove multiple fixes into a single patch when you have been told to
split it into multiple patches.

Okay, let me restate things a bit:

The trivial hunk of your patch (that should be applied independently,
because it is unrelated code cleanup) would be:

> #include 
> #include 
> #include 
>-//#include 
> #include 
>-#endif
>+#endif /* (__APPLE__) && (__MACH__) */

and then the part for _this_ commit (adding a new required header) would be:

> #include 
> #include 
> #include 
>+#include 
> #include 
> #endif /* (__APPLE__) && (__MACH__) */


>>> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t 
>>> *mediaIterator,
>>> +char 
>>> *mediaType)
>>
>> No, your indentation is still wrong.  I tried to point out on your v8
>> that we don't right-justify to 80 columns, but rather left-justify to
>> the point just after the (.
> 
> If you feel it is that important, I will do it. I just thought it was easier 
> to read when your
> eye is already in the area. There is less time spend finding the next 
> argument that way.

When you have read THOUSANDS of lines of code indented in one style,
then your eye is already very trained to look in the same place for the
continued line.  One-off code that places the code in somewhere other
than the usual place is actually HARDER to read, because it violates the
conventions that you have already trained to read.

> I don't remember hearing about not using \n in the error_report() call, but I 
> will
> fix this in the next patch.

v8:
https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05806.html
"Several violations of convention.  error_setg() does not need a
redundant "Error: " prefix, should not end in '!' (we aren't shouting),
and should not end in newline.  And with those fixes, you won't even
need the weird indentation."

-- 
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 v7 14/24] nbd: Switch from close to eject notifier

[Max Reitz]

On 30.11.2015 16:36, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> The NBD code uses the BDS close notifier to determine when a medium is
>> ejected. However, now it should use the BB's BDS removal notifier for
>> that instead of the BDS's close notifier.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  blockdev-nbd.c | 37 +
>>  nbd.c  | 13 +
>>  2 files changed, 14 insertions(+), 36 deletions(-)
>>
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index bcdd18b..b28a55b 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -46,37 +46,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error 
>> **errp)
>>  }
>>  }
>>  
>> -/*
>> - * Hook into the BlockBackend notifiers to close the export when the
>> - * backend is closed.
>> - */
>> -typedef struct NBDCloseNotifier {
>> -Notifier n;
>> -NBDExport *exp;
>> -QTAILQ_ENTRY(NBDCloseNotifier) next;
>> -} NBDCloseNotifier;
>> -
>> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
>> -QTAILQ_HEAD_INITIALIZER(close_notifiers);
>> -
>> -static void nbd_close_notifier(Notifier *n, void *data)
>> -{
>> -NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
>> -
>> -notifier_remove(>n);
>> -QTAILQ_REMOVE(_notifiers, cn, next);
>> -
>> -nbd_export_close(cn->exp);
>> -nbd_export_put(cn->exp);
> 
> Here you remove a close/put pair, but in the new code you only add the
> close call. Why is this correct?

Thanks for putting so much unfounded faith in me. :-)

After having put quite some time into looking into it, first I have to
say that it's a very good question.

First off, it's wrong. This is because I thought every export would have
a refcount of one. Turns out this is wrong, they have a refcount of two:
It is created with a refcount of one, and then it gains another by
giving it a name and entering it into the export list.

I did know about the second but I completely forgot about the former.
And indeed I do think it is wrong. qmp_nbd_server_add() does not keep
the reference to the export, once the function returns, it is gone.
Therefore, it should call nbd_export_put() before returning.

So in my opinion the current code is wrong and I didn't fix it enough.
*cough*

So, with the nbd_export_put() added to qmp_nbd_server_add(), every
export will have a refcount of 1 + |clients|. The eject notifier will
call nbd_close_notifier(), which first disconnects the clients, thus
reducing the refcount by |clients|, and then sets the name to NULL, thus
dropping the final refcount and destroying the export.

In the old code, we needed another nbd_export_put() because of the
superfluous reference from nbd_export_new(), which doesn't actually
exist for blockdev-nbd (it does for qemu-nbd, though).

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 01/40] 9pfs: allocate pdus with g_malloc/g_free

[Greg Kurz]

On Tue, 24 Nov 2015 19:00:52 +0100
Paolo Bonzini  wrote:

> Prepare for moving the allocation to virtqueue_pop.
> 
> Signed-off-by: Paolo Bonzini 
> ---

And aside from this series goal, it makes the code nicer :)

>  hw/9pfs/virtio-9p-device.c |  7 +--
>  hw/9pfs/virtio-9p.c| 10 +++---
>  hw/9pfs/virtio-9p.h|  2 --
>  3 files changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index e3abcfa..72a93c2 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -57,7 +57,7 @@ static void virtio_9p_device_realize(DeviceState *dev, 
> Error **errp)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>  V9fsState *s = VIRTIO_9P(dev);
> -int i, len;
> +int len;
>  struct stat stat;
>  FsDriverEntry *fse;
>  V9fsPath path;
> @@ -65,12 +65,7 @@ static void virtio_9p_device_realize(DeviceState *dev, 
> Error **errp)
>  virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P,
>  sizeof(struct virtio_9p_config) + MAX_TAG_LEN);
> 
> -/* initialize pdu allocator */
> -QLIST_INIT(>free_list);
>  QLIST_INIT(>active_list);
> -for (i = 0; i < (MAX_REQ - 1); i++) {
> -QLIST_INSERT_HEAD(>free_list, >pdus[i], next);
> -}
> 
>  s->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
> 
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index f972731..747306b 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -565,13 +565,9 @@ static int fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp, 
> V9fsQID *qidp)
> 
>  static V9fsPDU *alloc_pdu(V9fsState *s)
>  {
> -V9fsPDU *pdu = NULL;
> +V9fsPDU *pdu = g_new(V9fsPDU, 1);
> 
> -if (!QLIST_EMPTY(>free_list)) {
> -pdu = QLIST_FIRST(>free_list);
> -QLIST_REMOVE(pdu, next);
> -QLIST_INSERT_HEAD(>active_list, pdu, next);
> -}
> +QLIST_INSERT_HEAD(>active_list, pdu, next);
>  return pdu;
>  }
> 
> @@ -584,8 +580,8 @@ static void free_pdu(V9fsState *s, V9fsPDU *pdu)
>   */
>  if (!pdu->cancelled) {
>  QLIST_REMOVE(pdu, next);
> -QLIST_INSERT_HEAD(>free_list, pdu, next);
>  }
> +g_free(pdu);
>  }
>  }
> 
> diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
> index d7a4dc1..1fb4ff9 100644
> --- a/hw/9pfs/virtio-9p.h
> +++ b/hw/9pfs/virtio-9p.h
> @@ -201,8 +201,6 @@ typedef struct V9fsState
>  {
>  VirtIODevice parent_obj;
>  VirtQueue *vq;
> -V9fsPDU pdus[MAX_REQ];
> -QLIST_HEAD(, V9fsPDU) free_list;
>  QLIST_HEAD(, V9fsPDU) active_list;
>  V9fsFidState *fid_list;
>  FileOperations *ops;

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

[Programmingkid]

On Nov 30, 2015, at 11:26 AM, Kevin Wolf wrote:

> Am 30.11.2015 um 17:19 hat Eric Blake geschrieben:
>> On 11/27/2015 12:35 PM, Programmingkid wrote:
>> 
 Unusual indentation; more typical is:
 
 | static kern_return_t FindEjectableOpticalMedia(io_iterator_t
 *mediaIterator,
 | char *mediatType)
>>> 
>>> I agree. I wanted the second long to be right justified with the 80 
>>> character line count.
>> 
>> No.  We don't right-justify code to 80 columns.  That's not how it is
>> done.  Trying to do it just makes you look like the proverbial 'kid' in
>> your pseudonym, rather than an adult to be taken seriously.
>> 
>> Really, PLEASE follow the indentation patterns of the rest of the code
>> base - where continued lines are left-justified to be underneath the
>> character after (, and NOT right-justified to 80 columns.  Violating
>> style doesn't make your code invalid, but does make your patches less
>> likely to be applied.
>> 
>> 
> +/* If you found a match, leave the loop */
> +if (*mediaIterator != 0) {
> +DPRINTF("Matching using %s\n", matching_array[index]);
> +snprintf(mediaType, strlen(matching_array[index])+1, "%s",
 
 Spaces around binary '+'.
>>> 
>>> What's wrong with no spaces around the plus sign?
>> 
>> Again, the prevailing conventions in the code base is that you put
>> spaces around every binary operator.  Yes, there is existing old code
>> that does not meet the conventions, but it is not an excuse to add new
>> code that is gratuitously different.
>> 
>>> 
 
> +/* if a working partition on the device was not found */
> +if (partition_found == false) {
> +error_setg(errp, "Error: Failed to find a working partition on "
> + 
> "disc!\n");
 
 and I already pointed out on v8 that this is not the correct usage of
 error_setg().  So, here's hoping v10 addresses the comments here and
 elsewhere.
>>> 
>>> Kevin Wolf wanted it this way. What would you do instead?
>> 
>> Keven and I both want you to use error_setg(), but to use it correctly -
>> and the correct way is to NOT supply a trailing \n.
> 
> Nor leading "Error:", for that matter.

I just think that using "Error" does communicate the fact that something is 
wrong
a lot better than just printing the message. 

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

[Programmingkid]

On Nov 30, 2015, at 11:26 AM, Eric Blake wrote:

> On 11/27/2015 02:49 PM, Programmingkid wrote:
>> 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.
>> 
>> Signed-off-by: John Arbuckle 
>> 
>> ---
>> Fixed some spacing issues. 
>> Removed else condition in FindEjectableOpticalMedia.
>> Added continue statement to FindEjectableOpticalMedia.
>> Replaced printf() with error_report() in FindEjectableOpticalMedia.
>> Altered comment in FindEjectableOpticalMedia.
>> If the spacing in this patch looks off, try changing the font to something
>> that is mono-spaced.
> 
> Patches are best read in monospaced fonts, anyways; it's better to make
> that part of your workflow, and assume that everyone else has already
> done likewise, than to advertise that you are only making life harder
> for yourself.
> 
>> 
>> block/raw-posix.c |  140 ++--
>> 1 files changed, 102 insertions(+), 38 deletions(-)
>> 
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index ccfec1c..9e7de11 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -42,9 +42,9 @@
>> #include 
>> #include 
>> #include 
>> -//#include 
>> +#include 
>> #include 
>> -#endif
>> +#endif /* (__APPLE__) && (__MACH__) */
>> 
> 
> I have now mentioned in both v8 and v9 that this hunk should be its own
> patch (and is simple enough to cc qemu-trivial).  Disregarding reviewers
> suggestions is not a good idea - it only serves to waste time (both
> yours and reviewers) and earn you black marks, such that it will be even
> less likely that anyone wants to review your patches in the first place.
> I'm trying to help you be a better contributor, but it feels like you
> are ignoring advice, and so my natural reaction is to ignore you.

I assure you that this change is *required* for my patch. Without it the patch 
would
not even compile. I need a macro from IODVDMedia.h. If removing the IOCDTypes.h
is what is bothering you, it is a very small change that no one is going to 
miss. That
header file was commented out but not removed for some reason. 

I do thank you for your patients. I think it might be better if instead of 
saying "this is wrong",
you talk about what should be done differently more.

> 
>> #ifdef __sun__
>> #define _POSIX_PTHREAD_SEMANTICS 1
>> @@ -1975,32 +1975,46 @@ 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 kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>> +char 
>> *mediaType)
> 
> No, your indentation is still wrong.  I tried to point out on your v8
> that we don't right-justify to 80 columns, but rather left-justify to
> the point just after the (.

If you feel it is that important, I will do it. I just thought it was easier to 
read when your
eye is already in the area. There is less time spend finding the next argument 
that way.

> 
>> +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.\n",
> 
> No. Don't use trailing '.' or trailing '\n' in error_report() calls.
> I've already mentioned this, and feel like I'm becoming a broken record.
> When you disregard my review comments, I become disinclined to review
> your patches any further.

I don't remember hearing about not using \n in the error_report() call, but I 
will
fix this in the next patch.

> 
>> + 
>> matching_array[index]);
> 
> Indentation is still wrong.

Will left justify with the left parenthesis. 

> 
>> +continue;
>> +}
>> +CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
>> +
>> kCFBooleanTrue);
>> +kernResult = IOServiceGetMatchingServices(masterPort, 
>> classesToMatch,
>> + 
>> mediaIterator);
>> +if (kernResult != KERN_SUCCESS) {
>> +error_report("Note: IOServiceGetMatchingServices returned %d\n",
>> +
>> kernResult);
> 
> No trailing \n in error_report(), indentation is wrong.

Ok. 

> 
>> +}
>> 
>> +/* 

Re: [Qemu-block] [Qemu-devel] [PATCH v6 20/21] iotests: add incremental backup failure recovery test

[John Snow]

On 11/27/2015 12:14 PM, Kevin Wolf wrote:
> Am 18.04.2015 um 01:50 hat John Snow geschrieben:
>> Test the failure case for incremental backups.
>>
>> Signed-off-by: John Snow 
>> Reviewed-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/124 | 57 
>> ++
>>  tests/qemu-iotests/124.out |  4 ++--
>>  2 files changed, 59 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>> index 5c3b434..95f6de5 100644
>> --- a/tests/qemu-iotests/124
>> +++ b/tests/qemu-iotests/124
>> @@ -240,6 +240,63 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>  self.check_backups()
>>  
>>  
>> +def test_incremental_failure(self):
>> +'''Test: Verify backups made after a failure are correct.
>> +
>> +Simulate a failure during an incremental backup block job,
>> +emulate additional writes, then create another incremental backup
>> +afterwards and verify that the backup created is correct.
>> +'''
>> +
>> +# Create a blkdebug interface to this img as 'drive1',
>> +# but don't actually create a new image.
>> +drive1 = self.add_node('drive1', self.drives[0]['fmt'],
>> +   path=self.drives[0]['file'],
>> +   backup=self.drives[0]['backup'])
>> +result = self.vm.qmp('blockdev-add', options={
>> +'id': drive1['id'],
>> +'driver': drive1['fmt'],
>> +'file': {
>> +'driver': 'blkdebug',
>> +'image': {
>> +'driver': 'file',
>> +'filename': drive1['file']
>> +},
>> +'set-state': [{
>> +'event': 'flush_to_disk',
>> +'state': 1,
>> +'new_state': 2
>> +}],
>> +'inject-error': [{
>> +'event': 'read_aio',
>> +'errno': 5,
>> +'state': 2,
>> +'immediately': False,
>> +'once': True
>> +}],
>> +}
>> +})
>> +self.assert_qmp(result, 'return', {})
> 
> John, how naughty of you!
> 

And here I thought it was OK because nobody yelled!

The yell was just delayed.

> It's interesting how many tests break now that I tried to add some
> advisory qcow2 locking so that people don't constantly break their
> images with 'qemu-img snapshot' while the VM is running.
> 
> I think this one is a bug in the test case. I'm not completely sure how
> to fix it, though. Can we move the blkdebug layer to the top level? I
> think reusing the same qcow2 BDS (using a node name reference) would be
> okay. We just need to avoid opening the qcow2 layer twice for the same
> image.
> 

I can either do that, or just fall back to fully allocating two images
and modify the test accordingly.

> Kevin
> 

Is this for 2.5?

--js

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

[Eric Blake]

On 11/27/2015 12:35 PM, Programmingkid wrote:

>> Unusual indentation; more typical is:
>>
>> | static kern_return_t FindEjectableOpticalMedia(io_iterator_t
>> *mediaIterator,
>> | char *mediatType)
> 
> I agree. I wanted the second long to be right justified with the 80 character 
> line count.

No.  We don't right-justify code to 80 columns.  That's not how it is
done.  Trying to do it just makes you look like the proverbial 'kid' in
your pseudonym, rather than an adult to be taken seriously.

Really, PLEASE follow the indentation patterns of the rest of the code
base - where continued lines are left-justified to be underneath the
character after (, and NOT right-justified to 80 columns.  Violating
style doesn't make your code invalid, but does make your patches less
likely to be applied.


>>> +/* If you found a match, leave the loop */
>>> +if (*mediaIterator != 0) {
>>> +DPRINTF("Matching using %s\n", matching_array[index]);
>>> +snprintf(mediaType, strlen(matching_array[index])+1, "%s",
>>
>> Spaces around binary '+'.
> 
> What's wrong with no spaces around the plus sign?

Again, the prevailing conventions in the code base is that you put
spaces around every binary operator.  Yes, there is existing old code
that does not meet the conventions, but it is not an excuse to add new
code that is gratuitously different.

> 
>>
>>> +/* if a working partition on the device was not found */
>>> +if (partition_found == false) {
>>> +error_setg(errp, "Error: Failed to find a working partition on "
>>> + 
>>> "disc!\n");
>>
>> and I already pointed out on v8 that this is not the correct usage of
>> error_setg().  So, here's hoping v10 addresses the comments here and
>> elsewhere.
> 
> Kevin Wolf wanted it this way. What would you do instead?

Keven and I both want you to use error_setg(), but to use it correctly -
and the correct way is to NOT supply a trailing \n.

> 
> Thank you very much for reviewing my patches.

The least you can do for showing that gratitude is to actually improve
your next revisions along the lines of the comments you have been given.
 Quit making it feel like pulling teeth just to get your patches to
match the coding conventions prevalent in the project.

-- 
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 v2 05/21] block: Consider all block layer options in append_open_options

[Alberto Garcia]

On Mon 23 Nov 2015 04:59:44 PM CET, Kevin Wolf wrote:
> The code already special-cased "node-name", which is currently the only
> option passed in the QDict that isn't driver-specific. Generalise the
> code to take all general block layer options into consideration.
>
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Eric Blake 
> Reviewed-by: Max Reitz 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH v7 14/24] nbd: Switch from close to eject notifier

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> The NBD code uses the BDS close notifier to determine when a medium is
> ejected. However, now it should use the BB's BDS removal notifier for
> that instead of the BDS's close notifier.
> 
> Signed-off-by: Max Reitz 
> ---
>  blockdev-nbd.c | 37 +
>  nbd.c  | 13 +
>  2 files changed, 14 insertions(+), 36 deletions(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index bcdd18b..b28a55b 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -46,37 +46,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error 
> **errp)
>  }
>  }
>  
> -/*
> - * Hook into the BlockBackend notifiers to close the export when the
> - * backend is closed.
> - */
> -typedef struct NBDCloseNotifier {
> -Notifier n;
> -NBDExport *exp;
> -QTAILQ_ENTRY(NBDCloseNotifier) next;
> -} NBDCloseNotifier;
> -
> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
> -QTAILQ_HEAD_INITIALIZER(close_notifiers);
> -
> -static void nbd_close_notifier(Notifier *n, void *data)
> -{
> -NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
> -
> -notifier_remove(>n);
> -QTAILQ_REMOVE(_notifiers, cn, next);
> -
> -nbd_export_close(cn->exp);
> -nbd_export_put(cn->exp);

Here you remove a close/put pair, but in the new code you only add the
close call. Why is this correct?

Kevin

Re: [Qemu-block] [PATCH v2 06/21] block: Exclude nested options only for children in append_open_options()

[Max Reitz]

On 30.11.2015 10:01, Kevin Wolf wrote:
> Am 27.11.2015 um 18:58 hat Max Reitz geschrieben:
>> On 23.11.2015 16:59, Kevin Wolf wrote:
>>> Some drivers have nested options (e.g. blkdebug rule arrays), which
>>> don't belong to a child node and shouldn't be removed. Don't remove all
>>> options with "." in their name, but check for the complete prefixes of
>>> actually existing child nodes.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  block.c   | 19 +++
>>>  include/block/block_int.h |  1 +
>>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>
>> Thanks, now I don't need to fix it myself. :-)
>>
>> (I would have had to do that for an in-work series of mine)
>>
>>> diff --git a/block.c b/block.c
>>> index 23d9e10..02125e2 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const 
>>> char **pfilename,
>>>  
>>>  static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>>>  BlockDriverState *child_bs,
>>> +const char *child_name,
>>>  const BdrvChildRole *child_role)
>>>  {
>>>  BdrvChild *child = g_new(BdrvChild, 1);
>>>  *child = (BdrvChild) {
>>>  .bs = child_bs,
>>> +.name   = child_name,
>>>  .role   = child_role,
>>>  };
>>>  
>>> @@ -1165,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
>>> BlockDriverState *backing_hd)
>>>  bs->backing = NULL;
>>>  goto out;
>>>  }
>>> -bs->backing = bdrv_attach_child(bs, backing_hd, _backing);
>>> +bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
>>> _backing);
>>>  bs->open_flags &= ~BDRV_O_NO_BACKING;
>>>  pstrcpy(bs->backing_file, sizeof(bs->backing_file), 
>>> backing_hd->filename);
>>>  pstrcpy(bs->backing_format, sizeof(bs->backing_format),
>>> @@ -1322,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename,
>>>  goto done;
>>>  }
>>>  
>>> -c = bdrv_attach_child(parent, bs, child_role);
>>> +c = bdrv_attach_child(parent, bs, bdref_key, child_role);
>>>  
>>>  done:
>>>  qdict_del(options, bdref_key);
>>> @@ -3952,13 +3954,22 @@ static bool append_open_options(QDict *d, 
>>> BlockDriverState *bs)
>>>  {
>>>  const QDictEntry *entry;
>>>  QemuOptDesc *desc;
>>> +BdrvChild *child;
>>>  bool found_any = false;
>>> +const char *p;
>>>  
>>>  for (entry = qdict_first(bs->options); entry;
>>>   entry = qdict_next(bs->options, entry))
>>>  {
>>> -/* Only take options for this level */
>>> -if (strchr(qdict_entry_key(entry), '.')) {
>>> +/* Exclude options for children */
>>> +QLIST_FOREACH(child, >children, next) {
>>> +if (strstart(qdict_entry_key(entry), child->name, )
>>> +&& (!*p || *p == '.'))
>>> +{
>>> +break;
>>> +}
>>> +}
>>> +if (child) {
>>>  continue;
>>>  }
>>>  
>>
>> A good general solution, but I think bdrv_refresh_filename() may be bad
>> enough to break with general solutions. ;-)
>>
>> bdrv_refresh_filename() only considers "file" and "backing" (actually,
>> it only supports "file" for now, I'm working on "backing", though). The
>> only drivers with other children are quorum, blkdebug, blkverify and
>> VMDK. The former three have their own implementation of
>> bdrv_refresh_filename(), so they don't use append_open_options() at all.
>> The latter, however, (VMDK) does not.
>>
>> This change to append_open_options results in the extent.%d options
>> simply being omitted altogether because bdrv_refresh_filename() does not
>> fetch them. Before, they were included in the VMDK BDS's options, which
>> is not ideal but works more or less.
> 
> Are you sure? As far as I can tell, this patch should only keep options
> that were previously removed, but not remove options that were
> previously kept (with the exception of direct use of child names, which
> I added here to address your review comments for v1).
> 
> Specifically for "extents.%d", this is a child name and is therefore
> omitted. However, it contains a '.', so it was already removed without
> this patch.

Right, it is broken already. The same applies to qcow2's options
containing a dot.

Max

> I'm accepting proof of the contrary in the form of a test case. ;-)
> 
>> In order to "fix" this, I see three ways right now:
>> 1. Just care about "file" and "backing". bdrv_refresh_filename()
>>doesn't support anything else, so that will be fine.
>> 2. Implement bdrv_refresh_filename() specifically for VMDK so
>>append_open_options() will never have to handle anything but "file"
>>and "backing".
>> 3. Fix bdrv_refresh_filename() so that it handles all children and not
>>just "file" and "backing".
>>
>> Since we are shooting for 2.6 anyway (I assume ;-)), I 

Re: [Qemu-block] [PATCH v7 15/24] block: Remove BDS close notifier

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> It is unused now, so we can remove it.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH for-2.5] blkdebug: silence warning under qtest

[Max Reitz]

On 30.11.2015 12:44, Michael S. Tsirkin wrote:
> make check always outputs warnings, this
> is not nice.  Disable blkdebug warnings under qtest.
> 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  block/blkdebug.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Thanks, applied to my block tree:

https://github.com/XanClic/qemu/commits/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v7 24/24] iotests: Add test for block jobs and BDS ejection

[Max Reitz]

On 30.11.2015 17:23, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> Suggested-by: Paolo Bonzini 
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/141 | 166 
>> +
>>  tests/qemu-iotests/141.out |  47 +
>>  tests/qemu-iotests/group   |   1 +
>>  3 files changed, 214 insertions(+)
>>  create mode 100755 tests/qemu-iotests/141
>>  create mode 100644 tests/qemu-iotests/141.out
>>
>> diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
>> new file mode 100755
>> index 000..6a32d56
>> --- /dev/null
>> +++ b/tests/qemu-iotests/141
>> @@ -0,0 +1,166 @@
>> +#!/bin/bash
>> +#
>> +# Test case for ejecting BDSs with block jobs still running on them
>> +#
>> +# Copyright (C) 2015 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=mre...@redhat.com
>> +
>> +seq="$(basename $0)"
>> +echo "QA output created by $seq"
>> +
>> +here="$PWD"
>> +tmp=/tmp/$$
>> +status=1# failure is the default!
>> +
>> +_cleanup()
>> +{
>> +_cleanup_test_img
>> +rm -f "$TEST_DIR/{b,o}.$IMGFMT"
>> +}
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +. ./common.rc
>> +. ./common.filter
>> +. ./common.qemu
>> +
>> +# Needs backing file support
>> +_supported_fmt qcow qcow2 qed
> 
> The test doesn't work for me on qcow1.

Hm, and I thought I had tested it. Well, block jobs creating an overlay
file not being supported on qcow1 is probably all right.

>> +echo
>> +echo '=== Testing block-commit ==='
>> +echo
>> +
>> +# block-commit will send BLOCK_JOB_READY basically immediately, and 
>> cancelling
>> +# the job will consequently result in BLOCK_JOB_COMPLETED being emitted.
>> +
>> +test_blockjob \
>> +"{'execute': 'block-commit',
>> +  'arguments': {'device': 'drv0'}}" \
>> +'BLOCK_JOB_READY' \
>> +'BLOCK_JOB_COMPLETED'
> 
> This is commit of the active layer, i.e. just a mirror in disguise.
> Should we test a "real" commit block job as well?

Well, the op blocker we are testing is set by block_job_create(), so a
single block job would have sufficed. But now that I'm trying to test
them all, there's no reason not to test the real commit job, too.

> Anyway, with qcow1 removed from the list:
> Reviewed-by: Kevin Wolf 

Thanks!

Max



signature.asc
Description: OpenPGP digital signature

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

[Programmingkid]

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.

Signed-off-by: John Arbuckle 

---
error_report()'s had their \n, '.', and "Error:" removed.
Indentations are now at the left parenthesis rather than
at the 80 character mark.
Added spaces between the + sign.

 block/raw-posix.c |  135 +++--
 1 files changed, 99 insertions(+), 36 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index ccfec1c..39e523b 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -43,6 +43,7 @@
 #include 
 #include 
 //#include 
+#include 
 #include 
 #endif
 
@@ -1975,32 +1976,46 @@ 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 kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
+   char *mediaType)
 {
 kern_return_t   kernResult;
 mach_port_t masterPort;
 CFMutableDictionaryRef  classesToMatch;
+const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
 
 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);
+}
 
+/* If a match was found, leave the loop */
+if (*mediaIterator != 0) {
+DPRINTF("Matching using %s\n", matching_array[index]);
+snprintf(mediaType, strlen(matching_array[index]) + 1, "%s",
+ matching_array[index]);
+break;
+}
+}
 return kernResult;
 }
 
@@ -2033,7 +2048,35 @@ 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;
+}
+#endif /* defined(__APPLE__) && defined(__MACH__) */
 
 static int hdev_probe_device(const char *filename)
 {
@@ -2115,6 +2158,17 @@ static bool hdev_is_sg(BlockDriverState *bs)
 return false;
 }
 
+/* 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 

[Qemu-block] [QEMU 2.12] block in bdrv_drain_all()

[Qian Peng]

Hi,

  I am using qemu 2.1.2 and some VMs hang while playing videos.

QEMU parameters and stack information is as follows:
/usr/bin/qemu-system-x86_64
-name S398_ABC-047
-S -machine pc-i440fx-2.1,accel=kvm,usb=off
-m 1024
-realtime mlock=off
-smp 1,sockets=1,cores=1,threads=1
-uuid 2f9fa97a-0061-436c-a40c-e7abc3d0cdb1
-no-user-config -nodefaults
-chardev
socket,id=charmonitor,path=/var/lib/libvirt/qemu/S398_ABC-047.monitor,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control
-rtc base=localtime
-no-shutdown -global PIIX4_PM.disable_s3=1
-global PIIX4_PM.disable_s4=0 -boot strict=on
-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2
-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5
-drive
file=/opt/cvm/win7_64_S398_ABC-047.inst,if=none,id=drive-ide0-0-0,format=qcow2,cache=writeback,discard=unmap
-device
ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1
-drive
file=/opt/data/hugedisk/win7_64_S398_ABC-047_share.add,if=none,id=drive-ide0-0-1,format=qcow2,cache=writeback
-device ide-hd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -netdev
tap,fd=21,id=hostnet0,vhost=on,vhostfd=23
-device
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:19:a6:93,bus=pci.0,addr=0x3
-chardev pty,id=charserial0
-device isa-serial,chardev=charserial0,id=serial0 -chardev
pty,id=charserial1
-device isa-serial,chardev=charserial1,id=serial1 -chardev
spicevmc,id=charchannel0,name=vdagent
-device
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
-chardev spiceport,id=charchannel1,name=webcam
-device
virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,name=com.redhat.spice.webcam
-device usb-tablet,id=input0 -spice
port=5902,addr=0.0.0.0,disable-ticketing,seamless-migration=on -vnc
0.0.0.0:3
-device
qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x2
-device intel-hda,id=sound0,bus=pci.0,addr=0x4
-device hda-micro,id=sound0-codec0,bus=sound0.0,cad=0 -device
hda-duplex,id=sound0-codec1,bus=sound0.0,cad=1
-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6
-readconfig /etc/qemu/ich9-ehci-uhci.cfg
-chardev spicevmc,name=usbredir,id=usbredirchardev1 -device
usb-redir,chardev=usbredirchardev1,id=usbredirdev1,bus=ehci.0
-chardev spicevmc,name=usbredir,id=usbredirchardev2 -device
usb-redir,chardev=usbredirchardev2,id=usbredirdev2,bus=ehci.0
-chardev spicevmc,name=usbredir,id=usbredirchardev3 -device
usb-redir,chardev=usbredirchardev3,id=usbredirdev3,bus=ehci.0
-cpu SandyBridge,host=on,+vmx,model_id=Intel(R) Xeon(R) CPU E5-26xx
series,hv-relaxed=on,hv-time=on,hv-spinlocks=0x1fff

Thread 4 (Thread 0x7fa09a527700 (LWP 48069)):
#0  0x7fa09ec7b3e7 in ppoll () from /lib64/libc.so.6
#1  0x7fa0a2f7e27b in qemu_poll_ns (fds=,
nfds=, timeout=)
at /root/pq/rcc/merge/qemu/qemu-timer.c:314
#2  0x7fa0a2f7edf4 in aio_poll (ctx=0x7fa0a5835620, blocking=true)
at /root/pq/rcc/merge/qemu/aio-posix.c:250
#3  0x7fa0a2f782e8 in bdrv_drain_all ()
at /root/pq/rcc/merge/qemu/block.c:1930
#4  0x7fa0a2eb602a in bmdma_cmd_writeb (bm=0x7fa0a5ce8cc8, val=0)
at /root/pq/rcc/merge/qemu/hw/ide/pci.c:314
#5  0x7fa0a2d17e68 in access_with_adjusted_size (addr=0,
value=0x7fa09a526aa0, size=1, access_size_min=,
access_size_max=,
access=0x7fa0a2d19b10 , mr=0x7fa0a5ce8e30)
at /root/pq/rcc/merge/qemu/memory.c:481
#6  0x7fa0a2d1988c in memory_region_dispatch_write (mr=0x7fa0a5ce8e30,
addr=0, val=, size=1)
at /root/pq/rcc/merge/qemu/memory.c:1143
#7  io_mem_write (mr=0x7fa0a5ce8e30, addr=0, val=,
size=1)
at /root/pq/rcc/merge/qemu/memory.c:1976
#8  0x7fa0a2cdd73a in address_space_rw (as=0x7fa0a3412380,
addr=, buf=, len=1,
is_write=true) at /root/pq/rcc/merge/qemu/exec.c:2087
#9  0x7fa0a2d155c4 in kvm_handle_io (cpu=)
at /root/pq/rcc/merge/qemu/kvm-all.c:1597
#10 kvm_cpu_exec (cpu=)
at /root/pq/rcc/merge/qemu/kvm-all.c:1734
#11 0x7fa0a2d0470c in qemu_kvm_cpu_thread_fn (arg=0x7fa0a5ca95c0)
at /root/pq/rcc/merge/qemu/cpus.c:874
#12 0x7fa0a17ec9d1 in start_thread () from /lib64/libpthread.so.0
#13 0x7fa09ec849dd in clone () from /lib64/libc.so.6

Thread 3 (Thread 0x7fa0995ff700 (LWP 48071)):
#0  0x7fa09ec7b1b3 in poll () from /lib64/libc.so.6
#1  0x7fa09fbfb306 in ?? () from /usr/lib64/libspice-server.so.1
#2  0x7fa0a17ec9d1 in start_thread () from /lib64/libpthread.so.0
#3  0x7fa09ec849dd in clone () from /lib64/libc.so.6

Thread 2 (Thread 0x7fa098bff700 (LWP 48072)):
#0  0x7fa0a17f05bc in pthread_cond_wait@@GLIBC_2.3.2 ()
   from /lib64/libpthread.so.0
#1  0x7fa0a2fca499 in qemu_cond_wait (cond=,
mutex=)
at /root/pq/rcc/merge/qemu/util/qemu-thread-posix.c:135
#2  0x7fa0a2f6c9d3 in vnc_worker_thread_loop (queue=0x7fa0a5e62170)
at /root/pq/rcc/merge/qemu/ui/vnc-jobs.c:222
#3  0x7fa0a2f6cf80 in vnc_worker_thread (arg=0x7fa0a5e62170)
at /root/pq/rcc/merge/qemu/ui/vnc-jobs.c:323
#4  

Re: [Qemu-block] [PATCH v2 20/21] qemu-iotests: Test cache mode option inheritance

[Kevin Wolf]

Am 27.11.2015 um 22:12 hat Max Reitz geschrieben:
> On 23.11.2015 16:59, Kevin Wolf wrote:
> > This is doing a more complete test on setting cache modes both while
> > opening an image (i.e. in a -drive command line) and in reopen
> > situations. It checks that reopen can specify options for child nodes
> > and that cache modes are correctly inherited from parent nodes where
> > they are not specified.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  tests/qemu-iotests/142 | 354 
> >  tests/qemu-iotests/142.out | 788 
> > +
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 1143 insertions(+)
> >  create mode 100755 tests/qemu-iotests/142
> >  create mode 100644 tests/qemu-iotests/142.out
> 
> Comments below, to make it short: With %s/bs->backing_hd/bs->backing/
> and a comment about why you commented 'out | grep "Cache"':

Because I'm stupid. I'll fix that, thanks.

Kevin


pgpbhpHhtBJMY.pgp
Description: PGP signature

[Qemu-block] [PATCH for-2.5] blkdebug: silence warning under qtest

[Michael S. Tsirkin]

make check always outputs warnings, this
is not nice.  Disable blkdebug warnings under qtest.

Signed-off-by: Michael S. Tsirkin 
---
 block/blkdebug.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 6860a2b..dee3a0e 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -30,6 +30,7 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qint.h"
 #include "qapi/qmp/qstring.h"
+#include "sysemu/qtest.h"
 
 typedef struct BDRVBlkdebugState {
 int state;
@@ -583,9 +584,13 @@ static void suspend_request(BlockDriverState *bs, 
BlkdebugRule *rule)
 remove_rule(rule);
 QLIST_INSERT_HEAD(>suspended_reqs, , next);
 
-printf("blkdebug: Suspended request '%s'\n", r.tag);
+if (!qtest_enabled()) {
+printf("blkdebug: Suspended request '%s'\n", r.tag);
+}
 qemu_coroutine_yield();
-printf("blkdebug: Resuming request '%s'\n", r.tag);
+if (!qtest_enabled()) {
+printf("blkdebug: Resuming request '%s'\n", r.tag);
+}
 
 QLIST_REMOVE(, next);
 g_free(r.tag);
-- 
MST

Re: [Qemu-block] [Qemu-devel] [PATCH 05/40] virtio: read/write the VirtQueueElement a field at a time

[Paolo Bonzini]

On 30/11/2015 10:47, Fam Zheng wrote:
>> > +swap = (elem->out_num & 0x) || (elem->in_num & 0x);
> This is interesting, out_num and in_num are 32 bit numbers but there max 
> values
> are both VIRTQUEUE_MAX_SIZE (thanks for explaining this on IRC), so it can be 
> a
> clue for the source using a different endianness.

Yes, good idea.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 07/40] virtio: slim down allocation of VirtQueueElements

[Paolo Bonzini]

On 30/11/2015 04:24, Fam Zheng wrote:
> > +for (i = 0; i < out_num; i++) {
> > +elem->out_addr[i] = addr[i];
> > +elem->out_sg[i] = iov[i];
> > +}
>
> Isn't memcpy more efficient here? Otherwise looks good.

Probably not, out_num/in_num is usually very small, in fact one of them
is often 0 or 1.

For example the memcpy in address_space_rw is awfully inefficient.  This
is roughly the same.

Paolo

Re: [Qemu-block] [PATCH v3 01/15] block: Add "file" output parameter to block status query functions

[Stefan Hajnoczi]

On Thu, Nov 26, 2015 at 01:05:21PM +0800, Fam Zheng wrote:
> @@ -1535,13 +1541,14 @@ static int64_t coroutine_fn 
> bdrv_co_get_block_status(BlockDriverState *bs,
>  }
>  }
>  
> -if (bs->file &&
> +if (*file && *file != bs &&
>  (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
>  (ret & BDRV_BLOCK_OFFSET_VALID)) {

What is the purpose of this change?


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v3 00/15] qemu-img map: Allow driver to return file of the allocated block

[Stefan Hajnoczi]

On Thu, Nov 26, 2015 at 01:05:20PM +0800, Fam Zheng wrote:
> v3: Add Eric's rev-by in patches 6, 7, 13, 14.
> 12: New, split out from the previous 13.
> 12->13: Refactor "entry_mergable" from imp_map().
> Don't mess the merge conditions. [Paolo]
> Address Eric's comments:
> - Check has_foo before using foo.
> - Remove blank line between comments and definition in schema.
> - Use PRId64 instead of %ld.
> - Merge short lines.
> 
> v2: Add Eric's rev-by in patches 2, 4, 5.
> 01: Refering -> referring in commit message. [Eric]
> Recurse to "file" for sensible "zero" flag. [Paolo]
> 12: New. Make MapEntry a QAPI struct. [Paolo, Markus]
> 
> I stumbled upon this when looking at external bitmap formats.
> 
> Current "qemu-img map" command only displays filename if the data is allocated
> in bs (bs->file) itself, or in the backing chain. Otherwise, it displays an
> unfriendly error message:
> 
> $ qemu-img create -f vmdk -o subformat=monolithicFlat /tmp/test.vmdk 1G
> 
> $ qemu-img map /tmp/test.vmdk 
> Offset  Length  Mapped to   File
> qemu-img: File contains external, encrypted or compressed clusters.
> 
> This can be improved. This series extends the .bdrv_co_get_block_status
> callback, to let block driver return the BDS of file; then updates all driver
> to implement it; and lastly, it changes qemu-img to use this information in
> "map" command:
> 
> $ qemu-img map /tmp/test.vmdk 
> Offset  Length  Mapped to   File
> 0   0x4000  0   /tmp/test-flat.vmdk
> 
> $ qemu-img map --output json /tmp/test.vmdk 
> [{"length": 1073741824, "start": 0, "zero": false, "offset": 0, "depth": 
> 0,
>   "file": "/tmp/test-flat.vmdk", "data": true}
> ]
> 
> 
> Fam Zheng (15):
>   block: Add "file" output parameter to block status query functions
>   qcow: Assign bs->file->bs to file in qcow_co_get_block_status
>   qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status
>   raw: Assign bs to file in raw_co_get_block_status
>   iscsi: Assign bs to file in iscsi_co_get_block_status
>   parallels: Assign bs->file->bs to file in
> parallels_co_get_block_status
>   qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status
>   sheepdog: Assign bs to file in sd_co_get_block_status
>   vdi: Assign bs->file->bs to file in vdi_co_get_block_status
>   vpc: Assign bs->file->bs to file in vpc_co_get_block_status
>   vmdk: Return extent's file in bdrv_get_block_status
>   qemu-img: In "map", use the returned "file" from bdrv_get_block_status
>   qemu-img: Make MapEntry a QAPI struct
>   qemu-img: Use QAPI visitor to generate JSON
>   iotests: Add "qemu-img map" test for VMDK extents
> 
>  block/io.c |  42 ---
>  block/iscsi.c  |   9 +++-
>  block/mirror.c |   3 +-
>  block/parallels.c  |   3 +-
>  block/qcow.c   |   3 +-
>  block/qcow2.c  |   3 +-
>  block/qed.c|   6 ++-
>  block/raw-posix.c  |   4 +-
>  block/raw_bsd.c|   4 +-
>  block/sheepdog.c   |   5 ++-
>  block/vdi.c|   3 +-
>  block/vmdk.c   |  13 +++---
>  block/vpc.c|   4 +-
>  block/vvfat.c  |   2 +-
>  include/block/block.h  |   6 ++-
>  include/block/block_int.h  |   3 +-
>  qapi/block-core.json   |  27 
>  qemu-img.c | 102 
> +
>  tests/qemu-iotests/059 |  10 +
>  tests/qemu-iotests/059.out |  38 +
>  tests/qemu-iotests/122.out |  96 +++---
>  21 files changed, 270 insertions(+), 116 deletions(-)

Looks good overall but I posted a question on one patch.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v3 01/15] block: Add "file" output parameter to block status query functions

[Fam Zheng]

On Mon, 11/30 16:38, Stefan Hajnoczi wrote:
> On Thu, Nov 26, 2015 at 01:05:21PM +0800, Fam Zheng wrote:
> > @@ -1535,13 +1541,14 @@ static int64_t coroutine_fn 
> > bdrv_co_get_block_status(BlockDriverState *bs,
> >  }
> >  }
> >  
> > -if (bs->file &&
> > +if (*file && *file != bs &&
> >  (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
> >  (ret & BDRV_BLOCK_OFFSET_VALID)) {
> 
> What is the purpose of this change?

The code here is to sensibly detect "zero" by going into the "file" which the
offset is valid for. Now the "file" is no longer always "bs->file", so we use
the returned "file" pointer instead.

Fam

Re: [Qemu-block] [PATCH v2 01/21] qcow2: Add .bdrv_join_options callback

[Alberto Garcia]

On Mon 23 Nov 2015 04:59:40 PM CET, Kevin Wolf  wrote:
> qcow2 accepts a few driver-specific options that overlap semantically
> (e.g. "overlap-check" is an alias of "overlap-check.template", and any
> missing cache size option is derived from the given ones).
>
> When bdrv_reopen() merges the set of updated options with left out
> options that should be kept at their old value, we need to consider this
> and filter out any duplicates (which would generally cause errors
> because new and old value would contradict each other).
>
> This patch adds a .bdrv_join_options callback to BlockDriver and
> implements it for qcow2.
>
> Signed-off-by: Kevin Wolf 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH v2 02/21] block: Fix reopen with semantically overlapping options

[Alberto Garcia]

On Mon 23 Nov 2015 04:59:41 PM CET, Kevin Wolf wrote:
> This fixes bdrv_reopen() calls like the following one:
>
> qemu-io -c 'open -o overlap-check.template=all /tmp/test.qcow2' \
> -c 'reopen -o overlap-check=none'
>
> The approach taken so far would result in an options QDict that has both
> "overlap-check.template=all" and "overlap-check=none", which obviously
> conflicts. In this case, the old option should be overridden by the
> newly specified option.
>
> Signed-off-by: Kevin Wolf 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH v2 03/21] mirror: Error out when a BDS would get two BBs

[Alberto Garcia]

On Mon 23 Nov 2015 04:59:42 PM CET, Kevin Wolf wrote:

> @@ -370,11 +371,22 @@ static void mirror_exit(BlockJob *job, void *opaque)
>  if (s->to_replace) {
>  to_replace = s->to_replace;
>  }
> +
> +/* This was checked in mirror_start_job(), but meanwhile one of the
> + * nodes could have been newly attached to a BlockBackend. */
> +if (to_replace->blk && s->target->blk) {
> +error_report("block job: Can't create node with two 
> BlockBackends");
> +data->ret = -EINVAL;
> +goto out;
> +}

Does it make sense to even allow attaching a BDS to a Block Backend
during this block job? Is there any use case for that?

Berto