[PATCH 1/8] Added a few attach-disk parameters

[Peter Krempa]

On Tue, Nov 10, 2020 at 15:56:13 -0600, Ryan Gahagan wrote:

Please describe your changes in the commit message.


> Signed-off-by: Ryan Gahagan <rgahagan at cs.utexas.edu>
> ---
>  tools/virsh-domain.c | 76 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 68 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 12b35c037d..16227085cc 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")
> +    },
>      {.name = "target",
>       .type = VSH_OT_DATA,
>       .flags = VSH_OFLAG_REQ,
> @@ -562,11 +582,13 @@ 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_name = NULL, *host_transport = NULL,
> +                *host_port = NULL, *host_socket = NULL;

We prefer one declaration per line with explicit type. Prior art here is
wrong, but there's no need to fix it. Just add your variables on
separate lines.


>      struct DiskAddress diskAddr;
>      bool isFile = false, functionReturn = false;
>      int ret;
> @@ -591,6 +613,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>          flags |= VIR_DOMAIN_AFFECT_LIVE;
>  
>      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 +628,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", &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) {
> @@ -659,9 +686,42 @@ 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) {
> +        virBufferAsprintf(&buf, "<source");
> +
> +        if (source)
> +            virBufferAsprintf(&buf, " %s='%s'",
>                            isFile ? "file" : "dev", source);
> +        if (source_protocol)
> +            virBufferAsprintf(&buf, " protocol='%s'", source_protocol);
> +        if (source_name)
> +            virBufferAsprintf(&buf, " name='%s'", source_name);

So, --source-name is mutually exclusive with --source. Please record
this using VSH_EXCLUSIVE_OPTIONS_VAR as we do for other arguments.

--source-name also requires --source-protocol, which also should be
recorded.

> +
> +        if (host_name || host_transport || host_socket) {
> +            virBufferAsprintf(">\n<host");
> +
> +            if (host_name) {
> +                host_port = strchr(host_name, ':');
> +
> +                if (!host_port)
> +                    virBufferAsprintf(" name='%s'", host_name);
> +                else {
> +                    host_name[host_port - host_name] = '\0';

You must not modify pointers you get from vshCommandOptStringReq. Copy
the string if you need to modify it.

> +                    virBufferAsprintf(" name='%s' port='%s'", host_name, host_port + 1);
> +                }
> +            }
> +            if (host_transport)
> +                virBufferAsprintf(" transport='%s'", host_transport);
> +            if (host_socket)
> +                virBufferAsprintf(" socket='%s'", host_socket);
> +
> +            virBufferAsprintf(" />\n</source>\n");


Note that the result of the above will be an ugly formatted XML, but we
strive for it to look correct. You'll need to use virBufferAdjustIndent
and use only one newline per formatting API since only the last newline
in a formating API use (virBufferAsprintf and such) triggers automatic
indentation.

> +        } else {
> +            virBufferAsprintf(" />\n");
> +        }
> +    }
> +
>      virBufferAsprintf(&buf, "<target dev='%s'", target);
>      if (targetbus)
>          virBufferAsprintf(&buf, " bus='%s'", targetbus);
> -- 
> 2.29.0
>