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

[Ashijeet Acharya]

On Mon, Oct 17, 2016 at 9:29 PM, Eric Blake  wrote:
> On 10/17/2016 10:44 AM, Ashijeet Acharya wrote:
>
>>
>> I think, its better to keep using atoi() and check if it returns a '0'
>
> Please not atoi(), as it lacks sane error checking. It cannot tell the
> difference between '1' and '1garbage'.  It's obvious that you want to
> treat both '0' and 'name' as an error, but that is not the only error
> you want to flag, thus atoi() is insufficient to flag all the errors you
> want.
>
Okay, will using qemu_strtol() be any good as I think it has better
error handling support? Otherwise I will resort to passing -ve value
as Kevin suggested earlier.

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

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

[Eric Blake]

On 10/17/2016 10:44 AM, Ashijeet Acharya wrote:

> 
> I think, its better to keep using atoi() and check if it returns a '0'

Please not atoi(), as it lacks sane error checking. It cannot tell the
difference between '1' and '1garbage'.  It's obvious that you want to
treat both '0' and 'name' as an error, but that is not the only error
you want to flag, thus atoi() is insufficient to flag all the errors you
want.


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



signature.asc
Description: OpenPGP digital signature

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

[Ashijeet Acharya]

On Mon, Oct 17, 2016 at 9:23 PM, Kevin Wolf  wrote:
> Am 17.10.2016 um 17:44 hat Ashijeet Acharya geschrieben:
>> On Mon, Oct 17, 2016 at 6:27 PM, Kevin Wolf  wrote:
>> > Am 17.10.2016 um 14:33 hat Ashijeet Acharya geschrieben:
>> >> On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz  wrote:
>> >> > 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(-)
>> >> >>
>> >> >>
>> >> >>  /* 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 key against known_hosts. */
>> >> >> -ret = check_host_key(s, host, port, host_key_check, errp);
>> >> >> +ret = check_host_key(s, s->inet->host, port, host_key_check,
>> >> >
>> >> > But then you're still using the port here... And I can't come up with a
>> >> > way (not even a bad one) to get the numeric port. Maybe interpret the
>> >> > addrinfo in inet_connect_saddr()? But getting that information out would
>> >> > be ugly, if even possible...
>> >> >
>> >> > So maybe the best is to keep it this way and put a FIXME above the
>> >> > atoi() call. :-/
>> >>
>> >> Kevin, I believe (after talking with Max) that regarding the atoi()
>> >> issue, I can't use any string to integer function since it won't
>> >> succeed for cases like port = 'ssh' and putting a FIXME over it seems
>> >> to be the only option. But Max did warn me, though, to get everybody's
>> >> opinion before I do so. So I am awaiting your response on this one.
>> >> Much better will be if you have a workaround solution in mind!! :-)
>> >
>> > The integer port is only needed for libssh2_knownhost_checkp(). One
>> > option could be to consider passing -1 instead:
>> >
>> > port is the port number used by the host (or a negative number to
>> > check the generic host). If the port number is given, libssh2 will
>> > check the key for the specific host + port number combination in
>> > addition to the plain host name only check.
>> >
>> > In 99% of the cases, this shouldn't make any difference.
>>
>> I think, its better to keep using atoi() and check if it returns a '0'
>>  value and display the error to the user to give the input as numeric.
>> This is possible since this will not clash with the possibility that
>> user gives the port input as port = '0' for no such port number exists
>> as far as I know. Will this work?
>
> It's fair enough. We will have a little inconsistency between ssh and
> other users of SocketAddress, but the driver never supported service
> names, so it isn't a regression either.

Great, I will do this change and post the patches shortly.

Ashijeet
>
> Kevin

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

[Kevin Wolf]

Am 17.10.2016 um 17:44 hat Ashijeet Acharya geschrieben:
> On Mon, Oct 17, 2016 at 6:27 PM, Kevin Wolf  wrote:
> > Am 17.10.2016 um 14:33 hat Ashijeet Acharya geschrieben:
> >> On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz  wrote:
> >> > 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(-)
> >> >>
> >> >>
> >> >>  /* 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 key against known_hosts. */
> >> >> -ret = check_host_key(s, host, port, host_key_check, errp);
> >> >> +ret = check_host_key(s, s->inet->host, port, host_key_check,
> >> >
> >> > But then you're still using the port here... And I can't come up with a
> >> > way (not even a bad one) to get the numeric port. Maybe interpret the
> >> > addrinfo in inet_connect_saddr()? But getting that information out would
> >> > be ugly, if even possible...
> >> >
> >> > So maybe the best is to keep it this way and put a FIXME above the
> >> > atoi() call. :-/
> >>
> >> Kevin, I believe (after talking with Max) that regarding the atoi()
> >> issue, I can't use any string to integer function since it won't
> >> succeed for cases like port = 'ssh' and putting a FIXME over it seems
> >> to be the only option. But Max did warn me, though, to get everybody's
> >> opinion before I do so. So I am awaiting your response on this one.
> >> Much better will be if you have a workaround solution in mind!! :-)
> >
> > The integer port is only needed for libssh2_knownhost_checkp(). One
> > option could be to consider passing -1 instead:
> >
> > port is the port number used by the host (or a negative number to
> > check the generic host). If the port number is given, libssh2 will
> > check the key for the specific host + port number combination in
> > addition to the plain host name only check.
> >
> > In 99% of the cases, this shouldn't make any difference.
> 
> I think, its better to keep using atoi() and check if it returns a '0'
>  value and display the error to the user to give the input as numeric.
> This is possible since this will not clash with the possibility that
> user gives the port input as port = '0' for no such port number exists
> as far as I know. Will this work?

It's fair enough. We will have a little inconsistency between ssh and
other users of SocketAddress, but the driver never supported service
names, so it isn't a regression either.

Kevin

> Ashijeet
> > Alternatively it could be possible to use getservbyname() to get the
> > port number from the name, but maybe that's a bit too much for a feature
> > that most people don't even know of.
> >
> > I'm also not completely opposed to simply requiring a numeric argument
> > for SSH. There is no real use to support service names here other than
> > being consistent with other places in qemu.
> >
> > Kevin

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

[Ashijeet Acharya]

On Mon, Oct 17, 2016 at 6:27 PM, Kevin Wolf  wrote:
> Am 17.10.2016 um 14:33 hat Ashijeet Acharya geschrieben:
>> On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz  wrote:
>> > 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(-)
>> >>
>> >>
>> >>  /* 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 key against known_hosts. */
>> >> -ret = check_host_key(s, host, port, host_key_check, errp);
>> >> +ret = check_host_key(s, s->inet->host, port, host_key_check,
>> >
>> > But then you're still using the port here... And I can't come up with a
>> > way (not even a bad one) to get the numeric port. Maybe interpret the
>> > addrinfo in inet_connect_saddr()? But getting that information out would
>> > be ugly, if even possible...
>> >
>> > So maybe the best is to keep it this way and put a FIXME above the
>> > atoi() call. :-/
>>
>> Kevin, I believe (after talking with Max) that regarding the atoi()
>> issue, I can't use any string to integer function since it won't
>> succeed for cases like port = 'ssh' and putting a FIXME over it seems
>> to be the only option. But Max did warn me, though, to get everybody's
>> opinion before I do so. So I am awaiting your response on this one.
>> Much better will be if you have a workaround solution in mind!! :-)
>
> The integer port is only needed for libssh2_knownhost_checkp(). One
> option could be to consider passing -1 instead:
>
> port is the port number used by the host (or a negative number to
> check the generic host). If the port number is given, libssh2 will
> check the key for the specific host + port number combination in
> addition to the plain host name only check.
>
> In 99% of the cases, this shouldn't make any difference.

I think, its better to keep using atoi() and check if it returns a '0'
 value and display the error to the user to give the input as numeric.
This is possible since this will not clash with the possibility that
user gives the port input as port = '0' for no such port number exists
as far as I know. Will this work?

Ashijeet
> Alternatively it could be possible to use getservbyname() to get the
> port number from the name, but maybe that's a bit too much for a feature
> that most people don't even know of.
>
> I'm also not completely opposed to simply requiring a numeric argument
> for SSH. There is no real use to support service names here other than
> being consistent with other places in qemu.
>
> Kevin

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

[Kevin Wolf]

Am 17.10.2016 um 14:33 hat Ashijeet Acharya geschrieben:
> On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz  wrote:
> > 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(-)
> >>
> >>
> >>  /* 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 key against known_hosts. */
> >> -ret = check_host_key(s, host, port, host_key_check, errp);
> >> +ret = check_host_key(s, s->inet->host, port, host_key_check,
> >
> > But then you're still using the port here... And I can't come up with a
> > way (not even a bad one) to get the numeric port. Maybe interpret the
> > addrinfo in inet_connect_saddr()? But getting that information out would
> > be ugly, if even possible...
> >
> > So maybe the best is to keep it this way and put a FIXME above the
> > atoi() call. :-/
> 
> Kevin, I believe (after talking with Max) that regarding the atoi()
> issue, I can't use any string to integer function since it won't
> succeed for cases like port = 'ssh' and putting a FIXME over it seems
> to be the only option. But Max did warn me, though, to get everybody's
> opinion before I do so. So I am awaiting your response on this one.
> Much better will be if you have a workaround solution in mind!! :-)

The integer port is only needed for libssh2_knownhost_checkp(). One
option could be to consider passing -1 instead:

port is the port number used by the host (or a negative number to
check the generic host). If the port number is given, libssh2 will
check the key for the specific host + port number combination in
addition to the plain host name only check.

In 99% of the cases, this shouldn't make any difference.

Alternatively it could be possible to use getservbyname() to get the
port number from the name, but maybe that's a bit too much for a feature
that most people don't even know of.

I'm also not completely opposed to simply requiring a numeric argument
for SSH. There is no real use to support service names here other than
being consistent with other places in qemu.

Kevin

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

[Ashijeet Acharya]

On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz  wrote:
> 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(-)
>>
>>
>>  /* 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 key against known_hosts. */
>> -ret = check_host_key(s, host, port, host_key_check, errp);
>> +ret = check_host_key(s, s->inet->host, port, host_key_check,
>
> But then you're still using the port here... And I can't come up with a
> way (not even a bad one) to get the numeric port. Maybe interpret the
> addrinfo in inet_connect_saddr()? But getting that information out would
> be ugly, if even possible...
>
> So maybe the best is to keep it this way and put a FIXME above the
> atoi() call. :-/

Kevin, I believe (after talking with Max) that regarding the atoi()
issue, I can't use any string to integer function since it won't
succeed for cases like port = 'ssh' and putting a FIXME over it seems
to be the only option. But Max did warn me, though, to get everybody's
opinion before I do so. So I am awaiting your response on this one.
Much better will be if you have a workaround solution in mind!! :-)

Ashijeet
>
> Max

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

[Kevin Wolf]

Am 16.10.2016 um 00:30 hat Max Reitz geschrieben:
> > +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).
> [...]
> In contrast, if you do not use the autocast version, blockdev-add will
> work just fine, but you can no longer specify non-string values from the
> command line.

Ah, right, I missed that. :-/

> I don't think this is your problem, though. There should be a way for
> the command line options to be converted to the correct types while we
> continue to use strict type-checking for blockdev-add.
> 
> Therefore, I think you'll have to sacrifice one or the other here. All
> of the non-string options are optional, so it won't be too bad in any case.

If we have to sacrifice one, then yes, blockdev-add is the one that must
work. The new -blockdev command line option will then automatically
work, too, so at least there will be a way to create such nodes.

The usual way to get around the type conflicts is going through a
QemuOpts. So maybe qemu_opts_from_dict() with a QemuOptionsList that
accepts anythign, and then qobject_input_visitor_new_opts() could be a
workaround to keep -drive working at the same time. It's kind of ugly,
though.

Kevin


pgpc0nEk2dX3A.pgp
Description: PGP signature

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

[Ashijeet Acharya]

>>>  /* Check the remote host's key against known_hosts. */
>>> -ret = check_host_key(s, host, port, host_key_check, errp);
>>> +ret = check_host_key(s, s->inet->host, port, host_key_check,
>>
>> But then you're still using the port here... And I can't come up with a
>> way (not even a bad one) to get the numeric port. Maybe interpret the
>> addrinfo in inet_connect_saddr()? But getting that information out would
>> be ugly, if even possible...
>>
>
> Will using strtol() do any good?

Better to use qemu_strtol() from cutils.c ?

Ashijeet

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

[Ashijeet Acharya]

On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz  wrote:
> 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.

Yes, I have dropped this.

>> +!strstart(qe->key, "server.", NULL))
>
> It should be just "strstart", not "!strstart", because the function
> returns 1 if the prefix matches.

Fixed.

>
>>  {
>>  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.

Yes, I have put this in a different 'if' like:

if (host) {
   qdict_put();
   ...
}

>
>> +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 

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

[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