docs/manpages/virsh.rst | 26 ++++++++++--- tools/virsh-domain.c | 85 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 100 insertions(+), 11 deletions(-)
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;
+ }
+
/* Make XML of disk */
virBufferAsprintf(&buf, "<disk type='%s'",
isFile ? "file" : "block");
@@ -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_port = strchr(host_name_copy, ':');
+
+ if (host_port) {
+ host_name_copy[(int)(host_port - host_name_copy)] = '\0';
+ virBufferAsprintf(&buf,
+ " name='%s' port='%s'",
+ host_name_copy, host_port);
+ } 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);
+ virBufferAddLit(&buf, "/>\n");
+ }
+ }
+
virBufferAsprintf(&buf, "<target dev='%s'", target);
if (targetbus)
virBufferAsprintf(&buf, " bus='%s'", targetbus);
--
2.29.0
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.
> > +
> > /* 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:
> $ ./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>
What specifically would you want to be changed?
On Mon, Nov 16, 2020 at 6:58 AM Peter Krempa <pkrempa@redhat.com> 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@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.
>
>
On Mon, Nov 16, 2020 at 18:03:24 -0600, Ryan Gahagan wrote:
I'd like to reiterate that you should avoid top-posts on technical
lists. Even if you copy the context into the top post. Please always
respond inline.
> > > +
> > > /* 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; }
This got horribly mangled by your client. I suggest you use plain-text
mode for maling list posts. Let me reformat it for now, but next time
please make sure it's readable as it takes a lot of effort.
> 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;
So this is very wrong. This should not be based on stat(). Firstly, virsh
may not run on the host the VM is running on (remote connection). That
is a bug in the current code too. As noted I'll fix it after your patch
as I want to reduce the number of backs-and-forths with this patch.
You can simply assume that the user want's a network disk when the
protocol is specified using the flag you've added. Since that is
mandatory for a network disk it removes any kind of guessing.
> } 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;
You also want to check that the protocol is specified when the stype is
explicitly set to 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
No. The old code is wrong and uses old formatter functions. I don't want
to refactor it before we get over this patch as you'd have to change it
significantly.
> misaligned, and what function would need to be refactored? Your example
> output at least looks aligned properly:
Yes, the XML is aligned properly. I meant that the C code you moved in
[3] is not aligned properly. See below as your quote doesn't include
that bit.
> > $ ./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>
>
> What specifically would you want to be changed?
Well, firstly "disk type='file'" is clearly wrong when you are trying to
attach a network disk, the focal point of this addition. I've mentioned
that problem at [1].
The second problem is that "--source-host-name test:1234" results into:
<host name='test' port=''/>. This is mentioned at [2].
> On Mon, Nov 16, 2020 at 6:58 AM Peter Krempa <pkrempa@redhat.com> 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@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.
[1]
> >
> > > @@ -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 ...
[2]
> >
> > > + 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.
[3]
> >
> > > + 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.
> >
> >
© 2016 - 2024 Red Hat, Inc.