Re: [Qemu-block] [v2 5/5] qapi: allow blockdev-add for ssh

2016-10-15 Thread Max Reitz 
On 15.10.2016 11:04, Ashijeet Acharya wrote:
> Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
> support blockdev-add for SSH network protocol driver. Use only 'struct
> InetSocketAddress' since SSH only supports connection over TCP.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
>  qapi/block-core.json | 24 ++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9d797b8..2e8a390 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1716,7 +1716,8 @@
>  'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
>  'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
>  'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> +'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
> +'vvfat' ] }
>  
>  ##
>  # @BlockdevOptionsFile
> @@ -1953,6 +1954,25 @@
>  '*vport': 'int',
>  '*segment': 'str' } }
>  
> +##
> +# @BlockdevoptionsSsh

Should be *BlockdevOptionsSsh.

> +#
> +# @server:  host address and port number

It could be argued that the port number is part of the host address. I'd
therefore just describe it as "host address" since you can specify some
other options to, like @ipv6.

> +#
> +# @path:path to the image on the host
> +#
> +# @user:user as which to connect

This can actually be an optional argument, and I'd make it one (it
defaults to the current local user name).

> +#
> +# @host_key_check   defines how and what to check the host key against

As you can see from other descriptions, we normally put an "#optional"
in front of descriptions of optional parameters, and it's also a good
idea to specify the default behavior or value, which in this case is
"yes" - intuitively I'd have expected "no", so you should probably
indeed make a note of that.

Max

> +#
> +# Since 2.8
> +##
> +{ 'struct': 'BlockdevoptionsSsh',
> +  'data': { 'server': 'InetSocketAddress',
> +'path': 'str',
> +'user': 'str',
> +'*host_key_check': 'str' } }
> +
>  
>  ##
>  # @BlkdebugEvent
> @@ -2281,7 +2301,7 @@
>  # TODO rbd: Wait for structured options
>'replication':'BlockdevOptionsReplication',
>  # TODO sheepdog: Wait for structured options
> -# TODO ssh: Should take InetSocketAddress for 'host'?
> +  'ssh':'BlockdevoptionsSsh',
>'tftp':   'BlockdevOptionsCurl',
>'vdi':'BlockdevOptionsGenericFormat',
>'vhdx':   'BlockdevOptionsGenericFormat',
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [v2 4/5] block/ssh: Use InetSocketAddress options

2016-10-15 Thread Max Reitz 
On 15.10.2016 11:04, Ashijeet Acharya wrote:
> Drop the use of legacy options in favour of the InetSocketAddress
> options.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
>  block/ssh.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 6420359..7fec0e1 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -199,6 +199,7 @@ static int parse_uri(const char *filename, QDict 
> *options, Error **errp)
>  {
>  URI *uri = NULL;
>  QueryParams *qp;
> +char *port_str;
>  int i;
>  
>  uri = uri_parse(filename);
> @@ -231,11 +232,10 @@ static int parse_uri(const char *filename, QDict 
> *options, Error **errp)
>  qdict_put(options, "user", qstring_from_str(uri->user));
>  }
>  
> -qdict_put(options, "host", qstring_from_str(uri->server));
> +qdict_put(options, "server.host", qstring_from_str(uri->server));
>  
> -if (uri->port) {
> -qdict_put(options, "port", qint_from_int(uri->port));
> -}
> +port_str = g_strdup_printf("%d", uri->port ?: 22);
> +qdict_put(options, "server.port", qstring_from_str(port_str));
>  
>  qdict_put(options, "path", qstring_from_str(uri->path));
>  
> @@ -251,6 +251,7 @@ static int parse_uri(const char *filename, QDict 
> *options, Error **errp)
>  
>  query_params_free(qp);
>  uri_free(uri);
> +g_free(port_str);

I'd put this right after qdict_put(..., qstring_from_str(port_str));.
But that's up to you, either way:

Reviewed-by: Max Reitz 

>  return 0;
>  
>   err:
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [v2 3/5] block/ssh: Use inet_connect_saddr() to establish socket connection

2016-10-15 Thread Max Reitz 
On 15.10.2016 11:04, Ashijeet Acharya wrote:
> Make inet_connect_saddr() in util/qemu-socktets.c public and use it

*util/qemu-sockets.c

> instead of inet_connect() because this directly takes the
> InetSocketAddress to establish a socket connection for the SSH
> block driver.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
>  block/ssh.c| 5 +
>  include/qemu/sockets.h | 2 ++
>  util/qemu-sockets.c| 2 +-
>  3 files changed, 4 insertions(+), 5 deletions(-)

As I said for patch 3, I'd split this one and pull the part of it that
is making inet_connect_saddr() public in front of patch 2 and squash the
rest into patch 2.

> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 3b18907..6420359 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -666,13 +666,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
> *options,
>  goto err;
>  }
>  
> -/* Construct the host:port name for inet_connect. */
> -g_free(s->hostport);
>  port = atoi(s->inet->port);
> -s->hostport = g_strdup_printf("%s:%d", s->inet->host, port);
>  
>  /* Open the socket and connect. */
> -s->sock = inet_connect(s->hostport, errp);
> +s->sock = inet_connect_saddr(s->inet, errp, NULL, NULL);
>  if (s->sock < 0) {
>  ret = -EIO;
>  goto err;
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 9eb2470..5589e68 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -34,6 +34,8 @@ typedef void NonBlockingConnectHandler(int fd, Error *err, 
> void *opaque);
>  
>  InetSocketAddress *inet_parse(const char *str, Error **errp);
>  int inet_connect(const char *str, Error **errp);
> +int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
> +   NonBlockingConnectHandler *callback, void *opaque);
>  
>  NetworkAddressFamily inet_netfamily(int family);
>  
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 4cef549..3411888 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -412,7 +412,7 @@ static struct addrinfo 
> *inet_parse_connect_saddr(InetSocketAddress *saddr,
>   * function succeeds, callback will be called when the connection
>   * completes, with the file descriptor on success, or -1 on error.
>   */
> -static int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
> +int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
>NonBlockingConnectHandler *callback, void 
> *opaque)

You should keep the second line aligned to the opening parenthesis.

Max

>  {
>  Error *local_err = NULL;
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [v2 2/5] block/ssh: Add InetSocketAddress and accept it

2016-10-15 Thread Max Reitz 
On 15.10.2016 11:04, Ashijeet Acharya wrote:
> Add InetSocketAddress compatibility to SSH driver.
> 
> Add a new option "server" to the SSH block driver which then accepts
> a InetSocketAddress.
> 
> "host" and "port" are supported as legacy options and are mapped to
> their InetSocketAddress representation.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
>  block/ssh.c | 83 
> ++---
>  1 file changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 75cb7bc..3b18907 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -30,10 +30,14 @@
>  #include "block/block_int.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
> +#include "qemu/cutils.h"
>  #include "qemu/sockets.h"
>  #include "qemu/uri.h"
> +#include "qapi-visit.h"
>  #include "qapi/qmp/qint.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qobject-output-visitor.h"
>  
>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>   * this block driver code.
> @@ -74,6 +78,8 @@ typedef struct BDRVSSHState {
>   */
>  LIBSSH2_SFTP_ATTRIBUTES attrs;
>  
> +InetSocketAddress *inet;
> +
>  /* Used to warn if 'flush' is not supported. */
>  char *hostport;
>  bool unsafe_flush_warning;
> @@ -263,7 +269,9 @@ static bool ssh_has_filename_options_conflict(QDict 
> *options, Error **errp)
>  !strcmp(qe->key, "port") ||
>  !strcmp(qe->key, "path") ||
>  !strcmp(qe->key, "user") ||
> -!strcmp(qe->key, "host_key_check"))
> +!strcmp(qe->key, "host_key_check") ||
> +!strcmp(qe->key, "server") ||

I know I've done the same in my series, but I'll actually drop this
condition from the next version; I'm not actually handling the case
where the destination address is not flattened, and neither are you
(we're both using qdict_extract_subqdict() instead of testing whether
"server" is an object on its own), so I think you should drop it as well
and just test for the prefix.

It doesn't harm to test for "server" itself, but I think it's a bit
confusing still, since you (we) are not dealing with that possibility
anywhere else.

> +!strstart(qe->key, "server.", NULL))

It should be just "strstart", not "!strstart", because the function
returns 1 if the prefix matches.

>  {
>  error_setg(errp, "Option '%s' cannot be used with a file name",
> qe->key);
> @@ -555,13 +563,66 @@ static QemuOptsList ssh_runtime_opts = {
>  },
>  };
>  
> +static bool ssh_process_legacy_socket_options(QDict *output_opts,
> +  QemuOpts *legacy_opts,
> +  Error **errp)
> +{
> +const char *host = qemu_opt_get(legacy_opts, "host");
> +const char *port = qemu_opt_get(legacy_opts, "port");
> +
> +if (!host && port) {
> +error_setg(errp, "port may not be used without host");
> +return false;
> +} else {
> +qdict_put(output_opts, "server.host", qstring_from_str(host));

This will segfault if host is NULL. You shouldn't go into this branch at
all if host is not set.

> +qdict_put(output_opts, "server.port",
> +  qstring_from_str(port ?: stringify(22)));
> +}
> +
> +return true;
> +}
> +
> +static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options,
> + Error **errp)
> +{
> +InetSocketAddress *inet = NULL;
> +QDict *addr = NULL;
> +QObject *crumpled_addr = NULL;
> +Visitor *iv = NULL;
> +Error *local_error = NULL;
> +
> +qdict_extract_subqdict(options, , "server.");
> +if (!qdict_size(addr)) {
> +error_setg(errp, "SSH server address missing");
> +goto out;
> +}
> +
> +crumpled_addr = qdict_crumple(addr, true, errp);
> +if (!crumpled_addr) {
> +goto out;
> +}
> +
> +iv = qobject_input_visitor_new_autocast(crumpled_addr, true, 1, true);

In contrast to what Kevin said in v1, I think you do not want to use
autocast here.

Or, to be more specific, it's difficult. The thing is that the autocast
documentation says: "Any scalar values in the @obj input data structure
should always be represented as strings".

So if you do use the autocast version, command line works great because
from there everything comes as a string. But blockdev-add no longer
works because from there everything comes with the correct type (and you
cannot give it the wrong type). Case in point, this happens if you try
to create an SSH BDS with "'ipv4': true":

{"error": {"class": "GenericError", "desc": "Invalid parameter type for
'ipv4', expected: string"}}

OK, let's try "'ipv4': 'true'" then:

{"error": {"class": "GenericError", "desc": "Invalid parameter type for
'ipv4', expected: boolean"}}

Too bad. The former is a message from

Re: [Qemu-block] [v2 1/5] block/ssh: Add ssh_has_filename_options_conflict()

2016-10-15 Thread Max Reitz 
On 15.10.2016 11:04, Ashijeet Acharya wrote:
> We have 5 options plus ("server") option which is added in the next
> patch that conflict with specifying a SSH filename. We need to iterate
> over all the options to check whether its key has an "server." prefix.
> 
> This iteration will help us adding the new option "server" easily.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
>  block/ssh.c | 27 +--
>  1 file changed, 21 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 0/3] iotests: Fix test 162

2016-10-15 Thread Max Reitz 
On 13.10.2016 07:20, Hao QingFeng wrote:
> 
> 
> 在 2016-10-13 3:46, Max Reitz 写道:
>> On 12.10.2016 10:55, Hao QingFeng wrote:
>>> Max,
>>>
>>> Just a common question for this case, if sshx block driver wasn't built
>>> into qemu-img, this case would fail as below:
>> Good point, and thanks for bringing it up, but it's not directly linked
>> to this series other than by its subject, of course, so I'd rather add a
>> fix on top.
> Thanks and sorry for sending to the improper mail series.
>>> exec /home/haoqf/KVMonz/qemu/tests/qemu-iotests/../../qemu-img info
>>> --image-opts driver=ssh,host=localhost,port=0.42,path=/foo
>>> qemu-img: Could not open
>>> 'driver=ssh,host=localhost,port=0.42,path=/foo': Unknown driver 'ssh'
>>>
>>> Adding 162.notrun can bypass this case but it would skip it even if
>>> qemu-img has sshx block driver, in which case I think it should be run.
>>>
>>> So How about adding a script to dynamically check at runtime if the
>>> current env qemu-img can meet the requirement to run the test or not?
>> Unfortunately, the list of block drivers listed by will not contain ssh
>> if ssh is built as a module, which is possible.
> Actually I am not sure if I understood it. Do you mean
> "CONFIG_LIBSSH2=m" set
> rather than "CONFIG_LIBSSH2=y" in config-host.mak? But in the configure
> it's
> set to be "CONFIG_LIBSSH2=y":
> if test "$libssh2" = "yes" ; then
>   echo "CONFIG_LIBSSH2=y" >> $config_host_mak
>   echo "LIBSSH2_CFLAGS=$libssh2_cflags" >> $config_host_mak
>   echo "LIBSSH2_LIBS=$libssh2_libs" >> $config_host_mak
> fi

I don't know which version of qemu you are looking at, but on master it
says "m" instead of "y" there:

http://git.qemu.org/?p=qemu.git;a=blob;f=configure;h=dd9e6792bbe04411d81eb5438d58eb1999d4dcd2;hb=HEAD#l5477

> Meanwhile I changed it to be "CONFIG_LIBSSH2=m" and reconfig, make the
> qemu,
> qemu-img --help can still prompt ssh.

Have you tried building master with --enable-modules specified for
configure?

Max

>> This is a bug that should be fixed, but I'd rather do so in a separate
>> series from this one.
>>
>> In any case, once it is fixed I'd rather just take the approach quorum
>> tests take already (e.g. test 081), which is something like:
>>
>> test_ssh=$($QEMU_IMG --help | grep '^Supported formats:.* ssh\( \|$\)')
>> [ "$test_ssh" = "" ] && _notrun "ssh support required"
> Cool. Agree with this like what was done in 081.  thanks
>> Max
>>
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

2016-10-15 Thread Vladimir Sementsov-Ogievskiy 

On 15.10.2016 20:03, Max Reitz wrote:

On 14.10.2016 20:44, Vladimir Sementsov-Ogievskiy wrote:

On 01.10.2016 19:26, Max Reitz wrote:

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:

Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are

...


+goto fail;
+}
+
+/* loop is safe because next entry offset is calculated after
conversion to
Should it be s/safe/unsafe/?

loop is safe. _unsafe is related to absence of assert in a loop, as it
loops through BE data. Bad wording, I'll change it somehow..

Yes, I know that the loop is safe in the sense of the word "safe", but I
meant that it's kind of confusing that the loop uses
"for_each_bitmap_dir_entry_unsafe" and thus is "unsafe", too.

Another idea would be to write "This is actually safe" instead of "loop
is safe".


Finally I've decided to introduce normal list of normal structures like 
in snapshots..





+ * cpu format */
+for_each_bitmap_dir_entry_unsafe(e, dir, size) {
+if ((uint8_t *)(e + 1) > dir_end) {
+goto broken_dir;
+}
+
+bitmap_dir_entry_to_cpu(e);
+
+if ((uint8_t *)next_dir_entry(e) > dir_end) {
+goto broken_dir;
+}
+
+if (e->extra_data_size != 0) {
+error_setg(errp, "Can't load bitmap '%.*s' from '%s':"
+   "extra data not supported.", e->name_size,

Full stop again.

Also, I'm not quite sure why you give the device/node name here. You
don't do that anywhere else and I think if we want to emit the
information where something failed, it should be added at some previous
point in the call chain.

For example, how? As I understand, I can emit it only by error_setg, so
actually it would be better to add node and bitmap name to all
error_setg in the code.. or create helper function for it.

error_prepend() would be the function.

This code will be invoked by any code that is opening an image. There
are of course a couple of places where that can be the case: For
external tools such as qemu-img, it's normally pretty clear which image
is meant (and it additionally uses error_reportf_err() to give you more
information).

For -drive, every error message will at least be prepended by the
corresponding -drive parameter, so that will help a bit.

blockdev-add, unfortunately, doesn't do anything like this. But the user
can of course choose to construct the BDS graph node by node and thus
always know which node an error originates from.

Anyway, if you want to add this information to every error message, you
should probably do so in bdrv_open_inherit(). The issue I'd take with
this is that most users probably won't set the node name themselves
(either they don't at all or it's some management tool that does), so
reporting the node name doesn't actually help them at all (and
management tools are not supposed to parse error messages, so it won't
help in that case either).

Max



Thanks for explanations, and for the whole review, it's great! Sorry for 
my laziness and for spelling(

O! I've discovered vim spell, so one problem less, I hope.

--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v4 12/12] iotests: Add test for NBD's blockdev-add interface

2016-10-15 Thread Max Reitz 
On 13.10.2016 15:26, Kevin Wolf wrote:
> Am 28.09.2016 um 22:56 hat Max Reitz geschrieben:
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/147 | 201 
>> +
>>  tests/qemu-iotests/147.out |   5 ++
>>  tests/qemu-iotests/group   |   1 +
>>  3 files changed, 207 insertions(+)
>>  create mode 100755 tests/qemu-iotests/147
>>  create mode 100644 tests/qemu-iotests/147.out
>>
>> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
>> new file mode 100755
>> index 000..61e1e6f
>> --- /dev/null
>> +++ b/tests/qemu-iotests/147
>> @@ -0,0 +1,201 @@
>> +#!/usr/bin/env python
>> +#
>> +# Test case for NBD's blockdev-add interface
>> +#
>> +# Copyright (C) 2016 Red Hat, Inc.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see .
>> +#
>> +
>> +import os
>> +import socket
>> +import stat
>> +import time
>> +import iotests
>> +from iotests import cachemode, imgfmt, qemu_img, qemu_nbd
>> +
>> +NBD_PORT = 10811
>> +
>> +test_img = os.path.join(iotests.test_dir, 'test.img')
>> +
>> +class NBDBlockdevAddBase(iotests.QMPTestCase):
>> +def blockdev_add_options(self, address, export=None):
>> +options = { 'node-name': 'nbd-blockdev',
>> +'driver': 'raw',
>> +'file': {
>> +'driver': 'nbd',
>> +'address': address
>> +} }
>> +if export is not None:
>> +options['file']['export'] = export
>> +return options
>> +
>> +def client_test(self, filename, address, export=None):
>> +bao = self.blockdev_add_options(address, export)
>> +result = self.vm.qmp('blockdev-add', options=bao)
> 
> This needs a rebase (**bao instead of options=bao).
> 
>> +self.assert_qmp(result, 'return', {})
>> +
>> +result = self.vm.qmp('query-named-block-nodes')
>> +for node in result['return']:
>> +if node['node-name'] == 'nbd-blockdev':
>> +self.assert_qmp(node, 'image/filename', filename)
>> +break
>> +
>> +result = self.vm.qmp('x-blockdev-del', node_name='nbd-blockdev')
>> +self.assert_qmp(result, 'return', {})
>> +
>> +
>> +class QemuNBD(NBDBlockdevAddBase):
>> +def setUp(self):
>> +qemu_img('create', '-f', iotests.imgfmt, test_img, '64k')
>> +self.vm = iotests.VM()
>> +self.vm.launch()
>> +
>> +def tearDown(self):
>> +self.vm.shutdown()
>> +os.remove(test_img)
>> +
>> +def _server_up(self, *args):
>> +self.assertEqual(qemu_nbd('-f', imgfmt, test_img, *args), 0)
>> +
>> +def test_inet(self):
>> +self._server_up('-p', str(NBD_PORT))
>> +address = { 'type': 'inet',
>> +'data': {
>> +'host': 'localhost',
>> +'port': str(NBD_PORT)
>> +} }
>> +self.client_test('nbd://localhost:%i' % NBD_PORT, address)
>> +
>> +def test_unix(self):
>> +socket = os.path.join(iotests.test_dir, 'qemu-nbd.socket')
>> +self._server_up('-k', socket)
>> +address = { 'type': 'unix',
>> +'data': { 'path': socket } }
>> +self.client_test('nbd+unix://?socket=' + socket, address)
>> +try:
>> +os.remove(socket)
>> +except OSError:
>> +pass
> 
> If the test case fails, the socket file is leaked. We should probably
> either create and remove it in setUp()/tearDown(), or use a try/finally
> block around the test_unix code.

Yes, will do, thanks.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 09/12] iotests.py: Add qemu_nbd function

2016-10-15 Thread Max Reitz 
On 13.10.2016 15:11, Kevin Wolf wrote:
> Am 28.09.2016 um 22:55 hat Max Reitz geschrieben:
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/iotests.py | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 3329bc1..5a2678f 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -39,6 +39,10 @@ qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
>>  if os.environ.get('QEMU_IO_OPTIONS'):
>>  qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ')
>>  
>> +qemu_nbd_args = [os.environ.get('QEMU_NBD_PROG', 'qemu-nbd')]
>> +if os.environ.get('QEMU_NBD_OPTIONS'):
>> +qemu_nbd_args += os.environ['QEMU_NBD_OPTIONS'].strip().split(' ')
>> +
>>  qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
>>  qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
>>  
>> @@ -87,6 +91,10 @@ def qemu_io(*args):
>>  sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' 
>> '.join(args)))
>>  return subp.communicate()[0]
>>  
>> +def qemu_nbd(*args):
>> +'''Run qemu-nbd in daemon mode and return the parent's exit code'''
>> +return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
> 
> Wouldn't it be better to always use -t, track the PID and shut it down
> explicitly when the test exits?

Probably. It's a lot more complicated, though. I'll see what I can do
but I'm not sure if I can do a lot before 2.8.

Max

> The way you're using qemu-nbd here is fine if the test case passes, but
> if it fails before we access the NBD server, the server keeps running in
> the background.
> 
> Kevin



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 06/12] block/nbd: Accept SocketAddress

2016-10-15 Thread Max Reitz 
On 13.10.2016 13:42, Kevin Wolf wrote:
> Am 28.09.2016 um 22:55 hat Max Reitz geschrieben:
>> Add a new option "address" to the NBD block driver which accepts a
>> SocketAddress.
>>
>> "path", "host" and "port" are still supported as legacy options and are
>> mapped to their corresponding SocketAddress representation.
>>
>> Signed-off-by: Max Reitz 
> 
> Not opposed in principle to your change, but we should try to keep the
> naming consistent between NBD and the other block drivers, notably the
> SSH work that is currently going on.
> 
> This patch uses 'address' for the SockAddress, the proposed SSH patch
> uses 'server'. I don't mind too much which one we choose, though I like
> 'server' a bit better. Anyway, we should choose one and stick to it in
> all drivers.

OK, I'll use "server" then. I don't mind either way, so even a weak
opinion is enough to persuade me. ;-)

>>  block/nbd.c   | 166 
>> ++
>>  tests/qemu-iotests/051.out|   4 +-
>>  tests/qemu-iotests/051.pc.out |   4 +-
>>  3 files changed, 106 insertions(+), 68 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index cdab20f..449f94e 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -32,6 +32,9 @@
>>  #include "qemu/uri.h"
>>  #include "block/block_int.h"
>>  #include "qemu/module.h"
>> +#include "qapi-visit.h"
>> +#include "qapi/qmp-input-visitor.h"
>> +#include "qapi/qmp-output-visitor.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "qapi/qmp/qjson.h"
>>  #include "qapi/qmp/qint.h"
>> @@ -44,7 +47,8 @@ typedef struct BDRVNBDState {
>>  NbdClientSession client;
>>  
>>  /* For nbd_refresh_filename() */
>> -char *path, *host, *port, *export, *tlscredsid;
>> +SocketAddress *saddr;
>> +char *export, *tlscredsid;
>>  } BDRVNBDState;
>>  
>>  static int nbd_parse_uri(const char *filename, QDict *options)
>> @@ -131,7 +135,9 @@ static bool nbd_has_filename_options_conflict(QDict 
>> *options, Error **errp)
>>  if (!strcmp(e->key, "host") ||
>>  !strcmp(e->key, "port") ||
>>  !strcmp(e->key, "path") ||
>> -!strcmp(e->key, "export"))
>> +!strcmp(e->key, "export") ||
>> +!strcmp(e->key, "address") ||
>> +!strncmp(e->key, "address.", 8))
> 
> strstart()?

Yep, will use.

>>  {
>>  error_setg(errp, "Option '%s' cannot be used with a file name",
>> e->key);
>> @@ -205,50 +211,67 @@ out:
>>  g_free(file);
>>  }
>>  
>> -static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error 
>> **errp)
>> +static bool nbd_process_legacy_socket_options(QDict *output_options,
>> +  QemuOpts *legacy_opts,
>> +  Error **errp)
>>  {
>> -SocketAddress *saddr;
>> -
>> -s->path = g_strdup(qemu_opt_get(opts, "path"));
>> -s->host = g_strdup(qemu_opt_get(opts, "host"));
>> -s->port = g_strdup(qemu_opt_get(opts, "port"));
>> -
>> -if (!s->path == !s->host) {
>> -if (s->path) {
>> -error_setg(errp, "path and host may not be used at the same 
>> time");
>> -} else {
>> -error_setg(errp, "one of path and host must be specified");
>> +const char *path = qemu_opt_get(legacy_opts, "path");
>> +const char *host = qemu_opt_get(legacy_opts, "host");
>> +const char *port = qemu_opt_get(legacy_opts, "port");
>> +
>> +if (path && host) {
>> +error_setg(errp, "path and host may not be used at the same time");
>> +return false;
>> +} else if (path) {
>> +if (port) {
>> +error_setg(errp, "port may not be used without host");
>> +return false;
>>  }
>> -return NULL;
>> +
>> +qdict_put(output_options, "address.type", qstring_from_str("unix"));
>> +qdict_put(output_options, "address.data.path", 
>> qstring_from_str(path));
>> +} else if (host) {
>> +qdict_put(output_options, "address.type", qstring_from_str("inet"));
>> +qdict_put(output_options, "address.data.host", 
>> qstring_from_str(host));
>> +qdict_put(output_options, "address.data.port",
>> +  qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT)));
>>  }
>> -if (s->port && !s->host) {
>> -error_setg(errp, "port may not be used without host");
>> -return NULL;
>> +
>> +return true;
>> +}
> 
> If both the legacy option and the new one are given, the legacy one
> takes precedence. Intentional?

No. I think it'll be easiest to just return an error when a user is
trying to use both.

Thanks,

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3] block: Remove "options" indirection from blockdev-add

2016-10-15 Thread Max Reitz 
On 11.10.2016 15:27, Kevin Wolf wrote:
> Now that QAPI supports boxed types, we can have unions at the top level
> of a command, so let's put our real options directly there for
> blockdev-add instead of having a single "options" dict that contains the
> real arguments.
> 
> blockdev-add is still experimental and we already made substantial
> changes to the API recently, so we're free to make changes like this
> one, too.
> 
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Eric Blake 
> Reviewed-by: Markus Armbruster 
> ---
>  docs/qmp-commands.txt  |  84 ---
>  qapi/block-core.json   |   4 +-
>  tests/qemu-iotests/041 |  11 +++--
>  tests/qemu-iotests/067 |  12 +++--
>  tests/qemu-iotests/071 | 118 
> +
>  tests/qemu-iotests/081 |  52 ++
>  tests/qemu-iotests/085 |   9 ++--
>  tests/qemu-iotests/087 |  76 +--
>  tests/qemu-iotests/117 |  12 ++---
>  tests/qemu-iotests/118 |  42 +-
>  tests/qemu-iotests/124 |  20 -
>  tests/qemu-iotests/139 |  10 ++---
>  tests/qemu-iotests/141 |  13 +++---
>  tests/qemu-iotests/155 |  10 ++---
>  14 files changed, 214 insertions(+), 259 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] hw/block/nvme: Simplify if-statements a little bit

2016-10-15 Thread Michael Tokarev 

12.10.2016 18:18, Thomas Huth wrote:

The condition  '!A || (A && B)' is equivalent to '!A || B'.


Applied to -trivial, thanks!

/mjt

Re: [Qemu-block] [Qemu-devel] [v2 0/5] Allow blockdev-add for SSH

2016-10-15 Thread no-reply 
Hi,

Your series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Subject: [Qemu-devel] [v2 0/5] Allow blockdev-add for SSH
Type: series
Message-id: 1476522280-23211-1-git-send-email-ashijeetacha...@gmail.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=16
make docker-test-quick@centos6
make docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] 
patchew/1476522280-23211-1-git-send-email-ashijeetacha...@gmail.com -> 
patchew/1476522280-23211-1-git-send-email-ashijeetacha...@gmail.com
Switched to a new branch 'test'
ff808eb qapi: allow blockdev-add for ssh
7c6e759 block/ssh: Use InetSocketAddress options
a30c804 block/ssh: Use inet_connect_saddr() to establish socket connection
5b5cfe0 block/ssh: Add InetSocketAddress and accept it
fd4fb5d block/ssh: Add ssh_has_filename_options_conflict()

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf'
  BUILD   centos6
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPYRUNNER
  RUN test-quick in centos6
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
ccache-3.1.6-2.el6.x86_64
epel-release-6-8.noarch
gcc-4.4.7-17.el6.x86_64
git-1.7.1-4.el6_7.1.x86_64
glib2-devel-2.28.8-5.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
make-3.81-23.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
tar-1.23-15.el6_8.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel 
glib2-devel SDL-devel pixman-devel epel-release
HOSTNAME=05347c7f0feb
TERM=xterm
MAKEFLAGS= -j16
HISTSIZE=1000
J=16
USER=root
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/var/tmp/qemu-build/install
No C++ compiler available; disabling C++ specific optional code
Install prefix/var/tmp/qemu-build/install
BIOS directory/var/tmp/qemu-build/install/share/qemu
binary directory  /var/tmp/qemu-build/install/bin
library directory /var/tmp/qemu-build/install/lib
module directory  /var/tmp/qemu-build/install/lib/qemu
libexec directory /var/tmp/qemu-build/install/libexec
include directory /var/tmp/qemu-build/install/include
config directory  /var/tmp/qemu-build/install/etc
local state directory   /var/tmp/qemu-build/install/var
Manual directory  /var/tmp/qemu-build/install/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /tmp/qemu-test/src
C compilercc
Host C compiler   cc
C++ compiler  
Objective-C compiler cc
ARFLAGS   rv
CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g 
QEMU_CFLAGS   -I/usr/include/pixman-1-pthread -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include   -fPIE -DPIE -m64 -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels -Wmissing-include-dirs 
-Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self 
-Wignored-qualifiers -Wold-style-declaration -Wold-style-definition 
-Wtype-limits -fstack-protector-all
LDFLAGS   -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g 
make  make
install   install
pythonpython -B
smbd  /usr/sbin/smbd
module supportno
host CPU  x86_64
host big endian   no
target list   x86_64-softmmu aarch64-softmmu
tcg debug enabled no
gprof enabled no
sparse enabledno
strip binariesyes
profiler  no
static build  no
pixmansystem
SDL support   yes (1.2.14)
GTK support   no 
GTK GL supportno
VTE support   no 
TLS priority  NORMAL
GNUTLS supportno
GNUTLS rndno
libgcrypt no
libgcrypt kdf no
nettleno 
nettle kdfno
libtasn1  no
curses supportno
virgl support no
curl support  no
mingw32 support   no
Audio drivers oss
Block whitelist (rw) 
Block whitelist (ro) 
VirtFS supportno
VNC support   yes
VNC SASL support  no
VNC JPEG support  no
VNC PNG support   no
xen support   no
brlapi supportno
bluez  supportno
Documentation no
PIE   yes

[Qemu-block] [v2 4/5] block/ssh: Use InetSocketAddress options

2016-10-15 Thread Ashijeet Acharya 
Drop the use of legacy options in favour of the InetSocketAddress
options.

Signed-off-by: Ashijeet Acharya 
---
 block/ssh.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 6420359..7fec0e1 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -199,6 +199,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 {
 URI *uri = NULL;
 QueryParams *qp;
+char *port_str;
 int i;
 
 uri = uri_parse(filename);
@@ -231,11 +232,10 @@ static int parse_uri(const char *filename, QDict 
*options, Error **errp)
 qdict_put(options, "user", qstring_from_str(uri->user));
 }
 
-qdict_put(options, "host", qstring_from_str(uri->server));
+qdict_put(options, "server.host", qstring_from_str(uri->server));
 
-if (uri->port) {
-qdict_put(options, "port", qint_from_int(uri->port));
-}
+port_str = g_strdup_printf("%d", uri->port ?: 22);
+qdict_put(options, "server.port", qstring_from_str(port_str));
 
 qdict_put(options, "path", qstring_from_str(uri->path));
 
@@ -251,6 +251,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 
 query_params_free(qp);
 uri_free(uri);
+g_free(port_str);
 return 0;
 
  err:
-- 
2.6.2

[Qemu-block] [v2 2/5] block/ssh: Add InetSocketAddress and accept it

2016-10-15 Thread Ashijeet Acharya 
Add InetSocketAddress compatibility to SSH driver.

Add a new option "server" to the SSH block driver which then accepts
a InetSocketAddress.

"host" and "port" are supported as legacy options and are mapped to
their InetSocketAddress representation.

Signed-off-by: Ashijeet Acharya 
---
 block/ssh.c | 83 ++---
 1 file changed, 74 insertions(+), 9 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 75cb7bc..3b18907 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -30,10 +30,14 @@
 #include "block/block_int.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "qemu/cutils.h"
 #include "qemu/sockets.h"
 #include "qemu/uri.h"
+#include "qapi-visit.h"
 #include "qapi/qmp/qint.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
 
 /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
  * this block driver code.
@@ -74,6 +78,8 @@ typedef struct BDRVSSHState {
  */
 LIBSSH2_SFTP_ATTRIBUTES attrs;
 
+InetSocketAddress *inet;
+
 /* Used to warn if 'flush' is not supported. */
 char *hostport;
 bool unsafe_flush_warning;
@@ -263,7 +269,9 @@ static bool ssh_has_filename_options_conflict(QDict 
*options, Error **errp)
 !strcmp(qe->key, "port") ||
 !strcmp(qe->key, "path") ||
 !strcmp(qe->key, "user") ||
-!strcmp(qe->key, "host_key_check"))
+!strcmp(qe->key, "host_key_check") ||
+!strcmp(qe->key, "server") ||
+!strstart(qe->key, "server.", NULL))
 {
 error_setg(errp, "Option '%s' cannot be used with a file name",
qe->key);
@@ -555,13 +563,66 @@ static QemuOptsList ssh_runtime_opts = {
 },
 };
 
+static bool ssh_process_legacy_socket_options(QDict *output_opts,
+  QemuOpts *legacy_opts,
+  Error **errp)
+{
+const char *host = qemu_opt_get(legacy_opts, "host");
+const char *port = qemu_opt_get(legacy_opts, "port");
+
+if (!host && port) {
+error_setg(errp, "port may not be used without host");
+return false;
+} else {
+qdict_put(output_opts, "server.host", qstring_from_str(host));
+qdict_put(output_opts, "server.port",
+  qstring_from_str(port ?: stringify(22)));
+}
+
+return true;
+}
+
+static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options,
+ Error **errp)
+{
+InetSocketAddress *inet = NULL;
+QDict *addr = NULL;
+QObject *crumpled_addr = NULL;
+Visitor *iv = NULL;
+Error *local_error = NULL;
+
+qdict_extract_subqdict(options, , "server.");
+if (!qdict_size(addr)) {
+error_setg(errp, "SSH server address missing");
+goto out;
+}
+
+crumpled_addr = qdict_crumple(addr, true, errp);
+if (!crumpled_addr) {
+goto out;
+}
+
+iv = qobject_input_visitor_new_autocast(crumpled_addr, true, 1, true);
+visit_type_InetSocketAddress(iv, NULL, , _error);
+if (local_error) {
+error_propagate(errp, local_error);
+goto out;
+}
+
+out:
+QDECREF(addr);
+qobject_decref(crumpled_addr);
+visit_free(iv);
+return inet;
+}
+
 static int connect_to_ssh(BDRVSSHState *s, QDict *options,
   int ssh_flags, int creat_mode, Error **errp)
 {
 int r, ret;
 QemuOpts *opts = NULL;
 Error *local_err = NULL;
-const char *host, *user, *path, *host_key_check;
+const char *user, *path, *host_key_check;
 int port;
 
 opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
@@ -572,15 +633,11 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 goto err;
 }
 
-host = qemu_opt_get(opts, "host");
-if (!host) {
+if (!ssh_process_legacy_socket_options(options, opts, errp)) {
 ret = -EINVAL;
-error_setg(errp, "No hostname was specified");
 goto err;
 }
 
-port = qemu_opt_get_number(opts, "port", 22);
-
 path = qemu_opt_get(opts, "path");
 if (!path) {
 ret = -EINVAL;
@@ -603,9 +660,16 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 host_key_check = "yes";
 }
 
+/* Pop the config into our state object, Exit if invalid */
+s->inet = ssh_config(s, options, errp);
+if (!s->inet) {
+goto err;
+}
+
 /* Construct the host:port name for inet_connect. */
 g_free(s->hostport);
-s->hostport = g_strdup_printf("%s:%d", host, port);
+port = atoi(s->inet->port);
+s->hostport = g_strdup_printf("%s:%d", s->inet->host, port);
 
 /* Open the socket and connect. */
 s->sock = inet_connect(s->hostport, errp);
@@ -634,7 +698,8 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 }
 
 /* Check the remote host's

[Qemu-block] [v2 1/5] block/ssh: Add ssh_has_filename_options_conflict()

2016-10-15 Thread Ashijeet Acharya 
We have 5 options plus ("server") option which is added in the next
patch that conflict with specifying a SSH filename. We need to iterate
over all the options to check whether its key has an "server." prefix.

This iteration will help us adding the new option "server" easily.

Signed-off-by: Ashijeet Acharya 
---
 block/ssh.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 5ce12b6..75cb7bc 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -254,15 +254,30 @@ static int parse_uri(const char *filename, QDict 
*options, Error **errp)
 return -EINVAL;
 }
 
+static bool ssh_has_filename_options_conflict(QDict *options, Error **errp)
+{
+const QDictEntry *qe;
+
+for (qe = qdict_first(options); qe; qe = qdict_next(options, qe)) {
+if (!strcmp(qe->key, "host") ||
+!strcmp(qe->key, "port") ||
+!strcmp(qe->key, "path") ||
+!strcmp(qe->key, "user") ||
+!strcmp(qe->key, "host_key_check"))
+{
+error_setg(errp, "Option '%s' cannot be used with a file name",
+   qe->key);
+return true;
+}
+}
+
+return false;
+}
+
 static void ssh_parse_filename(const char *filename, QDict *options,
Error **errp)
 {
-if (qdict_haskey(options, "user") ||
-qdict_haskey(options, "host") ||
-qdict_haskey(options, "port") ||
-qdict_haskey(options, "path") ||
-qdict_haskey(options, "host_key_check")) {
-error_setg(errp, "user, host, port, path, host_key_check cannot be 
used at the same time as a file option");
+if (ssh_has_filename_options_conflict(options, errp)) {
 return;
 }
 
-- 
2.6.2

[Qemu-block] [v2 5/5] qapi: allow blockdev-add for ssh

2016-10-15 Thread Ashijeet Acharya 
Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
support blockdev-add for SSH network protocol driver. Use only 'struct
InetSocketAddress' since SSH only supports connection over TCP.

Signed-off-by: Ashijeet Acharya 
---
 qapi/block-core.json | 24 ++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d797b8..2e8a390 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1716,7 +1716,8 @@
 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
-   'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
+'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
+'vvfat' ] }
 
 ##
 # @BlockdevOptionsFile
@@ -1953,6 +1954,25 @@
 '*vport': 'int',
 '*segment': 'str' } }
 
+##
+# @BlockdevoptionsSsh
+#
+# @server:  host address and port number
+#
+# @path:path to the image on the host
+#
+# @user:user as which to connect
+#
+# @host_key_check   defines how and what to check the host key against
+#
+# Since 2.8
+##
+{ 'struct': 'BlockdevoptionsSsh',
+  'data': { 'server': 'InetSocketAddress',
+'path': 'str',
+'user': 'str',
+'*host_key_check': 'str' } }
+
 
 ##
 # @BlkdebugEvent
@@ -2281,7 +2301,7 @@
 # TODO rbd: Wait for structured options
   'replication':'BlockdevOptionsReplication',
 # TODO sheepdog: Wait for structured options
-# TODO ssh: Should take InetSocketAddress for 'host'?
+  'ssh':'BlockdevoptionsSsh',
   'tftp':   'BlockdevOptionsCurl',
   'vdi':'BlockdevOptionsGenericFormat',
   'vhdx':   'BlockdevOptionsGenericFormat',
-- 
2.6.2

[Qemu-block] [v2 3/5] block/ssh: Use inet_connect_saddr() to establish socket connection

2016-10-15 Thread Ashijeet Acharya 
Make inet_connect_saddr() in util/qemu-socktets.c public and use it
instead of inet_connect() because this directly takes the
InetSocketAddress to establish a socket connection for the SSH
block driver.

Signed-off-by: Ashijeet Acharya 
---
 block/ssh.c| 5 +
 include/qemu/sockets.h | 2 ++
 util/qemu-sockets.c| 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 3b18907..6420359 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -666,13 +666,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 goto err;
 }
 
-/* Construct the host:port name for inet_connect. */
-g_free(s->hostport);
 port = atoi(s->inet->port);
-s->hostport = g_strdup_printf("%s:%d", s->inet->host, port);
 
 /* Open the socket and connect. */
-s->sock = inet_connect(s->hostport, errp);
+s->sock = inet_connect_saddr(s->inet, errp, NULL, NULL);
 if (s->sock < 0) {
 ret = -EIO;
 goto err;
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 9eb2470..5589e68 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -34,6 +34,8 @@ typedef void NonBlockingConnectHandler(int fd, Error *err, 
void *opaque);
 
 InetSocketAddress *inet_parse(const char *str, Error **errp);
 int inet_connect(const char *str, Error **errp);
+int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
+   NonBlockingConnectHandler *callback, void *opaque);
 
 NetworkAddressFamily inet_netfamily(int family);
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 4cef549..3411888 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -412,7 +412,7 @@ static struct addrinfo 
*inet_parse_connect_saddr(InetSocketAddress *saddr,
  * function succeeds, callback will be called when the connection
  * completes, with the file descriptor on success, or -1 on error.
  */
-static int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
+int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
   NonBlockingConnectHandler *callback, void 
*opaque)
 {
 Error *local_err = NULL;
-- 
2.6.2

[Qemu-block] [v2 3/5] block/ssh: Use inet_connect_saddr() to establish socket connection

2016-10-15 Thread Ashijeet Acharya 
Make inet_connect_saddr() in util/qemu-socktets.c public and use it
instead of inet_connect() because this directly takes the
InetSocketAddress to establish a socket connection for the SSH
block driver.

Signed-off-by: Ashijeet Acharya 
---
 block/ssh.c| 5 +
 include/qemu/sockets.h | 2 ++
 util/qemu-sockets.c| 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 3b18907..6420359 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -666,13 +666,10 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 goto err;
 }
 
-/* Construct the host:port name for inet_connect. */
-g_free(s->hostport);
 port = atoi(s->inet->port);
-s->hostport = g_strdup_printf("%s:%d", s->inet->host, port);
 
 /* Open the socket and connect. */
-s->sock = inet_connect(s->hostport, errp);
+s->sock = inet_connect_saddr(s->inet, errp, NULL, NULL);
 if (s->sock < 0) {
 ret = -EIO;
 goto err;
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 9eb2470..5589e68 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -34,6 +34,8 @@ typedef void NonBlockingConnectHandler(int fd, Error *err, 
void *opaque);
 
 InetSocketAddress *inet_parse(const char *str, Error **errp);
 int inet_connect(const char *str, Error **errp);
+int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
+   NonBlockingConnectHandler *callback, void *opaque);
 
 NetworkAddressFamily inet_netfamily(int family);
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 4cef549..3411888 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -412,7 +412,7 @@ static struct addrinfo 
*inet_parse_connect_saddr(InetSocketAddress *saddr,
  * function succeeds, callback will be called when the connection
  * completes, with the file descriptor on success, or -1 on error.
  */
-static int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
+int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
   NonBlockingConnectHandler *callback, void 
*opaque)
 {
 Error *local_err = NULL;
-- 
2.6.2

[Qemu-block] [v2 5/5] qapi: allow blockdev-add for ssh

2016-10-15 Thread Ashijeet Acharya 
Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
support blockdev-add for SSH network protocol driver. Use only 'struct
InetSocketAddress' since SSH only supports connection over TCP.

Signed-off-by: Ashijeet Acharya 
---
 qapi/block-core.json | 24 ++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d797b8..2e8a390 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1716,7 +1716,8 @@
 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
-   'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
+'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
+'vvfat' ] }
 
 ##
 # @BlockdevOptionsFile
@@ -1953,6 +1954,25 @@
 '*vport': 'int',
 '*segment': 'str' } }
 
+##
+# @BlockdevoptionsSsh
+#
+# @server:  host address and port number
+#
+# @path:path to the image on the host
+#
+# @user:user as which to connect
+#
+# @host_key_check   defines how and what to check the host key against
+#
+# Since 2.8
+##
+{ 'struct': 'BlockdevoptionsSsh',
+  'data': { 'server': 'InetSocketAddress',
+'path': 'str',
+'user': 'str',
+'*host_key_check': 'str' } }
+
 
 ##
 # @BlkdebugEvent
@@ -2281,7 +2301,7 @@
 # TODO rbd: Wait for structured options
   'replication':'BlockdevOptionsReplication',
 # TODO sheepdog: Wait for structured options
-# TODO ssh: Should take InetSocketAddress for 'host'?
+  'ssh':'BlockdevoptionsSsh',
   'tftp':   'BlockdevOptionsCurl',
   'vdi':'BlockdevOptionsGenericFormat',
   'vhdx':   'BlockdevOptionsGenericFormat',
-- 
2.6.2

[Qemu-block] [v2 1/5] block/ssh: Add ssh_has_filename_options_conflict()

2016-10-15 Thread Ashijeet Acharya 
We have 5 options plus ("server") option which is added in the next
patch that conflict with specifying a SSH filename. We need to iterate
over all the options to check whether its key has an "server." prefix.

This iteration will help us adding the new option "server" easily.

Signed-off-by: Ashijeet Acharya 
---
 block/ssh.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 5ce12b6..75cb7bc 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -254,15 +254,30 @@ static int parse_uri(const char *filename, QDict 
*options, Error **errp)
 return -EINVAL;
 }
 
+static bool ssh_has_filename_options_conflict(QDict *options, Error **errp)
+{
+const QDictEntry *qe;
+
+for (qe = qdict_first(options); qe; qe = qdict_next(options, qe)) {
+if (!strcmp(qe->key, "host") ||
+!strcmp(qe->key, "port") ||
+!strcmp(qe->key, "path") ||
+!strcmp(qe->key, "user") ||
+!strcmp(qe->key, "host_key_check"))
+{
+error_setg(errp, "Option '%s' cannot be used with a file name",
+   qe->key);
+return true;
+}
+}
+
+return false;
+}
+
 static void ssh_parse_filename(const char *filename, QDict *options,
Error **errp)
 {
-if (qdict_haskey(options, "user") ||
-qdict_haskey(options, "host") ||
-qdict_haskey(options, "port") ||
-qdict_haskey(options, "path") ||
-qdict_haskey(options, "host_key_check")) {
-error_setg(errp, "user, host, port, path, host_key_check cannot be 
used at the same time as a file option");
+if (ssh_has_filename_options_conflict(options, errp)) {
 return;
 }
 
-- 
2.6.2

[Qemu-block] [v2 2/5] block/ssh: Add InetSocketAddress and accept it

2016-10-15 Thread Ashijeet Acharya 
Add InetSocketAddress compatibility to SSH driver.

Add a new option "server" to the SSH block driver which then accepts
a InetSocketAddress.

"host" and "port" are supported as legacy options and are mapped to
their InetSocketAddress representation.

Signed-off-by: Ashijeet Acharya 
---
 block/ssh.c | 83 ++---
 1 file changed, 74 insertions(+), 9 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 75cb7bc..3b18907 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -30,10 +30,14 @@
 #include "block/block_int.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "qemu/cutils.h"
 #include "qemu/sockets.h"
 #include "qemu/uri.h"
+#include "qapi-visit.h"
 #include "qapi/qmp/qint.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
 
 /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
  * this block driver code.
@@ -74,6 +78,8 @@ typedef struct BDRVSSHState {
  */
 LIBSSH2_SFTP_ATTRIBUTES attrs;
 
+InetSocketAddress *inet;
+
 /* Used to warn if 'flush' is not supported. */
 char *hostport;
 bool unsafe_flush_warning;
@@ -263,7 +269,9 @@ static bool ssh_has_filename_options_conflict(QDict 
*options, Error **errp)
 !strcmp(qe->key, "port") ||
 !strcmp(qe->key, "path") ||
 !strcmp(qe->key, "user") ||
-!strcmp(qe->key, "host_key_check"))
+!strcmp(qe->key, "host_key_check") ||
+!strcmp(qe->key, "server") ||
+!strstart(qe->key, "server.", NULL))
 {
 error_setg(errp, "Option '%s' cannot be used with a file name",
qe->key);
@@ -555,13 +563,66 @@ static QemuOptsList ssh_runtime_opts = {
 },
 };
 
+static bool ssh_process_legacy_socket_options(QDict *output_opts,
+  QemuOpts *legacy_opts,
+  Error **errp)
+{
+const char *host = qemu_opt_get(legacy_opts, "host");
+const char *port = qemu_opt_get(legacy_opts, "port");
+
+if (!host && port) {
+error_setg(errp, "port may not be used without host");
+return false;
+} else {
+qdict_put(output_opts, "server.host", qstring_from_str(host));
+qdict_put(output_opts, "server.port",
+  qstring_from_str(port ?: stringify(22)));
+}
+
+return true;
+}
+
+static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options,
+ Error **errp)
+{
+InetSocketAddress *inet = NULL;
+QDict *addr = NULL;
+QObject *crumpled_addr = NULL;
+Visitor *iv = NULL;
+Error *local_error = NULL;
+
+qdict_extract_subqdict(options, , "server.");
+if (!qdict_size(addr)) {
+error_setg(errp, "SSH server address missing");
+goto out;
+}
+
+crumpled_addr = qdict_crumple(addr, true, errp);
+if (!crumpled_addr) {
+goto out;
+}
+
+iv = qobject_input_visitor_new_autocast(crumpled_addr, true, 1, true);
+visit_type_InetSocketAddress(iv, NULL, , _error);
+if (local_error) {
+error_propagate(errp, local_error);
+goto out;
+}
+
+out:
+QDECREF(addr);
+qobject_decref(crumpled_addr);
+visit_free(iv);
+return inet;
+}
+
 static int connect_to_ssh(BDRVSSHState *s, QDict *options,
   int ssh_flags, int creat_mode, Error **errp)
 {
 int r, ret;
 QemuOpts *opts = NULL;
 Error *local_err = NULL;
-const char *host, *user, *path, *host_key_check;
+const char *user, *path, *host_key_check;
 int port;
 
 opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
@@ -572,15 +633,11 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 goto err;
 }
 
-host = qemu_opt_get(opts, "host");
-if (!host) {
+if (!ssh_process_legacy_socket_options(options, opts, errp)) {
 ret = -EINVAL;
-error_setg(errp, "No hostname was specified");
 goto err;
 }
 
-port = qemu_opt_get_number(opts, "port", 22);
-
 path = qemu_opt_get(opts, "path");
 if (!path) {
 ret = -EINVAL;
@@ -603,9 +660,16 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 host_key_check = "yes";
 }
 
+/* Pop the config into our state object, Exit if invalid */
+s->inet = ssh_config(s, options, errp);
+if (!s->inet) {
+goto err;
+}
+
 /* Construct the host:port name for inet_connect. */
 g_free(s->hostport);
-s->hostport = g_strdup_printf("%s:%d", host, port);
+port = atoi(s->inet->port);
+s->hostport = g_strdup_printf("%s:%d", s->inet->host, port);
 
 /* Open the socket and connect. */
 s->sock = inet_connect(s->hostport, errp);
@@ -634,7 +698,8 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 }
 
 /* Check the remote host's

[Qemu-block] [v2 4/5] block/ssh: Use InetSocketAddress options

2016-10-15 Thread Ashijeet Acharya 
Drop the use of legacy options in favour of the InetSocketAddress
options.

Signed-off-by: Ashijeet Acharya 
---
 block/ssh.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 6420359..7fec0e1 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -199,6 +199,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 {
 URI *uri = NULL;
 QueryParams *qp;
+char *port_str;
 int i;
 
 uri = uri_parse(filename);
@@ -231,11 +232,10 @@ static int parse_uri(const char *filename, QDict 
*options, Error **errp)
 qdict_put(options, "user", qstring_from_str(uri->user));
 }
 
-qdict_put(options, "host", qstring_from_str(uri->server));
+qdict_put(options, "server.host", qstring_from_str(uri->server));
 
-if (uri->port) {
-qdict_put(options, "port", qint_from_int(uri->port));
-}
+port_str = g_strdup_printf("%d", uri->port ?: 22);
+qdict_put(options, "server.port", qstring_from_str(port_str));
 
 qdict_put(options, "path", qstring_from_str(uri->path));
 
@@ -251,6 +251,7 @@ static int parse_uri(const char *filename, QDict *options, 
Error **errp)
 
 query_params_free(qp);
 uri_free(uri);
+g_free(port_str);
 return 0;
 
  err:
-- 
2.6.2

[Qemu-block] [v2 0/5] Allow blockdev-add for SSH

2016-10-15 Thread Ashijeet Acharya 
Previously posted series patches:
v1: http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg02137.html

This series adds blockdev-add support for SSH block driver.

This patch series has an additional patch 'Patch 3'.

Patch 1 prepares the code for the addition of a new option prefix,
which is "server.". This is accomplished by adding a
ssh_has_filename_options_conflict() function which helps to iterate
over the various options and check for conflict.

Patch 2 first adds InetSocketAddress compatibility to SSH block driver
and then makes it accept a InetSocketAddress under the "server" option.
The old options "host" and "port" are supported as legacy options and
then translated to the respective InetSocketAddress representation.

Patch 3 makes use of inet_connect_saddr() instead of inet_connect()
to establish socket connection.

Patch 4 drops the usage of "host" and "port" outside of
ssh_has_filename_options_conflict() and
ssh_process_legacy_socket_options() functions in order to make them
legacy options completely.

Patch 5 helps to allow blockdev-add support for the SSH block driver
by making the SSH option available.


*** This series depends on the following patch: ***
"qdict: implement a qdict_crumple method for un-flattening a dict"
from Daniel's "QAPI/QOM work for non-scalar object properties"
series.

Changes in v2:
- Use strstart() instead of strcmp() (Kevin)
- Use qobject_input_visitor_new_autocast() instead of
  qmp_input_visitor_new() and change header files accordingly (Kevin)
- Use inet_connect_saddr() instead of inet_connect() (Kevin)
- Drop the contruction of : string (Kevin)
- Fix the bug in ssh_process_legacy_socket_options()

Ashijeet Acharya (5):
  block/ssh: Add ssh_has_filename_options_conflict()
  block/ssh: Add InetSocketAddress and accept it
  block/ssh: Use inet_connect_saddr() to establish socket connection
  block/ssh: Use InetSocketAddress options
  qapi: allow blockdev-add for ssh

 block/ssh.c| 120 -
 include/qemu/sockets.h |   2 +
 qapi/block-core.json   |  24 +-
 util/qemu-sockets.c|   2 +-
 4 files changed, 124 insertions(+), 24 deletions(-)

-- 
2.6.2

[Qemu-block] [PATCH v2] rbd: make the code more readable

2016-10-15 Thread Xiubo Li 
Make it a bit clearer and more readable.

Signed-off-by: Xiubo Li 
CC: John Snow 
---

V2:
- Advice from John Snow. Thanks.


 block/rbd.c | 25 -
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 0a5840d..d0d4b39 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -366,45 +366,44 @@ static int qemu_rbd_create(const char *filename, QemuOpts 
*opts, Error **errp)
 rados_conf_read_file(cluster, NULL);
 } else if (conf[0] != '\0' &&
qemu_rbd_set_conf(cluster, conf, true, _err) < 0) {
-rados_shutdown(cluster);
 error_propagate(errp, local_err);
-return -EIO;
+ret = -EIO;
+goto shutdown;
 }
 
 if (conf[0] != '\0' &&
 qemu_rbd_set_conf(cluster, conf, false, _err) < 0) {
-rados_shutdown(cluster);
 error_propagate(errp, local_err);
-return -EIO;
+ret = -EIO;
+goto shutdown;
 }
 
 if (qemu_rbd_set_auth(cluster, secretid, errp) < 0) {
-rados_shutdown(cluster);
-return -EIO;
+ret = -EIO;
+goto shutdown;
 }
 
 ret = rados_connect(cluster);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "error connecting");
-rados_shutdown(cluster);
-return ret;
+goto shutdown;
 }
 
 ret = rados_ioctx_create(cluster, pool, _ctx);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "error opening pool %s", pool);
-rados_shutdown(cluster);
-return ret;
+goto shutdown;
 }
 
 ret = rbd_create(io_ctx, name, bytes, _order);
-rados_ioctx_destroy(io_ctx);
-rados_shutdown(cluster);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "error rbd create");
-return ret;
 }
 
+rados_ioctx_destroy(io_ctx);
+
+shutdown:
+rados_shutdown(cluster);
 return ret;
 }
 
-- 
1.8.3.1

Re: [Qemu-block] [PATCH v11 02/19] block: Pause all jobs during bdrv_reopen_multiple()

2016-10-15 Thread Paolo Bonzini 


On 14/10/2016 15:08, Alberto Garcia wrote:
> When a BlockDriverState is about to be reopened it can trigger certain
> operations that need to write to disk. During this process a different
> block job can be woken up. If that block job completes and also needs
> to call bdrv_reopen() it can happen that it needs to do it on the same
> BlockDriverState that is still in the process of being reopened.
> 
> This can have fatal consequences, like in this example:
> 
>   1) Block job A starts and sleeps after a while.
>   2) Block job B starts and tries to reopen node1 (a qcow2 file).
>   3) Reopening node1 means flushing and replacing its qcow2 cache.
>   4) While the qcow2 cache is being flushed, job A wakes up.
>   5) Job A completes and reopens node1, replacing its cache.
>   6) Job B resumes, but the cache that was being flushed no longer
>  exists.
> 
> This patch splits the bdrv_drain_all() call to keep all block jobs
> paused during bdrv_reopen_multiple(), so that step 4 can never happen
> and the operation is safe.
> 
> Note that this scenario can only happen if both bdrv_reopen() calls
> are made by block jobs on the same backing chain. Otherwise there's no
> chance that the same BlockDriverState appears in both reopen queues.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 7f3e7bc..adbecd0 100644
> --- a/block.c
> +++ b/block.c
> @@ -2090,7 +2090,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
> Error **errp)
>  
>  assert(bs_queue != NULL);
>  
> -bdrv_drain_all();
> +bdrv_drain_all_begin();
>  
>  QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
>  if (bdrv_reopen_prepare(_entry->state, bs_queue, _err)) {
> @@ -2120,6 +2120,9 @@ cleanup:
>  g_free(bs_entry);
>  }
>  g_free(bs_queue);
> +
> +bdrv_drain_all_end();
> +
>  return ret;
>  }
>  
> 

Reviewed-by: Paolo Bonzini

Re: [Qemu-block] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

2016-10-15 Thread Paolo Bonzini 


On 14/10/2016 15:08, Alberto Garcia wrote:
> bdrv_drain_all() doesn't allow the caller to do anything after all
> pending requests have been completed but before block jobs are
> resumed.
> 
> This patch splits bdrv_drain_all() into _begin() and _end() for that
> purpose. It also adds aio_{disable,enable}_external() calls to disable
> external clients in the meantime.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/io.c| 24 +---
>  include/block/block.h |  2 ++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index b136c89..9418f8b 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -275,8 +275,11 @@ void bdrv_drain(BlockDriverState *bs)
>   *
>   * This function does not flush data to disk, use bdrv_flush_all() for that
>   * after calling this function.
> + *
> + * This pauses all block jobs and disables external clients. It must
> + * be paired with bdrv_drain_all_end().
>   */
> -void bdrv_drain_all(void)
> +void bdrv_drain_all_begin(void)
>  {
>  /* Always run first iteration so any pending completion BHs run */
>  bool busy = true;
> @@ -300,6 +303,7 @@ void bdrv_drain_all(void)
>  bdrv_parent_drained_begin(bs);
>  bdrv_io_unplugged_begin(bs);
>  bdrv_drain_recurse(bs);
> +aio_disable_external(aio_context);
>  aio_context_release(aio_context);
>  
>  if (!g_slist_find(aio_ctxs, aio_context)) {
> @@ -333,17 +337,25 @@ void bdrv_drain_all(void)
>  }
>  }
>  
> +g_slist_free(aio_ctxs);
> +}
> +
> +void bdrv_drain_all_end(void)
> +{
> +BlockDriverState *bs;
> +BdrvNextIterator it;
> +BlockJob *job = NULL;
> +
>  for (bs = bdrv_first(); bs; bs = bdrv_next()) {
>  AioContext *aio_context = bdrv_get_aio_context(bs);
>  
>  aio_context_acquire(aio_context);
> +aio_enable_external(aio_context);
>  bdrv_io_unplugged_end(bs);
>  bdrv_parent_drained_end(bs);
>  aio_context_release(aio_context);
>  }
> -g_slist_free(aio_ctxs);
>  
> -job = NULL;
>  while ((job = block_job_next(job))) {
>  AioContext *aio_context = blk_get_aio_context(job->blk);
>  
> @@ -353,6 +365,12 @@ void bdrv_drain_all(void)
>  }
>  }
>  
> +void bdrv_drain_all(void)
> +{
> +bdrv_drain_all_begin();
> +bdrv_drain_all_end();
> +}
> +
>  /**
>   * Remove an active request from the tracked requests list
>   *
> diff --git a/include/block/block.h b/include/block/block.h
> index 107c603..301d713 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -338,6 +338,8 @@ int bdrv_flush_all(void);
>  void bdrv_close_all(void);
>  void bdrv_drain(BlockDriverState *bs);
>  void coroutine_fn bdrv_co_drain(BlockDriverState *bs);
> +void bdrv_drain_all_begin(void);
> +void bdrv_drain_all_end(void);
>  void bdrv_drain_all(void);
>  
>  int bdrv_pdiscard(BlockDriverState *bs, int64_t offset, int count);
> 

Reviewed-by: Paolo Bonzini