[PATCH] nbd-client: support newstyle protocol, -b, -d

[Denys Vlasenko]

Apologies for the delay.


On Tue, Oct 9, 2018 at 1:42 AM Elvira Khabirova
<lineprinter at altlinux.org> wrote:
> Recognize the "newstyle" protocol and switch to it automatically.
> Add options for setting blocksize (-b) and for disconnecting
> a nbd device (-d).
>
> Signed-off-by: Elvira Khabirova <lineprinter at altlinux.org>
> ---
>  networking/nbd-client.c | 168 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 140 insertions(+), 28 deletions(-)
>
> diff --git a/networking/nbd-client.c b/networking/nbd-client.c
> index bedb01a1c..9e718ff93 100644
> --- a/networking/nbd-client.c
> +++ b/networking/nbd-client.c
> @@ -27,17 +27,17 @@
>  #define NBD_SET_SIZE_BLOCKS   _IO(0xab, 7)
>  #define NBD_DISCONNECT        _IO(0xab, 8)
>  #define NBD_SET_TIMEOUT       _IO(0xab, 9)
> +#define NBD_SET_FLAGS         _IO(0xab, 10)
>
>  //usage:#define nbdclient_trivial_usage
> -//usage:       "HOST PORT BLOCKDEV"
> +//usage:       "{ [-b BLKSIZE] [-N name] HOST PORT | -d } BLOCKDEV"
>  //usage:#define nbdclient_full_usage "\n\n"
>  //usage:       "Connect to HOST and provide a network block device on BLOCKDEV"
>
>  //TODO: more compat with nbd-client version 2.9.13 -
>  //Usage: nbd-client [bs=blocksize] [timeout=sec] host port nbd_device [-swap] [-persist] [-nofork]
> -//Or   : nbd-client -d nbd_device
>  //Or   : nbd-client -c nbd_device
> -//Default value for blocksize is 1024 (recommended for ethernet)
> +//Default value for blocksize is 4096
>  //Allowed values for blocksize are 512,1024,2048,4096
>  //Note, that kernel 2.4.2 and older ones do not work correctly with
>  //blocksizes other than 1024 without patches
> @@ -45,37 +45,90 @@
>  int nbdclient_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
>  int nbdclient_main(int argc UNUSED_PARAM, char **argv)
>  {
> -       unsigned long timeout = 0;
>  #if BB_MMU
>         int nofork = 0;
>  #endif
> -       char *host, *port, *device;
> +       char *host, *port, *device, *name;
> +       unsigned blksize, size_blocks;
> +       unsigned timeout;
> +       unsigned opt;
> +
> +       uint16_t handshake_flags;
> +       uint32_t client_flags = 0;
> +
>         struct nbd_header_t {
>                 uint64_t magic1; // "NBDMAGIC"
> -               uint64_t magic2; // 0x420281861253 big endian
> +               uint64_t magic2; // old style: 0x420281861253 big endian
> +                                // new style: 0x49484156454F5054 (IHAVEOPT)
> +       } nbd_header;
> +
> +       struct old_nbd_header_t {
>                 uint64_t devsize;
>                 uint32_t flags;
>                 char data[124];
> -       } nbd_header;
> +       } old_nbd_header;
>
> -       BUILD_BUG_ON(offsetof(struct nbd_header_t, data) != 8+8+8+4);
> +       struct new_nbd_header_t {
> +               uint64_t devsize;
> +               uint16_t transmission_flags;
> +               char data[124];
> +       } new_nbd_header;
>
> -       // Parse command line stuff (just a stub now)
> -       if (!argv[1] || !argv[2] || !argv[3] || argv[4])
> -               bb_show_usage();
> +       struct nbd_opt_t {
> +               uint64_t magic;
> +               uint32_t opt;
> +               uint32_t len;
> +       } nbd_opts;
> +
> +       BUILD_BUG_ON(offsetof(struct old_nbd_header_t, data) != 8+4);
> +       BUILD_BUG_ON(offsetof(struct new_nbd_header_t, data) != 8+2);
>
>  #if !BB_MMU
>         bb_daemonize_or_rexec(DAEMON_CLOSE_EXTRA_FDS, argv);
>  #endif
>
> -       host = argv[1];
> -       port = argv[2];
> -       device = argv[3];
> +       // Parse args
> +       opt = getopt32(argv, "b:+t:+N:d", &blksize, &timeout, &name);

-t is not documented in --help.
Many man pages list only long options, a-la:

SYNOPSIS
    nbd-client host [ port ] nbd-device [ -connections num ] [ -sdp ] [ -swap ]
    [ -persist ] [ -nofork ] [ -nonetlink ] [ -systemd-mark ] [
-block-size block size ]
    [ -timeout seconds ] [ -name name ] [ -certfile certfile ] [
-keyfile keyfile ]
    [ -cacertfile cacertfile ] [ -tlshostname hostname ]

    nbd-client nbd-device
    nbd-client -d nbd-device
    nbd-client -c nbd-device
    nbd-client -l host [ port ]
    nbd-client [ -netlink ] -l host

Because of this, I assume a lot of scripts would use long opts :/
We probably much better if we accept long opts.

> +       if (!(opt & 0x1)) {
> +               blksize = 4096;
> +       }
> +
> +       if (!(opt & 0x2)) {
> +               timeout = 0;
> +       }
> +
> +       if (!(opt & 0x4)) {
> +               name = NULL;
> +       }

Assign the values unconditionally before option parsing. Less generated code.

> +                               nbd_opts.len = htonl(strlen(name));
> +                               xwrite(sock, &nbd_opts,
> +                                      sizeof(nbd_opts.magic) +
> +                                      sizeof(nbd_opts.opt) +
> +                                      sizeof(nbd_opts.len));
> +                               xwrite(sock, name, strlen(name));

two times strlen()...

I applied a modified version of the patch,
please let me know whether it works.
Thanks!