Re: [Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it

[Ashijeet Acharya]

On Tue, Oct 11, 2016 at 1:07 PM, 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 | 87 
> ++---
>  1 file changed, 78 insertions(+), 9 deletions(-)
>
> diff --git a/block/ssh.c b/block/ssh.c
> index 75cb7bc..702871a 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -32,8 +32,11 @@
>  #include "qemu/error-report.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/qmp-input-visitor.h"
> +#include "qapi/qmp-output-visitor.h"
>
>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>   * this block driver code.
> @@ -74,6 +77,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 +268,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") ||
> +!strncmp(qe->key, "server.", 7))
>  {
>  error_setg(errp, "Option '%s' cannot be used with a file name",
> qe->key);
> @@ -555,13 +562,71 @@ 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;
> +}
> +
> +if (!host) {
> +error_setg(errp, "No hostname was specified");
> +return false;

I was debugging this part with gdb while making the changes for v2,
and I hit something very strange.
The code always gives the error of "No hostname was specified". To be
more clear, I reverted back to original driver state and the problem
did not seem to appear for the same qemu command line and I can't find
the bug.

Command I used:

$ ./bin/qemu-system-x86_64 -m 512 -drive
file=ssh://ashijeet@127.0.0.1/home/ashijeet/qemu_build/test.qcow2,
if=virtio

Is there something wrong with the command line?

Ashijeet

Re: [Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it

[Ashijeet Acharya]

On Wed, Oct 12, 2016 at 9:21 PM, Kevin Wolf  wrote:
> Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
>> 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 | 87 
>> ++---
>>  1 file changed, 78 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/ssh.c b/block/ssh.c
>> index 75cb7bc..702871a 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -32,8 +32,11 @@
>>  #include "qemu/error-report.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/qmp-input-visitor.h"
>> +#include "qapi/qmp-output-visitor.h"
>>
>>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>>   * this block driver code.
>> @@ -74,6 +77,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 +268,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") ||
>> +!strncmp(qe->key, "server.", 7))
>
> There is strstart() from cutils.c which looks a bit nicer (you don't
> have to specify the string length then).

Okay, I will use that.

>
>>  {
>>  error_setg(errp, "Option '%s' cannot be used with a file name",
>> qe->key);
>> @@ -555,13 +562,71 @@ 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;
>> +}
>
> This check is rather pointless if !host causes an error anyway.

Hmm... I will remove it.

>
>> +if (!host) {
>> +error_setg(errp, "No hostname was specified");
>> +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 = qmp_input_visitor_new(crumpled_addr, true);
>
> I believe you need qobject_input_visitor_new_autocast() here.
>
> Do integer properties like port work for you without it?

In InetSocketAddress 'port' is of the type 'char*' but now I think
using qobject_input_visitor_new_autocast() will be right since there
are other fields as well.

>
>> +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 +637,11 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
>> *options,
>>  goto err;
>>  }
>>
>> -host = qemu_opt_get(opts, "host");
>> -if (!host) {
>> +if 

Re: [Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it

[Kevin Wolf]

Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
> 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 | 87 
> ++---
>  1 file changed, 78 insertions(+), 9 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 75cb7bc..702871a 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -32,8 +32,11 @@
>  #include "qemu/error-report.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/qmp-input-visitor.h"
> +#include "qapi/qmp-output-visitor.h"
>  
>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>   * this block driver code.
> @@ -74,6 +77,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 +268,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") ||
> +!strncmp(qe->key, "server.", 7))

There is strstart() from cutils.c which looks a bit nicer (you don't
have to specify the string length then).

>  {
>  error_setg(errp, "Option '%s' cannot be used with a file name",
> qe->key);
> @@ -555,13 +562,71 @@ 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;
> +}

This check is rather pointless if !host causes an error anyway.

> +if (!host) {
> +error_setg(errp, "No hostname was specified");
> +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 = qmp_input_visitor_new(crumpled_addr, true);

I believe you need qobject_input_visitor_new_autocast() here.

Do integer properties like port work for you without it?

> +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 +637,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 +664,16 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
> *options,
>  

[Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it

[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 | 87 ++---
 1 file changed, 78 insertions(+), 9 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 75cb7bc..702871a 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -32,8 +32,11 @@
 #include "qemu/error-report.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/qmp-input-visitor.h"
+#include "qapi/qmp-output-visitor.h"
 
 /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
  * this block driver code.
@@ -74,6 +77,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 +268,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") ||
+!strncmp(qe->key, "server.", 7))
 {
 error_setg(errp, "Option '%s' cannot be used with a file name",
qe->key);
@@ -555,13 +562,71 @@ 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;
+}
+
+if (!host) {
+error_setg(errp, "No hostname was specified");
+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 = qmp_input_visitor_new(crumpled_addr, 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 +637,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 +664,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 +702,8 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 }
 
 /* Check the remote host's key against