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@xxxxxxxxxxxxx> > --- > 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.
