On Fri, Nov 13, 2020 at 12:52:32 -0600, Ryan Gahagan wrote:
> Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16
> Added in support for the following parameters in attach-disk:
> --source-protocol
> --source-host-name
> --source-host-socket
> --source-host-transport
>
> Added documentation to virsh.rst specifying usage.
>
> Signed-off-by: Ryan Gahagan <rgahagan@cs.utexas.edu>
> ---
>  docs/manpages/virsh.rst | 26 ++++++++++---
>  tools/virsh-domain.c    | 85 ++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 100 insertions(+), 11 deletions(-)
>
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index bfd26e3120..a4d70e9211 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -4500,14 +4500,18 @@ attach-disk
>        [--current]] | [--persistent]] [--targetbus bus]
>        [--driver driver] [--subdriver subdriver] [--iothread iothread]
>        [--cache cache] [--io io] [--type type] [--alias alias]
> -      [--mode mode] [--sourcetype sourcetype] [--serial serial]
> -      [--wwn wwn] [--rawio] [--address address] [--multifunction]
> -      [--print-xml]
> +      [--mode mode] [--sourcetype sourcetype]
> +      [--source-protocol protocol] [--source-host-name hostname:port]
> +      [--source-host-transport transport] [--source-host-socket socket]
> +      [--serial serial] [--wwn wwn] [--rawio] [--address address]
> +      [--multifunction] [--print-xml]
> 
>  Attach a new disk device to the domain.
> -*source* is path for the files and devices. *target* controls the bus or
> -device under which the disk is exposed to the guest OS. It indicates the
> -"logical" device name; the optional *targetbus* attribute specifies the type
> +*source* is path for the files and devices unless *--source-protocol*
> +is specified, in which case *source* is the name of a network disk.
> +*target* controls the bus or device under which the disk is exposed
> +to the guest OS. It indicates the "logical" device name;
> +the optional *targetbus* attribute specifies the type
>  of disk device to emulate; possible values are driver specific, with typical
>  values being *ide*, *scsi*, *virtio*, *xen*, *usb*, *sata*, or *sd*, if
>  omitted, the bus type is inferred from the style of the device name (e.g.  a
> @@ -4541,6 +4545,16 @@ ccw:cssid.ssid.devno. Virtio-ccw devices must have their cssid set to 0xfe.
>  *multifunction* indicates specified pci address is a multifunction pci device
>  address.
> 
> +There is also support for using a network disk. As specified, the user can
> +provide a *--source-protocol* in which case the *source* parameter will
> +be interpreted as the source name. *--source-protocol* must be provided
> +if the user also wishes to provide host information.
> +Host information can be provided using any of the tags
> +*--source-host-name*, *--source-host-transport*, and *--source-host-socket*,
> +which respectively denote the name of the host, the host's transport method,
> +and the socket that the host uses. The *--source-host-name* parameter
> +supports host:port syntax if the user wants to provide a port as well.
> +
>  If *--print-xml* is specified, then the XML of the disk that would be attached
>  is printed instead.
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 12b35c037d..4c43da7a2c 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -222,7 +222,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
>      {.name = "source",
>       .type = VSH_OT_DATA,
>       .flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK,
> -     .help = N_("source of disk device")
> +     .help = N_("source of disk device or name of network disk")
>      },
>      {.name = "target",
>       .type = VSH_OT_DATA,
> @@ -298,6 +298,22 @@ static const vshCmdOptDef opts_attach_disk[] = {
>       .type = VSH_OT_BOOL,
>       .help = N_("print XML document rather than attach the disk")
>      },
> +    {.name = "source-protocol",
> +     .type = VSH_OT_STRING,
> +     .help = N_("protocol used by disk device source")
> +    },
> +    {.name = "source-host-name",
> +     .type = VSH_OT_STRING,
> +     .help = N_("host name for source of disk device")
> +    },
> +    {.name = "source-host-transport",
> +     .type = VSH_OT_STRING,
> +     .help = N_("host transport for source of disk device")
> +    },
> +    {.name = "source-host-socket",
> +     .type = VSH_OT_STRING,
> +     .help = N_("host socket for source of disk device")
> +    },
>      VIRSH_COMMON_OPT_DOMAIN_PERSISTENT,
>      VIRSH_COMMON_OPT_DOMAIN_CONFIG,
>      VIRSH_COMMON_OPT_DOMAIN_LIVE,
> @@ -567,6 +583,12 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>                  *iothread = NULL, *cache = NULL, *io = NULL,
>                  *serial = NULL, *straddr = NULL, *wwn = NULL,
>                  *targetbus = NULL, *alias = NULL;
> +    const char *source_protocol = NULL;
> +    const char *host_name = NULL;
> +    const char *host_transport = NULL;
> +    const char *host_socket = NULL;
> +    char *host_name_copy = NULL;
> +    char *host_port = NULL;
>      struct DiskAddress diskAddr;
>      bool isFile = false, functionReturn = false;
>      int ret;
> @@ -604,7 +626,11 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>          vshCommandOptStringReq(ctl, cmd, "address", &straddr) < 0 ||
>          vshCommandOptStringReq(ctl, cmd, "targetbus", &targetbus) < 0 ||
>          vshCommandOptStringReq(ctl, cmd, "alias", &alias) < 0 ||
> -        vshCommandOptStringReq(ctl, cmd, "sourcetype", &stype) < 0)
> +        vshCommandOptStringReq(ctl, cmd, "sourcetype", &stype) < 0 ||
> +        vshCommandOptStringReq(ctl, cmd, "source-protocol", &source_protocol) < 0 ||
> +        vshCommandOptStringReq(ctl, cmd, "source-host-name", &host_name) < 0 ||
> +        vshCommandOptStringReq(ctl, cmd, "source-host-transport", &host_transport) < 0 ||
> +        vshCommandOptStringReq(ctl, cmd, "source-host-socket", &host_socket) < 0)
>          goto cleanup;
> 
>      if (!stype) {
> @@ -632,6 +658,13 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>      if (wwn && !virValidateWWN(wwn))
>          goto cleanup;
> 
> +    if (!source_protocol && (host_name || host_socket || host_transport)) {
> +        /* Error: cannot use network host without a source protocol */
> +        vshError(ctl, "%s",
> +                 _("Cannot use --source-host-* parameters without a --source-protocol"));
> +        goto cleanup;
> +    }

This can be replaced by:

VSH_REQUIRE_OPTION("source-host-name", "source-protocol");
VSH_REQUIRE_OPTION("source-host-transport", "source-protocol");
VSH_REQUIRE_OPTION("source-host-socket", "source-protocol");

You probably also want:

VSH_EXCLUSIVE_OPTIONS("source-host-name", "source-host-socket")

and also

VSH_REQUIRE_OPTION("source-host-socket", "source-host-transport")

as TCP is assumed by default and it wouldn't work with unix socket path.



> +
>      /* Make XML of disk */
>      virBufferAsprintf(&buf, "<disk type='%s'",
>                        isFile ? "file" : "block");

I didn't notice this previously. You obviously need "<disk type='network'"
if defining a network disk.

> @@ -659,9 +692,51 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>          virBufferAddLit(&buf, "/>\n");
>      }
> 
> -    if (source)
> -        virBufferAsprintf(&buf, "<source %s='%s'/>\n",
> -                          isFile ? "file" : "dev", source);
> +    if (source || source_protocol) {
> +        virBufferAddLit(&buf, "<source");
> +        if (source_protocol) {
> +            /* Using a network disk; source is --source-name */
> +            virBufferAsprintf(&buf, " protocol='%s'", source_protocol);
> +            if (source)
> +                virBufferAsprintf(&buf, " name='%s'", source);
> +
> +            if (host_name || host_socket || host_transport) {
> +                /* Host information provided, add a <host> tag */
> +                virBufferAddLit(&buf, ">\n");
> +                virBufferAdjustIndent(&buf, 2);
> +                virBufferAddLit(&buf, "<host");
> +
> +                if (host_name) {
> +                    /* Logic for host:port syntax */
> +                    host_name_copy = g_strdup(host_name);

host_name_copy variable is not freed, so the string is leaked. Since you
use the variable only in this scope, you can declare it here and also
use 'g_autofree' to free it on scope exit.

> +                    host_port = strchr(host_name_copy, ':');
> +
> +                    if (host_port) {
> +                        host_name_copy[(int)(host_port - host_name_copy)] = '\0';

So this pointer arithmetic expression is a bit pointless:

'ptr[X]' can be written as:

*(ptr + X)

Since strchr() returns 'ptr2 = ptr + n' and you calculate X as 'X = ptr2
- ptr'

Your calculation thus does:

*(ptr + (ptr2 - ptr)) -> *ptr2

You can thus clear the ':' char by:

    *host_port = '\0';

Additionally you forgot to move the 'host_port' pointer one after the
colon symbol which is now a NUL byte. That means that host_port actually
looks like an empty string ...

> +                        virBufferAsprintf(&buf,
> +                                          " name='%s' port='%s'",
> +                                          host_name_copy, host_port);

And thus this prints just:

$ ./build/libvirt/gcc/tools/virsh attach-disk upstream-bj /tmp/ble vda --source-host-name test:1234 --source-protocol test --print-xml
<disk type='file'>
  <source protocol='test' name='/tmp/ble'>
    <host name='test' port=''/>
  </source>
  <target dev='vda'/>
</disk>

In the above you can actually also see that <disk type='file'> is wrong
as mentioned above.

> +                    } else {
> +                        virBufferAsprintf(&buf, " name='%s'", host_name);
> +                    }
> +                }
> +
> +                if (host_transport)
> +                    virBufferAsprintf(&buf, " transport='%s'", host_transport);
> +                if (host_socket)
> +                    virBufferAsprintf(&buf, " socket='%s'", host_socket);
> +                virBufferAddLit(&buf, "/>\n");
> +                virBufferAdjustIndent(&buf, -2);
> +                virBufferAddLit(&buf, "</source>\n");
> +            }
> +        } else {
> +            /* Using a local disk; source is file or dev */
> +            virBufferAsprintf(&buf, " %s='%s'",
> +                        isFile ? "file" : "dev", source);

This is still misaligned.

> +            virBufferAddLit(&buf, "/>\n");
> +        }
> +    }
> +
>      virBufferAsprintf(&buf, "<target dev='%s'", target);
>      if (targetbus)
>          virBufferAsprintf(&buf, " bus='%s'", targetbus);

Unfortunately this function is very old and would need to be refactored,
the XML formatting is definitely not to our modern standards.