> > +
> > /* 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.
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:
> > /* 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.
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.
> > + /* 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.
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:
> > + 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.
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:
> $ ./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>
> <disk type='file'>
> <source protocol='test' name='/tmp/ble'>
> <host name='test' port=''/>
> </source>
> <target dev='vda'/>
> </disk>
What specifically would you want to be changed?
On Mon, Nov 16, 2020 at 6:58 AM Peter Krempa <pkrempa@xxxxxxxxxx> 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@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.
