Re: [Qemu-block] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> The "flags" bit mask is expanded to two booleans, "data" and "zero";
> "bs" is replaced with "filename" string.
> 
> Signed-off-by: Fam Zheng 
> ---
>  qapi/block-core.json | 28 
>  qemu-img.c   | 48 ++--
>  2 files changed, 50 insertions(+), 26 deletions(-)
> 

> +##
> +
> +{ 'struct': 'MapEntry',

Blank line is not typical here.

> +  'data': {'start': 'int', 'length': 'int', 'data': 'bool',
> +   'zero': 'bool', 'depth': 'int', '*offset': 'int',
> +   '*filename': 'str' } }

Struct looks fine.

> 
> -if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == 
> BDRV_BLOCK_DATA) {
> +if (e->data && !e->zero) {
>  printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
> -   e->start, e->length, e->offset, e->bs->filename);
> +   e->start, e->length, e->offset,
> +   e->has_filename ? e->filename : "");
>  }

This blindly prints e->offset, even though...[1]

>  case OFORMAT_JSON:
> -printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": 
> %d,"
> +printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> +   " \"depth\": %ld,"

%ld is wrong; it needs to be PRId64.

> " \"zero\": %s, \"data\": %s",

Worth joining the two short lines into one?

> @@ -2219,10 +2208,15 @@ static int get_block_status(BlockDriverState *bs, 
> int64_t sector_num,
>  
>  e->start = sector_num * BDRV_SECTOR_SIZE;
>  e->length = nb_sectors * BDRV_SECTOR_SIZE;
> -e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
> +e->data = !!(ret & BDRV_BLOCK_DATA);
> +e->zero = !!(ret & BDRV_BLOCK_ZERO);
>  e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
> +e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);

[1]... offset might be garbage if has_offset is false.

>  e->depth = depth;
> -e->bs = bs;
> +if (e->has_offset) {
> +e->has_filename = true;
> +e->filename = bs->filename;
> +}
>  return 0;
>  }

Are we guaranteed that bs->filename is non-NULL when offset is set?  Or
does this need to be if (e->has_offset && bs->filename)?

>  
> @@ -2307,9 +2301,11 @@ static int img_map(int argc, char **argv)
>  goto out;
>  }
>  
> -if (curr.length != 0 && curr.flags == next.flags &&
> +if (curr.length != 0 && curr.zero == next.zero &&
> +curr.data == next.data &&
>  curr.depth == next.depth &&
> -((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
> +!strcmp(curr.filename, next.filename) &&

Is this strcmp valid even when !has_filename?

> +(!curr.has_offset ||
>   curr.offset + curr.length == next.offset)) {
>  curr.length += next.length;
>  continue;
> 

-- 
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 13/24] virtio-scsi: Catch BDS-BB removal/insertion

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Make use of the BDS-BB removal and insertion notifiers to remove or set
> up, respectively, virtio-scsi's op blockers.
> 
> Signed-off-by: Max Reitz 

> @@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  if (s->ctx) {
>  blk_op_unblock_all(sd->conf.blk, s->blocker);
>  }
> +
> +QTAILQ_FOREACH(insert_notifier, >insert_notifiers, next) {
> +if (insert_notifier->sd == sd) {
> +break;
> +}
> +}
> +if (insert_notifier) {
> +notifier_remove(_notifier->n);
> +QTAILQ_REMOVE(>insert_notifiers, insert_notifier, next);
> +g_free(insert_notifier);
> +}

Why a separate if block instead of just doing that inside the loop?

> +QTAILQ_FOREACH(remove_notifier, >remove_notifiers, next) {
> +if (remove_notifier->sd == sd) {
> +break;
> +}
> +}
> +if (remove_notifier) {
> +notifier_remove(_notifier->n);
> +QTAILQ_REMOVE(>remove_notifiers, remove_notifier, next);
> +g_free(remove_notifier);
> +}
> +
>  qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>  }

Kevin

Re: [Qemu-block] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Max Reitz]

On 25.11.2015 16:57, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> Put the code for setting up and removing op blockers into an own
>> function, respectively. Then, we can invoke those functions whenever a
>> BDS is removed from an virtio-blk BB or inserted into it.
>>
>> Signed-off-by: Max Reitz 
> 
> Do you know of a case where this is observable?

Actually, no.

> I don't think you can
> change the medium of a virtio-blk device, and blk_set_bs() isn't
> converted to a wrapper around blk_remove/insert_bs() yet. If this patch
> is necessary to fix something observable, the latter is probably a bug.

So I guess that implies "Otherwise, this patch should be dropped"?

>> diff --git a/hw/block/dataplane/virtio-blk.c 
>> b/hw/block/dataplane/virtio-blk.c
>> index c42ddeb..4c95d5b 100644
>> --- a/hw/block/dataplane/virtio-blk.c
>> +++ b/hw/block/dataplane/virtio-blk.c
>> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane {
>>  EventNotifier *guest_notifier;  /* irq */
>>  QEMUBH *bh; /* bh for guest notification */
>>  
>> +Notifier insert_notifier, remove_notifier;
>> +
>>  /* Note that these EventNotifiers are assigned by value.  This is
>>   * fine as long as you do not call event_notifier_cleanup on them
>>   * (because you don't own the file descriptor or handle; you just
>> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e)
>>  blk_io_unplug(s->conf->conf.blk);
>>  }
>>  
>> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
>> +{
>> +assert(!s->blocker);
>> +error_setg(>blocker, "block device is in use by data plane");
>> +blk_op_block_all(s->conf->conf.blk, s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, 
>> s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, 
>> s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, 
>> s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
>> +   s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
>> +   s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, 
>> BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
>> +   s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
>> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
>> +}
> 
> This makes me wonder: What do we even block here any more? If I didn't
> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
> why this needs to be blocked, or if we simply forgot to enable it.

Well, even though in practice this wall of code doesn't make much sense,
of course it will be safe for potential additions of new op blockers.

And of course we actually don't want these blockers at all anymore...

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PULL 0/6] Block patches for 2.5.0-rc2

[Peter Maydell]

On 25 November 2015 at 14:10, Kevin Wolf  wrote:
> The following changes since commit 1aae36df4b8ed884c6ef6995e70c67fad79b49df:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-ivshmem-2015-11-25' 
> into staging (2015-11-25 11:38:03 +)
>
> are available in the git repository at:
>
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 8c34d891b1594840d8394a3c9b92236c13254fd8:
>
>   Merge remote-tracking branch 'mreitz/tags/pull-block-for-kevin-2015-11-25' 
> into queue-block (2015-11-25 14:33:01 +0100)
>
> 
>
> Block layer patches
>
> 
> Fam Zheng (1):
>   qemu-iotests: Add -nographic when starting QEMU in 119 and 120
>
> Kevin Wolf (3):
>   tests/Makefile: Add more dependencies for test-timed-average
>   test-aio: Fix event notifier cleanup
>   Merge remote-tracking branch 
> 'mreitz/tags/pull-block-for-kevin-2015-11-25' into queue-block
>
> Markus Armbruster (1):
>   block/qapi: Plug memory leak on query-block error path
>
> Programmingkid (1):
>   raw-posix.c: Make GetBSDPath() handle caching options
>
> Ricard Wanderlof (1):
>   nand: fix flash erase when oob is in memory
>
>  block/qapi.c   |  8 +++-
>  block/raw-posix.c  | 15 +--
>  hw/block/nand.c|  2 +-
>  tests/Makefile |  3 +--
>  tests/qemu-iotests/119 |  2 +-
>  tests/qemu-iotests/120 |  2 +-
>  tests/test-aio.c   |  1 +
>  7 files changed, 17 insertions(+), 16 deletions(-)

Applied, thanks.

-- PMM

Re: [Qemu-block] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Put the code for setting up and removing op blockers into an own
> function, respectively. Then, we can invoke those functions whenever a
> BDS is removed from an virtio-blk BB or inserted into it.
> 
> Signed-off-by: Max Reitz 

Do you know of a case where this is observable? I don't think you can
change the medium of a virtio-blk device, and blk_set_bs() isn't
converted to a wrapper around blk_remove/insert_bs() yet. If this patch
is necessary to fix something observable, the latter is probably a bug.

> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index c42ddeb..4c95d5b 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane {
>  EventNotifier *guest_notifier;  /* irq */
>  QEMUBH *bh; /* bh for guest notification */
>  
> +Notifier insert_notifier, remove_notifier;
> +
>  /* Note that these EventNotifiers are assigned by value.  This is
>   * fine as long as you do not call event_notifier_cleanup on them
>   * (because you don't own the file descriptor or handle; you just
> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e)
>  blk_io_unplug(s->conf->conf.blk);
>  }
>  
> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
> +{
> +assert(!s->blocker);
> +error_setg(>blocker, "block device is in use by data plane");
> +blk_op_block_all(s->conf->conf.blk, s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, 
> s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, 
> s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, 
> s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
> +   s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
> +   s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
> +   s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
> +}

This makes me wonder: What do we even block here any more? If I didn't
miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
why this needs to be blocked, or if we simply forgot to enable it.

Kevin

Re: [Qemu-block] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Kevin Wolf]

Am 25.11.2015 um 17:03 hat Max Reitz geschrieben:
> On 25.11.2015 16:57, Kevin Wolf wrote:
> > Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> >> Put the code for setting up and removing op blockers into an own
> >> function, respectively. Then, we can invoke those functions whenever a
> >> BDS is removed from an virtio-blk BB or inserted into it.
> >>
> >> Signed-off-by: Max Reitz 
> > 
> > Do you know of a case where this is observable?
> 
> Actually, no.
> 
> > I don't think you can
> > change the medium of a virtio-blk device, and blk_set_bs() isn't
> > converted to a wrapper around blk_remove/insert_bs() yet. If this patch
> > is necessary to fix something observable, the latter is probably a bug.
> 
> So I guess that implies "Otherwise, this patch should be dropped"?

I'm not sure. I guess you had a reason to include these patches other
than putting the notifiers to use?

With blk_set_bs() changed, I think it would have an effect: The op
blockers would move from the old BDS to the new top-level one. This
sounds desirable to me.

> >> diff --git a/hw/block/dataplane/virtio-blk.c 
> >> b/hw/block/dataplane/virtio-blk.c
> >> index c42ddeb..4c95d5b 100644
> >> --- a/hw/block/dataplane/virtio-blk.c
> >> +++ b/hw/block/dataplane/virtio-blk.c
> >> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane {
> >>  EventNotifier *guest_notifier;  /* irq */
> >>  QEMUBH *bh; /* bh for guest notification */
> >>  
> >> +Notifier insert_notifier, remove_notifier;
> >> +
> >>  /* Note that these EventNotifiers are assigned by value.  This is
> >>   * fine as long as you do not call event_notifier_cleanup on them
> >>   * (because you don't own the file descriptor or handle; you just
> >> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e)
> >>  blk_io_unplug(s->conf->conf.blk);
> >>  }
> >>  
> >> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
> >> +{
> >> +assert(!s->blocker);
> >> +error_setg(>blocker, "block device is in use by data plane");
> >> +blk_op_block_all(s->conf->conf.blk, s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, 
> >> s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, 
> >> s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, 
> >> s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, 
> >> s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
> >> +   s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
> >> +   s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, 
> >> BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
> >> +   s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
> >> +blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);
> >> +}
> > 
> > This makes me wonder: What do we even block here any more? If I didn't
> > miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
> > why this needs to be blocked, or if we simply forgot to enable it.
> 
> Well, even though in practice this wall of code doesn't make much sense,
> of course it will be safe for potential additions of new op blockers.
> 
> And of course we actually don't want these blockers at all anymore...

Yes, but dataplane shouldn't really be special enough any more that we
want to disable features for it initially. By now it sounds more like an
easy way to forget unblocking a new feature even though it would work.

So perhaps we should really just remove the blockers from dataplane.
Then we don't have to answer the question above...

Kevin


pgp6l8ch6aTn6.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH v2 14/14] iotests: Add "qemu-img map" test for VMDK extents

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  tests/qemu-iotests/059 | 10 ++
>  tests/qemu-iotests/059.out | 38 ++
>  2 files changed, 48 insertions(+)

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 v2 13/14] qemu-img: Use QAPI visitor to generate JSON

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> A visible improvement is that "filename" is now included in the output
> if it's valid.
> 
> Signed-off-by: Fam Zheng 
> ---
>  qemu-img.c | 39 ---
>  tests/qemu-iotests/122.out | 96 
> ++
>  2 files changed, 79 insertions(+), 56 deletions(-)
> 

> @@ -2155,21 +2161,26 @@ static void dump_map_entry(OutputFormat 
> output_format, MapEntry *e,
>  }
>  break;
>  case OFORMAT_JSON:
> -printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> -   " \"depth\": %ld,"
> -   " \"zero\": %s, \"data\": %s",
> -   (e->start == 0 ? "[" : ",\n"),
> -   e->start, e->length, e->depth,
> -   e->zero ? "true" : "false",
> -   e->data ? "true" : "false");
> -if (e->has_offset) {
> -printf(", \"offset\": %"PRId64"", e->offset);
> -}
> -putchar('}');
> +ov = qmp_output_visitor_new();
> +visit_type_MapEntry(qmp_output_get_visitor(ov),
> +, NULL, _abort);
> +obj = qmp_output_get_qobject(ov);
> +str = qobject_to_json(obj);

It would be nice to someday add a visitor that goes directly to json,
instead of through an intermediate QObject. But that's not for this
series; your conversion here is sane.

> @@ -2213,9 +2224,9 @@ static int get_block_status(BlockDriverState *bs, 
> int64_t sector_num,
>  e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
>  e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
>  e->depth = depth;
> -if (e->has_offset) {
> +if (file && e->has_offset) {
>  e->has_filename = true;
> -e->filename = bs->filename;
> +e->filename = file->filename;

Does this hunk belong in a different patch?

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 0/2] import nvme fix

[Keith Busch]

On Wed, Nov 25, 2015 at 10:28:42AM +0100, Kevin Wolf wrote:
> Am 18.11.2015 um 19:53 hat Christoph Hellwig geschrieben:
> > First one fixes Identify to behave as mandated by the spec, and the
> > second bumps the PCI revision so that guest drivers can detect
> > the fixed version of the device so that only the old version has
> > to be blacklisted.
> 
> Keith, this looks to me like a fix that should still be merged for 2.5,
> would you agree? Can you please have a look at the series and either
> give your Acked-by or comment?

The series looks good to me. I had some difficulty finding the right
patches in the midst of Christoph's Linux patch bombs. :)

Acked-by: Keith Busch 

Re: [Qemu-block] [PATCH v7 for-2.6 00/24] block: Rework bdrv_close_all()

[Kevin Wolf]

Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Currently, bdrv_close_all() force-closes all BDSs with a BlockBackend,
> which can lead to data corruption (see the iotest added in the final
> patch of this series) and is most certainly very ugly.
> 
> This series reworks bdrv_close_all() to instead eject the BDS trees from
> all BlockBackends and then close the monitor-owned BDS trees, which are
> the only BDSs without a BB. In effect, all BDSs are closed just by
> getting closed automatically due to their reference count becoming 0.
> 
> Note that the approach taken here leaks all BlockBackends. This does not
> really matter, however, since qemu is about to exit anyway.

Patches 4-11:
Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v2 07/14] qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  block/qed.c | 3 +++
>  1 file changed, 3 insertions(+)
> 

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 v7 13/24] virtio-scsi: Catch BDS-BB removal/insertion

[Max Reitz]

On 25.11.2015 17:03, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> Make use of the BDS-BB removal and insertion notifiers to remove or set
>> up, respectively, virtio-scsi's op blockers.
>>
>> Signed-off-by: Max Reitz 
> 
>> @@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler 
>> *hotplug_dev, DeviceState *dev,
>>  if (s->ctx) {
>>  blk_op_unblock_all(sd->conf.blk, s->blocker);
>>  }
>> +
>> +QTAILQ_FOREACH(insert_notifier, >insert_notifiers, next) {
>> +if (insert_notifier->sd == sd) {
>> +break;
>> +}
>> +}
>> +if (insert_notifier) {
>> +notifier_remove(_notifier->n);
>> +QTAILQ_REMOVE(>insert_notifiers, insert_notifier, next);
>> +g_free(insert_notifier);
>> +}
> 
> Why a separate if block instead of just doing that inside the loop?

Because I unconsciously try to reduce indentation.

Also, because when I wrote the code, to me it was "Find the relevant
notifier -- destroy that notifier", and that's how this pattern came about.

I personally still like it more the way I did it, but I can very well
imagine that I'm the only one who thinks so. Therefore, I wouldn't mind
changing it (besides the effort involved to change it).

Max

>> +QTAILQ_FOREACH(remove_notifier, >remove_notifiers, next) {
>> +if (remove_notifier->sd == sd) {
>> +break;
>> +}
>> +}
>> +if (remove_notifier) {
>> +notifier_remove(_notifier->n);
>> +QTAILQ_REMOVE(>remove_notifiers, remove_notifier, next);
>> +g_free(remove_notifier);
>> +}
>> +
>>  qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>>  }
> 
> Kevin
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 06/14] parallels: Assign bs->file->bs to file in parallels_co_get_block_status

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  block/parallels.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake 

> 
> diff --git a/block/parallels.c b/block/parallels.c
> index d1146f1..6552f32 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -273,6 +273,7 @@ static int64_t coroutine_fn 
> parallels_co_get_block_status(BlockDriverState *bs,
>  return 0;
>  }
>  
> +*file = bs->file->bs;
>  return (offset << BDRV_SECTOR_BITS) |
>  BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
>  }
> 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v8] 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 

---
 block/raw-posix.c |   98 +++--
 1 files changed, 72 insertions(+), 26 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index ccfec1c..d0190c1 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -42,9 +42,8 @@
 #include 
 #include 
 #include 
-//#include 
 #include 
-#endif
+#endif /* (__APPLE__) && (__MACH__) */
 
 #ifdef __sun__
 #define _POSIX_PTHREAD_SEMANTICS 1
@@ -2033,7 +2032,36 @@ 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, "Error: Failed to find a working partition on "
+ 
"disc!\n");
+} 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 +2143,17 @@ static bool hdev_is_sg(BlockDriverState *bs)
 return false;
 }
 
+/* Prints directions on mounting and unmounting a device */
+static void printUnmountingDirections(const char *file_name)
+{
+error_report("Error: If device %s is mounted on the desktop, unmount"
+ " it first before using it in QEMU.\n", 
file_name);
+error_report("Command to unmount device: diskutil unmountDisk %s\n",
+ 
file_name);
+error_report("Command to mount device: diskutil mountDisk %s\n",
+ 
file_name);
+}
+
 static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
  Error **errp)
 {
@@ -2123,32 +2162,32 @@ static int hdev_open(BlockDriverState *bs, QDict 
*options, int flags,
 int ret;
 
 #if defined(__APPLE__) && defined(__MACH__)
-const char *filename = qdict_get_str(options, "filename");
+const char *file_name = qdict_get_str(options, "filename");
 
-if (strstart(filename, "/dev/cdrom", NULL)) {
-kern_return_t kernResult;
+/* If using a real cdrom */
+if (strcmp(file_name, "/dev/cdrom") == 0) {
+char bsd_path[MAXPATHLEN];
 io_iterator_t mediaIterator;
-char bsdPath[ MAXPATHLEN ];
-int fd;
-
-kernResult = FindEjectableCDMedia(  );
-kernResult = GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath),
-flags);
-if ( bsdPath[ 0 ] != '\0' ) {
-strcat(bsdPath,"s0");
-/* some CDs don't have a partition 0 */
-fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
-if (fd < 0) {
-bsdPath[strlen(bsdPath)-1] = '1';
-} else {
-qemu_close(fd);
-}
-filename = bsdPath;
-qdict_put(options, "filename", qstring_from_str(filename));
+FindEjectableCDMedia();
+GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
+if (mediaIterator) {
+IOObjectRelease(mediaIterator);
+}
+
+/* If a real optical drive was not found */
+if (bsd_path[0] == '\0') {
+error_setg(errp, "Error: failed to obtain bsd path for optical"
+   " 
drive!\n");
+return -1;
 }
 
-if ( mediaIterator )
-IOObjectRelease( mediaIterator );
+/* If finding a partition on the cdrom disc failed */
+if (setup_cdrom(bsd_path, errp) == false) {
+printUnmountingDirections(bsd_path);
+return -1;
+}
+file_name = bsd_path;
+qdict_put(options, "filename", 

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

[Eric Blake]

On 11/25/2015 05:24 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 
> 
> ---

Right here (between the --- and diffstat) it's nice to post a changelog
of how v8 differs from v7, to help earlier reviewers focus on the
improvements.

>  block/raw-posix.c |   98 
> +++--
>  1 files changed, 72 insertions(+), 26 deletions(-)

> +++ b/block/raw-posix.c
> @@ -42,9 +42,8 @@
>  #include 
>  #include 
>  #include 
> -//#include 
>  #include 
> -#endif
> +#endif /* (__APPLE__) && (__MACH__) */
>  

This hunk looks to be an unrelated cleanup; you might want to just
propose it separately through the qemu-trivial queue (but don't forget
that even trivial patches must cc qemu-devel).

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

Unusual indentation.  More typical would be:

snprintf(test_partition, sizeof(test_partition), "%ss%d",
 bsd_path, index);

with the second line flush to the character after the ( of the first line.

> +fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);

Isn't qemu_open() supposed to provide O_LARGEFILE for ALL users
automatically?  (That is, why would we ever _want_ to handle a file
using only 32-bit off_t?)  But that's a separate issue; it looks like
you are copy-and-pasting from existing use of this idiom already in
raw-posix.c.

> +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, "Error: Failed to find a working partition on "
> + 
> "disc!\n");

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.

error_setg(errp, "failed to find a working partition on disk");

>  
> +/* Prints directions on mounting and unmounting a device */
> +static void printUnmountingDirections(const char *file_name)

Elsewhere, we use 'function_name', not 'functionName'.

> +{
> +error_report("Error: If device %s is mounted on the desktop, unmount"
> + " it first before using it in QEMU.\n", 
> file_name);
> +error_report("Command to unmount device: diskutil unmountDisk %s\n",
> + 
> file_name);
> +error_report("Command to mount device: diskutil mountDisk %s\n",
> + 
> file_name);

Again, weird indentation. And don't use \n at the end of error_report().

> @@ -2123,32 +2162,32 @@ static int hdev_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  int ret;
>  
>  #if defined(__APPLE__) && defined(__MACH__)
> -const char *filename = qdict_get_str(options, "filename");
> +const char *file_name = qdict_get_str(options, "filename");

No need to rename this variable.

> +
> +/* If a real optical drive was not found */
> +if (bsd_path[0] == '\0') {
> +error_setg(errp, "Error: failed to obtain bsd path for optical"
> +   " 
> drive!\n");

Again, weird indentation, redundant "Error: ", and no "!\n" at the end.

>  
> +#if defined(__APPLE__) && defined(__MACH__)
> +/* if a physical device experienced an error while being opened */
> +if (strncmp(file_name, "/dev/", 5) == 0 && ret != 0) {
> +printUnmountingDirections(file_name);

Is this advice appropriate to ALL things under /dev/, or just cdroms?

> +return -1;
> +}
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> +
>  /* Since this does ioctl the device must be already opened */
>  bs->sg = hdev_is_sg(bs);
>  
> 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v9] 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 

---
Added DVD support - real DVD media can now be used in QEMU!
Changed name of FindEjectableCDMedia to FindEjectableOpticalMedia.
Added mediaType parameter to FindEjectableOpticalMedia() for media 
indentification.
Removed unneeded FindEjectableCDMedia() prototype.
FindEjectableOpticalMedia() now searches for both DVD's and CD's.

 block/raw-posix.c |  138 ++---
 1 files changed, 100 insertions(+), 38 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index ccfec1c..a11a9e7 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__) */
 
 #ifdef __sun__
 #define _POSIX_PTHREAD_SEMANTICS 1
@@ -1975,32 +1975,44 @@ 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) {
+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);
+}
 
+/* 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",
+ 
matching_array[index]);
+break;
+}
+}
 return kernResult;
 }
 
@@ -2033,7 +2045,36 @@ 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, "Error: Failed to find a working partition on "
+ 
"disc!\n");
+} 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)
 {
@@ 

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/25/2015 09:10 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 
> 
> ---
> Added DVD support - real DVD media can now be used in QEMU!
> Changed name of FindEjectableCDMedia to FindEjectableOpticalMedia.
> Added mediaType parameter to FindEjectableOpticalMedia() for media 
> indentification.
> Removed unneeded FindEjectableCDMedia() prototype.
> FindEjectableOpticalMedia() now searches for both DVD's and CD's.
> 

> +++ b/block/raw-posix.c
> @@ -42,9 +42,9 @@
>  #include 
>  #include 
>  #include 
> -//#include 
> +#include 
>  #include 
> -#endif
> +#endif /* (__APPLE__) && (__MACH__) */
>  

I don't know if my review of v8 crossed your posting of this patch, but
I suggested that this hunk belongs in its own patch.

>  #ifdef __sun__
>  #define _POSIX_PTHREAD_SEMANTICS 1
> @@ -1975,32 +1975,44 @@ 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)

Unusual indentation; more typical is:

| static kern_return_t FindEjectableOpticalMedia(io_iterator_t
*mediaIterator,
| char *mediatType)


> +int index;
> +for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
> +classesToMatch = IOServiceMatching(matching_array[index]);
> +if (classesToMatch == NULL) {
> +printf("IOServiceMatching returned a NULL dictionary.\n");

Leftover debugging?

> +} else {
> +CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),

Missing indentation.

> +
> kCFBooleanTrue);
> +}
> +kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
> + 
> mediaIterator);

More unusual indentation.

> +if (KERN_SUCCESS != kernResult) {
> +printf("IOServiceGetMatchingServices returned %d\n", kernResult);
> +}
>  
> +/* 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 '+'.

> +/* 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.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] QEMU being able to use audio cdroms

[Programmingkid]

Is there any platform where a guest in QEMU can play an audio cd? If not, is 
this a feature that you would allow into QEMU?

Re: [Qemu-block] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct

[Fam Zheng]

On Wed, 11/25 08:36, Eric Blake wrote:
> On 11/25/2015 12:39 AM, Fam Zheng wrote:
> > The "flags" bit mask is expanded to two booleans, "data" and "zero";
> > "bs" is replaced with "filename" string.
> > 
> > Signed-off-by: Fam Zheng 
> > ---
> >  qapi/block-core.json | 28 
> >  qemu-img.c   | 48 ++--
> >  2 files changed, 50 insertions(+), 26 deletions(-)
> > 
> 
> > +##
> > +
> > +{ 'struct': 'MapEntry',
> 
> Blank line is not typical here.

This is copied from ImageInfo above. Removing.

> 
> > +  'data': {'start': 'int', 'length': 'int', 'data': 'bool',
> > +   'zero': 'bool', 'depth': 'int', '*offset': 'int',
> > +   '*filename': 'str' } }
> 
> Struct looks fine.
> 
> > 
> > -if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == 
> > BDRV_BLOCK_DATA) {
> > +if (e->data && !e->zero) {
> >  printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
> > -   e->start, e->length, e->offset, e->bs->filename);
> > +   e->start, e->length, e->offset,
> > +   e->has_filename ? e->filename : "");
> >  }
> 
> This blindly prints e->offset, even though...[1]

Will add check.

> 
> >  case OFORMAT_JSON:
> > -printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", 
> > \"depth\": %d,"
> > +printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> > +   " \"depth\": %ld,"
> 
> %ld is wrong; it needs to be PRId64.

Yes.

> 
> > " \"zero\": %s, \"data\": %s",
> 
> Worth joining the two short lines into one?

OK.

> 
> > @@ -2219,10 +2208,15 @@ static int get_block_status(BlockDriverState *bs, 
> > int64_t sector_num,
> >  
> >  e->start = sector_num * BDRV_SECTOR_SIZE;
> >  e->length = nb_sectors * BDRV_SECTOR_SIZE;
> > -e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
> > +e->data = !!(ret & BDRV_BLOCK_DATA);
> > +e->zero = !!(ret & BDRV_BLOCK_ZERO);
> >  e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
> > +e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
> 
> [1]... offset might be garbage if has_offset is false.
> 
> >  e->depth = depth;
> > -e->bs = bs;
> > +if (e->has_offset) {
> > +e->has_filename = true;
> > +e->filename = bs->filename;
> > +}
> >  return 0;
> >  }
> 
> Are we guaranteed that bs->filename is non-NULL when offset is set?  Or
> does this need to be if (e->has_offset && bs->filename)?

It's an array field:

struct BlockDriverState {
...
char filename[PATH_MAX];

So I think this is OK.

> 
> >  
> > @@ -2307,9 +2301,11 @@ static int img_map(int argc, char **argv)
> >  goto out;
> >  }
> >  
> > -if (curr.length != 0 && curr.flags == next.flags &&
> > +if (curr.length != 0 && curr.zero == next.zero &&
> > +curr.data == next.data &&
> >  curr.depth == next.depth &&
> > -((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
> > +!strcmp(curr.filename, next.filename) &&
> 
> Is this strcmp valid even when !has_filename?

No, will check it.

Thanks for reviewing!

Fam

> 
> > +(!curr.has_offset ||
> >   curr.offset + curr.length == next.offset)) {
> >  curr.length += next.length;
> >  continue;
> > 
> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

[Qemu-block] [PATCH v3 08/15] sheepdog: Assign bs to file in sd_co_get_block_status

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block/sheepdog.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0f6789e..d5e7ff8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2740,6 +2740,9 @@ sd_co_get_block_status(BlockDriverState *bs, int64_t 
sector_num, int nb_sectors,
 if (*pnum > nb_sectors) {
 *pnum = nb_sectors;
 }
+if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
+*file = bs;
+}
 return ret;
 }
 
-- 
2.4.3

[Qemu-block] [PATCH v3 10/15] vpc: Assign bs->file->bs to file in vpc_co_get_block_status

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block/vpc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/vpc.c b/block/vpc.c
index 912f5d0..412ff41 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -588,6 +588,7 @@ static int64_t coroutine_fn 
vpc_co_get_block_status(BlockDriverState *bs,
 
 if (be32_to_cpu(footer->type) == VHD_FIXED) {
 *pnum = nb_sectors;
+*file = bs->file->bs;
 return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
(sector_num << BDRV_SECTOR_BITS);
 }
@@ -609,6 +610,7 @@ static int64_t coroutine_fn 
vpc_co_get_block_status(BlockDriverState *bs,
 /* *pnum can't be greater than one block for allocated
  * sectors since there is always a bitmap in between. */
 if (allocated) {
+*file = bs->file->bs;
 return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
 }
 if (nb_sectors == 0) {
-- 
2.4.3

[Qemu-block] [PATCH v3 03/15] qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 836888c..7634c42 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1302,6 +1302,7 @@ static int64_t coroutine_fn 
qcow2_co_get_block_status(BlockDriverState *bs,
 !s->cipher) {
 index_in_cluster = sector_num & (s->cluster_sectors - 1);
 cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+*file = bs->file->bs;
 status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset;
 }
 if (ret == QCOW2_CLUSTER_ZERO) {
-- 
2.4.3

[Qemu-block] [PATCH v3 05/15] iscsi: Assign bs to file in iscsi_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake 
Signed-off-by: Fam Zheng 
---
 block/iscsi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 2d1e230..8c7f1b3 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -625,6 +625,9 @@ out:
 if (iTask.task != NULL) {
 scsi_free_scsi_task(iTask.task);
 }
+if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
+*file = bs;
+}
 return ret;
 }
 
-- 
2.4.3

Re: [Qemu-block] [PATCH v3 13/15] qemu-img: Make MapEntry a QAPI struct

[Eric Blake]

On 11/25/2015 10:05 PM, Fam Zheng wrote:
> The "flags" bit mask is expanded to two booleans, "data" and "zero";
> "bs" is replaced with "filename" string.
> 
> Refactor the merge conditions in img_map() into entry_mergable().

s/mergable/mergeable/ here and in the patch body

> 
> Signed-off-by: Fam Zheng 
> ---
>  qapi/block-core.json | 27 
>  qemu-img.c   | 71 
> +++-
>  2 files changed, 69 insertions(+), 29 deletions(-)
> 

With the spelling 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 v2 08/14] sheepdog: Assign bs to file in sd_co_get_block_status

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  block/sheepdog.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Eric Blake 

> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 0f6789e..d5e7ff8 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2740,6 +2740,9 @@ sd_co_get_block_status(BlockDriverState *bs, int64_t 
> sector_num, int nb_sectors,
>  if (*pnum > nb_sectors) {
>  *pnum = nb_sectors;
>  }
> +if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID) {
> +*file = bs;
> +}
>  return ret;
>  }
>  
> 

-- 
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 09/14] vdi: Assign bs->file->bs to file in vdi_co_get_block_status

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  block/vdi.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake 

> 
> diff --git a/block/vdi.c b/block/vdi.c
> index 2199fd3..6b1a57b 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -550,6 +550,7 @@ static int64_t coroutine_fn 
> vdi_co_get_block_status(BlockDriverState *bs,
>  offset = s->header.offset_data +
>(uint64_t)bmap_entry * s->block_size +
>sector_in_block * SECTOR_SIZE;
> +*file = bs->file->bs;
>  return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
>  }
>  
> 

-- 
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 01/14] block: Add "file" output parameter to block status query functions

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> The added parameter can be used to return the BDS pointer which the
> valid offset is referring to. It's value should be ignored unless

s/It's/Its/  (remember, "It's" is valid only if "It is" also works in
the same place)

> BDRV_BLOCK_OFFSET_VALID in ret is set.
> 
> Until block drivers fill in the right value, let's clear it explicitly
> right before calling .bdrv_get_block_status.
> 
> Signed-off-by: Fam Zheng 
> ---

With that 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

[Qemu-block] [PATCH v3 11/15] vmdk: Return extent's file in bdrv_get_block_status

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block/vmdk.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index f5a56fd..b60a5af 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1265,6 +1265,7 @@ static int64_t coroutine_fn 
vmdk_co_get_block_status(BlockDriverState *bs,
  0, 0);
 qemu_co_mutex_unlock(>lock);
 
+index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
 switch (ret) {
 case VMDK_ERROR:
 ret = -EIO;
@@ -1276,15 +1277,13 @@ static int64_t coroutine_fn 
vmdk_co_get_block_status(BlockDriverState *bs,
 ret = BDRV_BLOCK_ZERO;
 break;
 case VMDK_OK:
-ret = BDRV_BLOCK_DATA;
-if (extent->file == bs->file && !extent->compressed) {
-ret |= BDRV_BLOCK_OFFSET_VALID | offset;
-}
-
+ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS))
+& BDRV_BLOCK_OFFSET_MASK;
+*file = extent->file->bs;
 break;
 }
 
-index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
 n = extent->cluster_sectors - index_in_cluster;
 if (n > nb_sectors) {
 n = nb_sectors;
-- 
2.4.3

[Qemu-block] [PATCH v3 07/15] qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake 
Signed-off-by: Fam Zheng 
---
 block/qed.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/qed.c b/block/qed.c
index a6bbd8b..03af9c1 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -692,6 +692,7 @@ typedef struct {
 uint64_t pos;
 int64_t status;
 int *pnum;
+BlockDriverState **file;
 } QEDIsAllocatedCB;
 
 static void qed_is_allocated_cb(void *opaque, int ret, uint64_t offset, size_t 
len)
@@ -703,6 +704,7 @@ static void qed_is_allocated_cb(void *opaque, int ret, 
uint64_t offset, size_t l
 case QED_CLUSTER_FOUND:
 offset |= qed_offset_into_cluster(s, cb->pos);
 cb->status = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
+*cb->file = cb->bs->file->bs;
 break;
 case QED_CLUSTER_ZERO:
 cb->status = BDRV_BLOCK_ZERO;
@@ -734,6 +736,7 @@ static int64_t coroutine_fn 
bdrv_qed_co_get_block_status(BlockDriverState *bs,
 .pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE,
 .status = BDRV_BLOCK_OFFSET_MASK,
 .pnum = pnum,
+.file = file,
 };
 QEDRequest request = { .l2_table = NULL };
 
-- 
2.4.3

[Qemu-block] [PATCH v3 15/15] iotests: Add "qemu-img map" test for VMDK extents

[Fam Zheng]

Reviewed-by: Eric Blake 
Signed-off-by: Fam Zheng 
---
 tests/qemu-iotests/059 | 10 ++
 tests/qemu-iotests/059.out | 38 ++
 2 files changed, 48 insertions(+)

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 0ded0c3..261d8b0 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -133,6 +133,16 @@ $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | 
_filter_qemu_io
 $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io
 
 echo
+echo "=== Testing qemu-img map on extents ==="
+for fmt in twoGbMaxExtentSparse twoGbMaxExtentFlat; do
+IMGOPTS="subformat=$fmt" _make_test_img 31G
+$QEMU_IO -c "write 65024 1k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "write 2147483136 1k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "write 5G 1k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG map "$TEST_IMG" | _filter_testdir
+done
+
+echo
 echo "=== Testing afl image with a very large capacity ==="
 _use_sample_img afl9.vmdk.bz2
 _img_info
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 00057fe..54eb530 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2337,6 +2337,44 @@ e103f0:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00  
 read 1024/1024 bytes at offset 966367641600
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+=== Testing qemu-img map on extents ===
+Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 
subformat=twoGbMaxExtentSparse
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 2147483136
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 5368709120
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset  Length  Mapped to   File
+0   0x2 0x5 
TEST_DIR/iotest-version3-s001.vmdk
+0x7fff  0x1 0x7 
TEST_DIR/iotest-version3-s001.vmdk
+0x8000  0x1 0x5 
TEST_DIR/iotest-version3-s002.vmdk
+0x14000 0x1 0x5 
TEST_DIR/iotest-version3-s003.vmdk
+Formatting 'TEST_DIR/iotest-version3.IMGFMT', fmt=IMGFMT size=33285996544 
subformat=twoGbMaxExtentFlat
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 2147483136
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1024/1024 bytes at offset 5368709120
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset  Length  Mapped to   File
+0   0x8000  0   
TEST_DIR/iotest-version3-f001.vmdk
+0x8000  0x8000  0   
TEST_DIR/iotest-version3-f002.vmdk
+0x1 0x8000  0   
TEST_DIR/iotest-version3-f003.vmdk
+0x18000 0x8000  0   
TEST_DIR/iotest-version3-f004.vmdk
+0x2 0x8000  0   
TEST_DIR/iotest-version3-f005.vmdk
+0x28000 0x8000  0   
TEST_DIR/iotest-version3-f006.vmdk
+0x3 0x8000  0   
TEST_DIR/iotest-version3-f007.vmdk
+0x38000 0x8000  0   
TEST_DIR/iotest-version3-f008.vmdk
+0x4 0x8000  0   
TEST_DIR/iotest-version3-f009.vmdk
+0x48000 0x8000  0   
TEST_DIR/iotest-version3-f010.vmdk
+0x5 0x8000  0   
TEST_DIR/iotest-version3-f011.vmdk
+0x58000 0x8000  0   
TEST_DIR/iotest-version3-f012.vmdk
+0x6 0x8000  0   
TEST_DIR/iotest-version3-f013.vmdk
+0x68000 0x8000  0   
TEST_DIR/iotest-version3-f014.vmdk
+0x7 0x8000  0   
TEST_DIR/iotest-version3-f015.vmdk
+0x78000 0x4000  0   
TEST_DIR/iotest-version3-f016.vmdk
+
 === Testing afl image with a very large capacity ===
 qemu-img: Can't get size of device 'image': File too large
 *** done
-- 
2.4.3

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

[Fam Zheng]

On Wed, 11/25 22:03, Eric Blake wrote:
> On 11/25/2015 12:39 AM, Fam Zheng wrote:
> > The added parameter can be used to return the BDS pointer which the
> > valid offset is referring to. It's value should be ignored unless
> 
> s/It's/Its/  (remember, "It's" is valid only if "It is" also works in
> the same place)

Right, it's just easy to typo. Thanks :)

Fam

> 
> > BDRV_BLOCK_OFFSET_VALID in ret is set.
> > 
> > Until block drivers fill in the right value, let's clear it explicitly
> > right before calling .bdrv_get_block_status.
> > 
> > Signed-off-by: Fam Zheng 
> > ---
> 
> With that fix,
> Reviewed-by: Eric Blake 
> 
> -- 
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-block] [PATCH v3 0/4] Bitmap clean-up patches for 2.6

[Fam Zheng]

On Wed, 11/25 09:57, Vladimir Sementsov-Ogievskiy wrote:
> Hmm, stop. Very bad thing (sorry, that I didn't realize it before):
> 
> This breaks my dirty bitmap migration series with its meta bitmaps.
> Meta bitmap is an additional HBitmap in BdrvDirtyBitmap, which
> tracks dirtiness of this BdrvDirtyBitmap. And it (meta bitmap) have
> its own granularity of course. Or we will have to maintain
> additional meta_gran_shift and lots of duplicated code..
> 
> If we speak about splitting granularity out of HBitmap, then the
> most true way should be
> 
> struct HBitmapGranuled {
>   struct HBitmap *hb;
>   uint32_t gran_shift;
> }
> 
> with all supporting functions and iterators.

Then it's not worth making this change, let's drop patch 3 & 4 for now.

Fam

Re: [Qemu-block] [PATCH v7 12/24] virtio-blk: Functions for op blocker management

[Stefan Hajnoczi]

On Wed, Nov 25, 2015 at 05:26:02PM +0100, Max Reitz wrote:
> On 25.11.2015 17:18, Kevin Wolf wrote:
> > Am 25.11.2015 um 17:03 hat Max Reitz geschrieben:
> >> On 25.11.2015 16:57, Kevin Wolf wrote:
> >>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> >>> This makes me wonder: What do we even block here any more? If I didn't
> >>> miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure
> >>> why this needs to be blocked, or if we simply forgot to enable it.
> >>
> >> Well, even though in practice this wall of code doesn't make much sense,
> >> of course it will be safe for potential additions of new op blockers.
> >>
> >> And of course we actually don't want these blockers at all anymore...
> > 
> > Yes, but dataplane shouldn't really be special enough any more that we
> > want to disable features for it initially. By now it sounds more like an
> > easy way to forget unblocking a new feature even though it would work.
> > 
> > So perhaps we should really just remove the blockers from dataplane.
> > Then we don't have to answer the question above...
> 
> Well, maybe. I guess this is up to Stefan.

At this point blockdev.c and block jobs acquire/release AioContext,
hence all these op blockers are being unblocked.  I think we can switch
from whitelisting (unblocking) nearly everything to blacklisting
(blocking) only things that aren't supported yet.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 0/2] import nvme fix

[Kevin Wolf]

Am 18.11.2015 um 19:53 hat Christoph Hellwig geschrieben:
> First one fixes Identify to behave as mandated by the spec, and the
> second bumps the PCI revision so that guest drivers can detect
> the fixed version of the device so that only the old version has
> to be blacklisted.

Keith, this looks to me like a fix that should still be merged for 2.5,
would you agree? Can you please have a look at the series and either
give your Acked-by or comment?

Kevin

Re: [Qemu-block] [PATCH for-2.5] xen_disk: Remove ioreq.postsync

[Stefano Stabellini]

On Wed, 25 Nov 2015, Alberto Garcia wrote:
> This code has been dead for three years (since commit 7e7b7cba1).
> 
> Signed-off-by: Alberto Garcia 

Reviewed-by: Stefano Stabellini 

Kevin,

I'll send it out together with another fix today.



>  hw/block/xen_disk.c | 8 
>  1 file changed, 8 deletions(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 02eda6e..8146650 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -75,7 +75,6 @@ struct ioreq {
>  off_t   start;
>  QEMUIOVectorv;
>  int presync;
> -int postsync;
>  uint8_t mapped;
>  
>  /* grant mapping */
> @@ -144,7 +143,6 @@ static void ioreq_reset(struct ioreq *ioreq)
>  ioreq->status = 0;
>  ioreq->start = 0;
>  ioreq->presync = 0;
> -ioreq->postsync = 0;
>  ioreq->mapped = 0;
>  
>  memset(ioreq->domids, 0, sizeof(ioreq->domids));
> @@ -520,12 +518,6 @@ static void qemu_aio_complete(void *opaque, int ret)
>  if (ioreq->aio_inflight > 0) {
>  return;
>  }
> -if (ioreq->postsync) {
> -ioreq->postsync = 0;
> -ioreq->aio_inflight++;
> -blk_aio_flush(ioreq->blkdev->blk, qemu_aio_complete, ioreq);
> -return;
> -}
>  
>  ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
>  ioreq_unmap(ioreq);
> -- 
> 2.6.2
> 

Re: [Qemu-block] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct

[Paolo Bonzini]

On 25/11/2015 08:39, Fam Zheng wrote:
>   */
> -if (next &&
> -(next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) != 
> BDRV_BLOCK_DATA) {
> -next->flags &= ~BDRV_BLOCK_DATA;
> -next->flags |= BDRV_BLOCK_ZERO;
> +if (next && !next->data) {
> +next->zero = true;

before  after
0   ZEROZERO
DATADATADATA
DATA|ZERO   ZERODATA|ZERO
ZEROZEROZERO

This would not coalesce 0 with DATA|ZERO.

I think you need to do exactly as in the older code: test (!next->data
|| next->zero), and clear next->data.

Paolo

Re: [Qemu-block] [PATCH v2 8/8] block: Remove bdrv_states

[Kevin Wolf]

Am 10.11.2015 um 04:27 hat Max Reitz geschrieben:
> Every entry in this list should be a root BDS and as such either be
> anchored to a BlockBackend or be owned by the monitor.
> 
> Signed-off-by: Max Reitz 
> ---
>  block.c   | 30 +-
>  blockdev.c|  8 
>  include/block/block.h |  1 -
>  include/block/block_int.h |  4 
>  4 files changed, 1 insertion(+), 42 deletions(-)

>  void bdrv_make_anon(BlockDriverState *bs)
>  {
> -/*
> - * Take care to remove bs from bdrv_states only when it's actually
> - * in it.  Note that bs->device_list.tqe_prev is initially null,
> - * and gets set to non-null by QTAILQ_INSERT_TAIL().  Establish
> - * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by
> - * resetting it to null on remove.
> - */
> -if (bs->device_list.tqe_prev) {
> -QTAILQ_REMOVE(_states, bs, device_list);
> -bs->device_list.tqe_prev = NULL;
> -}
>  if (bs->node_name[0] != '\0') {
>  QTAILQ_REMOVE(_bdrv_states, bs, node_list);
>  }

bdrv_make_anon() has only a single user at this point and the remaining
part after this patch is so small that we could consider inlining it.

Kevin

[Qemu-block] [PULL 4/6] raw-posix.c: Make GetBSDPath() handle caching options

[Kevin Wolf]

From: Programmingkid 

Add support for caching options that can be specified from the command
line.

The CD-ROM raw char device bypasses the host page cache and therefore
has alignment requirements.  Alignment probing is necessary so only use
the raw char device if BDRV_O_NOCACHE is set.

This patch fixes -cdrom /dev/cdrom on Mac OS X hosts, where bdrv_read()
used to fail due to misaligned requests during image format probing.

Signed-off-by: John Arbuckle 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Kevin Wolf 
---
 block/raw-posix.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index aec9ec6..d9162fd 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1976,8 +1976,8 @@ BlockDriver bdrv_file = {
 
 #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 );
-
+static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
+CFIndex maxPathSize, int flags);
 kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
 {
 kern_return_t   kernResult;
@@ -2004,7 +2004,8 @@ kern_return_t FindEjectableCDMedia( io_iterator_t 
*mediaIterator )
 return kernResult;
 }
 
-kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex 
maxPathSize )
+kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
+ CFIndex maxPathSize, int flags)
 {
 io_object_t nextMedia;
 kern_return_t   kernResult = KERN_FAILURE;
@@ -2017,7 +2018,9 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, 
char *bsdPath, CFIndex ma
 if ( bsdPathAsCFString ) {
 size_t devPathLength;
 strcpy( bsdPath, _PATH_DEV );
-strcat( bsdPath, "r" );
+if (flags & BDRV_O_NOCACHE) {
+strcat(bsdPath, "r");
+}
 devPathLength = strlen( bsdPath );
 if ( CFStringGetCString( bsdPathAsCFString, bsdPath + 
devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
 kernResult = KERN_SUCCESS;
@@ -2129,8 +2132,8 @@ static int hdev_open(BlockDriverState *bs, QDict 
*options, int flags,
 int fd;
 
 kernResult = FindEjectableCDMedia(  );
-kernResult = GetBSDPath( mediaIterator, bsdPath, sizeof( bsdPath ) );
-
+kernResult = GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath),
+flags);
 if ( bsdPath[ 0 ] != '\0' ) {
 strcat(bsdPath,"s0");
 /* some CDs don't have a partition 0 */
-- 
1.8.3.1

[Qemu-block] [PULL 3/6] nand: fix flash erase when oob is in memory

[Kevin Wolf]

From: Ricard Wanderlof 

For the "main area on file, oob in memory" case, fix the shifts so that
we erase the correct number of pages.

Signed-off-by: Ricard Wanderlöf 
Signed-off-by: Kevin Wolf 
---
 hw/block/nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index a68266f..f0e3413 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -712,7 +712,7 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState 
*s)
 memset(s->storage + (PAGE(addr) << OOB_SHIFT),
 0xff, OOB_SIZE << s->erase_shift);
 i = SECTOR(addr);
-page = SECTOR(addr + (ADDR_SHIFT + s->erase_shift));
+page = SECTOR(addr + (1 << (ADDR_SHIFT + s->erase_shift)));
 for (; i < page; i ++)
 if (blk_write(s->blk, i, iobuf, 1) < 0) {
 printf("%s: write error in sector %" PRIu64 "\n", __func__, i);
-- 
1.8.3.1

[Qemu-block] [PULL 2/6] test-aio: Fix event notifier cleanup

[Kevin Wolf]

One test case closed an event notifier (event_notifier_cleanup())
without first disabling it (set_event_notifier(..., NULL)). This
resulted in a leftover handle 0 that was added to each subsequent
WaitForMultipleObjects() call, causing the function to fail (invalid
handle).

Signed-off-by: Kevin Wolf 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Stefan Hajnoczi 
---
 tests/test-aio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-aio.c b/tests/test-aio.c
index 1623803..e188d8c 100644
--- a/tests/test-aio.c
+++ b/tests/test-aio.c
@@ -393,6 +393,7 @@ static void test_aio_external_client(void)
 aio_enable_external(ctx);
 }
 assert(aio_poll(ctx, false));
+set_event_notifier(ctx, , NULL);
 event_notifier_cleanup();
 }
 }
-- 
1.8.3.1

[Qemu-block] [PULL 1/6] tests/Makefile: Add more dependencies for test-timed-average

[Kevin Wolf]

'make check' failed to compile the test case for mingw because of
undefined references. Pull in a few more dependencies so that it builds.

Signed-off-by: Kevin Wolf 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Stefan Hajnoczi 
---
 tests/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index b937984..0ef00a1 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -415,8 +415,7 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
 migration/qemu-file-unix.o qjson.o \
$(test-qom-obj-y)
 tests/test-timed-average$(EXESUF): tests/test-timed-average.o qemu-timer.o \
-   libqemuutil.a stubs/clock-warp.o stubs/cpu-get-icount.o \
-   stubs/notify-event.o stubs/replay.o
+   $(test-util-obj-y)
 
 tests/test-qapi-types.c tests/test-qapi-types.h :\
 $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json 
$(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
-- 
1.8.3.1

[Qemu-block] [PULL 0/6] Block patches for 2.5.0-rc2

[Kevin Wolf]

The following changes since commit 1aae36df4b8ed884c6ef6995e70c67fad79b49df:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-ivshmem-2015-11-25' 
into staging (2015-11-25 11:38:03 +)

are available in the git repository at:


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

for you to fetch changes up to 8c34d891b1594840d8394a3c9b92236c13254fd8:

  Merge remote-tracking branch 'mreitz/tags/pull-block-for-kevin-2015-11-25' 
into queue-block (2015-11-25 14:33:01 +0100)



Block layer patches


Fam Zheng (1):
  qemu-iotests: Add -nographic when starting QEMU in 119 and 120

Kevin Wolf (3):
  tests/Makefile: Add more dependencies for test-timed-average
  test-aio: Fix event notifier cleanup
  Merge remote-tracking branch 
'mreitz/tags/pull-block-for-kevin-2015-11-25' into queue-block

Markus Armbruster (1):
  block/qapi: Plug memory leak on query-block error path

Programmingkid (1):
  raw-posix.c: Make GetBSDPath() handle caching options

Ricard Wanderlof (1):
  nand: fix flash erase when oob is in memory

 block/qapi.c   |  8 +++-
 block/raw-posix.c  | 15 +--
 hw/block/nand.c|  2 +-
 tests/Makefile |  3 +--
 tests/qemu-iotests/119 |  2 +-
 tests/qemu-iotests/120 |  2 +-
 tests/test-aio.c   |  1 +
 7 files changed, 17 insertions(+), 16 deletions(-)

[Qemu-block] [PULL 5/6] block/qapi: Plug memory leak on query-block error path

[Kevin Wolf]

From: Markus Armbruster 

Spotted by Coverity.

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

diff --git a/block/qapi.c b/block/qapi.c
index d20262d..267f147 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -436,7 +436,9 @@ BlockInfoList *qmp_query_block(Error **errp)
 bdrv_query_info(blk, >value, _err);
 if (local_err) {
 error_propagate(errp, local_err);
-goto err;
+g_free(info);
+qapi_free_BlockInfoList(head);
+return NULL;
 }
 
 *p_next = info;
@@ -444,10 +446,6 @@ BlockInfoList *qmp_query_block(Error **errp)
 }
 
 return head;
-
- err:
-qapi_free_BlockInfoList(head);
-return NULL;
 }
 
 BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
-- 
1.8.3.1

[Qemu-block] [PULL 6/6] qemu-iotests: Add -nographic when starting QEMU in 119 and 120

[Kevin Wolf]

From: Fam Zheng 

Otherwise, a window flashes on my desktop (built with SDL). Add this as
other cases have it.

Signed-off-by: Fam Zheng 
Message-id: 1448245930-15031-1-git-send-email-f...@redhat.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/119 | 2 +-
 tests/qemu-iotests/120 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/119 b/tests/qemu-iotests/119
index 9a11f1b..cc6ec07 100755
--- a/tests/qemu-iotests/119
+++ b/tests/qemu-iotests/119
@@ -49,7 +49,7 @@ echo "{'execute': 'qmp_capabilities'}
   {'execute': 'human-monitor-command',
'arguments': {'command-line': 'qemu-io drv \"read -P 0 0 64k\"'}}
   {'execute': 'quit'}" \
-| $QEMU -drive id=drv,if=none,file="$TEST_IMG",driver=nbd \
+| $QEMU -nographic -drive id=drv,if=none,file="$TEST_IMG",driver=nbd \
 -qmp stdio -nodefaults \
 | _filter_qmp | _filter_qemu_io
 
diff --git a/tests/qemu-iotests/120 b/tests/qemu-iotests/120
index 9f13078..d899a3f 100755
--- a/tests/qemu-iotests/120
+++ b/tests/qemu-iotests/120
@@ -49,7 +49,7 @@ echo "{'execute': 'qmp_capabilities'}
   {'execute': 'human-monitor-command',
'arguments': {'command-line': 'qemu-io drv \"write -P 42 0 64k\"'}}
   {'execute': 'quit'}" \
-| $QEMU -qmp stdio -nodefaults \
+| $QEMU -qmp stdio -nographic -nodefaults \
 -drive 
id=drv,if=none,file="$TEST_IMG",driver=raw,file.driver=$IMGFMT \
 | _filter_qmp | _filter_qemu_io
 $QEMU_IO -c 'read -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
-- 
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

[Eric Blake]

On 11/25/2015 09:23 PM, Eric Blake wrote:

>> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>> +char 
>> *mediaType)
> 
> Unusual indentation; more typical is:
> 
> | static kern_return_t FindEjectableOpticalMedia(io_iterator_t
> *mediaIterator,
> | char *mediatType)

And then my mailer messes it up :(

> static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>char *mediatType)

Let's see if that's better (the 'char' is directly beneath the
'io_iterator_t').

-- 
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 10/14] vpc: Assign bs->file->bs to file in vpc_co_get_block_status

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---
>  block/vpc.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake 

> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 912f5d0..412ff41 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -588,6 +588,7 @@ static int64_t coroutine_fn 
> vpc_co_get_block_status(BlockDriverState *bs,
>  
>  if (be32_to_cpu(footer->type) == VHD_FIXED) {
>  *pnum = nb_sectors;
> +*file = bs->file->bs;
>  return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
> (sector_num << BDRV_SECTOR_BITS);
>  }
> @@ -609,6 +610,7 @@ static int64_t coroutine_fn 
> vpc_co_get_block_status(BlockDriverState *bs,
>  /* *pnum can't be greater than one block for allocated
>   * sectors since there is always a bitmap in between. */
>  if (allocated) {
> +*file = bs->file->bs;
>  return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
>  }
>  if (nb_sectors == 0) {
> 

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



signature.asc
Description: OpenPGP digital signature

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

[Fam Zheng]

The added parameter can be used to return the BDS pointer which the
valid offset is referring to. It's value should be ignored unless
BDRV_BLOCK_OFFSET_VALID in ret is set.

Until block drivers fill in the right value, let's clear it explicitly
right before calling .bdrv_get_block_status.

Signed-off-by: Fam Zheng 
---
 block/io.c| 42 --
 block/iscsi.c |  6 --
 block/mirror.c|  3 ++-
 block/parallels.c |  2 +-
 block/qcow.c  |  2 +-
 block/qcow2.c |  2 +-
 block/qed.c   |  3 ++-
 block/raw-posix.c |  3 ++-
 block/raw_bsd.c   |  3 ++-
 block/sheepdog.c  |  2 +-
 block/vdi.c   |  2 +-
 block/vmdk.c  |  2 +-
 block/vpc.c   |  2 +-
 block/vvfat.c |  2 +-
 include/block/block.h |  6 --
 include/block/block_int.h |  3 ++-
 qemu-img.c|  7 +--
 17 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/block/io.c b/block/io.c
index adc1eab..71930ed 100644
--- a/block/io.c
+++ b/block/io.c
@@ -656,6 +656,7 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t 
sector_num,
 int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
 {
 int64_t target_sectors, ret, nb_sectors, sector_num = 0;
+BlockDriverState *file;
 int n;
 
 target_sectors = bdrv_nb_sectors(bs);
@@ -668,7 +669,7 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags 
flags)
 if (nb_sectors <= 0) {
 return 0;
 }
-ret = bdrv_get_block_status(bs, sector_num, nb_sectors, );
+ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , );
 if (ret < 0) {
 error_report("error getting block status at sector %" PRId64 ": 
%s",
  sector_num, strerror(-ret));
@@ -1455,6 +1456,7 @@ int bdrv_flush_all(void)
 typedef struct BdrvCoGetBlockStatusData {
 BlockDriverState *bs;
 BlockDriverState *base;
+BlockDriverState **file;
 int64_t sector_num;
 int nb_sectors;
 int *pnum;
@@ -1479,7 +1481,8 @@ typedef struct BdrvCoGetBlockStatusData {
  */
 static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
  int64_t sector_num,
- int nb_sectors, int *pnum)
+ int nb_sectors, int *pnum,
+ BlockDriverState **file)
 {
 int64_t total_sectors;
 int64_t n;
@@ -1509,16 +1512,19 @@ static int64_t coroutine_fn 
bdrv_co_get_block_status(BlockDriverState *bs,
 return ret;
 }
 
-ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum);
+*file = NULL;
+ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
+file);
 if (ret < 0) {
 *pnum = 0;
+*file = NULL;
 return ret;
 }
 
 if (ret & BDRV_BLOCK_RAW) {
 assert(ret & BDRV_BLOCK_OFFSET_VALID);
 return bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
- *pnum, pnum);
+ *pnum, pnum, file);
 }
 
 if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
@@ -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)) {
+BlockDriverState *file2;
 int file_pnum;
 
-ret2 = bdrv_co_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
-*pnum, _pnum);
+ret2 = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
+*pnum, _pnum, );
 if (ret2 >= 0) {
 /* Ignore errors.  This is just providing extra information, it
  * is useful but not necessary.
@@ -1566,14 +1573,15 @@ static int64_t coroutine_fn 
bdrv_co_get_block_status_above(BlockDriverState *bs,
 BlockDriverState *base,
 int64_t sector_num,
 int nb_sectors,
-int *pnum)
+int *pnum,
+BlockDriverState **file)
 {
 BlockDriverState *p;
 int64_t ret = 0;
 
 assert(bs != base);
 for (p = bs; p != base; p = backing_bs(p)) {
-ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum);
+ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
 if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
 break;
 }
@@ -1592,7 +1600,8 @@ static void coroutine_fn 
bdrv_get_block_status_above_co_entry(void *opaque)
 data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
  

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

[Fam Zheng]

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(-)

-- 
2.4.3

[Qemu-block] [PATCH v3 02/15] qcow: Assign bs->file->bs to file in qcow_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake 
Signed-off-by: Fam Zheng 
---
 block/qcow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow.c b/block/qcow.c
index 558f443..b59383f 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -509,6 +509,7 @@ static int64_t coroutine_fn 
qcow_co_get_block_status(BlockDriverState *bs,
 return BDRV_BLOCK_DATA;
 }
 cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS);
+*file = bs->file->bs;
 return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | cluster_offset;
 }
 
-- 
2.4.3

[Qemu-block] [PATCH v3 04/15] raw: Assign bs to file in raw_co_get_block_status

[Fam Zheng]

Reviewed-by: Eric Blake 
Signed-off-by: Fam Zheng 
---
 block/raw-posix.c | 1 +
 block/raw_bsd.c   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2cd7d68..9988aa4 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1873,6 +1873,7 @@ static int64_t coroutine_fn 
raw_co_get_block_status(BlockDriverState *bs,
 *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
 ret = BDRV_BLOCK_ZERO;
 }
+*file = bs;
 return ret | BDRV_BLOCK_OFFSET_VALID | start;
 }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 7d23c08..2d4c896 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -118,6 +118,7 @@ static int64_t coroutine_fn 
raw_co_get_block_status(BlockDriverState *bs,
 BlockDriverState **file)
 {
 *pnum = nb_sectors;
+*file = bs;
 return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
(sector_num << BDRV_SECTOR_BITS);
 }
-- 
2.4.3

[Qemu-block] [PATCH v3 14/15] qemu-img: Use QAPI visitor to generate JSON

[Fam Zheng]

A visible improvement is that "filename" is now included in the output
if it's valid.

Reviewed-by: Eric Blake 
Signed-off-by: Fam Zheng 
---
 qemu-img.c | 34 ++--
 tests/qemu-iotests/122.out | 96 ++
 2 files changed, 77 insertions(+), 53 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 1d19c86..fccddc0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -25,6 +25,8 @@
 #include "qapi/qmp-output-visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qint.h"
+#include "qapi/qmp/qbool.h"
 #include "qemu-common.h"
 #include "qemu/option.h"
 #include "qemu/error-report.h"
@@ -2136,10 +2138,14 @@ static int img_info(int argc, char **argv)
 static void dump_map_entry(OutputFormat output_format, MapEntry *e,
MapEntry *next)
 {
+QString *str;
+QObject *obj;
+QmpOutputVisitor *ov;
+
 switch (output_format) {
 case OFORMAT_HUMAN:
 if (e->data && !e->has_offset) {
-error_report("File contains external, encrypted or compressed 
clusters.");
+error_report("File contains encrypted or compressed clusters.");
 exit(1);
 }
 if (e->data && !e->zero) {
@@ -2157,20 +2163,26 @@ static void dump_map_entry(OutputFormat output_format, 
MapEntry *e,
 }
 break;
 case OFORMAT_JSON:
-printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
-   " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
-   (e->start == 0 ? "[" : ",\n"),
-   e->start, e->length, e->depth,
-   e->zero ? "true" : "false",
-   e->data ? "true" : "false");
-if (e->has_offset) {
-printf(", \"offset\": %"PRId64"", e->offset);
-}
-putchar('}');
+ov = qmp_output_visitor_new();
+visit_type_MapEntry(qmp_output_get_visitor(ov),
+, NULL, _abort);
+obj = qmp_output_get_qobject(ov);
+str = qobject_to_json(obj);
+assert(str != NULL);
 
+if (e->start == 0) {
+printf("[");
+} else {
+printf(",");
+}
+printf("%s\n", qstring_get_str(str));
 if (!next) {
 printf("]\n");
 }
+
+qobject_decref(obj);
+qmp_output_visitor_cleanup(ov);
+QDECREF(str);
 break;
 }
 }
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 0068e96..760759e 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -49,12 +49,13 @@ read 65536/65536 bytes at offset 4194304
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 8388608
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 65536, "length": 4128768, "depth": 0, "zero": true, "data": false},
-{ "start": 4194304, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 4259840, "length": 4128768, "depth": 0, "zero": true, "data": 
false},
-{ "start": 8388608, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 8454144, "length": 4128768, "depth": 0, "zero": true, "data": 
false}]
+[{"length": 65536, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 65536, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 4194304, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 4259840, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 8388608, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 8454144, "zero": true, "depth": 0, "data": false}
+]
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 4194304
@@ -76,12 +77,13 @@ wrote 1024/1024 bytes at offset 1046528
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 65536, "length": 65536, "depth": 0, "zero": true, "data": false},
-{ "start": 131072, "length": 196608, "depth": 0, "zero": false, "data": true},
-{ "start": 327680, "length": 655360, "depth": 0, "zero": true, "data": false},
-{ "start": 983040, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 1048576, "length": 1046528, "depth": 0, "zero": true, "data": 
false}]
+[{"length": 65536, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 65536, "start": 65536, "zero": true, "depth": 0, "data": false}
+,{"length": 196608, "start": 131072, "zero": false, "depth": 0, "data": true}
+,{"length": 655360, "start": 327680, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 983040, "zero": false, 

Re: [Qemu-block] [PATCH v2 13/14] qemu-img: Use QAPI visitor to generate JSON

[Fam Zheng]

On Wed, 11/25 08:42, Eric Blake wrote:
> On 11/25/2015 12:39 AM, Fam Zheng wrote:
> > A visible improvement is that "filename" is now included in the output
> > if it's valid.
> > 
> > Signed-off-by: Fam Zheng 
> > ---
> >  qemu-img.c | 39 ---
> >  tests/qemu-iotests/122.out | 96 
> > ++
> >  2 files changed, 79 insertions(+), 56 deletions(-)
> > 
> 
> > @@ -2155,21 +2161,26 @@ static void dump_map_entry(OutputFormat 
> > output_format, MapEntry *e,
> >  }
> >  break;
> >  case OFORMAT_JSON:
> > -printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> > -   " \"depth\": %ld,"
> > -   " \"zero\": %s, \"data\": %s",
> > -   (e->start == 0 ? "[" : ",\n"),
> > -   e->start, e->length, e->depth,
> > -   e->zero ? "true" : "false",
> > -   e->data ? "true" : "false");
> > -if (e->has_offset) {
> > -printf(", \"offset\": %"PRId64"", e->offset);
> > -}
> > -putchar('}');
> > +ov = qmp_output_visitor_new();
> > +visit_type_MapEntry(qmp_output_get_visitor(ov),
> > +, NULL, _abort);
> > +obj = qmp_output_get_qobject(ov);
> > +str = qobject_to_json(obj);
> 
> It would be nice to someday add a visitor that goes directly to json,
> instead of through an intermediate QObject. But that's not for this
> series; your conversion here is sane.
> 
> > @@ -2213,9 +2224,9 @@ static int get_block_status(BlockDriverState *bs, 
> > int64_t sector_num,
> >  e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
> >  e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
> >  e->depth = depth;
> > -if (e->has_offset) {
> > +if (file && e->has_offset) {
> >  e->has_filename = true;
> > -e->filename = bs->filename;
> > +e->filename = file->filename;
> 
> Does this hunk belong in a different patch?
> 
> Reviewed-by: Eric Blake 
> 

Yes, I can split this into a separate patch.

Fam

[Qemu-block] [PATCH v3 12/15] qemu-img: In "map", use the returned "file" from bdrv_get_block_status

[Fam Zheng]

Now all drivers should return a correct "file", we can make use of it,
even with the recursion into backing chain above.

Signed-off-by: Fam Zheng 
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7954242..a7fa794 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -,7 +,7 @@ static int get_block_status(BlockDriverState *bs, int64_t 
sector_num,
 e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
 e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
 e->depth = depth;
-e->bs = bs;
+e->bs = file;
 return 0;
 }
 
-- 
2.4.3

[Qemu-block] [PATCH v3 13/15] qemu-img: Make MapEntry a QAPI struct

[Fam Zheng]

The "flags" bit mask is expanded to two booleans, "data" and "zero";
"bs" is replaced with "filename" string.

Refactor the merge conditions in img_map() into entry_mergable().

Signed-off-by: Fam Zheng 
---
 qapi/block-core.json | 27 
 qemu-img.c   | 71 +++-
 2 files changed, 69 insertions(+), 29 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index a07b13f..ee2294f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -186,6 +186,33 @@
'*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
 
 ##
+# @MapEntry:
+#
+# Mapping information from a virtual block range to a host file range
+#
+# @start: the start byte of the mapped virtual range
+#
+# @length: the number of bytes of the mapped virtual range
+#
+# @data: whether the mapped range has data
+#
+# @zero: whether the virtual blocks are zeroed
+#
+# @depth: the depth of the mapping
+#
+# @offset: #optional the offset in file that the virtual sectors are mapped to
+#
+# @filename: #optional filename that is referred to by @offset
+#
+# Since: 2.6
+#
+##
+{ 'struct': 'MapEntry',
+  'data': {'start': 'int', 'length': 'int', 'data': 'bool',
+   'zero': 'bool', 'depth': 'int', '*offset': 'int',
+   '*filename': 'str' } }
+
+##
 # @BlockdevCacheInfo
 #
 # Cache mode information for a block device
diff --git a/qemu-img.c b/qemu-img.c
index a7fa794..1d19c86 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2133,47 +2133,37 @@ static int img_info(int argc, char **argv)
 return 0;
 }
 
-
-typedef struct MapEntry {
-int flags;
-int depth;
-int64_t start;
-int64_t length;
-int64_t offset;
-BlockDriverState *bs;
-} MapEntry;
-
 static void dump_map_entry(OutputFormat output_format, MapEntry *e,
MapEntry *next)
 {
 switch (output_format) {
 case OFORMAT_HUMAN:
-if ((e->flags & BDRV_BLOCK_DATA) &&
-!(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
+if (e->data && !e->has_offset) {
 error_report("File contains external, encrypted or compressed 
clusters.");
 exit(1);
 }
-if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) 
{
+if (e->data && !e->zero) {
 printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
-   e->start, e->length, e->offset, e->bs->filename);
+   e->start, e->length,
+   e->has_offset ? e->offset : 0,
+   e->has_filename ? e->filename : "");
 }
 /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
  * Modify the flags here to allow more coalescing.
  */
-if (next &&
-(next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) != 
BDRV_BLOCK_DATA) {
-next->flags &= ~BDRV_BLOCK_DATA;
-next->flags |= BDRV_BLOCK_ZERO;
+if (next && (!next->data || next->zero)) {
+next->data = false;
+next->zero = true;
 }
 break;
 case OFORMAT_JSON:
-printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": 
%d,"
-   " \"zero\": %s, \"data\": %s",
+printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
+   " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
(e->start == 0 ? "[" : ",\n"),
e->start, e->length, e->depth,
-   (e->flags & BDRV_BLOCK_ZERO) ? "true" : "false",
-   (e->flags & BDRV_BLOCK_DATA) ? "true" : "false");
-if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
+   e->zero ? "true" : "false",
+   e->data ? "true" : "false");
+if (e->has_offset) {
 printf(", \"offset\": %"PRId64"", e->offset);
 }
 putchar('}');
@@ -2219,13 +2209,39 @@ static int get_block_status(BlockDriverState *bs, 
int64_t sector_num,
 
 e->start = sector_num * BDRV_SECTOR_SIZE;
 e->length = nb_sectors * BDRV_SECTOR_SIZE;
-e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
+e->data = !!(ret & BDRV_BLOCK_DATA);
+e->zero = !!(ret & BDRV_BLOCK_ZERO);
 e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
+e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
 e->depth = depth;
-e->bs = file;
+if (file && e->has_offset) {
+e->has_filename = true;
+e->filename = file->filename;
+}
 return 0;
 }
 
+static inline bool entry_mergable(const MapEntry *curr, const MapEntry *next)
+{
+if (curr->length == 0) {
+return false;
+}
+if (curr->zero != next->zero ||
+curr->data != next->data ||
+curr->depth != next->depth ||
+curr->has_filename != next->has_filename ||
+curr->has_offset != next->has_offset) {
+return false;
+}
+if (curr->has_filename && strcmp(curr->filename, next->filename)) {
+return false;
+}
+   

[Qemu-block] [PATCH v3 09/15] vdi: Assign bs->file->bs to file in vdi_co_get_block_status

[Fam Zheng]

Signed-off-by: Fam Zheng 
---
 block/vdi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vdi.c b/block/vdi.c
index 2199fd3..6b1a57b 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -550,6 +550,7 @@ static int64_t coroutine_fn 
vdi_co_get_block_status(BlockDriverState *bs,
 offset = s->header.offset_data +
   (uint64_t)bmap_entry * s->block_size +
   sector_in_block * SECTOR_SIZE;
+*file = bs->file->bs;
 return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset;
 }
 
-- 
2.4.3

Re: [Qemu-block] [PATCH v3 12/15] qemu-img: In "map", use the returned "file" from bdrv_get_block_status

[Eric Blake]

On 11/25/2015 10:05 PM, Fam Zheng wrote:
> Now all drivers should return a correct "file", we can make use of it,
> even with the recursion into backing chain above.
> 
> Signed-off-by: Fam Zheng 
> ---
>  qemu-img.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 7954242..a7fa794 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -,7 +,7 @@ static int get_block_status(BlockDriverState *bs, 
> int64_t sector_num,
>  e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
>  e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
>  e->depth = depth;
> -e->bs = bs;
> +e->bs = file;
>  return 0;
>  }
>  
> 

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



signature.asc
Description: OpenPGP digital signature