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

[Ashijeet Acharya]

On Thu, Oct 13, 2016 at 5:12 PM, Kevin Wolf <address@hidden> 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 <address@hidden>
>
> 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.
>
>>  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()?
>
>>          {
>>              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");

I am working on a similar task for the SSH block driver, and in my
'ssh_process_legacy_socket_options' I get the @host and @port fields
still pointing to NULL even after qemu_opt_get(). To clarify things a
bit more I tried to debug the same on your patch using gdb and faced
the same issue. So in your patch, ultimately the control flows
directly to the last statement i.e. 'return true;' and none of the
'if' conditions get satisfied. Reverting the patches and using the
same command-line seems to overcome the issue. Command line I used:

        $ ./bin/qemu-system-x86_64 -cdrom nbd://localhost/precise-5.7.1.iso

I can be wrong so I advise debugging yourself once and correcting me
if I am wrong.

Link to my patch:

        https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg02139.html

Thanks
Ashijeet

> Kevin