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