Re: [PATCH 8/8] Added new params to attach-disk docs
On Tue, Nov 10, 2020 at 15:56:20 -0600, Ryan Gahagan wrote: > Signed-off-by: Ryan Gahagan > --- > docs/manpages/virsh.rst | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst > index bfd26e3120..83134ba571 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] You also need to describe how the parameters work in the text below.
Re: [PATCH 6/8] Bump - rerun CI HTTP 500 err
On Tue, Nov 10, 2020 at 15:56:18 -0600, Ryan Gahagan wrote: > Signed-off-by: Ryan Gahagan > --- > tools/virsh-domain.c | 1 - > 1 file changed, 1 deletion(-) This definitely does not belong to a final submission.
Re: [PATCH 1/8] Added a few attach-disk parameters
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 3/8] Fixed code style bug for virBufferAddLit
On Tue, Nov 10, 2020 at 15:56:15 -0600, Ryan Gahagan wrote:
> Signed-off-by: Ryan Gahagan
> ---
> tools/virsh-domain.c | 21 +++--
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 810e55fa53..5862993464 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -688,7 +688,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>
> if (source || source_protocol || source_name ||
> host_name || host_transport || host_socket) {
> -virBufferAsprintf(, " +virBufferAddLit(, "
Re: [PATCH 00/28] a bunch of domain_conf cleanup
On 11/11/20 8:18 AM, Matt Coleman wrote: On Nov 9, 2020, at 9:43 AM, Michal Privoznik wrote: Nice cleanup. But as I say in 03/28 I think we might want glib adoption to be done in bigger chunks. Usually we rewrite VIR_ALLOC/VIR_REALLOC_N -> g_new0()/g_renew() in one patch (might be coupled with g_free() except VIR_FREE() resets the pointer to NULL and g_free() doesn't do that so I'm not really a fan of g_free()), then g_strdup() in another path, and so on. I had thought that g_free() was a drop-in replacement for VIR_FREE() based on other earlier commits that I had seen. You can drop the g_free() commits from this series if everything else looks good. Yes, please. VIR_FREE() is perfect as is :-) g_free() doesn't clear the pointer and g_clear_pointer(, g_free); is just too long. Michal
Re: [PATCH v2 1/2] docs: refactor mdev_types into new paragraph
Boris Fiuczynski [2020-11-10, 07:09PM +0100]: > To prevent copying the mdev_types description multiple times > it is refactored into a new paragraph for easy reuse. > > Signed-off-by: Boris Fiuczynski > --- > docs/formatnode.html.in | 70 - > 1 file changed, 41 insertions(+), 29 deletions(-) > > diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in > index 6928bdd69c..bd3112c5a8 100644 > --- a/docs/formatnode.html.in > +++ b/docs/formatnode.html.in Makes sense. Not entirely sure about the new location since I can't figure it out from the patch (why has this file not yet been converted to RST?), but still. Reviewed-by: Bjoern Walk signature.asc Description: PGP signature
Re: [PATCH v7 3/4] conf: Support to parse rbd namespace from source name
On Wed, Nov 11, 2020 at 10:24 AM Jason Dillaman wrote:
> On Tue, Nov 10, 2020 at 8:43 PM Han Han wrote:
> >
> > Signed-off-by: Han Han
> > ---
> > docs/formatdomain.rst | 16 ++
> > src/conf/domain_conf.c | 47 ++
> > 2 files changed, 59 insertions(+), 4 deletions(-)
> >
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index a6ab845f92..2b760e6a39 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -2409,6 +2409,16 @@ paravirtualized driver is specified via the
> ``disk`` element.
> >
> >
> >
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> >
> > ...
> >
> > @@ -2500,6 +2510,12 @@ paravirtualized driver is specified via the
> ``disk`` element.
> >the host by the ``nbd_tls`` and ``nbd_tls_x509_cert_dir`` in
> >/etc/libvirt/qemu.conf. ('tls' :since:`Since 4.5.0` )
> >
> > + For "rbd", the ``name`` attribute could be two formats: the
> format of
> > + ``pool_name/image_name`` includes the rbd pool name and image
> name with
> > + default rbd pool namespace; for the customized namespace, the
> format is
> > + ``pool_name/namespace/image_name`` ( :since:`Since 6.9.0 and QEMU
> 5.0` ).
> > + The pool name, namespace and image are separated by slash.
> > +
> >For protocols ``http`` and ``https`` an optional attribute
> ``query``
> >specifies the query string. ( :since:`Since 6.2.0` )
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index d84d0327ad..a337051e30 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -9554,6 +9554,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
> > g_autofree char *tlsCfg = NULL;
> > g_autofree char *sslverifystr = NULL;
> > xmlNodePtr tmpnode;
> > +char **tmp_split_paths;
> >
> > if (!(protocol = virXMLPropString(node, "protocol"))) {
> > virReportError(VIR_ERR_XML_ERROR, "%s",
> > @@ -9595,8 +9596,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
> > /* for historical reasons we store the volume and image name in one
> XML
> > * element although it complicates thing when attempting to access
> them. */
> > if (src->path &&
> > -(src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER ||
> > - src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
> > +src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
> > char *tmp;
> > if (!(tmp = strchr(src->path, '/')) ||
> > tmp == src->path) {
> > @@ -9613,6 +9613,41 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
> > tmp[0] = '\0';
> > }
> >
> > +/* the name of rbd could be / or
> // */
> > +if (src->path &&
> > +src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
> > +(tmp_split_paths = virStringSplit(src->path, "/", 3))) {
> > +if (virStringIsEmpty(tmp_split_paths[0]) ||
> > +!tmp_split_paths[1] ||
> > +STREQ_NULLABLE(tmp_split_paths[2], "") ||
> > +(virStringIsEmpty(tmp_split_paths[1]) &&
> > + !tmp_split_paths[2])) {
> > +virStringListFreeCount(tmp_split_paths, 3);
> > +virReportError(VIR_ERR_XML_ERROR,
> > + _("can't split path '%s' into pool name,
> pool "
> > + "namespace, image name OR pool name, image
> name"),
> > + src->path);
> > +return -1;
> > +}
> > +
> > +VIR_FREE(src->path);
> > +src->volume = g_strdup(tmp_split_paths[0]);
> > +/* the format of / */
> > +if (!tmp_split_paths[2])
> > +src->path = g_strdup(tmp_split_paths[1]);
> > +
> > +if (tmp_split_paths[2]) {
> > +/* the format of // */
> > +if (STRNEQ_NULLABLE(tmp_split_paths[1], ""))
> > +src->ns = g_strdup(tmp_split_paths[1]);
> > +
> > +/* the format of // */
> > +src->path = g_strdup(tmp_split_paths[2]);
>
> Nit: I think QEMU and the rbd CLI will both treat this case as an
> image named "/" instead of "".
>
The results of qemu(qemu-kvm-5.1.0-5.fc33.x86_64) and rbd
CLI(ceph-common-15.2.5-1.fc33.x86_64)
are different here:
➜ ~ qemu-img info rbd:rbd//copy:conf=$HOME/.ceph/ceph.conf:key=XX
image: json:{"driver": "raw", "file": {"pool": "rbd", "image": "copy",
"conf": "/home/hhan/.ceph/ceph.conf", "driver": "rbd"}}
file format: raw
virtual size: 10 GiB (10737418240 bytes)
disk size: unavailable
cluster_size: 4194304
➜ ~ rbd info rbd//copy
rbd: error opening image /copy: (2) No such file or directory
➜ ~ rbd info rbd/copy
rbd image 'copy':
size 10 GiB in 2560 objects
order 22 (4 MiB objects)
snapshot_count: 0
id: 4a4956b8b4567
block_name_prefix:
Re: [PATCH v7 3/4] conf: Support to parse rbd namespace from source name
On Tue, Nov 10, 2020 at 8:43 PM Han Han wrote:
>
> Signed-off-by: Han Han
> ---
> docs/formatdomain.rst | 16 ++
> src/conf/domain_conf.c | 47 ++
> 2 files changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index a6ab845f92..2b760e6a39 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -2409,6 +2409,16 @@ paravirtualized driver is specified via the ``disk``
> element.
>
>
>
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
>
> ...
>
> @@ -2500,6 +2510,12 @@ paravirtualized driver is specified via the ``disk``
> element.
>the host by the ``nbd_tls`` and ``nbd_tls_x509_cert_dir`` in
>/etc/libvirt/qemu.conf. ('tls' :since:`Since 4.5.0` )
>
> + For "rbd", the ``name`` attribute could be two formats: the format of
> + ``pool_name/image_name`` includes the rbd pool name and image name with
> + default rbd pool namespace; for the customized namespace, the format is
> + ``pool_name/namespace/image_name`` ( :since:`Since 6.9.0 and QEMU 5.0`
> ).
> + The pool name, namespace and image are separated by slash.
> +
>For protocols ``http`` and ``https`` an optional attribute ``query``
>specifies the query string. ( :since:`Since 6.2.0` )
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d84d0327ad..a337051e30 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9554,6 +9554,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
> g_autofree char *tlsCfg = NULL;
> g_autofree char *sslverifystr = NULL;
> xmlNodePtr tmpnode;
> +char **tmp_split_paths;
>
> if (!(protocol = virXMLPropString(node, "protocol"))) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -9595,8 +9596,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
> /* for historical reasons we store the volume and image name in one XML
> * element although it complicates thing when attempting to access them.
> */
> if (src->path &&
> -(src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER ||
> - src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
> +src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
> char *tmp;
> if (!(tmp = strchr(src->path, '/')) ||
> tmp == src->path) {
> @@ -9613,6 +9613,41 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
> tmp[0] = '\0';
> }
>
> +/* the name of rbd could be / or //
> */
> +if (src->path &&
> +src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
> +(tmp_split_paths = virStringSplit(src->path, "/", 3))) {
> +if (virStringIsEmpty(tmp_split_paths[0]) ||
> +!tmp_split_paths[1] ||
> +STREQ_NULLABLE(tmp_split_paths[2], "") ||
> +(virStringIsEmpty(tmp_split_paths[1]) &&
> + !tmp_split_paths[2])) {
> +virStringListFreeCount(tmp_split_paths, 3);
> +virReportError(VIR_ERR_XML_ERROR,
> + _("can't split path '%s' into pool name, pool "
> + "namespace, image name OR pool name, image
> name"),
> + src->path);
> +return -1;
> +}
> +
> +VIR_FREE(src->path);
> +src->volume = g_strdup(tmp_split_paths[0]);
> +/* the format of / */
> +if (!tmp_split_paths[2])
> +src->path = g_strdup(tmp_split_paths[1]);
> +
> +if (tmp_split_paths[2]) {
> +/* the format of // */
> +if (STRNEQ_NULLABLE(tmp_split_paths[1], ""))
> +src->ns = g_strdup(tmp_split_paths[1]);
> +
> +/* the format of // */
> +src->path = g_strdup(tmp_split_paths[2]);
Nit: I think QEMU and the rbd CLI will both treat this case as an
image named "/" instead of "".
Otherwise,
Reviewed-by: Jason Dillaman
> +}
> +
> +virStringListFreeCount(tmp_split_paths, 3);
> +}
> +
> /* snapshot currently works only for remote disks */
> src->snapshot = virXPathString("string(./snapshot/@name)", ctxt);
>
> @@ -25351,8 +25386,12 @@ virDomainDiskSourceFormatNetwork(virBufferPtr
> attrBuf,
> virBufferAsprintf(attrBuf, " protocol='%s'",
>virStorageNetProtocolTypeToString(src->protocol));
>
> -if (src->volume)
> -path = g_strdup_printf("%s/%s", src->volume, src->path);
> +if (src->volume) {
> +if (src->ns)
> +path = g_strdup_printf("%s/%s/%s", src->volume, src->ns,
> src->path);
> +else
> +path = g_strdup_printf("%s/%s", src->volume, src->path);
> +}
>
> virBufferEscapeString(attrBuf, " name='%s'", path ? path : src->path);
> virBufferEscapeString(attrBuf, " query='%s'", src->query);
> --
> 2.28.0
>
--
Jason
[PATCH v7 3/4] conf: Support to parse rbd namespace from source name
Signed-off-by: Han Han
---
docs/formatdomain.rst | 16 ++
src/conf/domain_conf.c | 47 ++
2 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index a6ab845f92..2b760e6a39 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -2409,6 +2409,16 @@ paravirtualized driver is specified via the ``disk``
element.
+
+
+
+
+
+
+
+
+
+
...
@@ -2500,6 +2510,12 @@ paravirtualized driver is specified via the ``disk``
element.
the host by the ``nbd_tls`` and ``nbd_tls_x509_cert_dir`` in
/etc/libvirt/qemu.conf. ('tls' :since:`Since 4.5.0` )
+ For "rbd", the ``name`` attribute could be two formats: the format of
+ ``pool_name/image_name`` includes the rbd pool name and image name with
+ default rbd pool namespace; for the customized namespace, the format is
+ ``pool_name/namespace/image_name`` ( :since:`Since 6.9.0 and QEMU 5.0` ).
+ The pool name, namespace and image are separated by slash.
+
For protocols ``http`` and ``https`` an optional attribute ``query``
specifies the query string. ( :since:`Since 6.2.0` )
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d84d0327ad..a337051e30 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9554,6 +9554,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
g_autofree char *tlsCfg = NULL;
g_autofree char *sslverifystr = NULL;
xmlNodePtr tmpnode;
+char **tmp_split_paths;
if (!(protocol = virXMLPropString(node, "protocol"))) {
virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -9595,8 +9596,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
/* for historical reasons we store the volume and image name in one XML
* element although it complicates thing when attempting to access them. */
if (src->path &&
-(src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER ||
- src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
+src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
char *tmp;
if (!(tmp = strchr(src->path, '/')) ||
tmp == src->path) {
@@ -9613,6 +9613,41 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
tmp[0] = '\0';
}
+/* the name of rbd could be / or // */
+if (src->path &&
+src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
+(tmp_split_paths = virStringSplit(src->path, "/", 3))) {
+if (virStringIsEmpty(tmp_split_paths[0]) ||
+!tmp_split_paths[1] ||
+STREQ_NULLABLE(tmp_split_paths[2], "") ||
+(virStringIsEmpty(tmp_split_paths[1]) &&
+ !tmp_split_paths[2])) {
+virStringListFreeCount(tmp_split_paths, 3);
+virReportError(VIR_ERR_XML_ERROR,
+ _("can't split path '%s' into pool name, pool "
+ "namespace, image name OR pool name, image name"),
+ src->path);
+return -1;
+}
+
+VIR_FREE(src->path);
+src->volume = g_strdup(tmp_split_paths[0]);
+/* the format of / */
+if (!tmp_split_paths[2])
+src->path = g_strdup(tmp_split_paths[1]);
+
+if (tmp_split_paths[2]) {
+/* the format of // */
+if (STRNEQ_NULLABLE(tmp_split_paths[1], ""))
+src->ns = g_strdup(tmp_split_paths[1]);
+
+/* the format of // */
+src->path = g_strdup(tmp_split_paths[2]);
+}
+
+virStringListFreeCount(tmp_split_paths, 3);
+}
+
/* snapshot currently works only for remote disks */
src->snapshot = virXPathString("string(./snapshot/@name)", ctxt);
@@ -25351,8 +25386,12 @@ virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf,
virBufferAsprintf(attrBuf, " protocol='%s'",
virStorageNetProtocolTypeToString(src->protocol));
-if (src->volume)
-path = g_strdup_printf("%s/%s", src->volume, src->path);
+if (src->volume) {
+if (src->ns)
+path = g_strdup_printf("%s/%s/%s", src->volume, src->ns,
src->path);
+else
+path = g_strdup_printf("%s/%s", src->volume, src->path);
+}
virBufferEscapeString(attrBuf, " name='%s'", path ? path : src->path);
virBufferEscapeString(attrBuf, " query='%s'", src->query);
--
2.28.0
[PATCH v7 4/4] qemu: Implement rbd namespace to the source name attribute
Since Nautilus ceph supports separate image namespaces within a pool for
tenant isolation and QEMU adds it as a rbd blockdev options from 5.0.0.
The source name with format "//" could be used to
access a rbd image with namespace.
Add unit tests for this attribute.
https://bugzilla.redhat.com/show_bug.cgi?id=1816909
Signed-off-by: Han Han
---
src/qemu/qemu_block.c | 1 +
src/qemu/qemu_domain.c| 8 +++
...k-network-rbd-namespace.x86_64-latest.args | 49 +++
.../disk-network-rbd-namespace.xml| 40 +++
tests/qemuxml2argvtest.c | 1 +
...sk-network-rbd-namespace.x86_64-latest.xml | 49 +++
tests/qemuxml2xmltest.c | 1 +
7 files changed, 149 insertions(+)
create mode 100644
tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
create mode 100644
tests/qemuxml2xmloutdata/disk-network-rbd-namespace.x86_64-latest.xml
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 4640e339c0..a1d85d3794 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -870,6 +870,7 @@ qemuBlockStorageSourceGetRBDProps(virStorageSourcePtr src,
"s:pool", src->volume,
"s:image", src->path,
"S:snapshot", src->snapshot,
+ "S:namespace", src->ns,
"S:conf", src->configFile,
"A:server", ,
"S:user", username,
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 02b5e7422c..9c21f564ce 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4628,6 +4628,14 @@ qemuDomainValidateStorageSource(virStorageSourcePtr src,
return -1;
}
+if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
+src->ns &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_RBD_NAMESPACE)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("rbd namespace is not supported by this QEMU"));
+return -1;
+}
+
return 0;
}
diff --git
a/tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
b/tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
new file mode 100644
index 00..49b3dd53f6
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
@@ -0,0 +1,49 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object secret,id=masterKey0,format=raw,\
+file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-cpu qemu64 \
+-m 214 \
+-object memory-backend-ram,id=pc.ram,size=224395264 \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot strict=on \
+-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
+-blockdev '{"driver":"rbd","pool":"pool","image":"image","namespace":"ns",\
+"server":[{"host":"mon1.example.org","port":"6321"},{"host":"mon2.example.org",\
+"port":"6322"},{"host":"mon3.example.org","port":"6322"}],\
+"node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw",\
+"file":"libvirt-2-storage"}' \
+-device virtio-blk-pci,bus=pci.0,addr=0x2,drive=libvirt-2-format,\
+id=virtio-disk0,bootindex=1 \
+-blockdev '{"driver":"rbd","pool":"pool","image":"image",\
+"server":[{"host":"mon1.example.org","port":"6321"}],\
+"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\
+"file":"libvirt-1-storage"}' \
+-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=libvirt-1-format,\
+id=virtio-disk1 \
+-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
+resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
b/tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
new file mode 100644
index 00..5389cbda2f
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
@@ -0,0 +1,40 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 219136
+ 219136
+ 1
+
+hvm
+
+
+
+ destroy
+
[PATCH v7 2/4] util: Add member ns to the storage source struct
The member ns will be used to store the namespace string.
Signed-off-by: Han Han
---
src/util/virstoragefile.c | 2 ++
src/util/virstoragefile.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 35e6dcf5de..5d2fd70889 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2375,6 +2375,7 @@ virStorageSourceCopy(const virStorageSource *src,
def->path = g_strdup(src->path);
def->volume = g_strdup(src->volume);
+def->ns = g_strdup(src->ns);
def->relPath = g_strdup(src->relPath);
def->backingStoreRaw = g_strdup(src->backingStoreRaw);
def->backingStoreRawFormat = src->backingStoreRawFormat;
@@ -2655,6 +2656,7 @@ virStorageSourceClear(virStorageSourcePtr def)
VIR_FREE(def->path);
VIR_FREE(def->volume);
+VIR_FREE(def->ns);
VIR_FREE(def->snapshot);
VIR_FREE(def->configFile);
VIR_FREE(def->query);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 87763cf389..8a6ddef8bf 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -284,6 +284,7 @@ struct _virStorageSource {
char *snapshot; /* for storage systems supporting internal snapshots */
char *configFile; /* some storage systems use config file as part of
the source definition */
+char *ns; /* for the storage systems supporting namespace */
char *query; /* query string for HTTP based protocols */
size_t nhosts;
virStorageNetHostDefPtr hosts;
--
2.28.0
[PATCH v7 0/4] qemu: Support rbd namespace in rbd source name
Diff from v6: rebase to 6.9.0 v6: https://www.redhat.com/archives/libvir-list/2020-October/msg00707.html gitlab repo: https://gitlab.com/hhan2/libvirt/-/commits/rbd-namespace-v7 Han Han (4): qemu_capabilities: Add QEMU_CAPS_RBD_NAMESPACE util: Add member ns to the storage source struct conf: Support to parse rbd namespace from source name qemu: Implement rbd namespace to the source name attribute docs/formatdomain.rst | 16 ++ src/conf/domain_conf.c| 47 -- src/qemu/qemu_block.c | 1 + src/qemu/qemu_capabilities.c | 4 ++ src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_domain.c| 8 +++ src/util/virstoragefile.c | 2 + src/util/virstoragefile.h | 1 + .../caps_5.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml| 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../caps_5.2.0.x86_64.xml | 1 + ...k-network-rbd-namespace.x86_64-latest.args | 49 +++ .../disk-network-rbd-namespace.xml| 40 +++ tests/qemuxml2argvtest.c | 1 + ...sk-network-rbd-namespace.x86_64-latest.xml | 49 +++ tests/qemuxml2xmltest.c | 1 + 19 files changed, 224 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-namespace.xml create mode 100644 tests/qemuxml2xmloutdata/disk-network-rbd-namespace.x86_64-latest.xml -- 2.28.0
[PATCH v7 1/4] qemu_capabilities: Add QEMU_CAPS_RBD_NAMESPACE
The capability flag will be used for rbd namespace option. The rbd namespace
is introduced since ceph Nautilus and qemu v5.0.0.
Signed-off-by: Han Han
---
src/qemu/qemu_capabilities.c | 4
src/qemu/qemu_capabilities.h | 3 +++
tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
8 files changed, 13 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 212ddf9206..b625463522 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -604,6 +604,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
"block-export-add",
"netdev.vhost-vdpa",
"fsdev.createmode",
+
+ /* 385 */
+ "rbd.namespace",
);
@@ -1538,6 +1541,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsQMPSchemaQueries[] = {
{ "migrate-set-parameters/arg-type/xbzrle-cache-size",
QEMU_CAPS_MIGRATION_PARAM_XBZRLE_CACHE_SIZE },
{ "set-numa-node/arg-type/+hmat-lb", QEMU_CAPS_NUMA_HMAT },
{ "netdev_add/arg-type/+vhost-vdpa", QEMU_CAPS_NETDEV_VHOST_VDPA },
+{ "blockdev-add/arg-type/+rbd/namespace", QEMU_CAPS_RBD_NAMESPACE },
};
typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 6b40ff4f15..22037f5651 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -585,6 +585,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_NETDEV_VHOST_VDPA, /* -netdev vhost-vdpa*/
QEMU_CAPS_FSDEV_CREATEMODE, /* fsdev.createmode */
+/* 385 */
+QEMU_CAPS_RBD_NAMESPACE, /* -blockdev
'{"driver":"rbd",...,"namespace":str}' */
+
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
index 8e4e888cd4..319dbec0da 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
@@ -201,6 +201,7 @@
+
500
0
61700241
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
index e0519fe7eb..1bc131a42d 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
@@ -210,6 +210,7 @@
+
500
0
42900241
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
index c9f7a24282..e7623f792b 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
@@ -197,6 +197,7 @@
+
500
0
0
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
index c897bd3c63..5e12bf73d4 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
@@ -245,6 +245,7 @@
+
500
0
43100241
diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
index 8dd48812b9..165db60f96 100644
--- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
@@ -247,6 +247,7 @@
+
5001000
0
43100242
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
index 94a1945639..167b9bff42 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
@@ -249,6 +249,7 @@
+
5001050
0
43100243
--
2.28.0
Re: [PATCH 1/8] Added a few attach-disk parameters
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
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(, "
[PATCH 6/8] Bump - rerun CI HTTP 500 err
Signed-off-by: Ryan Gahagan --- tools/virsh-domain.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 9d00cb8b21..ae9a2b1101 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -711,7 +711,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virBufferAsprintf(, " name='%s' port='%s'", host_name, host_port + 1); } } - if (host_transport) virBufferAsprintf(, " transport='%s'", host_transport); if (host_socket) -- 2.29.0
[PATCH 3/8] Fixed code style bug for virBufferAddLit
Signed-off-by: Ryan Gahagan
---
tools/virsh-domain.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 810e55fa53..5862993464 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -688,7 +688,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
if (source || source_protocol || source_name ||
host_name || host_transport || host_socket) {
-virBufferAsprintf(, "\n\n\n\n");
+virBufferAddLit(, " />\n\n");
} else {
-virBufferAsprintf(" />\n");
+virBufferAddLit(, " />\n");
}
}
--
2.29.0
[PATCH 2/8] Fixed compile error where was missing
Signed-off-by: Ryan Gahagan
---
tools/virsh-domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 16227085cc..810e55fa53 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -707,7 +707,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
if (!host_port)
virBufferAsprintf(" name='%s'", host_name);
else {
-host_name[host_port - host_name] = '\0';
+host_name[(int)(host_port - host_name)] = '\0';
virBufferAsprintf(" name='%s' port='%s'", host_name,
host_port + 1);
}
}
--
2.29.0
[PATCH 4/8] Fixed error of writing to ROM in host_name
Signed-off-by: Ryan Gahagan --- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5862993464..cda0531a37 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -587,8 +587,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) *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; +*host_transport = NULL, *host_port = NULL, *host_socket = NULL; struct DiskAddress diskAddr; bool isFile = false, functionReturn = false; int ret; @@ -596,6 +595,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) const char *stype = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; char *xml = NULL; +char *host_name = NULL; struct stat st; bool current = vshCommandOptBool(cmd, "current"); bool config = vshCommandOptBool(cmd, "config"); -- 2.29.0
[PATCH 8/8] Added new params to attach-disk docs
Signed-off-by: Ryan Gahagan --- docs/manpages/virsh.rst | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index bfd26e3120..83134ba571 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 -- 2.29.0
[PATCH 7/8] Re-wrote attach-disk to support multiple hosts
Signed-off-by: Ryan Gahagan
---
tools/virsh-domain.c | 70 ++--
1 file changed, 55 insertions(+), 15 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index ae9a2b1101..64c7c454bd 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -578,6 +578,49 @@ static int str2DiskAddress(const char *str, struct
DiskAddress *diskAddr)
return -1;
}
+static void attachDiskHostGen(virBufferPtr buf, const vshCmd *cmd)
+{
+// Can be multiple hosts so we have to scan
+// the cmd options to find all the host params
+// opts;
+char *host_name = NULL, *host_port = NULL;
+int closeTag = 0;
+
+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 \n");
+else
+closeTag = 1;
+
+host_name = candidate->data;
+host_port = strchr(host_name, ':');
+
+if (!host_port) {
+// If port isn't provided, only print name
+virBufferAsprintf(buf, "def->name, "source-host-socket")) {
+virBufferAsprintf(buf, " socket='%s'", candidate->data);
+} else if (STREQ(candidate->def->name, "source-host-transport")) {
+virBufferAsprintf(buf, " transport='%s'", candidate->data);
+}
+
+candidate = candidate->next;
+}
+// Close final \n");
+}
+
static bool
cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
{
@@ -587,7 +630,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
*mode = NULL, *iothread = NULL, *cache = NULL,
*io = NULL, *serial = NULL, *straddr = NULL,
*wwn = NULL, *targetbus = NULL, *alias = NULL,
-*host_transport = NULL, *host_port = NULL, *host_socket = NULL;
+*host_transport = NULL, *host_name = NULL, *host_socket = NULL;
struct DiskAddress diskAddr;
bool isFile = false, functionReturn = false;
int ret;
@@ -595,7 +638,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
const char *stype = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
char *xml = NULL;
-char *host_name = NULL;
struct stat st;
bool current = vshCommandOptBool(cmd, "current");
bool config = vshCommandOptBool(cmd, "config");
@@ -698,27 +740,25 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
if (source_name)
virBufferAsprintf(, " name='%s'", source_name);
-if (host_name || host_transport || host_socket) {
+if (!(host_name || host_transport || host_socket)) {
+// Close source if no host is provided
+virBufferAddLit(, "/>\n");
+} else if (!host_name) {
+// If no host name is provided but there is a host,
+// we have a single host with params
virBufferAddLit(, ">\n\n\n");
+virBufferAddLit(, "/>\n\n");
} else {
-virBufferAddLit(, " />\n");
+// May have multiple hosts, use helper method
+virBufferAddLit(, ">\n");
+attachDiskHostGen(, cmd);
+virBufferAddLit(, "\n");
}
}
--
2.29.0
[PATCH 5/8] Fixed error where char** not being promoted to const
Signed-off-by: Ryan Gahagan
---
tools/virsh-domain.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cda0531a37..9d00cb8b21 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -227,7 +227,7 @@ static const vshCmdOptDef opts_attach_disk[] = {
{.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")
@@ -629,7 +629,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0 ||
-vshCommandOptStringReq(ctl, cmd, "source-host-name", _name) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "source-host-name", (const char **)
_name) < 0 ||
vshCommandOptStringReq(ctl, cmd, "source-host-transport",
_transport) < 0 ||
vshCommandOptStringReq(ctl, cmd, "source-host-socket", _socket) <
0)
goto cleanup;
--
2.29.0
Re: [PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
On Tue, Nov 10, 2020 at 06:45:34PM +0100, Michal Privoznik wrote:
> On 11/10/20 6:25 PM, Masayoshi Mizuma wrote:
> > On Tue, Nov 10, 2020 at 10:10:16AM -0300, Daniel Henrique Barboza wrote:
> > >
> > >
> > > On 11/10/20 2:04 AM, Masayoshi Mizuma wrote:
> > > > From: Masayoshi Mizuma
> > > >
> > > > A qemu guest which has virtiofs config fails to start if the previous
> > > > starting failed because of invalid option or something.
> > > > For example of the reproduction:
> > > >
> > > > # virsh start guest
> > > > error: Failed to start domain guest
> > > > error: internal error: process exited while connecting to monitor:
> > > > qemu-system-x86_64: -foo: invalid option
> > > >
> > > > ... fix the option ...
> > > >
> > > > # virsh start guest
> > > > error: Failed to start domain guest
> > > > error: Cannot open log file:
> > > > '/var/log/libvirt/qemu/guest-fs0-virtiofsd.log': Device or resource busy
> > > > #
> > > >
> > > > That's because the virtiofsd which was executed on the former staring
> > > > remains
> > >
> > >
> > > s/staring/start ?
> > >
> > >
> > > > and virtlogd keeps to opening the log file.
> > > >
> > > > Stop virtiofsd when the qemu guest fails to start.
> > > >
> > > > Signed-off-by: Masayoshi Mizuma
> > > > ---
> > > >src/qemu/qemu_domain.h | 1 +
> > > >src/qemu/qemu_virtiofs.c | 5 +
> > > >2 files changed, 6 insertions(+)
> > > >
> > > > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> > > > index ca041e207b..7d47c030bd 100644
> > > > --- a/src/qemu/qemu_domain.h
> > > > +++ b/src/qemu/qemu_domain.h
> > > > @@ -426,6 +426,7 @@ struct _qemuDomainFSPrivate {
> > > >virObject parent;
> > > >char *vhostuser_fs_sock;
> > > > +pid_t virtiofsd_pid;
> > > >};
> > > > diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
> > > > index 2e239cad66..8684219915 100644
> > > > --- a/src/qemu/qemu_virtiofs.c
> > > > +++ b/src/qemu/qemu_virtiofs.c
> > > > @@ -250,6 +250,7 @@ qemuVirtioFSStart(virLogManagerPtr logManager,
> > > >}
> > > >QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock =
> > > > g_steal_pointer(_path);
> > > > +QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid = pid;
> > > >ret = 0;
> > > > cleanup:
> > > > @@ -273,6 +274,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver
> > > > G_GNUC_UNUSED,
> > > >{
> > > >g_autofree char *pidfile = NULL;
> > > >virErrorPtr orig_err;
> > > > +pid_t pid = QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid;
> > > >virErrorPreserveLast(_err);
> > > > @@ -286,6 +288,9 @@ qemuVirtioFSStop(virQEMUDriverPtr driver
> > > > G_GNUC_UNUSED,
> > > >unlink(QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock);
> > > >}
> > > > +if (virProcessKill(pid, 0) == 0)
> > > > +virProcessKillPainfully(pid, true);
> > > > +
> > >
> > >
> > > If you're adamant into killing 'pid' I suggest to just verify "if (pid)"
> > > and
> > > then execute virProcessKillPainfully(), like is being done in
> > > virPidFileForceCleanupPath()
> > > that is called right before.
> > >
> > > Speaking of which, isn't virPidFileForceCleanupPath() responsible of
> > > killing
> > > virtiofsd? If that's the case, the function is failing to do so in the
> > > scenario
> > > you described, but it will succeed in all other cases. The result is that
> > > you'll
> > > end up trying to kill the process twice.
> > >
> > > Making a guess about what might be happening, if 'pidfile' does not exist
> > > in your error
> > > scenario then virPidFileForceCleanupPath() is returning 0 but is doing
> > > nothing. This is
> > > the case where your code might come in and kill the pid manually.
> >
> > Thank you pointing it out! Your guess is correct. The pid file was removed
> > by
> > virFileDeleteTree(priv->libDir) in qemuProcessStop(), so virtiofsd isn't
> > killed
> > by virPidFileForceCleanupPath().
> >
> > I think we can fix the error to move virFileDeleteTree(priv->libDir) after
> > calling qemuExtDevicesStop(driver, vm).
> > Does the following make sense?
> >
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 0a36b49c85..fc6eb5ad13 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -7638,7 +7638,6 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> > /* Do this before we delete the tree and remove pidfile. */
> > qemuProcessKillManagedPRDaemon(vm);
> > -virFileDeleteTree(priv->libDir);
> > virFileDeleteTree(priv->channelTargetDir);
> > ignore_value(virDomainChrDefForeach(vm->def,
> > @@ -7655,6 +7654,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> > qemuDomainCleanupRun(driver, vm);
> > qemuExtDevicesStop(driver, vm);
> > +virFileDeleteTree(priv->libDir);
> > qemuDBusStop(driver, vm);
> > --
> >
>
> This looks better, but how about moving qemuExtDevicesStop() right below
>
Re: [PATCH v4 1/7] migration/dirtyrate: Introduce virDomainDirtyRateInfo structure
On 11/7/20 10:41 AM, Hao Wang wrote:
Introduce virDomainDirtyRateInfo structure used for domain's memory dirty rate
query.
Signed-off-by: Hao Wang
Reviewed-by: Chuan Zheng
---
include/libvirt/libvirt-domain.h | 24
1 file changed, 24 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index b3310729bf..145f517068 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -5096,4 +5096,28 @@ int virDomainBackupBegin(virDomainPtr domain,
char *virDomainBackupGetXMLDesc(virDomainPtr domain,
unsigned int flags);
+/**
+ * virDomainDirtyRateInfo:
+ *
+ * a virDomainDirtyRateInfo is a structure filled by virDomainGetDirtyRate()
and
+ * extracting dirty rate infomation for a given active Domain.
+ */
+
+typedef struct _virDomainDirtyRateInfo virDomainDirtyRateInfo;
+
+struct _virDomainDirtyRateInfo {
+int status; /* the status of dirtyrate calculation */
+long long dirtyRate;/* the dirtyrate in MB/s */
+long long startTime;/* the start time of dirtyrate calculation */
+long long calcTime; /* the period of dirtyrate calculation */
+};
+
+/**
+ * virDomainDirtyRateInfoPtr:
+ *
+ * a virDomainDirtyRateInfoPtr is a pointer to a virDomainDirtyRateInfo
structure.
+ */
+
+typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr;
+
#endif /* LIBVIRT_DOMAIN_H */
This should be squashed with 2/7 and bits of 5/7 and 6/7 which modify
this file. The key here is that public API should go into one patch,
driver implementation into other - it's easier to backport patches this way.
Michal
Re: [PATCH v4 2/7] migration/dirtyrate: set up framwork of domainGetDirtyRateInfo API
On 11/7/20 10:41 AM, Hao Wang wrote:
Introduce DomainGetDirtyRateInfo API for domain's memory dirty rate
calculation and query.
Signed-off-by: Hao Wang
Signed-off-by: Zhou Yimin
Reviewed-by: Chuan Zheng
---
include/libvirt/libvirt-domain.h | 5
src/driver-hypervisor.h | 7 +
src/libvirt-domain.c | 46
src/libvirt_public.syms | 5
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 21 ++-
6 files changed, 84 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 145f517068..b950736b67 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -5120,4 +5120,9 @@ struct _virDomainDirtyRateInfo {
typedef virDomainDirtyRateInfo *virDomainDirtyRateInfoPtr;
+int virDomainGetDirtyRateInfo(virDomainPtr domain,
+ virDomainDirtyRateInfoPtr info,
+ long long sec,
+ unsigned int flags);
+
#endif /* LIBVIRT_DOMAIN_H */
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index bce023017d..a77c29de54 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1387,6 +1387,12 @@ typedef char *
(*virDrvDomainBackupGetXMLDesc)(virDomainPtr domain,
unsigned int flags);
+typedef int
+(*virDrvDomainGetDirtyRateInfo)(virDomainPtr domain,
+virDomainDirtyRateInfoPtr info,
+long long sec,
+unsigned int flags);
+
typedef struct _virHypervisorDriver virHypervisorDriver;
typedef virHypervisorDriver *virHypervisorDriverPtr;
@@ -1650,4 +1656,5 @@ struct _virHypervisorDriver {
virDrvDomainAgentSetResponseTimeout domainAgentSetResponseTimeout;
virDrvDomainBackupBegin domainBackupBegin;
virDrvDomainBackupGetXMLDesc domainBackupGetXMLDesc;
+virDrvDomainGetDirtyRateInfo domainGetDirtyRateInfo;
};
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 3c5f55176a..ce3c40edf8 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -12758,3 +12758,49 @@ virDomainBackupGetXMLDesc(virDomainPtr domain,
virDispatchError(conn);
return NULL;
}
+
+
+/**
+ * virDomainGetDirtyRateInfo:
+ * @domain: a domain object.
+ * @info: return value of current domain's memory dirty rate info.
+ * @sec: show dirty rate within specified seconds.
+ * @flags: the flags of getdirtyrate action -- calculate and/or query.
What are the flags? Which enum should I look at?
+ *
+ * Get the current domain's memory dirty rate (in MB/s).
Can you expand on this a bit? Look at description to other APIs for
inspiration.
+ *
+ * Returns 0 in case of success, -1 otherwise.
+ */
+int
+virDomainGetDirtyRateInfo(virDomainPtr domain,
+ virDomainDirtyRateInfoPtr info,
+ long long sec,
Do we really need long long? That's more than 2.9*10^11 years. I don't
think any virtual machine will ever run for so long (considering that
the age of the universe is ~1.38*10^10 years)
+ unsigned int flags)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec);
@flags should also be logged.
+
+virResetLastError();
+
+virCheckDomainReturn(domain, -1);
+conn = domain->conn;
+
+virCheckNonNullArgGoto(info, error);
+virCheckReadOnlyGoto(conn->flags, error);
+
+if (info)
+memset(info, 0, sizeof(*info));
@info was checked for NULL just two lines above.
+
+if (conn->driver->domainGetDirtyRateInfo) {
+if (conn->driver->domainGetDirtyRateInfo(domain, info, sec, flags) < 0)
+goto error;
+VIR_DOMAIN_DEBUG(domain, "info = %p, seconds=%lld", info, sec);
This is not very useful debug log.
+return 0;
+}
+
+virReportUnsupportedError();
+ error:
+virDispatchError(conn);
+return -1;
+}
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 539d2e3943..11864f48b1 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -873,4 +873,9 @@ LIBVIRT_6.0.0 {
virDomainBackupGetXMLDesc;
} LIBVIRT_5.10.0;
+LIBVIRT_6.9.0 {
+global:
+virDomainGetDirtyRateInfo;
+} LIBVIRT_6.0.0;
+
Unfortunately, this missed 6.9.0 so this must be 6.10.0 instead.
# define new API here using predicted next version number
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 9cd2fd36ae..325968a22b 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8458,6 +8458,7 @@ static virHypervisorDriver hypervisor_driver = {
.domainAgentSetResponseTimeout = remoteDomainAgentSetResponseTimeout, /*
5.10.0 */
.domainBackupBegin = remoteDomainBackupBegin, /* 6.0.0 */
Re: [PATCH v4 7/7] migration/dirtyrate: Introduce getdirtyrate virsh api
On 11/7/20 10:41 AM, Hao Wang wrote:
Signed-off-by: Hao Wang
Signed-off-by: Zhou Yimin
Reviewed-by: Chuan Zheng
---
tools/virsh-domain.c | 112 +++
1 file changed, 112 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index ef347585e8..52608e2c1b 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -14374,6 +14374,112 @@ cmdGuestInfo(vshControl *ctl, const vshCmd *cmd)
return ret;
}
+/*
+ * "querydirtyrate" command
+ */
+static const vshCmdInfo info_getdirtyrate[] = {
+{.name = "help",
+ .data = N_("Get a vm's memory dirty rate")
+},
+{.name = "desc",
+ .data = N_("Get memory dirty rate of a domain in order to decide"
+" whether it's proper to be migrated out or not.")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_getdirtyrate[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL(0),
+{.name = "seconds",
+ .type = VSH_OT_INT,
+ .help = N_("calculate memory dirty rate within specified seconds,"
+" a valid range of values is [1, 60], and would default to
1s.")
+},
+{.name = "calculate",
+ .type = VSH_OT_BOOL,
+ .help = N_("calculate dirty rate only, can be used together with --query,"
+" either or both is expected, otherwise would default to
both.")
+},
+{.name = "query",
+ .type = VSH_OT_BOOL,
+ .help = N_("query dirty rate only, can be used together with --calculate,"
+" either or both is expected, otherwise would default to
both.")
+},
+{.name = NULL}
+};
+
+static bool
+cmdGetDirtyRateInfo(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+virDomainDirtyRateInfo info;
+long long sec = 0;
+const char *status = NULL;
+unsigned int flags = 0;
+int rc;
+bool ret = false;
+bool calc = vshCommandOptBool(cmd, "calculate");
+bool query = vshCommandOptBool(cmd, "query");
+
+if (calc)
+flags |= VIR_DOMAIN_DIRTYRATE_CALC;
+if (query)
+flags |= VIR_DOMAIN_DIRTYRATE_QUERY;
+
+/* if flag option is missing, default to both --calculate and --query */
+if (!calc && !query)
+flags |= VIR_DOMAIN_DIRTYRATE_CALC | VIR_DOMAIN_DIRTYRATE_QUERY;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+rc = vshCommandOptLongLong(ctl, cmd, "seconds", );
+if (rc < 0)
+goto done;
+
+/* if --seconds option is missing, default to 1s */
+if (!rc)
+sec = 1;
+
+if (virDomainGetDirtyRateInfo(dom, , sec, flags) < 0) {
+vshError(ctl, "%s", _("Get memory dirty-rate failed."));
This is redundant. virsh prints error reported by a public API on
failure. All that's needed is for this function to return false.
+goto done;
+}
+
+if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) {
+switch (info.status) {
+case VIR_DOMAIN_DIRTYRATE_UNSTARTED:
+status = _("unstarted");
+break;
+case VIR_DOMAIN_DIRTYRATE_MEASURING:
+status = _("measuring");
+break;
+case VIR_DOMAIN_DIRTYRATE_MEASURED:
+status = _("measured");
+break;
+default:
+status = _("unknown");
+}
+
+vshPrint(ctl, _("status:%s\n"), status);
+vshPrint(ctl, _("start time:%lld\n"), info.startTime);
+vshPrint(ctl, _("calc time: %lld s\n"), info.calcTime);
How about using vshTable so that you don't have to align this by hand?
+
+if (info.status == VIR_DOMAIN_DIRTYRATE_MEASURED)
+vshPrint(ctl, _("dirty rate:%lld MB/s\n"), info.dirtyRate);
+else
+vshPrint(ctl, _("dirty rate:the calculation is %s, please query
results later\n"),
+ status);
+} else {
+vshPrint(ctl, _("Memory dirty rate is calculating, use --query option to
display results.\n"));
+}
+
+ret = true;
+ done:
+virshDomainFree(dom);
+return ret;
+}
+
const vshCmdDef domManagementCmds[] = {
{.name = "attach-device",
.handler = cmdAttachDevice,
@@ -15001,5 +15107,11 @@ const vshCmdDef domManagementCmds[] = {
.info = info_guestinfo,
.flags = 0
},
+{.name = "getdirtyrate",
+ .handler = cmdGetDirtyRateInfo,
+ .opts = opts_getdirtyrate,
+ .info = info_getdirtyrate,
+ .flags = 0
+},
{.name = NULL}
};
Missing manpage: it lives under docs/manpages/virsh.rst
Michal
Re: [PATCH v4 6/7] migration/dirtyrate: Implement qemuDomainGetDirtyRateInfo
On 11/7/20 10:41 AM, Hao Wang wrote:
Implement qemuDomainGetDirtyRateInfo:
using flags to control behaviors -- calculate and/or query dirtyrate.
Signed-off-by: Hao Wang
Reviewed-by: Chuan Zheng
---
include/libvirt/libvirt-domain.h | 11 ++
src/qemu/qemu_driver.c | 68
2 files changed, 79 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 51d8685086..fc45f42dcf 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -5096,6 +5096,17 @@ int virDomainBackupBegin(virDomainPtr domain,
char *virDomainBackupGetXMLDesc(virDomainPtr domain,
unsigned int flags);
+/**
+ * virDomainDirtyRateFlags:
+ *
+ * Details on the flags used by getdirtyrate api.
+ */
+
+typedef enum {
+VIR_DOMAIN_DIRTYRATE_CALC = 1 << 0, /* calculate domain's dirtyrate */
+VIR_DOMAIN_DIRTYRATE_QUERY = 1 << 1, /* query domain's dirtyrate */
+} virDomainDirtyRateFlags;
+
/**
* virDomainDirtyRateStatus:
*
Again, doesn't belong here.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cb56fbbfcf..93d5a23630 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20121,6 +20121,73 @@ qemuDomainAgentSetResponseTimeout(virDomainPtr dom,
}
+#define MIN_DIRTYRATE_CALCULATION_PERIOD1 /* supported min dirtyrate calc time: 1s */
+#define MAX_DIRTYRATE_CALCULATION_PERIOD60 /* supported max dirtyrate calc
time: 60s */
+
+static int
+qemuDomainGetDirtyRateInfo(virDomainPtr dom,
+ virDomainDirtyRateInfoPtr info,
+ long long sec,
+ unsigned int flags)
+{
+virDomainObjPtr vm = NULL;
+virQEMUDriverPtr driver = dom->conn->privateData;
+int ret = -1;
+
+if (!(vm = qemuDomainObjFromDomain(dom)))
+return ret;
+
+if (virDomainGetDirtyRateInfoEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+goto cleanup;
+
+if (!qemuMigrationSrcIsAllowed(driver, vm, false, 0))
+goto endjob;
+
Why is this check needed? I don't understand it, can you please explain?
+if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
+if (sec < MIN_DIRTYRATE_CALCULATION_PERIOD || sec >
MAX_DIRTYRATE_CALCULATION_PERIOD) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("seconds=%lld is invalid, please choose value within
[1, 60]."),
+ sec);
+goto endjob;
+}
+
+if (qemuDomainCalculateDirtyRate(dom, vm, sec) < 0) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("can't calculate domain's dirty rate"));
This overwrites a more accurate error reported by
qemuDomainCalculateDirtyRate().
+goto endjob;
+}
+}
+
+if (flags & VIR_DOMAIN_DIRTYRATE_QUERY) {
+if (flags & VIR_DOMAIN_DIRTYRATE_CALC) {
+struct timespec ts = { .tv_sec = sec, .tv_nsec = 50 * 1000 *
1000ull };
+
+virObjectUnlock(vm);
+nanosleep(, NULL);
+virObjectLock(vm);
At first I was afraid, I was petrified that this waits for 50 seconds,
until I realized it's nanosleep(). Perhaps g_usleep(50*1000); would look
better?
+}
+
+if (qemuDomainQueryDirtyRate(dom, vm, info) < 0) {
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("can't query domain's dirty rate"));
Again, error overwrite.
+goto endjob;
+}
+}
So if no flag is specified then nothing happens? I know you handle that
in virsh, but I think that logic should live here. And perhaps the
public API description should document that no flags is equal to
specifying both flags together.
+
+ret = 0;
+
+ endjob:
+qemuDomainObjEndJob(driver, vm);
+
+ cleanup:
+virDomainObjEndAPI();
+return ret;
+}
+
+
static virHypervisorDriver qemuHypervisorDriver = {
.name = QEMU_DRIVER_NAME,
.connectURIProbe = qemuConnectURIProbe,
@@ -20360,6 +20427,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainAgentSetResponseTimeout = qemuDomainAgentSetResponseTimeout, /*
5.10.0 */
.domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */
.domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */
+.domainGetDirtyRateInfo = qemuDomainGetDirtyRateInfo, /* 6.9.0 */
6.10.0 :-(
};
I think the following should be squashed in:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4cbbe8dd84..4058fb487c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20130,12 +20130,12 @@ qemuDomainGetDirtyRateInfo(virDomainPtr dom,
long long sec,
unsigned int flags)
{
-virDomainObjPtr vm = NULL;
Re: [PATCH v4 3/7] migration/dirtyrate: Implement qemuDomainCalculateDirtyRate
On 11/7/20 10:41 AM, Hao Wang wrote:
Implement qemuDomainCalculateDirtyRate which calculates domain's memory
dirty rate calling qmp "calc-dirty-rate".
Signed-off-by: Hao Wang
Signed-off-by: Zhou Yimin
Reviewed-by: Chuan Zheng
---
src/qemu/qemu_migration.c| 28
src/qemu/qemu_migration.h| 5 +
src/qemu/qemu_monitor.c | 12
src/qemu/qemu_monitor.h | 4
src/qemu/qemu_monitor_json.c | 22 ++
src/qemu/qemu_monitor_json.h | 4
6 files changed, 75 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 6f764b0c73..8029e24415 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5836,3 +5836,31 @@ qemuMigrationSrcFetchMirrorStats(virQEMUDriverPtr driver,
virHashFree(blockinfo);
return 0;
}
+
+
+int
+qemuDomainCalculateDirtyRate(virDomainPtr dom,
The caller (implemented later in the series) has pointer to the driver.
Might as well pass it here. Alternativelly, there is a piggy back
pointer stored inside @vm's private data: QEMU_DOMAIN_PRIVATE(vm)->driver
+ virDomainObjPtr vm,
+ long long sec)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+qemuDomainObjPrivatePtr priv;
+int ret = -1;
+
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+return ret;
+}
+
+priv = vm->privateData;
It's okay to initialize priv when declaring it.
+
+VIR_DEBUG("Calculate dirty rate during %lld seconds", sec);
+qemuDomainObjEnterMonitor(driver, vm);
+
+ret = qemuMonitorCalculateDirtyRate(priv->mon, sec);
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ret = -1;
+
+return ret;
+}
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index fd9eb7cab0..0522b375c0 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -258,3 +258,8 @@ qemuMigrationSrcFetchMirrorStats(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuDomainAsyncJob asyncJob,
qemuDomainJobInfoPtr jobInfo);
+
+int
+qemuDomainCalculateDirtyRate(virDomainPtr dom,
+ virDomainObjPtr vm,
+ long long sec);
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 5481bd99a0..06603b8691 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4769,3 +4769,15 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions,
return qemuMonitorJSONTransactionBackup(actions, device, jobname, target,
bitmap, syncmode);
}
+
+
+int
+qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon,
+ long long sec)
+{
+VIR_DEBUG("seconds=%lld", sec);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONCalculateDirtyRate(mon, sec);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 54fbb41ef7..afb97780cf 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1527,3 +1527,7 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions,
const char *target,
const char *bitmap,
qemuMonitorTransactionBackupSyncMode syncmode);
+
+int
+qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon,
+ long long sec);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 843a555952..65691522fb 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -9635,3 +9635,25 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon,
return virJSONValueGetBoolean(virJSONValueObjectGet(reply, "return"),
migratable);
}
+
+
+int
+qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon,
+ long long sec)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+
+if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate",
+ "I:calc-time", (long)sec,
I'm not convinced on this typecast. qemuMonitorJSONMakeCommand() will
under the hood pop long long. But anyway, @sec shouldn't be long long in
the first place.
+ NULL)))
+return -1;
+
+if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
+return -1;
+
+if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+return -1;
+
+return 0;
+}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 38e23ef3c5..c9556fc19a 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -710,3 +710,7 @@ int qemuMonitorJSONSetDBusVMStateIdList(qemuMonitorPtr mon,
int
Re: [PATCH v4 4/7] migration/dirtyrate: Implement qemuDomainQueryDirtyRate
On 11/7/20 10:41 AM, Hao Wang wrote:
Implement qemuDomainQueryDirtyRate which query domain's memory
dirty rate calling qmp "query-dirty-rate".
Signed-off-by: Hao Wang
Signed-off-by: Zhou Yimin
Reviewed-by: Chuan Zheng
---
src/qemu/qemu_migration.c| 31 +++
src/qemu/qemu_migration.h| 5 +
src/qemu/qemu_monitor.c | 12
src/qemu/qemu_monitor.h | 4
src/qemu/qemu_monitor_json.c | 22 ++
src/qemu/qemu_monitor_json.h | 4
6 files changed, 78 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 8029e24415..3d07ba3ac4 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5864,3 +5864,34 @@ qemuDomainCalculateDirtyRate(virDomainPtr dom,
return ret;
}
+
+
+int
+qemuDomainQueryDirtyRate(virDomainPtr dom,
+ virDomainObjPtr vm,
+ virDomainDirtyRateInfoPtr info)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
Again, driver is accessible from the caller, just pass it directly.
+qemuDomainObjPrivatePtr priv;
+int ret = -1;
+
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+return ret;
+}
+
+priv = vm->privateData;
+
+qemuDomainObjEnterMonitor(driver, vm);
+
+ret = qemuMonitorQueryDirtyRate(priv->mon, info);
+if (ret < 0) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+ "%s", _("get vm's dirty rate failed."));
No, this is not correct, qemuMonitorQueryDirtyRate() will report an
error if something goes wrong. And with pretty accurate error message.
This overwrites it with more generic one.
+}
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ret = -1;
+
+return ret;
+}
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index 0522b375c0..8baae512b7 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -263,3 +263,8 @@ int
qemuDomainCalculateDirtyRate(virDomainPtr dom,
virDomainObjPtr vm,
long long sec);
+
+int
+qemuDomainQueryDirtyRate(virDomainPtr dom,
+ virDomainObjPtr vm,
+ virDomainDirtyRateInfoPtr info);
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 06603b8691..2fa6879467 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4781,3 +4781,15 @@ qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon,
return qemuMonitorJSONCalculateDirtyRate(mon, sec);
}
+
+
+int
+qemuMonitorQueryDirtyRate(qemuMonitorPtr mon,
+ virDomainDirtyRateInfoPtr info)
+{
+VIR_DEBUG("info=%p", info);
+
+QEMU_CHECK_MONITOR(mon);
+
+return qemuMonitorJSONQueryDirtyRate(mon, info);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index afb97780cf..25105c3ad9 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1531,3 +1531,7 @@ qemuMonitorTransactionBackup(virJSONValuePtr actions,
int
qemuMonitorCalculateDirtyRate(qemuMonitorPtr mon,
long long sec);
+
+int
+qemuMonitorQueryDirtyRate(qemuMonitorPtr mon,
+ virDomainDirtyRateInfoPtr info);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 65691522fb..1924c7229b 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -9657,3 +9657,25 @@ qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon,
return 0;
}
+
+
+int
+qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon,
+ virDomainDirtyRateInfoPtr info)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+
+if (!(cmd = qemuMonitorJSONMakeCommand("query-dirty-rate", NULL)))
+return -1;
+
+if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
+return -1;
+
+if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+return -1;
+
+/* TODO: extract dirtyrate data from reply and store in
virDomainDirtyRateInfoPtr */
How about instead of noting down to finish this, squash 5/7 here? It's
not like somebody could backport only this or that patch.
+info->status = 0;
+return 0;
+}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index c9556fc19a..487f2e6e58 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -714,3 +714,7 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitorPtr mon,
int
qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon,
long long sec);
+
+int
+qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon,
+ virDomainDirtyRateInfoPtr info);
I think the following should be squashed in:
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index
Re: [PATCH v4 5/7] migration/dirtyrate: Implement qemuMonitorJSONExtractDirtyRateInfo
On 11/7/20 10:41 AM, Hao Wang wrote:
Implement qemuMonitorJSONExtractDirtyRateInfo to deal with the return from
qmp "query-dirty-rate", and store them in virDomainDirtyRateInfo.
Signed-off-by: Hao Wang
Reviewed-by: Chuan Zheng
---
include/libvirt/libvirt-domain.h | 17 +
src/qemu/qemu_monitor_json.c | 59 ++--
2 files changed, 73 insertions(+), 3 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index b950736b67..51d8685086 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -5096,6 +5096,23 @@ int virDomainBackupBegin(virDomainPtr domain,
char *virDomainBackupGetXMLDesc(virDomainPtr domain,
unsigned int flags);
+/**
+ * virDomainDirtyRateStatus:
+ *
+ * Details on the cause of a dirtyrate calculation status.
+ */
+
+typedef enum {
+VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has not
been started */
+VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is
measuring */
+VIR_DOMAIN_DIRTYRATE_MEASURED = 2, /* the dirtyrate calculation is
+completed */
+
+# ifdef VIR_ENUM_SENTINELS
+VIR_DOMAIN_DIRTYRATE_LAST
+# endif
+} virDomainDirtyRateStatus;
+
As advertised earlier, this doesn't belong into this commit really.
/**
* virDomainDirtyRateInfo:
*
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 1924c7229b..ca7d8d23c0 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -9659,12 +9659,61 @@ qemuMonitorJSONCalculateDirtyRate(qemuMonitorPtr mon,
}
+VIR_ENUM_DECL(qemuDomainDirtyRateStatus);
+VIR_ENUM_IMPL(qemuDomainDirtyRateStatus,
+ VIR_DOMAIN_DIRTYRATE_LAST,
+ "unstarted",
+ "measuring",
+ "measured");
+
+static int
+qemuMonitorJSONExtractDirtyRateInfo(virJSONValuePtr data,
+virDomainDirtyRateInfoPtr info)
+{
+const char *status;
+int statusID;
+
+if (!(status = virJSONValueObjectGetString(data, "status"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("query-dirty-rate reply was missing 'status' data"));
+return -1;
+}
+
+if ((statusID = qemuDomainDirtyRateStatusTypeFromString(status)) < 0) {
+return -1;
So if qemu sends us some other string, this fails silently.
+}
+info->status = statusID;
+
+if ((info->status == VIR_DOMAIN_DIRTYRATE_MEASURED) &&
+(virJSONValueObjectGetNumberLong(data, "dirty-rate", &(info->dirtyRate))
< 0)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("query-dirty-rate reply was missing 'dirty-rate'
data"));
+return -1;
+}
+
+if (virJSONValueObjectGetNumberLong(data, "start-time", &(info->startTime))
< 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("query-dirty-rate reply was missing 'start-time'
data"));
+return -1;
+}
+
+if (virJSONValueObjectGetNumberLong(data, "calc-time", &(info->calcTime))
< 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("query-dirty-rate reply was missing 'calc-time'
data"));
+return -1;
+}
+
+return 0;
+}
+
+
int
qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon,
virDomainDirtyRateInfoPtr info)
{
g_autoptr(virJSONValue) cmd = NULL;
g_autoptr(virJSONValue) reply = NULL;
+virJSONValuePtr data = NULL;
if (!(cmd = qemuMonitorJSONMakeCommand("query-dirty-rate", NULL)))
return -1;
@@ -9675,7 +9724,11 @@ qemuMonitorJSONQueryDirtyRate(qemuMonitorPtr mon,
if (qemuMonitorJSONCheckError(cmd, reply) < 0)
return -1;
-/* TODO: extract dirtyrate data from reply and store in virDomainDirtyRateInfoPtr */
-info->status = 0;
-return 0;
+if (!(data = virJSONValueObjectGetObject(reply, "return"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("query-dirty-rate reply was missing 'return' data"));
+return -1;
+}
+
+return qemuMonitorJSONExtractDirtyRateInfo(data, info);
}
I think the following should be squashed in:
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 78ad10bc24..4715b33254 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -9680,24 +9680,26 @@
qemuMonitorJSONExtractDirtyRateInfo(virJSONValuePtr data,
}
if ((statusID = qemuDomainDirtyRateStatusTypeFromString(status)) <
0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown dirty rate status: %s"), status);
return -1;
}
info->status = statusID;
if ((info->status == VIR_DOMAIN_DIRTYRATE_MEASURED) &&
-(virJSONValueObjectGetNumberLong(data, "dirty-rate",
&(info->dirtyRate)) < 0)) {
+
[PATCH v2 0/2] Support mdev_types on CSS devices
All refactoring patches of this series in v1 were accepted except for actual patch enabling the mdev_types support on CSS devices. v2 adds one more refactoring patch in docs before the already sent enablement patch follows again. Boris Fiuczynski (2): docs: refactor mdev_types into new paragraph node_device: detecting mdev_types capability on CSS devices docs/drvnodedev.html.in | 5 +- docs/formatnode.html.in | 85 +++-- docs/schemas/nodedev.rng | 4 + src/conf/node_device_conf.c | 92 ++- src/conf/node_device_conf.h | 11 +++ src/conf/virnodedeviceobj.c | 7 +- src/libvirt_private.syms | 1 + src/node_device/node_device_udev.c| 3 + .../css_0_0_fffe_mdev_types.xml | 17 tests/nodedevxml2xmltest.c| 1 + 10 files changed, 192 insertions(+), 34 deletions(-) create mode 100644 tests/nodedevschemadata/css_0_0_fffe_mdev_types.xml -- 2.26.2
[PATCH v2 1/2] docs: refactor mdev_types into new paragraph
To prevent copying the mdev_types description multiple times it is refactored into a new paragraph for easy reuse. Signed-off-by: Boris Fiuczynski --- docs/formatnode.html.in | 70 - 1 file changed, 41 insertions(+), 29 deletions(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 6928bdd69c..bd3112c5a8 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -159,35 +159,10 @@ mdev_types -This device is capable of creating mediated devices, and -the capability will contain a list of type -elements, which list all mdev types supported on the -physical device. Since 3.4.0 -Each type element has a single id -attribute that holds an official vendor-supplied identifier -for the type. It supports the following sub-elements: - -name - - The name element holds a vendor-supplied - code name for the given mediated device type. This is - an optional element. - - deviceAPI - -The value of this element describes how an instance of -the given type will be presented to the guest by the -VFIO framework. - - availableInstances - -This element reports the current state of resource -allocation. In other words, how many instances of the -given type can still be successfully created on the -physical device. - - - +This device is capable of creating mediated devices. +The sub-elements are summarized in +mdev_types capability. + @@ -445,6 +420,43 @@ +mdev_types capability + + + PCI devices can be capable of + creating mediated devices. + If they are capable the attribute type of the + element capability is mdev_types. + This capability will contain a list of type + elements, which list all mdev types supported on the + physical device. Since 3.4.0 + Each type element has a single id + attribute that holds an official vendor-supplied identifier + for the type. It supports the following sub-elements: + +name + + The name element holds a vendor-supplied + code name for the given mediated device type. This is + an optional element. + +deviceAPI + + The value of this element describes how an instance of + the given type will be presented to the guest by the + VFIO framework. + +availableInstances + + This element reports the current state of resource + allocation. In other words, how many instances of the + given type can still be successfully created on the + physical device. + + + + + Examples The following are some example node device XML outputs: -- 2.26.2
[PATCH v2 2/2] node_device: detecting mdev_types capability on CSS devices
Add detection of mdev_types capability to channel subsystem devices.
Signed-off-by: Boris Fiuczynski
Reviewed-by: Bjoern Walk
---
docs/drvnodedev.html.in | 5 +-
docs/formatnode.html.in | 19 +++-
docs/schemas/nodedev.rng | 4 +
src/conf/node_device_conf.c | 92 ++-
src/conf/node_device_conf.h | 11 +++
src/conf/virnodedeviceobj.c | 7 +-
src/libvirt_private.syms | 1 +
src/node_device/node_device_udev.c| 3 +
.../css_0_0_fffe_mdev_types.xml | 17
tests/nodedevxml2xmltest.c| 1 +
10 files changed, 153 insertions(+), 7 deletions(-)
create mode 100644 tests/nodedevschemadata/css_0_0_fffe_mdev_types.xml
diff --git a/docs/drvnodedev.html.in b/docs/drvnodedev.html.in
index 0823c1888d..d5191d6d93 100644
--- a/docs/drvnodedev.html.in
+++ b/docs/drvnodedev.html.in
@@ -139,12 +139,13 @@
MDEV capability
- A PCI device capable of creating mediated devices will include a nested
+ A device capable of creating mediated devices will include a nested
capability mdev_types which enumerates all supported mdev
types on the physical device, along with the type attributes available
through sysfs. A detailed description of the XML format for the
mdev_types capability can be found
- here.
+ here for PCI or
+ here for CSS.
The following example shows how we might represent an NVIDIA GPU device
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index bd3112c5a8..d9abba0efa 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -405,6 +405,21 @@
The subchannel-set identifier.
devno
The device number.
+ capability
+
+This optional element can occur multiple times. If it
+exists, it has a mandatory type attribute
+which will be set to:
+
+ mdev_types
+
+Since 6.10.0
+This device is capable of creating mediated devices.
+The sub-elements are summarized in
+mdev_types capability.
+
+
+
vdpa
@@ -423,8 +438,8 @@
mdev_types capability
- PCI devices can be capable of
- creating mediated devices.
+ PCI and CSS
+ devices can be capable of creating mediated devices.
If they are capable the attribute type of the
element capability is mdev_types.
This capability will contain a list of type
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index 231afa0218..b3e986659e 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -654,6 +654,9 @@
+
+
+
@@ -702,6 +705,7 @@
vfio-pci
+ vfio-ccw
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index a57505a27e..4e2837c1cd 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -552,6 +552,10 @@ virNodeDeviceCapCCWDefFormat(virBufferPtr buf,
data->ccw_dev.ssid);
virBufferAsprintf(buf, "0x%04x\n",
data->ccw_dev.devno);
+if (data->ccw_dev.flags & VIR_NODE_DEV_CAP_FLAG_CSS_MDEV)
+virNodeDeviceCapMdevTypesFormat(buf,
+data->ccw_dev.mdev_types,
+data->ccw_dev.nmdev_types);
}
@@ -843,6 +847,33 @@ virNodeDevCapMdevTypesParseXML(xmlXPathContextPtr ctxt,
}
+static int
+virNodeDevCSSCapabilityParseXML(xmlXPathContextPtr ctxt,
+xmlNodePtr node,
+virNodeDevCapCCWPtr ccw_dev)
+{
+g_autofree char *type = virXMLPropString(node, "type");
+VIR_XPATH_NODE_AUTORESTORE(ctxt)
+
+ctxt->node = node;
+
+if (!type) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing capability type"));
+return -1;
+}
+
+if (STREQ(type, "mdev_types")) {
+if (virNodeDevCapMdevTypesParseXML(ctxt,
+ _dev->mdev_types,
+ _dev->nmdev_types) < 0)
+return -1;
+ccw_dev->flags |= VIR_NODE_DEV_CAP_FLAG_CSS_MDEV;
+}
+
+return 0;
+}
+
+
static int
virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt,
virNodeDeviceDefPtr def,
@@ -850,6 +881,9 @@ virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt,
virNodeDevCapCCWPtr ccw_dev)
{
VIR_XPATH_NODE_AUTORESTORE(ctxt)
+g_autofree xmlNodePtr *nodes = NULL;
+int n = 0;
+
Re: [PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
On 11/10/20 6:25 PM, Masayoshi Mizuma wrote:
On Tue, Nov 10, 2020 at 10:10:16AM -0300, Daniel Henrique Barboza wrote:
On 11/10/20 2:04 AM, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma
A qemu guest which has virtiofs config fails to start if the previous
starting failed because of invalid option or something.
For example of the reproduction:
# virsh start guest
error: Failed to start domain guest
error: internal error: process exited while connecting to monitor:
qemu-system-x86_64: -foo: invalid option
... fix the option ...
# virsh start guest
error: Failed to start domain guest
error: Cannot open log file:
'/var/log/libvirt/qemu/guest-fs0-virtiofsd.log': Device or resource busy
#
That's because the virtiofsd which was executed on the former staring remains
s/staring/start ?
and virtlogd keeps to opening the log file.
Stop virtiofsd when the qemu guest fails to start.
Signed-off-by: Masayoshi Mizuma
---
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_virtiofs.c | 5 +
2 files changed, 6 insertions(+)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index ca041e207b..7d47c030bd 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -426,6 +426,7 @@ struct _qemuDomainFSPrivate {
virObject parent;
char *vhostuser_fs_sock;
+pid_t virtiofsd_pid;
};
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 2e239cad66..8684219915 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -250,6 +250,7 @@ qemuVirtioFSStart(virLogManagerPtr logManager,
}
QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock =
g_steal_pointer(_path);
+QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid = pid;
ret = 0;
cleanup:
@@ -273,6 +274,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
{
g_autofree char *pidfile = NULL;
virErrorPtr orig_err;
+pid_t pid = QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid;
virErrorPreserveLast(_err);
@@ -286,6 +288,9 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
unlink(QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock);
}
+if (virProcessKill(pid, 0) == 0)
+virProcessKillPainfully(pid, true);
+
If you're adamant into killing 'pid' I suggest to just verify "if (pid)" and
then execute virProcessKillPainfully(), like is being done in
virPidFileForceCleanupPath()
that is called right before.
Speaking of which, isn't virPidFileForceCleanupPath() responsible of killing
virtiofsd? If that's the case, the function is failing to do so in the scenario
you described, but it will succeed in all other cases. The result is that you'll
end up trying to kill the process twice.
Making a guess about what might be happening, if 'pidfile' does not exist in
your error
scenario then virPidFileForceCleanupPath() is returning 0 but is doing nothing.
This is
the case where your code might come in and kill the pid manually.
Thank you pointing it out! Your guess is correct. The pid file was removed by
virFileDeleteTree(priv->libDir) in qemuProcessStop(), so virtiofsd isn't killed
by virPidFileForceCleanupPath().
I think we can fix the error to move virFileDeleteTree(priv->libDir) after
calling qemuExtDevicesStop(driver, vm).
Does the following make sense?
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0a36b49c85..fc6eb5ad13 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7638,7 +7638,6 @@ void qemuProcessStop(virQEMUDriverPtr driver,
/* Do this before we delete the tree and remove pidfile. */
qemuProcessKillManagedPRDaemon(vm);
-virFileDeleteTree(priv->libDir);
virFileDeleteTree(priv->channelTargetDir);
ignore_value(virDomainChrDefForeach(vm->def,
@@ -7655,6 +7654,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
qemuDomainCleanupRun(driver, vm);
qemuExtDevicesStop(driver, vm);
+virFileDeleteTree(priv->libDir);
qemuDBusStop(driver, vm);
--
This looks better, but how about moving qemuExtDevicesStop() right below
qemuProcessKillManagedPRDaemon()? Both, pr helper and external devices
are supplementary processes and thus alike.
Michal
Re: [libvirt] [PATCH v2 05/13] virsh: Add interface mac completion to iface-name command
On 11/10/20 10:50 AM, Lin Ma wrote:
Signed-off-by: Lin Ma
---
tools/virsh-completer-interface.c | 9 +
tools/virsh-completer-interface.h | 4
tools/virsh-interface.c | 1 +
3 files changed, 14 insertions(+)
diff --git a/tools/virsh-completer-interface.c
b/tools/virsh-completer-interface.c
index 407021485f..dea8d645cd 100644
--- a/tools/virsh-completer-interface.c
+++ b/tools/virsh-completer-interface.c
@@ -69,3 +69,12 @@ virshInterfaceNameCompleter(vshControl *ctl,
{
return virshInterfaceStringHelper(ctl, cmd, flags, virInterfaceGetName);
}
+
+char **
+virshInterfaceMacCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags)
+{
+return virshInterfaceStringHelper(ctl, cmd, flags,
+ virInterfaceGetMACString);
+}
diff --git a/tools/virsh-completer-interface.h
b/tools/virsh-completer-interface.h
index 32da01e766..b48ded6a12 100644
--- a/tools/virsh-completer-interface.h
+++ b/tools/virsh-completer-interface.h
@@ -28,3 +28,7 @@ typedef const char *
char ** virshInterfaceNameCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshInterfaceMacCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
index 8cdbc6e85f..ae2b52bfc3 100644
--- a/tools/virsh-interface.c
+++ b/tools/virsh-interface.c
@@ -404,6 +404,7 @@ static const vshCmdOptDef opts_interface_name[] = {
{.name = "interface",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshInterfaceMacCompleter,
.help = N_("interface mac")
},
{.name = NULL}
Beautiful.
Michal
Re: [libvirt] [PATCH v2 09/13] virsh: Add mac completion to net-dhcp-leases command
On 11/10/20 10:51 AM, Lin Ma wrote:
Signed-off-by: Lin Ma
---
tools/virsh-completer-network.c | 44 +
tools/virsh-completer-network.h | 4 +++
tools/virsh-network.c | 15 +++
3 files changed, 58 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-completer-network.c b/tools/virsh-completer-network.c
index 73f7115ab2..3094b9d5d6 100644
--- a/tools/virsh-completer-network.c
+++ b/tools/virsh-completer-network.c
@@ -174,3 +174,47 @@ virshNetworkUUIDCompleter(vshControl *ctl,
g_free(nets);
return ret;
}
+
+
+char **
+virshNetworkDhcpMacCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+virNetworkDHCPLeasePtr *leases = NULL;
+virNetworkPtr network = NULL;
+int nleases;
+size_t i = 0;
+char **ret = NULL;
+VIR_AUTOSTRINGLIST tmp = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+if (!(network = virshCommandOptNetwork(ctl, cmd, NULL)))
+return NULL;
+
+if ((nleases = virNetworkGetDHCPLeases(network, NULL, , flags)) < 0)
+goto cleanup;
+
+tmp = g_new0(char *, nleases + 1);
+
+for (i = 0; i < nleases; i++) {
+virNetworkDHCPLeasePtr lease = leases[i];
+tmp[i] = g_strdup(lease->mac);
+}
+
+ret = g_steal_pointer();
+
+ cleanup:
+if (leases) {
+for (i = 0; i < nleases; i++)
+virNetworkDHCPLeaseFree(leases[i]);
+VIR_FREE(leases);
+}
+virNetworkFree(network);
+return ret;
+}
diff --git a/tools/virsh-completer-network.h b/tools/virsh-completer-network.h
index 8910e4525c..80df5c468e 100644
--- a/tools/virsh-completer-network.h
+++ b/tools/virsh-completer-network.h
@@ -37,3 +37,7 @@ char ** virshNetworkPortUUIDCompleter(vshControl *ctl,
char ** virshNetworkUUIDCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshNetworkDhcpMacCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index 745afc537d..30b231f7d6 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -61,6 +61,15 @@
.completer_flags = cflags, \
}
+#define VIRSH_COMMON_OPT_NETWORK_DHCP_MAC(_helpstr, cflags) \
+{.name = "mac", \
+ .type = VSH_OT_STRING, \
+ .flags = VSH_OFLAG_NONE, \
+ .help = _helpstr, \
+ .completer = virshNetworkDhcpMacCompleter, \
+ .completer_flags = cflags, \
+}
+
You don't seem to use this macro anywhere else. I think we can just add
.completer ...
virNetworkPtr
virshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd,
@@ -1373,11 +1382,7 @@ static const vshCmdInfo info_network_dhcp_leases[] = {
static const vshCmdOptDef opts_network_dhcp_leases[] = {
VIRSH_COMMON_OPT_NETWORK_FULL(VIR_CONNECT_LIST_NETWORKS_ACTIVE),
-{.name = "mac",
- .type = VSH_OT_STRING,
- .flags = VSH_OFLAG_NONE,
- .help = N_("MAC address")
.. here.
-},
+VIRSH_COMMON_OPT_NETWORK_DHCP_MAC(N_("MAC address"), 0),
{.name = NULL}
};
Michal
Re: [libvirt] [PATCH v2 01/13] virsh: Add vcpu list completion to guestvcpus command
On 11/10/20 10:50 AM, Lin Ma wrote:
Signed-off-by: Lin Ma
---
tools/virsh-completer-domain.c | 75 ++
tools/virsh-completer-domain.h | 4 ++
tools/virsh-domain.c | 1 +
3 files changed, 80 insertions(+)
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
index c657627ac1..f653471b89 100644
--- a/tools/virsh-completer-domain.c
+++ b/tools/virsh-completer-domain.c
@@ -30,6 +30,7 @@
#include "virstring.h"
#include "virxml.h"
#include "virperf.h"
+#include "virbitmap.h"
char **
virshDomainNameCompleter(vshControl *ctl,
@@ -581,3 +582,77 @@ virshDomainCpulistCompleter(vshControl *ctl,
return virshCommaStringListComplete(cpuid, (const char **)cpulist);
}
+
+char **
+virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags)
+{
+virDomainPtr dom;
+bool enable = vshCommandOptBool(cmd, "enable");
+bool disable = vshCommandOptBool(cmd, "disable");
+virTypedParameterPtr params = NULL;
+unsigned int nparams = 0;
+int dummy;
+size_t i;
+size_t offset = 0;
+int nvcpus;
+VIR_AUTOSTRINGLIST cpulist = NULL;
+g_autoptr(virBitmap) vcpus = NULL;
+g_autofree unsigned char *vcpumap = NULL;
+g_autofree char *onlineVcpuStr = NULL;
+const char *vcpuid = NULL;
+char **ret = NULL;
Some of these variables ^^ are used ..
+
+virCheckFlags(0, NULL);
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return NULL;
+
+if (vshCommandOptStringQuiet(ctl, cmd, "cpulist", ) < 0)
+goto cleanup;
+
+/* retrieve vcpu count from the guest instead of the hypervisor */
+if ((nvcpus = virDomainGetVcpusFlags(dom, VIR_DOMAIN_VCPU_GUEST |
+ VIR_DOMAIN_VCPU_MAXIMUM)) < 0)
+goto cleanup;
+
+if (!enable && !disable) {
+cpulist = g_new0(char *, nvcpus + 1);
+for (i = 0; i < nvcpus; i++)
+cpulist[i] = g_strdup_printf("%zu", i);
+} else {
.. in this body exclusively. I think it makes sense to move them here.
+if (virDomainGetGuestVcpus(dom, , , 0) < 0)
+goto cleanup;
+onlineVcpuStr = vshGetTypedParamValue(ctl, [1]);
+if (virBitmapParse(onlineVcpuStr, , nvcpus) < 0)
+goto cleanup;
+
+if (virBitmapToData(vcpus, , ) < 0)
+goto cleanup;
+if (enable) {
+cpulist = g_new0(char *, nvcpus - virBitmapCountBits(vcpus) + 1);
+for (i = 0; i < nvcpus; i++) {
+if (VIR_CPU_USED(vcpumap, i) != 0)
+continue;
+
+cpulist[offset++] = g_strdup_printf("%zu", i);
+}
+} else if (disable) {
+cpulist = g_new0(char *, virBitmapCountBits(vcpus) + 1);
+for (i = 0; i < nvcpus; i++) {
+if (VIR_CPU_USED(vcpumap, i) == 0)
+continue;
+
+cpulist[offset++] = g_strdup_printf("%zu", i);
+}
+}
+}
+
+ret = virshCommaStringListComplete(vcpuid, (const char **)cpulist);
+
+ cleanup:
+virTypedParamsFree(params, nparams);
+virshDomainFree(dom);
+return ret;
+}
diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
index d38efd5ea8..d5021f6aa6 100644
--- a/tools/virsh-completer-domain.h
+++ b/tools/virsh-completer-domain.h
@@ -90,3 +90,7 @@ char ** virshDomainVcpulistCompleter(vshControl *ctl,
char ** virshDomainCpulistCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1ae936c6b2..675d96440d 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -7436,6 +7436,7 @@ static const vshCmdOptDef opts_guestvcpus[] = {
VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
{.name = "cpulist",
.type = VSH_OT_STRING,
+ .completer = virshDomainVcpulistViaAgentCompleter,
.help = N_("list of cpus to enable or disable")
},
{.name = "enable",
Michal
Re: [libvirt] [PATCH v2 00/13] virsh completion improvement
On 11/10/20 10:50 AM, Lin Ma wrote: Add more bash completions for some of virsh subcommands/subparameters. v1->v2 * Follow Michal's suggestions: - guestvcpus command: complete offlined vCPUs if --enable, complete onlined vCPUs if --disable - Added a static helper function for interface string provider, - Adjusted some needlessly complicated loops. * Refered to Peter and Michal's comments, I droped the vnc completion patches. * Concentrate on completion itself, Drop all of patches which related to macros to make review easier. Lin Ma (13): virsh: Add vcpu list completion to guestvcpus command virsh: Add logical CPU IDs completion for nodecpustats command virsh: Add serial/parallel device name completion to console command virsh-interface: Add a static helper virshInterfaceStringHelper virsh: Add interface mac completion to iface-name command virsh: Add interface name completion to iface-bridge command virsh: Add interface name completion to iface-mac command vsh: Fix completion error in case of multiple mac addresses virsh: Add mac completion to net-dhcp-leases command virsh: Move/add some of function declarations to virsh-domain.h virsh: Add signal name completion to send-process-signal command virsh: Add lifecycle type completion to set-lifecycle-action command Add lifecycle action completion to set-lifecycle-action command tools/bash-completion/vsh | 1 + tools/virsh-completer-domain.c| 190 ++ tools/virsh-completer-domain.h| 20 tools/virsh-completer-host.c | 31 + tools/virsh-completer-host.h | 4 + tools/virsh-completer-interface.c | 34 -- tools/virsh-completer-interface.h | 7 ++ tools/virsh-completer-network.c | 44 +++ tools/virsh-completer-network.h | 4 + tools/virsh-domain.c | 6 +- tools/virsh-domain.h | 4 + tools/virsh-host.c| 1 + tools/virsh-interface.c | 3 + tools/virsh-network.c | 15 ++- 14 files changed, 349 insertions(+), 15 deletions(-) I've fixed all the small problems I found and Reviewed-by: Michal Privoznik and pushed. Michal
Re: [libvirt] [PATCH v2 04/13] virsh-interface: Add a static helper virshInterfaceStringHelper
On 11/10/20 10:50 AM, Lin Ma wrote:
It will be helpful to get the desired string of interface name/mac in a
consistent way.
Signed-off-by: Lin Ma
---
tools/virsh-completer-interface.c | 25 -
tools/virsh-completer-interface.h | 3 +++
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-completer-interface.c
b/tools/virsh-completer-interface.c
index 8028db8746..407021485f 100644
--- a/tools/virsh-completer-interface.c
+++ b/tools/virsh-completer-interface.c
@@ -25,16 +25,16 @@
#include "virsh.h"
#include "virstring.h"
-char **
-virshInterfaceNameCompleter(vshControl *ctl,
-const vshCmd *cmd G_GNUC_UNUSED,
-unsigned int flags)
+static char **
+virshInterfaceStringHelper(vshControl *ctl,
+ const vshCmd *cmd G_GNUC_UNUSED,
+ unsigned int flags,
+ virInterfaceStringCallback cb)
{
virshControlPtr priv = ctl->privData;
virInterfacePtr *ifaces = NULL;
int nifaces = 0;
size_t i = 0;
-char **ret = NULL;
VIR_AUTOSTRINGLIST tmp = NULL;
virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE |
@@ -50,15 +50,22 @@ virshInterfaceNameCompleter(vshControl *ctl,
tmp = g_new0(char *, nifaces + 1);
for (i = 0; i < nifaces; i++) {
-const char *name = virInterfaceGetName(ifaces[i]);
+const char *name = (cb)(ifaces[i]);
tmp[i] = g_strdup(name);
}
-ret = g_steal_pointer();
-
for (i = 0; i < nifaces; i++)
virInterfaceFree(ifaces[i]);
g_free(ifaces);
-return ret;
+
+return g_steal_pointer();
+}
+
+char **
+virshInterfaceNameCompleter(vshControl *ctl,
+const vshCmd *cmd,
+unsigned int flags)
+{
+return virshInterfaceStringHelper(ctl, cmd, flags, virInterfaceGetName);
}
diff --git a/tools/virsh-completer-interface.h
b/tools/virsh-completer-interface.h
index 893dee5a6b..32da01e766 100644
--- a/tools/virsh-completer-interface.h
+++ b/tools/virsh-completer-interface.h
@@ -22,6 +22,9 @@
#include "vsh.h"
+typedef const char *
+(*virInterfaceStringCallback)(virInterfacePtr iface);
+
This definition belongs to virsh-completer-interface.c because the
function using it is static and thus there is no way anybody else can
call it hence less header pollution.
Michal
Re: [libvirt] [PATCH v2 03/13] virsh: Add serial/parallel device name completion to console command
On 11/10/20 10:50 AM, Lin Ma wrote:
Signed-off-by: Lin Ma
---
tools/virsh-completer-domain.c | 55 ++
tools/virsh-completer-domain.h | 4 +++
tools/virsh-domain.c | 1 +
3 files changed, 60 insertions(+)
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
index f653471b89..dc66317561 100644
--- a/tools/virsh-completer-domain.c
+++ b/tools/virsh-completer-domain.c
@@ -656,3 +656,58 @@ virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
virshDomainFree(dom);
return ret;
}
+
+char **
+virshDomainConsoleCompleter(vshControl *ctl,
+const vshCmd *cmd,
+unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+g_autoptr(xmlDoc) xmldoc = NULL;
+g_autoptr(xmlXPathContext) ctxt = NULL;
+int nserials;
+int nparallels;
+g_autofree xmlNodePtr *serials = NULL;
+g_autofree xmlNodePtr *parallels = NULL;
+size_t i;
+size_t offset = 0;
+VIR_AUTOSTRINGLIST tmp = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+if (virshDomainGetXML(ctl, cmd, 0, , ) < 0)
+return NULL;
+
+nserials = virXPathNodeSet("./devices/serial", ctxt, );
+if (nserials < 0)
+return NULL;
+
+nparallels = virXPathNodeSet("./devices/parallel", ctxt, );
+if (nparallels < 0)
+return NULL;
+
+tmp = g_new0(char *, nserials + nparallels + 1);
+
+for (i = 0; i < nserials; i++) {
+ctxt->node = serials[i];
+
+if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty"))
virXPathString() returns an allocated string which is leaked here ..
+continue;
+
+tmp[offset++] = virXPathString("string(./alias/@name)", ctxt);
+}
+
+for (i = 0; i < nparallels; i++) {
+ctxt->node = parallels[i];
+
+if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty"))
.. and here.
+continue;
+
+tmp[offset++] = virXPathString("string(./alias/@name)", ctxt);
+}
+
+return g_steal_pointer();
+}
diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
index d5021f6aa6..02fea2fe94 100644
--- a/tools/virsh-completer-domain.h
+++ b/tools/virsh-completer-domain.h
@@ -94,3 +94,7 @@ char ** virshDomainCpulistCompleter(vshControl *ctl,
char ** virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshDomainConsoleCompleter(vshControl *ctl,
+const vshCmd *cmd,
+unsigned int flags);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 675d96440d..022dbdca3c 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2960,6 +2960,7 @@ static const vshCmdOptDef opts_console[] = {
VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
{.name = "devname", /* sc_prohibit_devname */
.type = VSH_OT_STRING,
+ .completer = virshDomainConsoleCompleter,
.help = N_("character device name")
},
{.name = "force",
Michal
Re: [libvirt] [PATCH v2 02/13] virsh: Add logical CPU IDs completion for nodecpustats command
On 11/10/20 10:50 AM, Lin Ma wrote: Signed-off-by: Lin Ma --- tools/virsh-completer-host.c | 31 +++ tools/virsh-completer-host.h | 4 tools/virsh-host.c | 1 + 3 files changed, 36 insertions(+) diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c index 339390aa00..685fa23fd4 100644 --- a/tools/virsh-completer-host.c +++ b/tools/virsh-completer-host.c @@ -136,3 +136,34 @@ virshCellnoCompleter(vshControl *ctl, return g_steal_pointer(); } + + +char ** +virshCpuCompleter(vshControl *ctl, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int flags) With so many CPU completers I think this should include Node in its name, e.g. virshNodeCpuCompleter() so that it's obvious it's completing host's CPUs. Michal
Re: [PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
On Tue, Nov 10, 2020 at 10:10:16AM -0300, Daniel Henrique Barboza wrote:
>
>
> On 11/10/20 2:04 AM, Masayoshi Mizuma wrote:
> > From: Masayoshi Mizuma
> >
> > A qemu guest which has virtiofs config fails to start if the previous
> > starting failed because of invalid option or something.
> > For example of the reproduction:
> >
> ># virsh start guest
> >error: Failed to start domain guest
> >error: internal error: process exited while connecting to monitor:
> > qemu-system-x86_64: -foo: invalid option
> >
> >... fix the option ...
> >
> ># virsh start guest
> >error: Failed to start domain guest
> >error: Cannot open log file:
> > '/var/log/libvirt/qemu/guest-fs0-virtiofsd.log': Device or resource busy
> >#
> >
> > That's because the virtiofsd which was executed on the former staring
> > remains
>
>
> s/staring/start ?
>
>
> > and virtlogd keeps to opening the log file.
> >
> > Stop virtiofsd when the qemu guest fails to start.
> >
> > Signed-off-by: Masayoshi Mizuma
> > ---
> > src/qemu/qemu_domain.h | 1 +
> > src/qemu/qemu_virtiofs.c | 5 +
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> > index ca041e207b..7d47c030bd 100644
> > --- a/src/qemu/qemu_domain.h
> > +++ b/src/qemu/qemu_domain.h
> > @@ -426,6 +426,7 @@ struct _qemuDomainFSPrivate {
> > virObject parent;
> > char *vhostuser_fs_sock;
> > +pid_t virtiofsd_pid;
> > };
> > diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
> > index 2e239cad66..8684219915 100644
> > --- a/src/qemu/qemu_virtiofs.c
> > +++ b/src/qemu/qemu_virtiofs.c
> > @@ -250,6 +250,7 @@ qemuVirtioFSStart(virLogManagerPtr logManager,
> > }
> > QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock =
> > g_steal_pointer(_path);
> > +QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid = pid;
> > ret = 0;
> >cleanup:
> > @@ -273,6 +274,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
> > {
> > g_autofree char *pidfile = NULL;
> > virErrorPtr orig_err;
> > +pid_t pid = QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid;
> > virErrorPreserveLast(_err);
> > @@ -286,6 +288,9 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
> > unlink(QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock);
> > }
> > +if (virProcessKill(pid, 0) == 0)
> > +virProcessKillPainfully(pid, true);
> > +
>
>
> If you're adamant into killing 'pid' I suggest to just verify "if (pid)" and
> then execute virProcessKillPainfully(), like is being done in
> virPidFileForceCleanupPath()
> that is called right before.
>
> Speaking of which, isn't virPidFileForceCleanupPath() responsible of killing
> virtiofsd? If that's the case, the function is failing to do so in the
> scenario
> you described, but it will succeed in all other cases. The result is that
> you'll
> end up trying to kill the process twice.
>
> Making a guess about what might be happening, if 'pidfile' does not exist in
> your error
> scenario then virPidFileForceCleanupPath() is returning 0 but is doing
> nothing. This is
> the case where your code might come in and kill the pid manually.
Thank you pointing it out! Your guess is correct. The pid file was removed by
virFileDeleteTree(priv->libDir) in qemuProcessStop(), so virtiofsd isn't killed
by virPidFileForceCleanupPath().
I think we can fix the error to move virFileDeleteTree(priv->libDir) after
calling qemuExtDevicesStop(driver, vm).
Does the following make sense?
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0a36b49c85..fc6eb5ad13 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7638,7 +7638,6 @@ void qemuProcessStop(virQEMUDriverPtr driver,
/* Do this before we delete the tree and remove pidfile. */
qemuProcessKillManagedPRDaemon(vm);
-virFileDeleteTree(priv->libDir);
virFileDeleteTree(priv->channelTargetDir);
ignore_value(virDomainChrDefForeach(vm->def,
@@ -7655,6 +7654,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
qemuDomainCleanupRun(driver, vm);
qemuExtDevicesStop(driver, vm);
+virFileDeleteTree(priv->libDir);
qemuDBusStop(driver, vm);
--
Thanks,
Masa
[PATCH 5/6] qemu: Implement OpenSSH authorized key file mgmt APIs
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1888537
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_driver.c | 81 ++
1 file changed, 81 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5f0bb69dd5..7fd29f934f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20121,6 +20121,85 @@ qemuDomainAgentSetResponseTimeout(virDomainPtr dom,
}
+static int
+qemuDomainAuthorizedSSHKeysGet(virDomainPtr dom,
+ const char *user,
+ char ***keys,
+ unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+qemuAgentPtr agent;
+int rv = -1;
+
+virCheckFlags(0, -1);
+
+if (!(vm = qemuDomainObjFromDomain(dom)))
+return -1;
+
+if (virDomainAuthorizedSshKeysGetEnsureACL(dom->conn, vm->def) < 0)
+return -1;
+
+if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
+return -1;
+
+if (!qemuDomainAgentAvailable(vm, true))
+goto endagentjob;
+
+agent = qemuDomainObjEnterAgent(vm);
+rv = qemuAgentSSHGetAuthorizedKeys(agent, user, keys);
+qemuDomainObjExitAgent(vm, agent);
+
+ endagentjob:
+qemuDomainObjEndAgentJob(vm);
+virDomainObjEndAPI();
+return rv;
+}
+
+
+static int
+qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom,
+ const char *user,
+ const char **keys,
+ int nkeys,
+ unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+g_autoptr(virDomainObj) vm = NULL;
+qemuAgentPtr agent;
+const bool append = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND;
+const bool remove = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE;
+int rv = -1;
+
+virCheckFlags(VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND |
+ VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE, -1);
+
+if (!(vm = qemuDomainObjFromDomain(dom)))
+return -1;
+
+if (virDomainAuthorizedSshKeysSetEnsureACL(dom->conn, vm->def) < 0)
+return -1;
+
+if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
+return -1;
+
+if (!qemuDomainAgentAvailable(vm, true))
+goto endagentjob;
+
+agent = qemuDomainObjEnterAgent(vm);
+if (remove)
+rv = qemuAgentSSHRemoveAuthorizedKeys(agent, user, keys, nkeys);
+else
+rv = qemuAgentSSHAddAuthorizedKeys(agent, user, keys, nkeys, !append);
+qemuDomainObjExitAgent(vm, agent);
+
+ endagentjob:
+qemuDomainObjEndAgentJob(vm);
+virDomainObjEndAPI();
+return rv;
+}
+
+
static virHypervisorDriver qemuHypervisorDriver = {
.name = QEMU_DRIVER_NAME,
.connectURIProbe = qemuConnectURIProbe,
@@ -20360,6 +20439,8 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainAgentSetResponseTimeout = qemuDomainAgentSetResponseTimeout, /*
5.10.0 */
.domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */
.domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */
+.domainAuthorizedSSHKeysGet = qemuDomainAuthorizedSSHKeysGet, /* 6.10.0 */
+.domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */
};
--
2.26.2
[PATCH 1/6] Introduce OpenSSH authorized key file mgmt APIs
When setting up a new guest or when a management software wants
to allow access to an existing guest the
virDomainSetUserPassword() API can be used, but that might be not
good enough if user want to ssh into the guest. Not only sshd has
to be configured to accept password authentication (which is
usually not the case for root), user have to type in their
password. Using SSH keys is more convenient. Therefore, two new
APIs are introduced:
virDomainAuthorizedSSHKeysGet() which lists authorized keys for
given user, and
virDomainAuthorizedSSHKeysSet() which modifies the authorized
keys file for given user (append, set or remove keys from the
file).
It's worth nothing that while authorized_keys file entries have
some structure (as defined by sshd(8)), expressing that structure
goes beyond libvirt's focus and thus "keys" are nothing but an
opaque string to libvirt.
Signed-off-by: Michal Privoznik
---
include/libvirt/libvirt-domain.h | 17 +
src/driver-hypervisor.h | 15
src/libvirt-domain.c | 115 +++
src/libvirt_public.syms | 6 ++
4 files changed, 153 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index b3310729bf..60a7ddefa8 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -5096,4 +5096,21 @@ int virDomainBackupBegin(virDomainPtr domain,
char *virDomainBackupGetXMLDesc(virDomainPtr domain,
unsigned int flags);
+int virDomainAuthorizedSSHKeysGet(virDomainPtr domain,
+ const char *user,
+ char ***keys,
+ unsigned int flags);
+
+typedef enum {
+VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND = (1 << 0), /* don't truncate
file, just append */
+VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE = (1 << 1), /* remove keys,
instead of adding them */
+
+} virDomainAuthorizedSSHKeysSetFlags;
+
+int virDomainAuthorizedSSHKeysSet(virDomainPtr domain,
+ const char *user,
+ const char **keys,
+ int nkeys,
+ unsigned int flags);
+
#endif /* LIBVIRT_DOMAIN_H */
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index bce023017d..5a5ea95c51 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1387,6 +1387,19 @@ typedef char *
(*virDrvDomainBackupGetXMLDesc)(virDomainPtr domain,
unsigned int flags);
+typedef int
+(*virDrvDomainAuthorizedSSHKeysGet)(virDomainPtr domain,
+const char *user,
+char ***keys,
+unsigned int flags);
+
+typedef int
+(*virDrvDomainAuthorizedSSHKeysSet)(virDomainPtr domain,
+const char *user,
+const char **keys,
+int nkeys,
+unsigned int flags);
+
typedef struct _virHypervisorDriver virHypervisorDriver;
typedef virHypervisorDriver *virHypervisorDriverPtr;
@@ -1650,4 +1663,6 @@ struct _virHypervisorDriver {
virDrvDomainAgentSetResponseTimeout domainAgentSetResponseTimeout;
virDrvDomainBackupBegin domainBackupBegin;
virDrvDomainBackupGetXMLDesc domainBackupGetXMLDesc;
+virDrvDomainAuthorizedSSHKeysGet domainAuthorizedSSHKeysGet;
+virDrvDomainAuthorizedSSHKeysSet domainAuthorizedSSHKeysSet;
};
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 3c5f55176a..0a55a48952 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -12758,3 +12758,118 @@ virDomainBackupGetXMLDesc(virDomainPtr domain,
virDispatchError(conn);
return NULL;
}
+
+
+/**
+ * virDomainAuthorizedSSHKeysGet:
+ * @domain: a domain object
+ * @user: user to list keys for
+ * @keys: pointer to a variable to store authorized keys
+ * @flags: extra flags; not used yet, so callers should always pass 0
+ *
+ * For given @user in @domain fetch list of public SSH authorized
+ * keys and store them into @keys array which is allocated upon
+ * successful return. The caller is responsible for freeing @keys
+ * when no longer needed.
+ *
+ * Keys are in OpenSSH format (see sshd(8)) but from libvirt's
+ * point of view are opaque strings, i.e. not interpreted.
+ *
+ * Returns: number of keys stored in @keys,
+ * -1 otherwise.
+ */
+int virDomainAuthorizedSSHKeysGet(virDomainPtr domain,
+ const char *user,
+ char ***keys,
+ unsigned int flags)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "user=%s, keys=%p, flags=0x%x",
+ user, keys, flags);
+
+virResetLastError();
+
+virCheckDomainReturn(domain, -1);
+
[PATCH 3/6] virsh: Expose OpenSSH authorized key file mgmt APIs
The new virsh commands are:
get-user-sshkeys
set-user-sshkeys
Signed-off-by: Michal Privoznik
---
docs/manpages/virsh.rst | 37 ++
tools/virsh-domain.c| 152
2 files changed, 189 insertions(+)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index bfd26e3120..30b234aeab 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -2636,6 +2636,21 @@ When *--timestamp* is used, a human-readable timestamp
will be printed
before the event.
+get-user-sshkeys
+
+
+**Syntax:**
+
+::
+
+ get-user-sshkeys domain user
+
+Print SSH authorized keys for given *user* in the guest *domain*. Please note,
+that an entry in the file has internal structure as defined by *sshd(8)* and
+virsh/libvirt does handle keys as opaque strings, i.e. does not interpret
+them.
+
+
guest-agent-timeout
---
@@ -4004,6 +4019,28 @@ For QEMU/KVM, this requires the guest agent to be
configured
and running.
+set-user-sshkeys
+
+
+**Syntax:**
+
+::
+
+ set-user-sshkeys domain user [{--append | --remove}] [--keys keys]
+
+Replace the contents of *user*'s SSH authorized file in the guest *domain* with
+*keys*. Please note, that an entry in the file has internal structure as
+defined by *sshd(8)* and virsh/libvirt does handle keys as opaque strings,
+i.e. does not interpret them.
+
+If *--append* is specified, then the file content is not replaced and new keys
+are just appended.
+
+If *--remove* is specified, then instead of adding any new keys these are
+removed from the file. It is not considered an error if the key does not exist
+in the file.
+
+
setmaxmem
-
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1ae936c6b2..f51765cb42 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -14259,6 +14259,146 @@ cmdGuestInfo(vshControl *ctl, const vshCmd *cmd)
return ret;
}
+/*
+ * "get-user-sshkeys" command
+ */
+static const vshCmdInfo info_get_user_sshkeys[] = {
+{.name = "help",
+ .data = N_("list authorized SSH keys for given user (via agent)")
+},
+{.name = "desc",
+ .data = N_("Use the guest agent to query authorized SSH keys for given "
+"user")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_get_user_sshkeys[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
+{.name = "user",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("user to list authorized keys for"),
+},
+{.name = NULL}
+};
+
+static bool
+cmdGetUserSSHKeys(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+const char *user;
+char **keys = NULL;
+int nkeys = 0;
+size_t i;
+const unsigned int flags = 0;
+vshTablePtr table = NULL;
+bool ret = false;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+if (vshCommandOptStringReq(ctl, cmd, "user", ) < 0)
+goto cleanup;
+
+nkeys = virDomainAuthorizedSSHKeysGet(dom, user, , flags);
+if (nkeys < 0)
+goto cleanup;
+
+if (!(table = vshTableNew(_("key"), NULL)))
+goto cleanup;
+
+for (i = 0; i < nkeys; i++) {
+if (vshTableRowAppend(table, keys[i], NULL) < 0)
+goto cleanup;
+}
+
+vshTablePrintToStdout(table, ctl);
+
+ret = true;
+ cleanup:
+vshTableFree(table);
+if (nkeys > 0)
+virStringListFreeCount(keys, nkeys);
+virshDomainFree(dom);
+return ret;
+}
+
+
+/*
+ * "set-user-sshkeys" command
+ */
+static const vshCmdInfo info_set_user_sshkeys[] = {
+{.name = "help",
+ .data = N_("manipulate authorized SSH keys file for given user (via
agent)")
+},
+{.name = "desc",
+ .data = N_("Append, reset or remove specified key from the authorized "
+"keys file for given user")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_set_user_sshkeys[] = {
+VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
+{.name = "user",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("user to list authorized keys for"),
+},
+{.name = "append",
+ .type = VSH_OT_BOOL,
+ .help = N_("append keys to the file"),
+},
+{.name = "remove",
+ .type = VSH_OT_BOOL,
+ .help = N_("remove keys from the file"),
+},
+{.name = "keys",
+ .type = VSH_OT_ARGV,
+ .help = N_("OpenSSH keys"),
+},
+{.name = NULL}
+};
+
+static bool
+cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+const char *user;
+const vshCmdOpt *opt = NULL;
+g_autofree const char **keys = NULL;
+int nkeys = 0;
+unsigned int flags = 0;
+bool ret = false;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+if (vshCommandOptStringReq(ctl, cmd, "user", ) < 0)
+goto cleanup;
+
+if
[PATCH 4/6] qemu_agent: add qemuAgentSSH{Add, Remove, Get}AuthorizedKeys
From: Marc-André Lureau
In QEMU 5.2, the guest agent learned to manipulate a user
~/.ssh/authorized_keys. Bind the JSON API to libvirt.
https://wiki.qemu.org/ChangeLog/5.2#Guest_agent
Signed-off-by: Marc-André Lureau
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_agent.c | 140 ++
src/qemu/qemu_agent.h | 15 +
tests/qemuagenttest.c | 79
3 files changed, 234 insertions(+)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 7fbb4a9431..9fa79338bc 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -2496,3 +2496,143 @@ qemuAgentSetResponseTimeout(qemuAgentPtr agent,
{
agent->timeout = timeout;
}
+
+/**
+ * qemuAgentSSHGetAuthorizedKeys:
+ * @agent: agent object
+ * @user: user to get authorized keys for
+ * @keys: Array of authorized keys
+ *
+ * Fetch the public keys from @user's $HOME/.ssh/authorized_keys.
+ *
+ * Returns: number of keys returned on success,
+ * -1 otherwise (error is reported)
+ */
+int
+qemuAgentSSHGetAuthorizedKeys(qemuAgentPtr agent,
+ const char *user,
+ char ***keys)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+virJSONValuePtr data = NULL;
+size_t ndata;
+size_t i;
+char **keys_ret = NULL;
+
+if (!(cmd = qemuAgentMakeCommand("guest-ssh-get-authorized-keys",
+ "s:username", user,
+ NULL)))
+return -1;
+
+if (qemuAgentCommand(agent, cmd, , agent->timeout) < 0)
+return -1;
+
+if (!(data = virJSONValueObjectGetObject(reply, "return")) ||
+!(data = virJSONValueObjectGetArray(data, "keys"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("qemu agent didn't return an array of keys"));
+return -1;
+}
+
+ndata = virJSONValueArraySize(data);
+
+keys_ret = g_new0(char *, ndata);
+
+for (i = 0; i < ndata; i++) {
+virJSONValuePtr entry = virJSONValueArrayGet(data, i);
+
+if (!entry) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("array element missing in
guest-ssh-get-authorized-keys return "
+ "value"));
+goto error;
+}
+
+keys_ret[i] = g_strdup(virJSONValueGetString(entry));
+}
+
+*keys = g_steal_pointer(_ret);
+return ndata;
+
+ error:
+virStringListFreeCount(keys_ret, ndata);
+return -1;
+}
+
+
+/**
+ * qemuAgentSSHAddAuthorizedKeys:
+ * @agent: agent object
+ * @user: user to add authorized keys for
+ * @keys: Array of authorized keys
+ * @nkeys: number of items in @keys array
+ * @reset: whether to truncate authorized keys file before writing
+ *
+ * Append SSH @keys into the @user's authorized keys file. If
+ * @reset is true then the file is truncated before write and
+ * thus contains only newly added @keys.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (error is reported)
+ */
+int qemuAgentSSHAddAuthorizedKeys(qemuAgentPtr agent,
+ const char *user,
+ const char **keys,
+ size_t nkeys,
+ bool reset)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+g_autoptr(virJSONValue) jkeys = NULL;
+
+jkeys = qemuAgentMakeStringsArray(keys, nkeys);
+if (jkeys == NULL)
+return -1;
+
+if (!(cmd = qemuAgentMakeCommand("guest-ssh-add-authorized-keys",
+ "s:username", user,
+ "a:keys", ,
+ "b:reset", reset,
+ NULL)))
+return -1;
+
+return qemuAgentCommand(agent, cmd, , agent->timeout);
+}
+
+
+/**
+ * qemuAgentSSHRemoveAuthorizedKeys:
+ * @agent: agent object
+ * @user: user to remove authorized keys for
+ * @keys: Array of authorized keys
+ * @nkeys: number of items in @keys array
+ *
+ * Remove SSH @keys from the @user's authorized keys file. It's
+ * not considered an error when trying to remove a non-existent
+ * key.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (error is reported)
+ */
+int qemuAgentSSHRemoveAuthorizedKeys(qemuAgentPtr agent,
+ const char *user,
+ const char **keys,
+ size_t nkeys)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+g_autoptr(virJSONValue) jkeys = NULL;
+
+jkeys = qemuAgentMakeStringsArray(keys, nkeys);
+if (jkeys == NULL)
+return -1;
+
+if (!(cmd = qemuAgentMakeCommand("guest-ssh-remove-authorized-keys",
+ "s:username", user,
+
[PATCH 6/6] news: Document recent OpenSSH authorized key file mgmt APIs
Signed-off-by: Michal Privoznik --- NEWS.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 0e56f5dbca..3fba272543 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -12,6 +12,12 @@ v6.10.0 (unreleased) * **New features** + * qemu: Implement OpenSSH authorized key file management APIs + +New APIs (``virDomainAuthorizedSSHKeysGet()`` and +``virDomainAuthorizedSSHKeysSet()``) and virsh commands +(``get-user-sshkeys`` and ``set-user-sshkeys``) are added to manage +authorized_keys SSH file for user. * **Improvements** -- 2.26.2
[PATCH 0/6] Introduce OpenSSH authorized key file mgmt APIs
Marc-André posted a patch that implements agent handling. I've written
the rest.
Marc-André Lureau (1):
qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
Michal Prívozník (5):
Introduce OpenSSH authorized key file mgmt APIs
remote: Implement OpenSSH authorized key file mgmt APIs
virsh: Expose OpenSSH authorized key file mgmt APIs
qemu: Implement OpenSSH authorized key file mgmt APIs
news: Document recent OpenSSH authorized key file mgmt APIs
NEWS.rst| 6 ++
docs/manpages/virsh.rst | 37 +++
include/libvirt/libvirt-domain.h| 17
src/driver-hypervisor.h | 15 +++
src/libvirt-domain.c| 115 +
src/libvirt_public.syms | 6 ++
src/qemu/qemu_agent.c | 140 +
src/qemu/qemu_agent.h | 15 +++
src/qemu/qemu_driver.c | 81 +++
src/remote/remote_daemon_dispatch.c | 82 +++
src/remote/remote_driver.c | 87
src/remote/remote_protocol.x| 34 ++-
src/remote_protocol-structs | 22
tests/qemuagenttest.c | 79 +++
tools/virsh-domain.c| 152
15 files changed, 887 insertions(+), 1 deletion(-)
--
2.26.2
[PATCH 2/6] remote: Implement OpenSSH authorized key file mgmt APIs
Since both APIs accept/return an array of strings we can't have
client/server dispatch code generated. But implementation is
fairly trivial, although verbose.
Signed-off-by: Michal Privoznik
---
src/remote/remote_daemon_dispatch.c | 82 +++
src/remote/remote_driver.c | 87 +
src/remote/remote_protocol.x| 34 ++-
src/remote_protocol-structs | 22
4 files changed, 224 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index eb5f6ebb0c..46683aa4a7 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -7381,3 +7381,85 @@ remoteDispatchDomainGetGuestInfo(virNetServerPtr server
G_GNUC_UNUSED,
return rv;
}
+
+static int
+remoteDispatchDomainAuthorizedSshKeysGet(virNetServerPtr server G_GNUC_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessageErrorPtr rerr,
+
remote_domain_authorized_ssh_keys_get_args *args,
+
remote_domain_authorized_ssh_keys_get_ret *ret)
+{
+int rv = -1;
+virConnectPtr conn = remoteGetHypervisorConn(client);
+int nkeys = 0;
+char **keys = NULL;
+virDomainPtr dom = NULL;
+
+if (!conn)
+goto cleanup;
+
+if (!(dom = get_nonnull_domain(conn, args->dom)))
+goto cleanup;
+
+if ((nkeys = virDomainAuthorizedSSHKeysGet(dom, args->user,
+ , args->flags)) < 0)
+goto cleanup;
+
+if (nkeys > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Number of keys %d, which exceeds max liit: %d"),
+ nkeys, REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX);
+goto cleanup;
+}
+
+ret->keys.keys_val = g_steal_pointer();
+ret->keys.keys_len = nkeys;
+
+rv = nkeys;
+
+ cleanup:
+if (rv < 0)
+virNetMessageSaveError(rerr);
+if (nkeys > 0)
+virStringListFreeCount(keys, nkeys);
+virObjectUnref(dom);
+
+return rv;
+}
+
+static int
+remoteDispatchDomainAuthorizedSshKeysSet(virNetServerPtr server G_GNUC_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessageErrorPtr rerr,
+
remote_domain_authorized_ssh_keys_set_args *args)
+{
+int rv = -1;
+virConnectPtr conn = remoteGetHypervisorConn(client);
+virDomainPtr dom = NULL;
+
+if (!conn)
+goto cleanup;
+
+if (!(dom = get_nonnull_domain(conn, args->dom)))
+goto cleanup;
+
+if (args->keys.keys_len > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Number of keys %d, which exceeds max liit: %d"),
+ args->keys.keys_len,
REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX);
+goto cleanup;
+}
+
+rv = virDomainAuthorizedSSHKeysSet(dom, args->user,
+ (const char **) args->keys.keys_val,
+ args->keys.keys_len, args->flags);
+
+ cleanup:
+if (rv < 0)
+virNetMessageSaveError(rerr);
+virObjectUnref(dom);
+
+return rv;
+}
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 9cd2fd36ae..0b8d1e753f 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8027,6 +8027,91 @@ remoteDomainGetGuestInfo(virDomainPtr dom,
return rv;
}
+static int
+remoteDomainAuthorizedSSHKeysGet(virDomainPtr domain,
+ const char *user,
+ char ***keys,
+ unsigned int flags)
+{
+int rv = -1;
+size_t i;
+struct private_data *priv = domain->conn->privateData;
+remote_domain_authorized_ssh_keys_get_args args;
+remote_domain_authorized_ssh_keys_get_ret ret;
+
+remoteDriverLock(priv);
+
+make_nonnull_domain(, domain);
+args.user = (char *) user;
+args.flags = flags;
+memset(, 0, sizeof(ret));
+
+if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_GET,
+ (xdrproc_t) xdr_remote_domain_authorized_ssh_keys_get_args, (char
*),
+ (xdrproc_t) xdr_remote_domain_authorized_ssh_keys_get_ret, (char
*)) == -1) {
+goto cleanup;
+}
+
+if (ret.keys.keys_len > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) {
+virReportError(VIR_ERR_RPC, "%s",
+ _("remoteDomainAuthorizedSSHKeysGet: "
+ "returned number of keys exceeds limit"));
+goto cleanup;
+
Re: [libvirt PATCH] qemu: add qemuAgentSSH{Add, Remove, Get}AuthorizedKeys
Hi On Mon, Nov 9, 2020 at 4:44 PM Michal Privoznik wrote: > > I'm stopping my review here. The wrappers are okay, but we really need > the public API and RPC first. I can work on that if you don't feel like it. I am on holiday this week and perhaps a few more days. If you have some free time, feel free to update the patch. Otherwise, it will wait a bit longer. thanks
Re: [PATCH] qemu: virtiofs: Stop virtiofsd when the qemu guest fails to start
On 11/10/20 2:04 AM, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma
A qemu guest which has virtiofs config fails to start if the previous
starting failed because of invalid option or something.
For example of the reproduction:
# virsh start guest
error: Failed to start domain guest
error: internal error: process exited while connecting to monitor:
qemu-system-x86_64: -foo: invalid option
... fix the option ...
# virsh start guest
error: Failed to start domain guest
error: Cannot open log file:
'/var/log/libvirt/qemu/guest-fs0-virtiofsd.log': Device or resource busy
#
That's because the virtiofsd which was executed on the former staring remains
s/staring/start ?
and virtlogd keeps to opening the log file.
Stop virtiofsd when the qemu guest fails to start.
Signed-off-by: Masayoshi Mizuma
---
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_virtiofs.c | 5 +
2 files changed, 6 insertions(+)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index ca041e207b..7d47c030bd 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -426,6 +426,7 @@ struct _qemuDomainFSPrivate {
virObject parent;
char *vhostuser_fs_sock;
+pid_t virtiofsd_pid;
};
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 2e239cad66..8684219915 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -250,6 +250,7 @@ qemuVirtioFSStart(virLogManagerPtr logManager,
}
QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock = g_steal_pointer(_path);
+QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid = pid;
ret = 0;
cleanup:
@@ -273,6 +274,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
{
g_autofree char *pidfile = NULL;
virErrorPtr orig_err;
+pid_t pid = QEMU_DOMAIN_FS_PRIVATE(fs)->virtiofsd_pid;
virErrorPreserveLast(_err);
@@ -286,6 +288,9 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
unlink(QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock);
}
+if (virProcessKill(pid, 0) == 0)
+virProcessKillPainfully(pid, true);
+
If you're adamant into killing 'pid' I suggest to just verify "if (pid)" and
then execute virProcessKillPainfully(), like is being done in
virPidFileForceCleanupPath()
that is called right before.
Speaking of which, isn't virPidFileForceCleanupPath() responsible of killing
virtiofsd? If that's the case, the function is failing to do so in the scenario
you described, but it will succeed in all other cases. The result is that you'll
end up trying to kill the process twice.
Making a guess about what might be happening, if 'pidfile' does not exist in
your error
scenario then virPidFileForceCleanupPath() is returning 0 but is doing nothing.
This is
the case where your code might come in and kill the pid manually.
Thanks,
DHB
cleanup:
virErrorRestore(_err);
}
Re: [PATCH v2 00/10] qemu: support renaming domains with snapshots/checkpoints
On 09.11.2020 16:51, Daniel Henrique Barboza wrote: > > > On 11/3/20 8:59 AM, Nikolay Shirokovskiy wrote: >> This is basically just rebase of [1] as it was not get any attention at that >> time. >> >> [1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints >> https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html > > Code LGTM: > > Reviewed-by: Daniel Henrique Barboza > > > Shouldn't you add some test cases for this new behavior though? I'm a bit > nervous with pushing this upstream without any coverage. > > So basically we need to test how qemuDomainRenameCallback works. Currently there is no test for this function or virDomainRename. I spent some time looking at existing tests that need to mock driver/vm objects and it looks like it requires a good deal of effort in order to prepare the test in this case. At the same time those tests has many inputs so it looks like worth heavy preparation. In case of rename the code we test does not depend greatly on inputs so may be it does not worth adding such test given heavy preparation we have to do. Of course I give the patch series some manual testing. Nikolay
Re: [libvirt] [PATCH 13/19] virsh-domain: Introduce 2 macros for domain options 'interface' and 'mac'
On 2020-11-02 19:40, Michal Privoznik wrote:
On 11/2/20 9:26 AM, Lin Ma wrote:
The macro VIRSH_DOMAIN_OPT_INTERFACE for domain option '--interface',
The macro VIRSH_DOMAIN_OPT_MAC for domain option '--mac'.
Signed-off-by: Lin Ma
---
tools/virsh-domain.h | 18 ++
1 file changed, 18 insertions(+)
diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h
index 0d59c579d4..ac05f983f9 100644
--- a/tools/virsh-domain.h
+++ b/tools/virsh-domain.h
@@ -39,3 +39,21 @@ typedef enum {
VIR_ENUM_DECL(virshDomainHostnameSource);
extern const vshCmdDef domManagementCmds[];
+
+#define VIRSH_DOMAIN_OPT_INTERFACE(_helpstr, oflags, cflags) \
+{.name = "interface", \
+ .type = VSH_OT_STRING, \
+ .flags = oflags, \
+ .help = _helpstr, \
+ .completer = virshDomainInterfaceCompleter, \
+ .completer_flags = cflags, \
+}
+
+#define VIRSH_DOMAIN_OPT_MAC(_helpstr, oflags) \
+{.name = "mac", \
+ .type = VSH_OT_STRING, \
+ .flags = oflags, \
+ .help = _helpstr, \
+ .completer = virshDomainInterfaceCompleter, \
+ .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, \
+}
So, if we had two distinct completers, as I suggested, then this would
look a bit different and both macros could accept cflags. We could
then print MAC addresses only of running/inactive/all interfaces,
which could be helpful.
The idea about two distinct completers, It's about virsh-interface, But
in
virsh-domain, Here is only one interface completer
virshDomainInterfaceCompleter
and it can handle name/mac well with a cflags.
IMO, The question is that we have two form of parameters: "interface"
and "mac"
in virsh-domain. That's why I created two macros.
If we replace '--mac' by '--interface' in virsh-domain, Then things go
easy:
With cflags's help, We don't need 2 macros any more, But obviously it
changed
the existing virsh cmd usage, Probably It's not acceptable. So I created
macro
VIRSH_DOMAIN_OPT_MAC only for the parameter '--mac', The '--mac' should
accept
mac address string only, Does a cflags really make sense to this macro?
The discussion about macros perhaps impact the review of the patchset
v2, so I
removed them in patchset v2, If we need them, I'll post them later.
I'm stopping my review here. I think you get the gist of my review.
Looking forward to v2.
Thank you so much for your nice review! I just posted v2 to libvirt ml.
Thank again,
LLin
[libvirt] [PATCH v2 02/13] virsh: Add logical CPU IDs completion for nodecpustats command
Signed-off-by: Lin Ma
---
tools/virsh-completer-host.c | 31 +++
tools/virsh-completer-host.h | 4
tools/virsh-host.c | 1 +
3 files changed, 36 insertions(+)
diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c
index 339390aa00..685fa23fd4 100644
--- a/tools/virsh-completer-host.c
+++ b/tools/virsh-completer-host.c
@@ -136,3 +136,34 @@ virshCellnoCompleter(vshControl *ctl,
return g_steal_pointer();
}
+
+
+char **
+virshCpuCompleter(vshControl *ctl,
+ const vshCmd *cmd G_GNUC_UNUSED,
+ unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+VIR_AUTOSTRINGLIST tmp = NULL;
+size_t i;
+int cpunum;
+size_t offset = 0;
+unsigned int online;
+g_autofree unsigned char *cpumap = NULL;
+
+virCheckFlags(0, NULL);
+
+if ((cpunum = virNodeGetCPUMap(priv->conn, , , 0)) < 0)
+return NULL;
+
+tmp = g_new0(char *, online + 1);
+
+for (i = 0; i < cpunum; i++) {
+if (VIR_CPU_USED(cpumap, i) == 0)
+continue;
+
+tmp[offset++] = g_strdup_printf("%zu", i);
+}
+
+return g_steal_pointer();
+}
diff --git a/tools/virsh-completer-host.h b/tools/virsh-completer-host.h
index 921beb7a2d..83b908 100644
--- a/tools/virsh-completer-host.h
+++ b/tools/virsh-completer-host.h
@@ -29,3 +29,7 @@ char ** virshAllocpagesPagesizeCompleter(vshControl *ctl,
char ** virshCellnoCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshCpuCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index cda2ef4ac1..4774f76ed8 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -751,6 +751,7 @@ static const vshCmdInfo info_nodecpustats[] = {
static const vshCmdOptDef opts_node_cpustats[] = {
{.name = "cpu",
.type = VSH_OT_INT,
+ .completer = virshCpuCompleter,
.help = N_("prints specified cpu statistics only.")
},
{.name = "percent",
--
2.26.0
[libvirt] [PATCH v2 06/13] virsh: Add interface name completion to iface-bridge command
Signed-off-by: Lin Ma
---
tools/virsh-interface.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
index ae2b52bfc3..ae234e1e22 100644
--- a/tools/virsh-interface.c
+++ b/tools/virsh-interface.c
@@ -786,6 +786,7 @@ static const vshCmdOptDef opts_interface_bridge[] = {
{.name = "interface",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshInterfaceNameCompleter,
.help = N_("existing interface name")
},
{.name = "bridge",
--
2.26.0
[libvirt] [PATCH v2 11/13] virsh: Add signal name completion to send-process-signal command
Signed-off-by: Lin Ma
---
tools/virsh-completer-domain.c | 20
tools/virsh-completer-domain.h | 4
tools/virsh-domain.c | 1 +
3 files changed, 25 insertions(+)
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
index dc66317561..b8b6c74f8b 100644
--- a/tools/virsh-completer-domain.c
+++ b/tools/virsh-completer-domain.c
@@ -711,3 +711,23 @@ virshDomainConsoleCompleter(vshControl *ctl,
return g_steal_pointer();
}
+
+char **
+virshDomainSignalCompleter(vshControl *ctl G_GNUC_UNUSED,
+ const vshCmd *cmd G_GNUC_UNUSED,
+ unsigned int flags)
+{
+size_t i = 0;
+VIR_AUTOSTRINGLIST tmp = NULL;
+
+virCheckFlags(0, NULL);
+
+tmp = g_new0(char *, VIR_DOMAIN_PROCESS_SIGNAL_LAST + 1);
+
+for (i = 0; i < VIR_DOMAIN_PROCESS_SIGNAL_LAST; i++) {
+const char *name = virDomainProcessSignalTypeToString(i);
+tmp[i] = g_strdup(name);
+}
+
+return g_steal_pointer();
+}
diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
index 02fea2fe94..cdec66f23e 100644
--- a/tools/virsh-completer-domain.h
+++ b/tools/virsh-completer-domain.h
@@ -98,3 +98,7 @@ char ** virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
char ** virshDomainConsoleCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshDomainSignalCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 472b510a6b..1406bb4583 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8905,6 +8905,7 @@ static const vshCmdOptDef opts_send_process_signal[] = {
{.name = "signame",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshDomainSignalCompleter,
.help = N_("the signal number or name")
},
{.name = NULL}
--
2.26.0
[libvirt] [PATCH v2 04/13] virsh-interface: Add a static helper virshInterfaceStringHelper
It will be helpful to get the desired string of interface name/mac in a
consistent way.
Signed-off-by: Lin Ma
---
tools/virsh-completer-interface.c | 25 -
tools/virsh-completer-interface.h | 3 +++
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-completer-interface.c
b/tools/virsh-completer-interface.c
index 8028db8746..407021485f 100644
--- a/tools/virsh-completer-interface.c
+++ b/tools/virsh-completer-interface.c
@@ -25,16 +25,16 @@
#include "virsh.h"
#include "virstring.h"
-char **
-virshInterfaceNameCompleter(vshControl *ctl,
-const vshCmd *cmd G_GNUC_UNUSED,
-unsigned int flags)
+static char **
+virshInterfaceStringHelper(vshControl *ctl,
+ const vshCmd *cmd G_GNUC_UNUSED,
+ unsigned int flags,
+ virInterfaceStringCallback cb)
{
virshControlPtr priv = ctl->privData;
virInterfacePtr *ifaces = NULL;
int nifaces = 0;
size_t i = 0;
-char **ret = NULL;
VIR_AUTOSTRINGLIST tmp = NULL;
virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE |
@@ -50,15 +50,22 @@ virshInterfaceNameCompleter(vshControl *ctl,
tmp = g_new0(char *, nifaces + 1);
for (i = 0; i < nifaces; i++) {
-const char *name = virInterfaceGetName(ifaces[i]);
+const char *name = (cb)(ifaces[i]);
tmp[i] = g_strdup(name);
}
-ret = g_steal_pointer();
-
for (i = 0; i < nifaces; i++)
virInterfaceFree(ifaces[i]);
g_free(ifaces);
-return ret;
+
+return g_steal_pointer();
+}
+
+char **
+virshInterfaceNameCompleter(vshControl *ctl,
+const vshCmd *cmd,
+unsigned int flags)
+{
+return virshInterfaceStringHelper(ctl, cmd, flags, virInterfaceGetName);
}
diff --git a/tools/virsh-completer-interface.h
b/tools/virsh-completer-interface.h
index 893dee5a6b..32da01e766 100644
--- a/tools/virsh-completer-interface.h
+++ b/tools/virsh-completer-interface.h
@@ -22,6 +22,9 @@
#include "vsh.h"
+typedef const char *
+(*virInterfaceStringCallback)(virInterfacePtr iface);
+
char ** virshInterfaceNameCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
--
2.26.0
[libvirt] [PATCH v2 05/13] virsh: Add interface mac completion to iface-name command
Signed-off-by: Lin Ma
---
tools/virsh-completer-interface.c | 9 +
tools/virsh-completer-interface.h | 4
tools/virsh-interface.c | 1 +
3 files changed, 14 insertions(+)
diff --git a/tools/virsh-completer-interface.c
b/tools/virsh-completer-interface.c
index 407021485f..dea8d645cd 100644
--- a/tools/virsh-completer-interface.c
+++ b/tools/virsh-completer-interface.c
@@ -69,3 +69,12 @@ virshInterfaceNameCompleter(vshControl *ctl,
{
return virshInterfaceStringHelper(ctl, cmd, flags, virInterfaceGetName);
}
+
+char **
+virshInterfaceMacCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags)
+{
+return virshInterfaceStringHelper(ctl, cmd, flags,
+ virInterfaceGetMACString);
+}
diff --git a/tools/virsh-completer-interface.h
b/tools/virsh-completer-interface.h
index 32da01e766..b48ded6a12 100644
--- a/tools/virsh-completer-interface.h
+++ b/tools/virsh-completer-interface.h
@@ -28,3 +28,7 @@ typedef const char *
char ** virshInterfaceNameCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshInterfaceMacCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
index 8cdbc6e85f..ae2b52bfc3 100644
--- a/tools/virsh-interface.c
+++ b/tools/virsh-interface.c
@@ -404,6 +404,7 @@ static const vshCmdOptDef opts_interface_name[] = {
{.name = "interface",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshInterfaceMacCompleter,
.help = N_("interface mac")
},
{.name = NULL}
--
2.26.0
[libvirt] [PATCH v2 10/13] virsh: Move/add some of function declarations to virsh-domain.h
The upcoming patches introduce completers into virsh-completer-domain.c,
They will invoke the functions which are defined in virsh-domain.c, So
these functions need to be declared in virsh-domain.h.
Signed-off-by: Lin Ma
---
tools/virsh-domain.c | 1 -
tools/virsh-domain.h | 4
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 022dbdca3c..472b510a6b 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8910,7 +8910,6 @@ static const vshCmdOptDef opts_send_process_signal[] = {
{.name = NULL}
};
-VIR_ENUM_DECL(virDomainProcessSignal);
VIR_ENUM_IMPL(virDomainProcessSignal,
VIR_DOMAIN_PROCESS_SIGNAL_LAST,
"nop","hup", "int", "quit", "ill", /* 0-4 */
diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h
index 0d59c579d4..70e2aba1b1 100644
--- a/tools/virsh-domain.h
+++ b/tools/virsh-domain.h
@@ -39,3 +39,7 @@ typedef enum {
VIR_ENUM_DECL(virshDomainHostnameSource);
extern const vshCmdDef domManagementCmds[];
+
+VIR_ENUM_DECL(virDomainProcessSignal);
+VIR_ENUM_DECL(virDomainLifecycle);
+VIR_ENUM_DECL(virDomainLifecycleAction);
--
2.26.0
[libvirt] [PATCH v2 00/13] virsh completion improvement
Add more bash completions for some of virsh subcommands/subparameters. v1->v2 * Follow Michal's suggestions: - guestvcpus command: complete offlined vCPUs if --enable, complete onlined vCPUs if --disable - Added a static helper function for interface string provider, - Adjusted some needlessly complicated loops. * Refered to Peter and Michal's comments, I droped the vnc completion patches. * Concentrate on completion itself, Drop all of patches which related to macros to make review easier. Lin Ma (13): virsh: Add vcpu list completion to guestvcpus command virsh: Add logical CPU IDs completion for nodecpustats command virsh: Add serial/parallel device name completion to console command virsh-interface: Add a static helper virshInterfaceStringHelper virsh: Add interface mac completion to iface-name command virsh: Add interface name completion to iface-bridge command virsh: Add interface name completion to iface-mac command vsh: Fix completion error in case of multiple mac addresses virsh: Add mac completion to net-dhcp-leases command virsh: Move/add some of function declarations to virsh-domain.h virsh: Add signal name completion to send-process-signal command virsh: Add lifecycle type completion to set-lifecycle-action command Add lifecycle action completion to set-lifecycle-action command tools/bash-completion/vsh | 1 + tools/virsh-completer-domain.c| 190 ++ tools/virsh-completer-domain.h| 20 tools/virsh-completer-host.c | 31 + tools/virsh-completer-host.h | 4 + tools/virsh-completer-interface.c | 34 -- tools/virsh-completer-interface.h | 7 ++ tools/virsh-completer-network.c | 44 +++ tools/virsh-completer-network.h | 4 + tools/virsh-domain.c | 6 +- tools/virsh-domain.h | 4 + tools/virsh-host.c| 1 + tools/virsh-interface.c | 3 + tools/virsh-network.c | 15 ++- 14 files changed, 349 insertions(+), 15 deletions(-) -- 2.26.0
[libvirt] [PATCH v2 03/13] virsh: Add serial/parallel device name completion to console command
Signed-off-by: Lin Ma
---
tools/virsh-completer-domain.c | 55 ++
tools/virsh-completer-domain.h | 4 +++
tools/virsh-domain.c | 1 +
3 files changed, 60 insertions(+)
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
index f653471b89..dc66317561 100644
--- a/tools/virsh-completer-domain.c
+++ b/tools/virsh-completer-domain.c
@@ -656,3 +656,58 @@ virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
virshDomainFree(dom);
return ret;
}
+
+char **
+virshDomainConsoleCompleter(vshControl *ctl,
+const vshCmd *cmd,
+unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+g_autoptr(xmlDoc) xmldoc = NULL;
+g_autoptr(xmlXPathContext) ctxt = NULL;
+int nserials;
+int nparallels;
+g_autofree xmlNodePtr *serials = NULL;
+g_autofree xmlNodePtr *parallels = NULL;
+size_t i;
+size_t offset = 0;
+VIR_AUTOSTRINGLIST tmp = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+if (virshDomainGetXML(ctl, cmd, 0, , ) < 0)
+return NULL;
+
+nserials = virXPathNodeSet("./devices/serial", ctxt, );
+if (nserials < 0)
+return NULL;
+
+nparallels = virXPathNodeSet("./devices/parallel", ctxt, );
+if (nparallels < 0)
+return NULL;
+
+tmp = g_new0(char *, nserials + nparallels + 1);
+
+for (i = 0; i < nserials; i++) {
+ctxt->node = serials[i];
+
+if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty"))
+continue;
+
+tmp[offset++] = virXPathString("string(./alias/@name)", ctxt);
+}
+
+for (i = 0; i < nparallels; i++) {
+ctxt->node = parallels[i];
+
+if (STRNEQ(virXPathString("string(./@type)", ctxt), "pty"))
+continue;
+
+tmp[offset++] = virXPathString("string(./alias/@name)", ctxt);
+}
+
+return g_steal_pointer();
+}
diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
index d5021f6aa6..02fea2fe94 100644
--- a/tools/virsh-completer-domain.h
+++ b/tools/virsh-completer-domain.h
@@ -94,3 +94,7 @@ char ** virshDomainCpulistCompleter(vshControl *ctl,
char ** virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshDomainConsoleCompleter(vshControl *ctl,
+const vshCmd *cmd,
+unsigned int flags);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 675d96440d..022dbdca3c 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2960,6 +2960,7 @@ static const vshCmdOptDef opts_console[] = {
VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
{.name = "devname", /* sc_prohibit_devname */
.type = VSH_OT_STRING,
+ .completer = virshDomainConsoleCompleter,
.help = N_("character device name")
},
{.name = "force",
--
2.26.0
[libvirt] [PATCH v2 08/13] vsh: Fix completion error in case of multiple mac addresses
We know that the bash completer automatically handle colon by preceding
it with an escape character backslash.
While our bash autompletion file vsh completes multiple items, In case
there're multiple items which have same prefix and the content of completion
items contain colon(say mac address), The vsh needs to correctly hands
the backslash which are added by bash completer, Otherwise the completion
won't be successful. This patch fixes this problem.
e.g.:
# virsh domiflist --domain VM
Interface Type SourceModelMAC
-
vnet0 network default virtio 52:54:00:fb:7b:f5
vnet1 bridgebr0 virtio 52:54:00:80:1b:21
Before:
# virsh detach-interface --domain VM --mac
# virsh detach-interface --domain VM --mac 52\:54\:00\:
After:
# virsh detach-interface --domain VM --mac
# virsh detach-interface --domain VM --mac 52\:54\:00\:
52:54:00:80:1b:21 52:54:00:fb:7b:f5
# virsh detach-interface --domain VM --mac 52\:54\:00\:
Signed-off-by: Lin Ma
---
tools/bash-completion/vsh | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/bash-completion/vsh b/tools/bash-completion/vsh
index 8493cad28b..fb38e8616f 100644
--- a/tools/bash-completion/vsh
+++ b/tools/bash-completion/vsh
@@ -39,6 +39,7 @@ _vsh_complete()
fi
INPUT=( "${COMP_WORDS[@]:$i:$COMP_CWORD}" )
+INPUT[-1]=${INPUT[-1]//\\:/:}
# Uncomment these lines for easy debug.
#echo;
--
2.26.0
[libvirt] [PATCH v2 12/13] virsh: Add lifecycle type completion to set-lifecycle-action command
Signed-off-by: Lin Ma
---
tools/virsh-completer-domain.c | 20
tools/virsh-completer-domain.h | 4
tools/virsh-domain.c | 1 +
3 files changed, 25 insertions(+)
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
index b8b6c74f8b..b1b670ffc5 100644
--- a/tools/virsh-completer-domain.c
+++ b/tools/virsh-completer-domain.c
@@ -731,3 +731,23 @@ virshDomainSignalCompleter(vshControl *ctl G_GNUC_UNUSED,
return g_steal_pointer();
}
+
+char **
+virshDomainLifecycleCompleter(vshControl *ctl G_GNUC_UNUSED,
+ const vshCmd *cmd G_GNUC_UNUSED,
+ unsigned int flags)
+{
+size_t i = 0;
+VIR_AUTOSTRINGLIST tmp = NULL;
+
+virCheckFlags(0, NULL);
+
+tmp = g_new0(char *, VIR_DOMAIN_LIFECYCLE_LAST + 1);
+
+for (i = 0; i < VIR_DOMAIN_LIFECYCLE_LAST; i++) {
+const char *name = virDomainLifecycleTypeToString(i);
+tmp[i] = g_strdup(name);
+}
+
+return g_steal_pointer();
+}
diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
index cdec66f23e..70f6e30947 100644
--- a/tools/virsh-completer-domain.h
+++ b/tools/virsh-completer-domain.h
@@ -102,3 +102,7 @@ char ** virshDomainConsoleCompleter(vshControl *ctl,
char ** virshDomainSignalCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshDomainLifecycleCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1406bb4583..be91bd48fc 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5717,6 +5717,7 @@ static const vshCmdOptDef opts_setLifecycleAction[] = {
{.name = "type",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshDomainLifecycleCompleter,
.help = N_("lifecycle type to modify")
},
{.name = "action",
--
2.26.0
[libvirt] [PATCH v2 13/13] Add lifecycle action completion to set-lifecycle-action command
Signed-off-by: Lin Ma
---
tools/virsh-completer-domain.c | 20
tools/virsh-completer-domain.h | 4
tools/virsh-domain.c | 1 +
3 files changed, 25 insertions(+)
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
index b1b670ffc5..a44ee65642 100644
--- a/tools/virsh-completer-domain.c
+++ b/tools/virsh-completer-domain.c
@@ -751,3 +751,23 @@ virshDomainLifecycleCompleter(vshControl *ctl
G_GNUC_UNUSED,
return g_steal_pointer();
}
+
+char **
+virshDomainLifecycleActionCompleter(vshControl *ctl G_GNUC_UNUSED,
+const vshCmd *cmd G_GNUC_UNUSED,
+unsigned int flags)
+{
+size_t i = 0;
+VIR_AUTOSTRINGLIST tmp = NULL;
+
+virCheckFlags(0, NULL);
+
+tmp = g_new0(char *, VIR_DOMAIN_LIFECYCLE_ACTION_LAST + 1);
+
+for (i = 0; i < VIR_DOMAIN_LIFECYCLE_ACTION_LAST; i++) {
+const char *action = virDomainLifecycleActionTypeToString(i);
+tmp[i] = g_strdup(action);
+}
+
+return g_steal_pointer();
+}
diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
index 70f6e30947..92c57bce75 100644
--- a/tools/virsh-completer-domain.h
+++ b/tools/virsh-completer-domain.h
@@ -106,3 +106,7 @@ char ** virshDomainSignalCompleter(vshControl *ctl,
char ** virshDomainLifecycleCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshDomainLifecycleActionCompleter(vshControl *ctl,
+const vshCmd *cmd,
+unsigned int flags);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index be91bd48fc..12b35c037d 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5723,6 +5723,7 @@ static const vshCmdOptDef opts_setLifecycleAction[] = {
{.name = "action",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshDomainLifecycleActionCompleter,
.help = N_("lifecycle action to set")
},
VIRSH_COMMON_OPT_DOMAIN_CONFIG,
--
2.26.0
[libvirt] [PATCH v2 07/13] virsh: Add interface name completion to iface-mac command
Signed-off-by: Lin Ma
---
tools/virsh-interface.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
index ae234e1e22..858052d341 100644
--- a/tools/virsh-interface.c
+++ b/tools/virsh-interface.c
@@ -441,6 +441,7 @@ static const vshCmdOptDef opts_interface_mac[] = {
{.name = "interface",
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
+ .completer = virshInterfaceNameCompleter,
.help = N_("interface name")
},
{.name = NULL}
--
2.26.0
[libvirt] [PATCH v2 01/13] virsh: Add vcpu list completion to guestvcpus command
Signed-off-by: Lin Ma
---
tools/virsh-completer-domain.c | 75 ++
tools/virsh-completer-domain.h | 4 ++
tools/virsh-domain.c | 1 +
3 files changed, 80 insertions(+)
diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
index c657627ac1..f653471b89 100644
--- a/tools/virsh-completer-domain.c
+++ b/tools/virsh-completer-domain.c
@@ -30,6 +30,7 @@
#include "virstring.h"
#include "virxml.h"
#include "virperf.h"
+#include "virbitmap.h"
char **
virshDomainNameCompleter(vshControl *ctl,
@@ -581,3 +582,77 @@ virshDomainCpulistCompleter(vshControl *ctl,
return virshCommaStringListComplete(cpuid, (const char **)cpulist);
}
+
+char **
+virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags)
+{
+virDomainPtr dom;
+bool enable = vshCommandOptBool(cmd, "enable");
+bool disable = vshCommandOptBool(cmd, "disable");
+virTypedParameterPtr params = NULL;
+unsigned int nparams = 0;
+int dummy;
+size_t i;
+size_t offset = 0;
+int nvcpus;
+VIR_AUTOSTRINGLIST cpulist = NULL;
+g_autoptr(virBitmap) vcpus = NULL;
+g_autofree unsigned char *vcpumap = NULL;
+g_autofree char *onlineVcpuStr = NULL;
+const char *vcpuid = NULL;
+char **ret = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return NULL;
+
+if (vshCommandOptStringQuiet(ctl, cmd, "cpulist", ) < 0)
+goto cleanup;
+
+/* retrieve vcpu count from the guest instead of the hypervisor */
+if ((nvcpus = virDomainGetVcpusFlags(dom, VIR_DOMAIN_VCPU_GUEST |
+ VIR_DOMAIN_VCPU_MAXIMUM)) < 0)
+goto cleanup;
+
+if (!enable && !disable) {
+cpulist = g_new0(char *, nvcpus + 1);
+for (i = 0; i < nvcpus; i++)
+cpulist[i] = g_strdup_printf("%zu", i);
+} else {
+if (virDomainGetGuestVcpus(dom, , , 0) < 0)
+goto cleanup;
+onlineVcpuStr = vshGetTypedParamValue(ctl, [1]);
+if (virBitmapParse(onlineVcpuStr, , nvcpus) < 0)
+goto cleanup;
+
+if (virBitmapToData(vcpus, , ) < 0)
+goto cleanup;
+if (enable) {
+cpulist = g_new0(char *, nvcpus - virBitmapCountBits(vcpus) + 1);
+for (i = 0; i < nvcpus; i++) {
+if (VIR_CPU_USED(vcpumap, i) != 0)
+continue;
+
+cpulist[offset++] = g_strdup_printf("%zu", i);
+}
+} else if (disable) {
+cpulist = g_new0(char *, virBitmapCountBits(vcpus) + 1);
+for (i = 0; i < nvcpus; i++) {
+if (VIR_CPU_USED(vcpumap, i) == 0)
+continue;
+
+cpulist[offset++] = g_strdup_printf("%zu", i);
+}
+}
+}
+
+ret = virshCommaStringListComplete(vcpuid, (const char **)cpulist);
+
+ cleanup:
+virTypedParamsFree(params, nparams);
+virshDomainFree(dom);
+return ret;
+}
diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
index d38efd5ea8..d5021f6aa6 100644
--- a/tools/virsh-completer-domain.h
+++ b/tools/virsh-completer-domain.h
@@ -90,3 +90,7 @@ char ** virshDomainVcpulistCompleter(vshControl *ctl,
char ** virshDomainCpulistCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshDomainVcpulistViaAgentCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1ae936c6b2..675d96440d 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -7436,6 +7436,7 @@ static const vshCmdOptDef opts_guestvcpus[] = {
VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
{.name = "cpulist",
.type = VSH_OT_STRING,
+ .completer = virshDomainVcpulistViaAgentCompleter,
.help = N_("list of cpus to enable or disable")
},
{.name = "enable",
--
2.26.0
[libvirt] [PATCH v2 09/13] virsh: Add mac completion to net-dhcp-leases command
Signed-off-by: Lin Ma
---
tools/virsh-completer-network.c | 44 +
tools/virsh-completer-network.h | 4 +++
tools/virsh-network.c | 15 +++
3 files changed, 58 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-completer-network.c b/tools/virsh-completer-network.c
index 73f7115ab2..3094b9d5d6 100644
--- a/tools/virsh-completer-network.c
+++ b/tools/virsh-completer-network.c
@@ -174,3 +174,47 @@ virshNetworkUUIDCompleter(vshControl *ctl,
g_free(nets);
return ret;
}
+
+
+char **
+virshNetworkDhcpMacCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags)
+{
+virshControlPtr priv = ctl->privData;
+virNetworkDHCPLeasePtr *leases = NULL;
+virNetworkPtr network = NULL;
+int nleases;
+size_t i = 0;
+char **ret = NULL;
+VIR_AUTOSTRINGLIST tmp = NULL;
+
+virCheckFlags(0, NULL);
+
+if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
+return NULL;
+
+if (!(network = virshCommandOptNetwork(ctl, cmd, NULL)))
+return NULL;
+
+if ((nleases = virNetworkGetDHCPLeases(network, NULL, , flags)) < 0)
+goto cleanup;
+
+tmp = g_new0(char *, nleases + 1);
+
+for (i = 0; i < nleases; i++) {
+virNetworkDHCPLeasePtr lease = leases[i];
+tmp[i] = g_strdup(lease->mac);
+}
+
+ret = g_steal_pointer();
+
+ cleanup:
+if (leases) {
+for (i = 0; i < nleases; i++)
+virNetworkDHCPLeaseFree(leases[i]);
+VIR_FREE(leases);
+}
+virNetworkFree(network);
+return ret;
+}
diff --git a/tools/virsh-completer-network.h b/tools/virsh-completer-network.h
index 8910e4525c..80df5c468e 100644
--- a/tools/virsh-completer-network.h
+++ b/tools/virsh-completer-network.h
@@ -37,3 +37,7 @@ char ** virshNetworkPortUUIDCompleter(vshControl *ctl,
char ** virshNetworkUUIDCompleter(vshControl *ctl,
const vshCmd *cmd,
unsigned int flags);
+
+char ** virshNetworkDhcpMacCompleter(vshControl *ctl,
+ const vshCmd *cmd,
+ unsigned int flags);
diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index 745afc537d..30b231f7d6 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -61,6 +61,15 @@
.completer_flags = cflags, \
}
+#define VIRSH_COMMON_OPT_NETWORK_DHCP_MAC(_helpstr, cflags) \
+{.name = "mac", \
+ .type = VSH_OT_STRING, \
+ .flags = VSH_OFLAG_NONE, \
+ .help = _helpstr, \
+ .completer = virshNetworkDhcpMacCompleter, \
+ .completer_flags = cflags, \
+}
+
virNetworkPtr
virshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd,
@@ -1373,11 +1382,7 @@ static const vshCmdInfo info_network_dhcp_leases[] = {
static const vshCmdOptDef opts_network_dhcp_leases[] = {
VIRSH_COMMON_OPT_NETWORK_FULL(VIR_CONNECT_LIST_NETWORKS_ACTIVE),
-{.name = "mac",
- .type = VSH_OT_STRING,
- .flags = VSH_OFLAG_NONE,
- .help = N_("MAC address")
-},
+VIRSH_COMMON_OPT_NETWORK_DHCP_MAC(N_("MAC address"), 0),
{.name = NULL}
};
--
2.26.0
Re: [PATCH] util: convert char pointers to use g_autofree
On Mon, Nov 09, 2020 at 04:34:22PM -0600, Ryan Gahagan wrote:
> From: Barrett Schonefeld
>
> additional conversions to the GLib API in src/util per issue #11.
>
> files updated are:
> - src/util/vircgroupv1.c
> - src/util/virhostcpu.c
> - src/util/virlockspace.c
> - src/util/virmacmap.c
> - src/util/virresctrl.c
> - src/util/virsysinfo.c
>
> Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11
>
> Signed-off-by: bschoney
> Signed-off-by: Barrett Schonefeld
> ---
> src/util/vircgroupv1.c | 3 +--
> src/util/virhostcpu.c | 4 +---
> src/util/virlockspace.c | 6 ++
> src/util/virmacmap.c| 3 +--
> src/util/virresctrl.c | 25 -
> src/util/virsysinfo.c | 9 +++--
> 6 files changed, 16 insertions(+), 34 deletions(-)
>
> diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
> index 731e9d61d4..984cd50409 100644
> --- a/src/util/vircgroupv1.c
> +++ b/src/util/vircgroupv1.c
> @@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
> unsigned long long *unevictable)
> {
> int ret = -1;
> -char *stat = NULL;
> +g_autofree char *stat = NULL;
> char *line = NULL;
> unsigned long long cacheVal = 0;
> unsigned long long activeAnonVal = 0;
> @@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
> ret = 0;
>
> cleanup:
> -VIR_FREE(stat);
> return ret;
Usually the whole exercise of using g_autofree or g_autoptr is to
simplify the code by removing all the `goto` and cleanup labels
where it is possible.
Otherwise looks good.
Pavel
signature.asc
Description: PGP signature
