Re: [Qemu-block] [PATCH] block: add missing call to bdrv_drain_recurse

[Kevin Wolf]

Am 23.12.2015 um 11:48 hat Paolo Bonzini geschrieben:
> This is also needed in bdrv_drain_all, not just in bdrv_drain.
> 
> Signed-off-by: Paolo Bonzini 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v9 01/16] block: Release named dirty bitmaps in bdrv_close()

[Fam Zheng]

On Fri, 01/29 16:36, Max Reitz wrote:
> bdrv_delete() is not very happy about deleting BlockDriverStates with
> dirty bitmaps still attached to them. In the past, we got around that
> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
> bdrv_close() simply ignoring that condition. We should fix that by
> releasing all named dirty bitmaps in bdrv_close() (there should not be
> any unnamed bitmaps left) and moving the assertion from bdrv_delete()
> there.
> 
> Signed-off-by: Max Reitz 
> ---
>  block.c | 39 +++
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 5709d3d..41ab00e 100644
> --- a/block.c
> +++ b/block.c
> @@ -88,6 +88,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const 
> char *filename,
>   const BdrvChildRole *child_role, Error **errp);
>  
>  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> +static void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
> +
>  /* If non-zero, use only whitelisted block drivers */
>  static int use_bdrv_whitelist;
>  
> @@ -2157,6 +2159,9 @@ void bdrv_close(BlockDriverState *bs)
>  
>  notifier_list_notify(>close_notifiers, bs);
>  
> +bdrv_release_named_dirty_bitmaps(bs);
> +assert(QLIST_EMPTY(>dirty_bitmaps));
> +
>  if (bs->blk) {
>  blk_dev_change_media_cb(bs->blk, false);
>  }
> @@ -2366,7 +2371,6 @@ static void bdrv_delete(BlockDriverState *bs)
>  assert(!bs->job);
>  assert(bdrv_op_blocker_is_empty(bs));
>  assert(!bs->refcnt);
> -assert(QLIST_EMPTY(>dirty_bitmaps));
>  
>  bdrv_close(bs);
>  
> @@ -3582,21 +3586,40 @@ static void 
> bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>  }
>  }
>  
> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
> +  BdrvDirtyBitmap *bitmap,
> +  bool only_named)
>  {
>  BdrvDirtyBitmap *bm, *next;
>  QLIST_FOREACH_SAFE(bm, >dirty_bitmaps, list, next) {
> -if (bm == bitmap) {
> +if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) {
>  assert(!bdrv_dirty_bitmap_frozen(bm));
> -QLIST_REMOVE(bitmap, list);
> -hbitmap_free(bitmap->bitmap);
> -g_free(bitmap->name);
> -g_free(bitmap);
> -return;
> +QLIST_REMOVE(bm, list);
> +hbitmap_free(bm->bitmap);
> +g_free(bm->name);
> +g_free(bm);
> +
> +if (bitmap) {
> +return;
> +}
>  }
>  }
>  }
>  
> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false);
> +}
> +
> +/**
> + * Release all named dirty bitmaps attached to a BDS (for use in 
> bdrv_close()).
> + * There must not be any frozen bitmaps attached.
> + */
> +static void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
> +{
> +bdrv_do_release_matching_dirty_bitmap(bs, NULL, true);
> +}
> +
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
>  assert(!bdrv_dirty_bitmap_frozen(bitmap));
> -- 
> 2.7.0
> 

Reviewed-by: Fam Zheng 

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

[Kevin Wolf]

Am 02.02.2016 um 20:29 hat Eric Blake geschrieben:
> On 02/02/2016 10:28 AM, Programmingkid wrote:
> 
> >> Whats the rationale here ? Using pre-allocated fixed
> >> length arrays is pretty bad practice in general, but
> >> especially so for filenames
> > 
> > With an automatic variable there is no worry about when to release it. 
> 
> Yeah, but it comes with the downside of having to worry about exhausting
> stack space (there are platforms where MAXPATHLEN is intentionally
> undefined [hello, GNU Hurd]), or with the downside of arbitrary
> limitations.  And at the same time, MAXPATHLEN is usually wasteful -
> allocating 1k or 4k of stack to store what is typically less than 100
> bytes is dumb.  Really, storing file names in fixed length arrays is
> better off to avoid, and just get used to dynamic management.

Just let me add that while it's probably harmless here in .bdrv_open(),
other paths in the block layer which run in a coroutine have a much more
limited stack size and the danger of stack overflows is very real there.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] block: add missing call to bdrv_drain_recurse

[Paolo Bonzini]

On 11/01/2016 09:32, Paolo Bonzini wrote:
> 
> 
> On 25/12/2015 02:55, Fam Zheng wrote:
>> On Wed, 12/23 11:48, Paolo Bonzini wrote:
>>> This is also needed in bdrv_drain_all, not just in bdrv_drain.
>>>
>>> Signed-off-by: Paolo Bonzini 
>>> ---
>>>  block/io.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/block/io.c b/block/io.c
>>> index 841f5b5..bfe2544 100644
>>> --- a/block/io.c
>>> +++ b/block/io.c
>>> @@ -293,6 +293,7 @@ void bdrv_drain_all(void)
>>>  if (bs->job) {
>>>  block_job_pause(bs->job);
>>>  }
>>> +bdrv_drain_recurse(bs);
>>>  aio_context_release(aio_context);
>>>  
>>>  if (!g_slist_find(aio_ctxs, aio_context)) {
>>> -- 
>>> 2.5.0
>>>
>>>
>>
>> Reviewed-by: Fam Zheng 
>>
>>
> 
> Ping?

Ping^2?

Re: [Qemu-block] [PATCH v4] blockjob: Fix hang in block_job_finish_sync

[Stefan Hajnoczi]

On Tue, Feb 02, 2016 at 10:12:24AM +0800, Fam Zheng wrote:
> With a mirror job running on a virtio-blk dataplane disk, sending "q" to
> HMP will cause a dead loop in block_job_finish_sync.
> 
> This is because the aio_poll() only processes the AIO context of bs
> which has no more work to do, while the main loop BH that is scheduled
> for setting the job->completed flag is never processed.
> 
> Fix this by adding a flag in BlockJob structure, to track which context
> to poll for the block job to make progress. Its value is set to true
> when block_job_coroutine_complete() is called, and is checked in
> block_job_finish_sync to determine which context to poll.
> 
> Suggested-by: Stefan Hajnoczi 
> Signed-off-by: Fam Zheng 
> ---
>  blockjob.c   | 6 +-
>  include/block/blockjob.h | 5 +
>  2 files changed, 10 insertions(+), 1 deletion(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v9 05/16] virtio-scsi: Catch BDS-BB removal/insertion

[Fam Zheng]

On Fri, 01/29 16:36, Max Reitz wrote:
> 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 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH] block: add missing call to bdrv_drain_recurse

[Stefan Hajnoczi]

On Wed, Dec 23, 2015 at 11:48:25AM +0100, Paolo Bonzini wrote:
> This is also needed in bdrv_drain_all, not just in bdrv_drain.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/io.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 841f5b5..bfe2544 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -293,6 +293,7 @@ void bdrv_drain_all(void)
>  if (bs->job) {
>  block_job_pause(bs->job);
>  }
> +bdrv_drain_recurse(bs);
>  aio_context_release(aio_context);

Applied to my block tree.

Thanks,
Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v5 05/10] qemu-io: allow specifying image as a set of options args

[Eric Blake]

On 02/02/2016 05:57 AM, Daniel P. Berrange wrote:
> Currently qemu-io allows an image filename to be passed on the
> command line, but unless using the JSON format, it does not have
> a way to set any options except the format eg
> 
>  qemu-io https://127.0.0.1/images/centos7.iso
>  qemu-io /home/berrange/demo.qcow2
> 
> This adds a --image-opts arg that indicates that the positional
> filename should be interpreted as a full option string, not
> just a filename.
> 
>  qemu-io --image-opts driver=https,url=https://127.0.0.1/images,sslverify=off
>  qemu-io --image-opts driver=file,filename=/home/berrange/demo.qcow2
> 
> This flag is mutually exclusive with the '-f' flag.

I guess it's easier to enforce the mutual exclusion, than it is to try
and figure out how to make -f work with the --image-opts filename as
long as the two aren't specifying conflicting formats.  Seems okay as
long as it is documented well.

> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  qemu-io.c | 31 ++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 

> @@ -515,7 +531,20 @@ int main(int argc, char **argv)
>  flags |= BDRV_O_RDWR;
>  }
>  
> -if ((argc - optind) == 1) {
> +if (imageOpts) {
> +QemuOpts *qopts;
> +qopts = qemu_opts_parse_noisily(_opts, argv[optind], false);

Ouch. If argc == optind (possible if I type 'qemu-io --image-opts'
without a filename), then argv[optind] == NULL, and you end up calling
strncmp(NULL, "id=", 3) inside opts_parse().

Also, I noticed that running 'qemu-io' without arguments puts you into a
shell mode, where you can then open files after the fact via the
open_f() callback function (the 'open' command) - either that function
should that function be given --image-opts support, or use of
--image-opts from the command line should globally affect all subsequent
use of open_f().

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] nbd: avoid unaligned uint64_t store

[John Snow]

cpu_to_be64w can't be used to make unaligned stores, but stq_be_p can.
The other stores in this routine are left alone, they're aligned already.

Signed-off-by: John Snow 
---
 nbd/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 1ec79cf..5b65059 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -441,7 +441,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData 
*data)
 }
 
 assert ((client->exp->nbdflags & ~65535) == 0);
-cpu_to_be64w((uint64_t*)(buf + 18), client->exp->size);
+stq_be_p((uint64_t *)(buf + 18), client->exp->size);
 cpu_to_be16w((uint16_t*)(buf + 26), client->exp->nbdflags | myflags);
 if (nbd_negotiate_write(csock, buf + 18,
 sizeof(buf) - 18) != sizeof(buf) - 18) {
-- 
2.4.3

Re: [Qemu-block] [PATCH] qemu-img: initialize MapEntry object

[Fam Zheng]

On Wed, 02/03 18:38, John Snow wrote:
> Commit 16b0d555 introduced an issue where we are not initializing
> has_filename for the 'next' MapEntry object, which leads to interesting
> errors in Valgrind and Clang -fsanitize=undefined both.
> 
> Zero the stack object at allocation AND make sure the utility to
> populate the fields properly marks has_filename as false if applicable.
> 
> Signed-off-by: John Snow 
> ---
>  qemu-img.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index f121980..5a85178 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2231,6 +2231,9 @@ static int get_block_status(BlockDriverState *bs, 
> int64_t sector_num,
>  if (file && e->has_offset) {
>  e->has_filename = true;
>  e->filename = file->filename;
> +} else {
> +e->has_filename = false;
> +e->filename = NULL;
>  }
>  return 0;
>  }
> @@ -2264,7 +2267,7 @@ static int img_map(int argc, char **argv)
>  BlockDriverState *bs;
>  const char *filename, *fmt, *output;
>  int64_t length;
> -MapEntry curr = { .length = 0 }, next;
> +MapEntry curr = { .length = 0 }, next = { .length = 0 };
>  int ret = 0;
>  
>  fmt = NULL;
> -- 
> 2.4.3
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH v13 00/10] Block replication for continuous checkpoints

[Changlong Xie]

On 02/01/2016 09:18 AM, Wen Congyang wrote:

On 01/29/2016 06:47 PM, Dr. David Alan Gilbert wrote:

* Wen Congyang (we...@cn.fujitsu.com) wrote:

On 01/29/2016 06:07 PM, Dr. David Alan Gilbert wrote:

* Wen Congyang (we...@cn.fujitsu.com) wrote:

On 01/27/2016 07:03 PM, Dr. David Alan Gilbert wrote:

Hi,
   I've got a block error if I kill the secondary.

Start both primary & secondary
kill -9 secondary qemu
x_colo_lost_heartbeat on primary

The guest sees a block error and the ext4 root switches to read-only.

I gdb'd the primary with a breakpoint on quorum_report_bad; see
backtrace below.
(This is based on colo-v2.4-periodic-mode of the framework
code with the block and network proxy merged in; so it could be my
merging but I don't think so ?)


(gdb) where
#0  quorum_report_bad (node_name=0x7f2946a0892c "node0", ret=-5, 
acb=0x7f2946cb3910, acb=0x7f2946cb3910)
 at /root/colo/jan-2016/qemu/block/quorum.c:222
#1  0x7f2943b23058 in quorum_aio_cb (opaque=, ret=)
 at /root/colo/jan-2016/qemu/block/quorum.c:315
#2  0x7f2943b311be in bdrv_co_complete (acb=0x7f2946cb3f60) at 
/root/colo/jan-2016/qemu/block/io.c:2122
#3  0x7f2943ae777d in aio_bh_call (bh=) at 
/root/colo/jan-2016/qemu/async.c:64
#4  aio_bh_poll (ctx=ctx@entry=0x7f2945b771d0) at 
/root/colo/jan-2016/qemu/async.c:92
#5  0x7f2943af5090 in aio_dispatch (ctx=0x7f2945b771d0) at 
/root/colo/jan-2016/qemu/aio-posix.c:305
#6  0x7f2943ae756e in aio_ctx_dispatch (source=, 
callback=,
 user_data=) at /root/colo/jan-2016/qemu/async.c:231
#7  0x7f293b84a79a in g_main_context_dispatch () from 
/lib64/libglib-2.0.so.0
#8  0x7f2943af3a00 in glib_pollfds_poll () at 
/root/colo/jan-2016/qemu/main-loop.c:211
#9  os_host_main_loop_wait (timeout=) at 
/root/colo/jan-2016/qemu/main-loop.c:256
#10 main_loop_wait (nonblocking=) at 
/root/colo/jan-2016/qemu/main-loop.c:504
#11 0x7f29438529ee in main_loop () at /root/colo/jan-2016/qemu/vl.c:1945
#12 main (argc=, argv=, envp=) at 
/root/colo/jan-2016/qemu/vl.c:4707

(gdb) p s->num_children
$1 = 2
(gdb) p acb->success_count
$2 = 0
(gdb) p acb->is_read
$5 = false


Sorry for the late reply.


No problem.


What it the value of acb->count?


(gdb) p acb->count
$1 = 1


Note, the count is 1, not 2. Writing to children.0 is in flight. If writing to 
children.0 successes,
the guest doesn't know this error.

If secondary host is down, you should remove quorum's children.1. Otherwise, 
you will get
I/O error event.


Is that safe?  If the secondary fails, do you always have time to issue the 
command to
remove the children.1  before the guest sees the error?


We will write to two children, and expect that writing to children.0 will 
success. If so,
the guest doesn't know this error. You just get the I/O error event.


I think children.0 is the disk, and that should be OK - so only the 
children.1/replication should
be failing - so in that case why do I see the error?


I don't know, and I will check the codes.


The 'node0' in the backtrace above is the name of the replication, so it does 
look like the error
is coming from the replication.


No, the backtrace is just report an I/O error events to the management 
application.




Anyway, I tried removing children.1 but it segfaults now, I guess the 
replication is unhappy:

(qemu) x_block_change colo-disk0 -d children.1
(qemu) x_colo_lost_heartbeat


Hmm, you should not remove the child before failover. I will check it how to 
avoid it in the codes.


  But you said 'If secondary host is down, you should remove quorum's 
children.1' - is that not
what you meant?


Yes, you should excute 'x_colo_lost_heartbeat' fist, and then excute 
'x_block_change ... -d ...'.


Hi david

It seems we missed 'drive_del' command, and will document it in next 
version. Here is the right commands order:


{ "execute": "x-colo-lost-heartbeat" }
{ 'execute': 'x-blockdev-change', 'arguments': {'parent': 'colo-disk', 
'child': 'children.1'}}
{ 'execute': 'human-monitor-command', 'arguments': {'command-line': 
'drive_del x'}}


Thanks
-Xie



12973 Segmentation fault  (core dumped) 
./try/x86_64-softmmu/qemu-system-x86_64 -enable-kvm $console_param -S -boot c 
-m 4080 -smp 4 -machine pc-i440fx-2.5,accel=kvm -name debug-threads=on -trace 
events=trace-file -device virtio-rng-pci $block_param $net_param

#0  0x7f0a398a864c in bdrv_stop_replication (bs=0x7f0a3b0a8430, 
failover=true, errp=0x7fff6a5c3420)
 at /root/colo/jan-2016/qemu/block.c:4426

(gdb) p drv
$1 = (BlockDriver *) 0x5d2a

   it looks like the whole of bs is bogus.

#1  0x7f0a398d87f6 in quorum_stop_replication (bs=, 
failover=,
 errp=) at /root/colo/jan-2016/qemu/block/quorum.c:1213

(gdb) p s->replication_index
$3 = 1

I guess quorum_del_child needs to stop replication before it removes the child?


Yes, but in the newest version, quorum doesn't know the block replication, and 
I think
we shoud add an reference to the bs when starting block replication.

Re: [Qemu-block] [PATCH 1/2] block/nbd: Reject port parameter without host

[Max Reitz]

On 03.02.2016 17:38, Eric Blake wrote:
> On 02/03/2016 09:33 AM, Max Reitz wrote:
>> This is better than the generic block layer finding out later that the
>> port parameter has not been used.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  block/nbd.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 1a90bc7..063c403 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -201,6 +201,10 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict 
>> *options, char **export,
>>  }
>>  return NULL;
>>  }
>> +if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
>> +error_setg(errp, "port may not be used without host.");
> 
> No trailing '.'

I plead guilty. I was tempted by the error_setg() calls above this one,
I'll add a patch to fix them in v2.

> With that fixed,
> Reviewed-by: Eric Blake 

Thanks,

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Eric Blake]

On 02/03/2016 09:33 AM, Max Reitz wrote:
> We have to introduce a new object (BlockdevOptionsNbd) for several
> reasons:
> - Neither of InetSocketAddress nor UnixSocketAddress alone is
>   sufficient, because both are supported
> - We cannot use SocketAddress because NBD does not support an fd,
>   and because it is not a flat union which BlockdevOptionsNbd is

Can we do it anyways, and just error out/document that fd is unsupported?

> - We cannot use a flat union of InetSocketAddress and
>   UnixSocketAddress because we would need some kind of discriminator
>   which we do not have; we could inline the UnixSocketAddress as a
>   string and then make it an 'alternate' type instead of a union, but
>   this will not work either, because:
> - InetSocketAddress itself is not suitable for NBD because the port is
>   not optional (which it is for NBD) and because it offers more options
>   (like choosing between ipv4 and ipv6) which NBD does not support.

That, and qapi doesn't (yet) support the use of a flat union as the
branch of yet another flat union.

I'd like to reach the point where we can have a flat union with an
implicit discriminator (if the discriminator was not present, the
require a default branch), but don't think it should hold up this patch.
I also think that future qapi improvements may make it possible to
retrofit this struct to make the mutual exclusion between host/file more
obvious during introspection, rather than just by documentation.

> 
> Signed-off-by: Max Reitz 
> ---
>  qapi/block-core.json | 28 ++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 

>  ##
> +# @BlockdevOptionsNbd
> +#
> +# Driver specific block device options for NBD. Either of @host or @path 
> must be
> +# specified, but not both.
> +#
> +# @host:#optional Connects to the given host using TCP.
> +#
> +# @port:#optional Specifies the TCP port to connect to; may be used only 
> in
> +#   conjunction with @host. Defaults to 10809.
> +#
> +# @path:#optional Connects to the given Unix socket path.
> +#
> +# @export:  #optional Name of the NBD export to open.

Maybe mention that the default is no export name.

> +#
> +# Since: 2.6
> +##
> +{ 'struct': 'BlockdevOptionsNbd',
> +  'data': { '*host':'str',
> +'*port':'str',
> +'*path':'str',
> +'*export':  'str' } }

I'm not entirely convinced this is the final representation we want, but
I can't immediately propose anything nicer.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] qemu-img: initialize MapEntry object

[John Snow]

Commit 16b0d555 introduced an issue where we are not initializing
has_filename for the 'next' MapEntry object, which leads to interesting
errors in Valgrind and Clang -fsanitize=undefined both.

Zero the stack object at allocation AND make sure the utility to
populate the fields properly marks has_filename as false if applicable.

Signed-off-by: John Snow 
---
 qemu-img.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index f121980..5a85178 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2231,6 +2231,9 @@ static int get_block_status(BlockDriverState *bs, int64_t 
sector_num,
 if (file && e->has_offset) {
 e->has_filename = true;
 e->filename = file->filename;
+} else {
+e->has_filename = false;
+e->filename = NULL;
 }
 return 0;
 }
@@ -2264,7 +2267,7 @@ static int img_map(int argc, char **argv)
 BlockDriverState *bs;
 const char *filename, *fmt, *output;
 int64_t length;
-MapEntry curr = { .length = 0 }, next;
+MapEntry curr = { .length = 0 }, next = { .length = 0 };
 int ret = 0;
 
 fmt = NULL;
-- 
2.4.3

Re: [Qemu-block] [PATCH v5 06/10] qemu-nbd: allow specifying image as a set of options args

[Eric Blake]

On 02/02/2016 05:57 AM, Daniel P. Berrange wrote:
> Currently qemu-nbd allows an image filename to be passed on the
> command line, but unless using the JSON format, it does not have
> a way to set any options except the format eg
> 
>qemu-nbd https://127.0.0.1/images/centos7.iso
>qemu-nbd /home/berrange/demo.qcow2
> 
> This adds a --image-opts arg that indicates that the positional
> filename should be interpreted as a full option string, not
> just a filename.
> 
>qemu-nbd --image-opts 
> driver=https,url=https://127.0.0.1/images,sslverify=off
>qemu-nbd --image-opts driver=file,filename=/home/berrange/demo.qcow2
> 
> This flag is mutually exclusive with the '-f' flag.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  qemu-nbd.c | 42 +-
>  1 file changed, 37 insertions(+), 5 deletions(-)

Where is this new option documented? At a minimum, 'qemu-nbd --help'
should mention it. If later in the series, mention that in the commit
message.

> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 0e019c1..ee91e47 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -47,6 +47,7 @@
>  #define QEMU_NBD_OPT_DISCARD   3
>  #define QEMU_NBD_OPT_DETECT_ZEROES 4
>  #define QEMU_NBD_OPT_OBJECT5
> +#define QEMU_NBD_OPT_IMAGE_OPTS6

Churn here where 8/10 has to touch the same line; but I'm not sure
rearranging the series is worth the effort, so I don't mind it.

> @@ -724,13 +740,29 @@ int main(int argc, char **argv)
>  bdrv_init();
>  atexit(bdrv_close_all);

There's an earlier use of argv[optind] for the --disconnect option;
should that code be tweaked at all, or is it always safe for that path
to blindly open(name) without trying to parse options?


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



signature.asc
Description: OpenPGP digital signature

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

[Programmingkid]

On Feb 2, 2016, at 11:36 PM, Eric Blake wrote:

> On 02/02/2016 09:21 PM, Programmingkid wrote:
> 
 #if defined(__APPLE__) && defined(__MACH__)
   /* if a physical device experienced an error while being opened */
   if (strncmp((*bsd_path ? bsd_path : filename), "/dev/", 5) == 0) {
   print_unmounting_directions(*bsd_path ? bsd_path : filename);
   return -1;
   }
>>> 
>>> A bit repetitive. You don't use filename after the fact, so shorter
>>> would be:
>>> 
>>> #if defined(__APPLE__)...
>>>   if (*bsd_path) {
>>>   filename = filename;
> 
> Oops, I shouldn't be writing emails late at night.  Let's try this again.
> 
> if (*bsd_path) {
>filename = bsd_path;
> }
> if (strncmp(filename, ...
> 
> Hopefully that makes more sense.

So you want this:

#if defined(__APPLE__) && defined(__MACH__)
if (*bsd_path) {
filename = bsd_path;
}
/* if a physical device experienced an error while being opened */
if (strncmp(filename, "/dev/", 5) == 0) {
print_unmounting_directions(filename);
return -1;
}
#endif /* defined(__APPLE__) && defined(__MACH__) */

[Qemu-block] [PATCH 0/2] qapi: Allow blockdev-add for NBD

[Max Reitz]

While there are more formats still missing blockdev-add support, I
personally felt like NBD really is one we do need, and also it was
pretty simple to do.

Therefore, I started with NBD. Maybe I'll follow up with other formats.


Max Reitz (2):
  block/nbd: Reject port parameter without host
  qapi: Allow blockdev-add for NBD

 block/nbd.c  |  4 
 qapi/block-core.json | 28 ++--
 2 files changed, 30 insertions(+), 2 deletions(-)

-- 
2.7.0

[Qemu-block] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Max Reitz]

We have to introduce a new object (BlockdevOptionsNbd) for several
reasons:
- Neither of InetSocketAddress nor UnixSocketAddress alone is
  sufficient, because both are supported
- We cannot use SocketAddress because NBD does not support an fd,
  and because it is not a flat union which BlockdevOptionsNbd is
- We cannot use a flat union of InetSocketAddress and
  UnixSocketAddress because we would need some kind of discriminator
  which we do not have; we could inline the UnixSocketAddress as a
  string and then make it an 'alternate' type instead of a union, but
  this will not work either, because:
- InetSocketAddress itself is not suitable for NBD because the port is
  not optional (which it is for NBD) and because it offers more options
  (like choosing between ipv4 and ipv6) which NBD does not support.

Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 28 ++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 33012b8..e1b8543 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1567,13 +1567,14 @@
 # Drivers that are supported in block device operations.
 #
 # @host_device, @host_cdrom: Since 2.1
+# @nbd: Since 2.6
 #
 # Since: 2.0
 ##
 { 'enum': 'BlockdevDriver',
   'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
-'http', 'https', 'null-aio', 'null-co', 'parallels',
+'http', 'https', 'nbd', 'null-aio', 'null-co', 'parallels',
 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
 'vmdk', 'vpc', 'vvfat' ] }
 
@@ -2002,6 +2003,29 @@
 '*read-pattern': 'QuorumReadPattern' } }
 
 ##
+# @BlockdevOptionsNbd
+#
+# Driver specific block device options for NBD. Either of @host or @path must 
be
+# specified, but not both.
+#
+# @host:#optional Connects to the given host using TCP.
+#
+# @port:#optional Specifies the TCP port to connect to; may be used only in
+#   conjunction with @host. Defaults to 10809.
+#
+# @path:#optional Connects to the given Unix socket path.
+#
+# @export:  #optional Name of the NBD export to open.
+#
+# Since: 2.6
+##
+{ 'struct': 'BlockdevOptionsNbd',
+  'data': { '*host':'str',
+'*port':'str',
+'*path':'str',
+'*export':  'str' } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.
@@ -2027,7 +2051,7 @@
   'http':   'BlockdevOptionsFile',
   'https':  'BlockdevOptionsFile',
 # TODO iscsi: Wait for structured options
-# TODO nbd: Should take InetSocketAddress for 'host'?
+  'nbd':'BlockdevOptionsNbd',
 # TODO nfs: Wait for structured options
   'null-aio':   'BlockdevOptionsNull',
   'null-co':'BlockdevOptionsNull',
-- 
2.7.0

[Qemu-block] [PATCH 1/2] block/nbd: Reject port parameter without host

[Max Reitz]

This is better than the generic block layer finding out later that the
port parameter has not been used.

Signed-off-by: Max Reitz 
---
 block/nbd.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 1a90bc7..063c403 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -201,6 +201,10 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict 
*options, char **export,
 }
 return NULL;
 }
+if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
+error_setg(errp, "port may not be used without host.");
+return NULL;
+}
 
 saddr = g_new0(SocketAddress, 1);
 
-- 
2.7.0

Re: [Qemu-block] [PATCH 1/2] block/nbd: Reject port parameter without host

[Eric Blake]

On 02/03/2016 09:33 AM, Max Reitz wrote:
> This is better than the generic block layer finding out later that the
> port parameter has not been used.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/nbd.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 1a90bc7..063c403 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -201,6 +201,10 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict 
> *options, char **export,
>  }
>  return NULL;
>  }
> +if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
> +error_setg(errp, "port may not be used without host.");

No trailing '.'

With that fixed,
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 v11 1/2] mirror: Rewrite mirror_iteration

[Max Reitz]

On 03.02.2016 02:52, Fam Zheng wrote:
> The "pnum < nb_sectors" condition in deciding whether to actually copy
> data is unnecessarily strict, and the qiov initialization is
> unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
> 
> Rewrite mirror_iteration to fix both flaws.
> 
> The output of iotests 109 is updated because we now report the offset
> and len slightly differently in mirroring progress.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/mirror.c | 335 
> +++--
>  tests/qemu-iotests/109.out |  80 +--
>  trace-events   |   1 -
>  3 files changed, 243 insertions(+), 173 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Max Reitz]

On 03.02.2016 18:06, Daniel P. Berrange wrote:
> On Wed, Feb 03, 2016 at 05:33:16PM +0100, Max Reitz wrote:
>> We have to introduce a new object (BlockdevOptionsNbd) for several
>> reasons:
>> - Neither of InetSocketAddress nor UnixSocketAddress alone is
>>   sufficient, because both are supported
>> - We cannot use SocketAddress because NBD does not support an fd,
>>   and because it is not a flat union which BlockdevOptionsNbd is
> 
> With my patch series that converts NBD to use QIOChannel, all the
> entry points for client & server *do* take a SocketAddress struct
> to provide address info. So internally the code does in fact allow
> use of an FD, if there were a way to specify it a the QAPI level...
> 
> eg see
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04159.html
> 
>> - We cannot use a flat union of InetSocketAddress and
>>   UnixSocketAddress because we would need some kind of discriminator
>>   which we do not have; we could inline the UnixSocketAddress as a
>>   string and then make it an 'alternate' type instead of a union, but
>>   this will not work either, because:
>> - InetSocketAddress itself is not suitable for NBD because the port is
>>   not optional (which it is for NBD) and because it offers more options
>>   (like choosing between ipv4 and ipv6) which NBD does not support.
> 
> The *should* support ipv4 and ipv6 options for NBD. We should also make
> the port optional in the SocketAddress struct - I tried to do that previously
> but my patch was flawed, but we should revisit this.
> 
> So IMHO all the things you list above are reasons *for* using SocketAddress
> and not re-inventing it poorly with explicit host + port fields.

That's good news, thanks!

However, the issue remains that the NBD block driver expects flattened
options which is syntactically incompatible to SocketAddress. Maybe the
best way to address this would be to just make block/nbd.c directly
accept a SocketAddress and keep the old flattened @host, @port, and
@path options only as a legacy mapping to inet.host, inet.port, and
unix.path.

Anyway, I'll wait for your series to get merged, then.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Daniel P. Berrange]

On Wed, Feb 03, 2016 at 05:33:16PM +0100, Max Reitz wrote:
> We have to introduce a new object (BlockdevOptionsNbd) for several
> reasons:
> - Neither of InetSocketAddress nor UnixSocketAddress alone is
>   sufficient, because both are supported
> - We cannot use SocketAddress because NBD does not support an fd,
>   and because it is not a flat union which BlockdevOptionsNbd is

With my patch series that converts NBD to use QIOChannel, all the
entry points for client & server *do* take a SocketAddress struct
to provide address info. So internally the code does in fact allow
use of an FD, if there were a way to specify it a the QAPI level...

eg see

https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04159.html

> - We cannot use a flat union of InetSocketAddress and
>   UnixSocketAddress because we would need some kind of discriminator
>   which we do not have; we could inline the UnixSocketAddress as a
>   string and then make it an 'alternate' type instead of a union, but
>   this will not work either, because:
> - InetSocketAddress itself is not suitable for NBD because the port is
>   not optional (which it is for NBD) and because it offers more options
>   (like choosing between ipv4 and ipv6) which NBD does not support.

The *should* support ipv4 and ipv6 options for NBD. We should also make
the port optional in the SocketAddress struct - I tried to do that previously
but my patch was flawed, but we should revisit this.

So IMHO all the things you list above are reasons *for* using SocketAddress
and not re-inventing it poorly with explicit host + port fields.

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

Re: [Qemu-block] [PATCH 2/2] qapi: Allow blockdev-add for NBD

[Max Reitz]

On 03.02.2016 17:48, Eric Blake wrote:
> On 02/03/2016 09:33 AM, Max Reitz wrote:
>> We have to introduce a new object (BlockdevOptionsNbd) for several
>> reasons:
>> - Neither of InetSocketAddress nor UnixSocketAddress alone is
>>   sufficient, because both are supported
>> - We cannot use SocketAddress because NBD does not support an fd,
>>   and because it is not a flat union which BlockdevOptionsNbd is
> 
> Can we do it anyways, and just error out/document that fd is unsupported?

Would be possible, if InetSocketAddress's port was optional and if it
was a flat union.

(Note that the port not being optional is not a real issue; it just
means the user cannot omit it when using blockdev-add, but that's not so
bad.)

>> - We cannot use a flat union of InetSocketAddress and
>>   UnixSocketAddress because we would need some kind of discriminator
>>   which we do not have; we could inline the UnixSocketAddress as a
>>   string and then make it an 'alternate' type instead of a union, but
>>   this will not work either, because:
>> - InetSocketAddress itself is not suitable for NBD because the port is
>>   not optional (which it is for NBD) and because it offers more options
>>   (like choosing between ipv4 and ipv6) which NBD does not support.
> 
> That, and qapi doesn't (yet) support the use of a flat union as the
> branch of yet another flat union.
> 
> I'd like to reach the point where we can have a flat union with an
> implicit discriminator (if the discriminator was not present, the
> require a default branch), but don't think it should hold up this patch.
> I also think that future qapi improvements may make it possible to
> retrofit this struct to make the mutual exclusion between host/file more
> obvious during introspection, rather than just by documentation.

The problem here is that we really just want to merge and flatten
{Inet,Unix}SocketAddress into a single union. The discriminator
basically is of which object all the non-optional fields are present.

>> Signed-off-by: Max Reitz 
>> ---
>>  qapi/block-core.json | 28 ++--
>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>
> 
>>  ##
>> +# @BlockdevOptionsNbd
>> +#
>> +# Driver specific block device options for NBD. Either of @host or @path 
>> must be
>> +# specified, but not both.
>> +#
>> +# @host:#optional Connects to the given host using TCP.
>> +#
>> +# @port:#optional Specifies the TCP port to connect to; may be used 
>> only in
>> +#   conjunction with @host. Defaults to 10809.
>> +#
>> +# @path:#optional Connects to the given Unix socket path.
>> +#
>> +# @export:  #optional Name of the NBD export to open.
> 
> Maybe mention that the default is no export name.

Can do (and will do, because you asked for it), but I thought that "Not
specifying an export name means no export name" to be self-evident. :-)

>> +#
>> +# Since: 2.6
>> +##
>> +{ 'struct': 'BlockdevOptionsNbd',
>> +  'data': { '*host':'str',
>> +'*port':'str',
>> +'*path':'str',
>> +'*export':  'str' } }
> 
> I'm not entirely convinced this is the final representation we want, but
> I can't immediately propose anything nicer.

Ideally, this would just be a SocketAddress. Unfortunately, this is not
possible because SocketAddress is not flat but this has to be.

The representation I guess I'd want under the circumstances would be a
flat union of InetSocketAddress and UnixSocketAddress with a semantic
discriminator as described above (check which non-optional fields are
present).

However, in order for this to work, the code generator would need to
support flat unions with such a semantic discriminator[1], and also the
@port parameter in InetSocketAddress should be optional (which might
require non-trivial changes to existing code that expects an
InetSocketAddress). However, as written above, the port not being
optional would not be too bad.

But in any case, the on-wire interface is stable and defined by
block/nbd.c already, so I believe we can enrich the definition later on.
Maybe we should define @port to be non-optional if @host is specified,
though.

Max


[1] And I don't believe I feel quite comfortable with extending the code
generator...



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 05/10] qemu-io: allow specifying image as a set of options args

[Daniel P. Berrange]

On Wed, Feb 03, 2016 at 08:37:15AM -0700, Eric Blake wrote:
> On 02/02/2016 05:57 AM, Daniel P. Berrange wrote:
> > Currently qemu-io allows an image filename to be passed on the
> > command line, but unless using the JSON format, it does not have
> > a way to set any options except the format eg
> > 
> >  qemu-io https://127.0.0.1/images/centos7.iso
> >  qemu-io /home/berrange/demo.qcow2
> > 
> > This adds a --image-opts arg that indicates that the positional
> > filename should be interpreted as a full option string, not
> > just a filename.
> > 
> >  qemu-io --image-opts 
> > driver=https,url=https://127.0.0.1/images,sslverify=off
> >  qemu-io --image-opts driver=file,filename=/home/berrange/demo.qcow2
> > 
> > This flag is mutually exclusive with the '-f' flag.
> 
> I guess it's easier to enforce the mutual exclusion, than it is to try
> and figure out how to make -f work with the --image-opts filename as
> long as the two aren't specifying conflicting formats.  Seems okay as
> long as it is documented well.
> 
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  qemu-io.c | 31 ++-
> >  1 file changed, 30 insertions(+), 1 deletion(-)
> > 
> 
> > @@ -515,7 +531,20 @@ int main(int argc, char **argv)
> >  flags |= BDRV_O_RDWR;
> >  }
> >  
> > -if ((argc - optind) == 1) {
> > +if (imageOpts) {
> > +QemuOpts *qopts;
> > +qopts = qemu_opts_parse_noisily(_opts, argv[optind], false);
> 
> Ouch. If argc == optind (possible if I type 'qemu-io --image-opts'
> without a filename), then argv[optind] == NULL, and you end up calling
> strncmp(NULL, "id=", 3) inside opts_parse().

Yeah, I should not have removed the ((argc - optind) ==1) check here - it
should be the first thing checked, and imageOpts the second.

> Also, I noticed that running 'qemu-io' without arguments puts you into a
> shell mode, where you can then open files after the fact via the
> open_f() callback function (the 'open' command) - either that function
> should that function be given --image-opts support, or use of
> --image-opts from the command line should globally affect all subsequent
> use of open_f().

That function already has a --option / -o argument that has a similar
result, but I agree that if --image-opts is given on the cli we should
probably use that exclusively.


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