Re: [PATCH] virsh: Added attach-disk support for network disk

We're confused on what the exact semantics of the disk type we should use is, particularly if the driver or stype field has unexpected values.. We've drafted up some example code to show our interpretation of when "network" should be used over "file" or "block", but it is by no means final and we want your opinion on it. Here's what we've got: typedef enum { VIRSH_SOURCE_TYPE_FILE, VIRSH_SOURCE_TYPE_BLOCK, VIRSH_SOURCE_TYPE_NETWORK, VIRSH_SOURCE_TYPE_UNKNOWN, } virshAttachDiskSourceType; ... virshAttachDiskSourceType disk_type = VIRSH_SOURCE_TYPE_UNKNOWN; if (!stype) { if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) { /* Disk type declared to be file */ disk_type = VIRSH_SOURCE_TYPE_FILE; } else { if (source && !stat(source, &st)) { /* Found a file at path source, test file type */ if (S_ISREG(st.st_mode)) disk_type = VIRSH_SOURCE_TYPE_FILE; else if (S_ISBLK(st.st_mode)) disk_type = VIRSH_SOURCE_TYPE_BLOCK; else disk_type = VIRSH_SOURCE_TYPE_NETWORK; } else { /* Either file not found or not specified, default network */ disk_type = VIRSH_SOURCE_TYPE_NETWORK; } } } else if (STREQ(stype, "file")) { disk_type = VIRSH_SOURCE_TYPE_FILE; } else if (STREQ(stype, "block")) { disk_type = VIRSH_SOURCE_TYPE_BLOCK; } else if (STREQ(stype, "network")) { disk_type = VIRSH_SOURCE_TYPE_NETWORK; } else { vshError(ctl, _("Unknown source type: '%s'"), stype); goto cleanup; } Let us know if this looks right or what you think should be used instead. We're not entirely sure what you mean by this. What part of the code looks misaligned, and what function would need to be refactored? Your example output at least looks aligned properly: --source-host-name test:1234 --source-protocol test --print-xml What specifically would you want to be changed? On Mon, Nov 16, 2020 at 6:58 AM Peter Krempa <pkrempa(a)redhat.com&gt; wrote: > 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(a)cs.utexas.edu&gt; > > --- > > 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. > > > > > > @@ -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: > > --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. > >