On Wed, Nov 11, 2020 at 17:00:51 -0600, Ryan Gahagan wrote: > Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16 > Added in support for the following parameters in attach-disk: > --source-name > --source-protocol > --source-host-name > --source-host-socket > --source-host-transport > > Allowed for multiple hosts to be added to a single source. I'm not really a fan of this. Once there is a complex enough configuration the user still can use 'virsh attach-device' manually. The code for allowing multiple disks is not very elegant and not very commonly used either. > Multiple hosts can be defined by providing multiple instances of > --source-host-name, followed by optional transport and socket > parameters. > Using a single host does not require a host name. > > Added documentation to virsh.rst specifying usage. > > Signed-off-by: Ryan Gahagan <rgahagan@xxxxxxxxxxxxx> I'd like to ask you to follow all review feedback. Reviewer time is prescious and if you don't follow it you make it less likely that you'll get any feedback the next time. > --- > docs/manpages/virsh.rst | 24 +++++++- > tools/virsh-domain.c | 125 +++++++++++++++++++++++++++++++++++++--- > 2 files changed, 138 insertions(+), 11 deletions(-) > > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst > index bfd26e3120..60e5f5ebe4 100644 > --- a/docs/manpages/virsh.rst > +++ b/docs/manpages/virsh.rst > @@ -4500,9 +4500,11 @@ 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-name name] > + [--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 > @@ -4541,6 +4543,22 @@ 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. > > +If *--source-protocol* or *--source-name* is specified, then the parameters > +will be inserted into the XML that is generated for the source. > +If any of *--source-host-name*, *--source-host-transport*, or > +*--source-host-socket* are specified, then a ``<host>`` tag > +will be generated under the ``<source>`` tag containing whichever This describes what's done internally rather than describing what disk this will add. `virsh attach-disk` is a syntax-suggar command which generates the XML for you so the user is not actually interested in the XML itself. Describe what the semantics of the command are and not the logic inside. > +parameters were provided. If needed, the user can provide multiple hosts > +by providing each host with a *--source-host-name*. Each host will > +receive the host parameters which come between it and the next instance > +of *--source-host-name* or between it and the end of the command. > +If a user tries to provide multiple of the same host parameter > +for any single host, only the first one will be generated as > +part of the XML output. As said, `virsh attach-disk`'s purpose is not the XML output but rather attaching the disk. > + > +--source-host-name me --source-host-transport t1 --source-host-transport t2 --source-host-transport t3 --soure-host-name you This line is too long ... > +<host transport='t1' transport='t2' transport='t3'> ... and this doesn't make sense since it's invalid disk XML. > + > 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..609189e398 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -224,6 +224,26 @@ static const vshCmdOptDef opts_attach_disk[] = { > .flags = VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK, > .help = N_("source of disk device") > }, > + {.name = "source-protocol", > + .type = VSH_OT_STRING, > + .help = N_("protocol used by disk device source") > + }, > + {.name = "source-name", > + .type = VSH_OT_STRING, > + .help = N_("name of 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") > + }, Firstly, something here is broken: When I start 'virsh attach-disk' I get: $ ~/build/libvirt/gcc/tools/virsh attach-disk error: internal error: bad options in command: 'attach-disk' So the command options are wrong. Specifically any VSH_OT_STRING must be after any VSH_OT_DATA type entries, so you need to move all of those after it. Secondly, our test suite is broken because it didn't catch it. I'll look into that. > {.name = "target", > .type = VSH_OT_DATA, > .flags = VSH_OFLAG_REQ, > @@ -558,15 +578,68 @@ static int str2DiskAddress(const char *str, struct DiskAddress *diskAddr) > return -1; > } > > +static void attachDiskHostGen(virBufferPtr buf, const vshCmd *cmd) Please use customary function names and syntax. Virsh stuff usually starts with the 'vsh' prefix. > +{ > + // Can be multiple hosts so we have to scan > + // the cmd options to find all the host params > + // <source tag in XML not yet closed Line comments are not allowed in our code per code style guidelines: https://www.libvirt.org/coding-style.html#code-formatting-especially-for-new-code > + vshCmdOpt *candidate = cmd->opts; > + char *host_name = NULL, *host_port = NULL; As noted in my previous response we strongly prefer one variable definition per line especially in new code. > + int close_tag = 0, seen_socket = 0, seen_transport = 0; All of these are booleans so use proper 'bool' type and also one per line. > + > + while (candidate) { > + // Iterate candidates to find each host-name > + if (STREQ(candidate->def->name, "source-host-name")) { > + // After the first host-name, we need to terminate > + // the <host ... tag > + // It's left open so socket and transport can be added later > + > + // Only include the first example of socket or transport per host > + // When a socket or transport is seen, these are set to true > + // until the next name is encountered. > + seen_socket = 0; > + seen_transport = 0; > + > + if (close_tag) > + virBufferAddLit(buf, "/>\n"); > + else > + close_tag = 1; > + > + host_name = candidate->data; > + host_port = strchr(host_name, ':'); > + > + if (!host_port) { > + // If port isn't provided, only print name > + virBufferAsprintf(buf, "<host name='%s'", host_name); > + } else { > + // If port is provided, manipulate strings and print both > + host_name[(int)(host_port - host_name)] = '\0'; As noted in my previous review, you _must not_ modify the vshCmdOpt data directly. Make a copy of it if you are modifying it. > + virBufferAsprintf(buf, "<host name='%s' port='%s'", host_name, host_port + 1); > + } > + } else if (!seen_socket && STREQ(candidate->def->name, "source-host-socket")) { > + seen_socket = 1; > + virBufferAsprintf(buf, " socket='%s'", candidate->data); > + } else if (!seen_transport && STREQ(candidate->def->name, "source-host-transport")) { I'm not a fan of this algorithm at all. It works only if the arguments are ordered properly. If you don't do it properly you end up like this: $ ~/build/libvirt/gcc/tools/virsh attach-disk --print-xml --source /tmp/ble --target blsdf --domain backup-test --source-name test --source-host-transport asdf --source-host-socket bheah --source-host-name sdfgasdfg <disk type='file'> <source file='/tmp/ble' name='test'> transport='asdf' socket='bheah'<host name='sdfgasdfg'/> </source> <target dev='blsdf'/> </disk> which is completely wrong. As noted I don't really think it's worth having multiple host support. If you think it is you must ensure that it's somehow made reasonable. Such as parsing transport, host and port from a string or something. But all of that is just too much syntax sugar. IMO users needing multiple host can be made to use the XML based command. > + seen_transport = 1; > + virBufferAsprintf(buf, " transport='%s'", candidate->data); > + } > + > + candidate = candidate->next; > + } > + // Close final <host tag > + virBufferAddLit(buf, "/>\n"); > +} > + > static bool > cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) > { > virDomainPtr dom = NULL; > - const char *source = NULL, *target = NULL, *driver = NULL, > - *subdriver = NULL, *type = NULL, *mode = NULL, > - *iothread = NULL, *cache = NULL, *io = NULL, > - *serial = NULL, *straddr = NULL, *wwn = NULL, > - *targetbus = NULL, *alias = NULL; > + const char *source = NULL, *source_name = NULL, *source_protocol = NULL, > + *target = NULL, *driver = NULL, *subdriver = NULL, *type = NULL, > + *mode = NULL, *iothread = NULL, *cache = NULL, > + *io = NULL, *serial = NULL, *straddr = NULL, > + *wwn = NULL, *targetbus = NULL, *alias = NULL, > + *host_transport = NULL, *host_name = NULL, *host_socket = NULL; As noted in my previous review, we strongly prefer one variable declaration per line. > struct DiskAddress diskAddr; > bool isFile = false, functionReturn = false; > int ret; > @@ -591,6 +664,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) > flags |= VIR_DOMAIN_AFFECT_LIVE; You still didn't configure the conflicting options as mutually exclusive as requested in previous review. Additionally --source is a mandatory argument but is mutually exclusive with --source-name. Using both together creates an invalid disk XML: $ ~/build/libvirt/gcc/tools/virsh attach-disk --print-xml --source /tmp/ble --target blsdf --domain backup-test --source-name test <disk type='file'> <source file='/tmp/ble' name='test'/> <target dev='blsdf'/> </disk> > if (vshCommandOptStringReq(ctl, cmd, "source", &source) < 0 || > + vshCommandOptStringReq(ctl, cmd, "source-name", &source_name) < 0 || > + vshCommandOptStringReq(ctl, cmd, "source-protocol", &source_protocol) < 0 || > vshCommandOptStringReq(ctl, cmd, "target", &target) < 0 || > vshCommandOptStringReq(ctl, cmd, "driver", &driver) < 0 || > vshCommandOptStringReq(ctl, cmd, "subdriver", &subdriver) < 0 || > @@ -604,7 +679,10 @@ 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-host-name", (const char **) &host_name) < 0 || If you need to change the variable, copy it but you _must not_ modify the one returned from vshCommandOptStringReq. > + vshCommandOptStringReq(ctl, cmd, "source-host-transport", &host_transport) < 0 || > + vshCommandOptStringReq(ctl, cmd, "source-host-socket", &host_socket) < 0) > goto cleanup; > > if (!stype) { > @@ -659,9 +737,40 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) > virBufferAddLit(&buf, "/>\n"); > } > > - if (source) > - virBufferAsprintf(&buf, "<source %s='%s'/>\n", > + if (source || source_protocol || source_name || > + host_name || host_transport || host_socket) { > + virBufferAddLit(&buf, "<source"); > + > + if (source) > + virBufferAsprintf(&buf, " %s='%s'", > isFile ? "file" : "dev", source); This line will not be aligned properly aftery your patch. > + if (source_protocol) > + virBufferAsprintf(&buf, " protocol='%s'", source_protocol); > + if (source_name) > + virBufferAsprintf(&buf, " name='%s'", source_name); --source-protocol is not made mandatory when --source-name is used so users can omit it. > + if (!(host_name || host_transport || host_socket)) { > + // Close source if no host is provided > + virBufferAddLit(&buf, "/>\n"); > + } else if (!host_name) { > + // If no host name is provided but there is a host, > + // we have a single host with params > + virBufferAddLit(&buf, ">\n<host"); I've also mentioned this before. It's a quirk of our APIs but if you have a newline in a string added to a buffer and it's not at the end, our auto-indent is NOT applied: $ ~/build/libvirt/gcc/tools/virsh attach-disk --print-xml --source /tmp/ble --target blsdf --domain backup-test --source-name test --source-host-transport asdf --source-host-socket bheah <disk type='file'> <source file='/tmp/ble' name='test'> <host transport='asdf' socket='bheah'/> </source> <target dev='blsdf'/> </disk> Please make sure that there's nothing following newline after tag closing to keep indentation working. Also adjust the indent level before adding nested elements. > + > + if (host_transport) > + virBufferAsprintf(&buf, " transport='%s'", host_transport); > + if (host_socket) > + virBufferAsprintf(&buf, " socket='%s'", host_socket); > + > + virBufferAddLit(&buf, "/>\n</source>\n"); Same here. > + } else { > + // May have multiple hosts, use helper method > + virBufferAddLit(&buf, ">\n"); > + attachDiskHostGen(&buf, cmd); > + virBufferAddLit(&buf, "</source>\n"); > + } > + }
