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

2020-11-10 Thread 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 
> ---
>  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", ) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "source-name", _name) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "source-protocol", 
> _protocol) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "target", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "driver", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "subdriver", ) < 0 ||
> @@ -604,7 +628,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>  vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
> -vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
> +vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "source-host-name", _name) < 0 
> ||
> +vshCommandOptStringReq(ctl, cmd, "source-host-transport", 
> _transport) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "source-host-socket", _socket) 
> < 0)
>  goto cleanup;
>  
>  if (!stype) {
> @@ -659,9 +686,42 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>  virBufferAddLit(, "/>\n");
>  }
>  
> -if (source)
> -virBufferAsprintf(, "\n",
> +if (source || source_protocol || source_name ||
> +host_name || host_transport || host_socket) {
> +virBufferAsprintf(, " +
> +if (source)
> +virBufferAsprintf(, " %s='%s'",
>isFile ? "file" : "dev", source);
> +if (source_protocol)
> +virBufferAsprintf(, " protocol='%s'", source_protocol);
> +if (source_name)
> +virBufferAsprintf(, " 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 +
> +if (host_name) {
> +host_port = strchr(host_name, ':');
> +
> +if (!host_port)
> +virBufferAsprintf(" name='%s'", host_name);
> +else {
> +

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

2020-11-10 Thread Han Han 
It is better to format the patch summary like this format: "SUBSYSTEM:
TITLE"
For example, this patch could be "virsh: Added a few attach-disk parameters"
You can see the git log of libvirt for more reference:
https://libvirt.org/git/?p=libvirt.git;a=summary

On Wed, Nov 11, 2020 at 5:58 AM Ryan Gahagan  wrote:

> Signed-off-by: Ryan Gahagan 
> ---
>  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;
>  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", ) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "source-name", _name) < 0
> ||
> +vshCommandOptStringReq(ctl, cmd, "source-protocol",
> _protocol) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "target", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "driver", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "subdriver", ) < 0 ||
> @@ -604,7 +628,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>  vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
>  vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
> -vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
> +vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "source-host-name", _name)
> < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "source-host-transport",
> _transport) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "source-host-socket",
> _socket) < 0)
>  goto cleanup;
>
>  if (!stype) {
> @@ -659,9 +686,42 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>  virBufferAddLit(, "/>\n");
>  }
>
> -if (source)
> -virBufferAsprintf(, "\n",
> +if (source || source_protocol || source_name ||
> +host_name || host_transport || host_socket) {
> +virBufferAsprintf(, " +
> +if (source)
> +virBufferAsprintf(, " %s='%s'",
>isFile ? "file" : "dev", source);
> +if (source_protocol)
> +virBufferAsprintf(, " protocol='%s'", source_protocol);
> +if (source_name)
> +virBufferAsprintf(, " name='%s'", source_name);
> +
> +if (host_name || host_transport || host_socket) {
> +virBufferAsprintf(">\n +
> +if (host_name) {
> +host_port = strchr(host_name, ':');
> +
> +if (!host_port)
> +virBufferAsprintf(" name='%s'", host_name);
> +else {
> +host_name[host_port - host_name] = '\0';
> +virBufferAsprintf(" name='%s' port='%s'", host_name,
> host_port + 1);
> +}
> +

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

2020-11-10 Thread Ryan Gahagan 
Signed-off-by: Ryan Gahagan 
---
 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;
 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", ) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-name", _name) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-protocol", _protocol) 
< 0 ||
 vshCommandOptStringReq(ctl, cmd, "target", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "driver", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "subdriver", ) < 0 ||
@@ -604,7 +628,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
 vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
-vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
+vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-host-name", _name) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-host-transport", 
_transport) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-host-socket", _socket) < 
0)
 goto cleanup;
 
 if (!stype) {
@@ -659,9 +686,42 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 virBufferAddLit(, "/>\n");
 }
 
-if (source)
-virBufferAsprintf(, "\n",
+if (source || source_protocol || source_name ||
+host_name || host_transport || host_socket) {
+virBufferAsprintf(, "\n\n\n");
+} else {
+virBufferAsprintf(" />\n");
+}
+}
+
 virBufferAsprintf(, "