On 11/17/2014 06:26 PM, Tomoki Sekiyama wrote:

virDomainGetFSInfo returns a list of filesystems information mounted in the guest, which contains mountpoints, device names, filesystem types, and device aliases named by libvirt. This will be useful, for example, to specify mountpoints to fsfreeze when taking snapshot of a part of disks.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- include/libvirt/libvirt-domain.h | 21 ++++++++++++ src/driver-hypervisor.h | 6 +++ src/libvirt.c | 66 ++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 +++ 4 files changed, 99 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 1fac2a3..c889cd0 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3456,6 +3456,27 @@ int virDomainFSThaw(virDomainPtr dom, unsigned int nmountpoints, unsigned int flags);

+/** + * virDomainFSInfo: + * + * The data structure containing mounted file systems within a guset + * + */ +typedef struct _virDomainFSInfo virDomainFSInfo; +typedef virDomainFSInfo *virDomainFSInfoPtr; +struct _virDomainFSInfo { + char *mountpoint; /* path to mount point */ + char *name; /* device name in the guest (e.g. "sda1") */ + char *type; /* filesystem type */ + char **devAlias; /* NULL-terminated array of disk device aliases */ +}; + +void virDomainFSInfoFree(virDomainFSInfoPtr info); + +int virDomainGetFSInfo(virDomainPtr dom, + virDomainFSInfoPtr **info, + unsigned int flags); + int virDomainGetTime(virDomainPtr dom, long long *seconds, unsigned int *nseconds, diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index ad66629..9f26b13 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1139,6 +1139,11 @@ typedef int unsigned int flags);

typedef int +(*virDrvDomainGetFSInfo)(virDomainPtr dom, + virDomainFSInfoPtr **info, + unsigned int flags); + +typedef int (*virDrvNodeGetFreePages)(virConnectPtr conn, unsigned int npages, unsigned int *pages, @@ -1390,6 +1395,7 @@ struct _virHypervisorDriver { virDrvConnectGetDomainCapabilities connectGetDomainCapabilities; virDrvConnectGetAllDomainStats connectGetAllDomainStats; virDrvNodeAllocPages nodeAllocPages; + virDrvDomainGetFSInfo domainGetFSInfo; };

diff --git a/src/libvirt.c b/src/libvirt.c index 3abedb4..34eaed1 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1401,6 +1401,72 @@ virConnectOpenAuth(const char *name,

/** + * virDomainGetFSInfo: + * @dom: a domain object + * @info: a pointer to a variable to store an array of mount points information + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Get a list of mapping informaiton for each mounted file systems within the

Still have the typo on informaiton (information)

+ * specified guest and the disks. + * + * Returns the number of returned mount points, or -1 in case of error. + * On success, the array of the information is stored into @info. The caller is + * responsible for calling virDomainFSInfoFree() on each array element, then + * calling free() on @info. On error, @info is set to NULL. + */ +int +virDomainGetFSInfo(virDomainPtr dom, + virDomainFSInfoPtr **info, + unsigned int flags) +{ + VIR_DOMAIN_DEBUG(dom, "info=%p, flags=%x", info, flags); + + virResetLastError(); + + virCheckDomainReturn(dom, -1); + virCheckReadOnlyGoto(dom->conn->flags, error); +

You also missed: virCheckNonNullArgGoto(info, error); *info = NULL;

+ if (dom->conn->driver->domainGetFSInfo) { + int ret = dom->conn->driver->domainGetFSInfo(dom, info, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dom->conn); + return -1; +} + + +/** + * virDomainFSInfoFree: + * @info: pointer to a FSInfo object + * + * Frees all the memory occupied by @info. + */ +void +virDomainFSInfoFree(virDomainFSInfoPtr info) +{ + char **alias; + + if (!info) + return; + + VIR_FREE(info->mountpoint); + VIR_FREE(info->name); + VIR_FREE(info->type); + + for (alias = info->devAlias; alias && *alias; alias++) + VIR_FREE(*alias); + VIR_FREE(info->devAlias); +} + + + +/** * virConnectClose: * @conn: pointer to the hypervisor connection *

I will squash the following in: diff --git a/src/libvirt.c b/src/libvirt.c index 34eaed1..6b4786d 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1406,7 +1406,7 @@ virConnectOpenAuth(const char *name, * @info: a pointer to a variable to store an array of mount points information * @flags: extra flags; not used yet, so callers should always pass 0 * - * Get a list of mapping informaiton for each mounted file systems within the + * Get a list of mapping information for each mounted file systems within the * specified guest and the disks. * * Returns the number of returned mount points, or -1 in case of error. @@ -1425,6 +1425,8 @@ virDomainGetFSInfo(virDomainPtr dom, virCheckDomainReturn(dom, -1); virCheckReadOnlyGoto(dom->conn->flags, error); + virCheckNonNullArgGoto(info, error); + *info = NULL; if (dom->conn->driver->domainGetFSInfo) { int ret = dom->conn->driver->domainGetFSInfo(dom, info, flags);

diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 5f95802..e4c2df1 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -684,4 +684,10 @@ LIBVIRT_1.2.9 { virNodeAllocPages; } LIBVIRT_1.2.8;

+LIBVIRT_1.2.11 { + global: + virDomainFSInfoFree; + virDomainGetFSInfo; +} LIBVIRT_1.2.9; + # .... define new API here using predicted next version number ....

On 11/20/14, 14:09 , "Eric Blake" <eblake@redhat.com> wrote:

On 11/17/2014 04:26 PM, Tomoki Sekiyama wrote:

...

virDomainGetFSInfo returns a list of filesystems information mounted in the guest, which contains mountpoints, device names, filesystem types, and device aliases named by libvirt. This will be useful, for example, to specify mountpoints to fsfreeze when taking snapshot of a part of disks.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- include/libvirt/libvirt-domain.h | 21 ++++++++++++ src/driver-hypervisor.h | 6 +++ src/libvirt.c | 66 ++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 +++ 4 files changed, 99 insertions(+)

...

+++ b/include/libvirt/libvirt-domain.h @@ -3456,6 +3456,27 @@ int virDomainFSThaw(virDomainPtr dom, unsigned int nmountpoints, unsigned int flags);

+/** + * virDomainFSInfo: + * + * The data structure containing mounted file systems within a guset + * + */ +typedef struct _virDomainFSInfo virDomainFSInfo; +typedef virDomainFSInfo *virDomainFSInfoPtr; +struct _virDomainFSInfo { + char *mountpoint; /* path to mount point */ + char *name; /* device name in the guest (e.g. "sda1") */ + char *type; /* filesystem type */ + char **devAlias; /* NULL-terminated array of disk device aliases */ +};

Is it worth also having a size_t ndevAlias that says how long the array is? It may make client life easier if they have an up-front count.

OK, I¹ll add ndevAlias and iterate the devAlias array using that counter. Thanks, Tomoki Sekiyama

On 11/17/2014 06:26 PM, Tomoki Sekiyama wrote:

Add daemon and driver code to (de-)serialize virDomainFSInfo.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- daemon/remote.c | 117 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 92 +++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 32 +++++++++++ src/remote_protocol-structs | 21 ++++++++ src/rpc/gendispatch.pl | 1 5 files changed, 262 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 1d7082e..9b89fd0 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6336,6 +6336,123 @@ remoteDispatchNodeAllocPages(virNetServerPtr server ATTRIBUTE_UNUSED, }

+static int +remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_get_fsinfo_args *args, + remote_domain_get_fsinfo_ret *ret) +{ + int rv = -1; + size_t i, j; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + virDomainFSInfoPtr *info = NULL; + virDomainPtr dom = NULL; + remote_domain_fsinfo *dst; + char **alias; + int ninfo = 0, ndisk; + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + if ((ninfo = virDomainGetFSInfo(dom, &info, args->flags)) < 0) + goto cleanup;

[2] Since ninfo can be negative here... rv == -1 and ret->info.info_val is whatever it was passed in...

+ + if (ninfo > REMOTE_DOMAIN_FSINFO_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many mountpoints in fsinfo: %d for limit %d"), + ninfo, REMOTE_DOMAIN_FSINFO_MAX); + goto cleanup; + } + + if (info && ninfo) {

Here again, check both info & ninfo here causes Coverity to complain later [1]. If we just check 'ninfo' here, then the else condition takes care of setting the ret->info.info_val and ret->info.info_len. Thus this if (ninfo) can handle things when 'info' has something returned.

+ if (VIR_ALLOC_N(ret->info.info_val, ninfo) < 0) + goto cleanup; + + ret->info.info_len = ninfo; + + for (i = 0; i < ninfo; i++) { + dst = &ret->info.info_val[i]; + if (VIR_STRDUP(dst->mountpoint, info[i]->mountpoint) < 0) + goto cleanup; + + if (VIR_STRDUP(dst->name, info[i]->name) < 0) + goto cleanup; + + if (VIR_STRDUP(dst->type, info[i]->type) < 0) + goto cleanup; + + ndisk = 0; + for (alias = info[i]->devAlias; alias && *alias; alias++) + ndisk++; + + if (ndisk > REMOTE_DOMAIN_FSINFO_DISKS_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many disks in fsinfo: %d for limit %d"), + ndisk, REMOTE_DOMAIN_FSINFO_DISKS_MAX); + goto cleanup; + } + + if (ndisk > 0) { + if (VIR_ALLOC_N(dst->dev_aliases.dev_aliases_val, ndisk) < 0) + goto cleanup; + + for (j = 0; j < ndisk; j++) { + if (VIR_STRDUP(dst->dev_aliases.dev_aliases_val[j], + info[i]->devAlias[j]) < 0) + goto cleanup; + } + + dst->dev_aliases.dev_aliases_len = ndisk; + } else { + dst->dev_aliases.dev_aliases_val = NULL; + dst->dev_aliases.dev_aliases_len = 0; + } + } + + } else { + ret->info.info_len = 0; + ret->info.info_val = NULL; + } + + ret->ret = ninfo; + + rv = 0; + + cleanup: + if (rv < 0) { + virNetMessageSaveError(rerr); + + if (ret->info.info_val) { + for (i = 0; i < ninfo; i++) {

[2] Coverity has issues here because we can get here with ninfo < 0. Since it seems we really only care to enter this loop when ret->info.info_val && ninfo > 0, we can do that

+ dst = &ret->info.info_val[i]; + VIR_FREE(dst->mountpoint); + if (dst->dev_aliases.dev_aliases_val) { + for (j = 0; j < dst->dev_aliases.dev_aliases_len; j++) + VIR_FREE(dst->dev_aliases.dev_aliases_val[j]); + VIR_FREE(dst->dev_aliases.dev_aliases_val); + } + } + VIR_FREE(ret->info.info_val); + } + } + if (dom) + virDomainFree(dom); + if (ninfo >= 0) + for (i = 0; i < ninfo; i++) + virDomainFSInfoFree(info[i]);

[1] Because you checked 'ninfo' and 'info' above, Coverity complains here because you're not checking if 'info' (yes, even though we know it's obvious

+ VIR_FREE(info); + + return rv; +} + + /*----- Helpers. -----*/

/* get_nonnull_domain and get_nonnull_network turn an on-wire

I will squash the following in: diff --git a/daemon/remote.c b/daemon/remote.c index 9b89fd0..4439af7 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6371,7 +6371,7 @@ remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRI goto cleanup; } - if (info && ninfo) { + if (ninfo) { if (VIR_ALLOC_N(ret->info.info_val, ninfo) < 0) goto cleanup; @@ -6429,7 +6429,7 @@ remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRI if (rv < 0) { virNetMessageSaveError(rerr); - if (ret->info.info_val) { + if (ret->info.info_val && ninfo > 0) { for (i = 0; i < ninfo; i++) { dst = &ret->info.info_val[i]; VIR_FREE(dst->mountpoint);

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 04e5360..4c0758f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7829,6 +7829,97 @@ remoteNodeAllocPages(virConnectPtr conn, }

+static int +remoteDomainGetFSInfo(virDomainPtr dom, + virDomainFSInfoPtr **info, + unsigned int flags) +{ + int rv = -1; + size_t i, j, len; + struct private_data *priv = dom->conn->privateData; + remote_domain_get_fsinfo_args args; + remote_domain_get_fsinfo_ret ret; + remote_domain_fsinfo *src; + virDomainFSInfoPtr *info_ret = NULL; + + remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, dom); + + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + + if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_FSINFO, + (xdrproc_t)xdr_remote_domain_get_fsinfo_args, (char *)&args, + (xdrproc_t)xdr_remote_domain_get_fsinfo_ret, (char *)&ret) == -1) + goto done; + + if (ret.info.info_len > REMOTE_DOMAIN_FSINFO_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Too many mountpoints in fsinfo: %d for limit %d"), + ret.info.info_len, REMOTE_DOMAIN_FSINFO_MAX); + goto cleanup; + } + + if (info) { + if (!ret.info.info_len) { + *info = NULL; + rv = ret.ret; + goto cleanup; + } + + if (VIR_ALLOC_N(info_ret, ret.info.info_len) < 0) + goto cleanup; + + for (i = 0; i < ret.info.info_len; i++) { + src = &ret.info.info_val[i]; + + if (VIR_ALLOC(info_ret[i]) < 0) + goto cleanup; + + if (VIR_STRDUP(info_ret[i]->mountpoint, src->mountpoint) < 0) + goto cleanup; + + if (VIR_STRDUP(info_ret[i]->name, src->name) < 0) + goto cleanup; + + if (VIR_STRDUP(info_ret[i]->type, src->type) < 0) + goto cleanup; + + len = src->dev_aliases.dev_aliases_len; + if (len && + VIR_ALLOC_N(info_ret[i]->devAlias, len + 1) < 0) + goto cleanup; + + for (j = 0; j < len; j++) { + if (VIR_STRDUP(info_ret[i]->devAlias[j], + src->dev_aliases.dev_aliases_val[j]) < 0) + goto cleanup; + } + } + + *info = info_ret; + info_ret = NULL; + } + + rv = ret.ret; + + cleanup: + if (info_ret) { + for (i = 0; i < ret.info.info_len; i++) + virDomainFSInfoFree(info_ret[i]); + VIR_FREE(info_ret); + } + xdr_free((xdrproc_t)xdr_remote_domain_get_fsinfo_ret, + (char *) &ret); + + done: + remoteDriverUnlock(priv); + return rv; +} + + /* get_nonnull_domain and get_nonnull_network turn an on-wire * (name, uuid) pair into virDomainPtr or virNetworkPtr object. * These can return NULL if underlying memory allocations fail, @@ -8171,6 +8262,7 @@ static virHypervisorDriver hypervisor_driver = { .connectGetDomainCapabilities = remoteConnectGetDomainCapabilities, /* 1.2.7 */ .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8 */ .nodeAllocPages = remoteNodeAllocPages, /* 1.2.9 */ + .domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.10 */ };

static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index ebf4530..10c8068 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -250,6 +250,12 @@ const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 4096; /* Upper limit of message size for tunable event. */ const REMOTE_DOMAIN_EVENT_TUNABLE_MAX = 2048;

+/* Upper limit on number of mountpoints in fsinfo */ +const REMOTE_DOMAIN_FSINFO_MAX = 256; + +/* Upper limit on number of disks per mountpoint in fsinfo */ +const REMOTE_DOMAIN_FSINFO_DISKS_MAX = 256; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];

@@ -3111,6 +3117,24 @@ struct remote_connect_get_all_domain_stats_args { struct remote_connect_get_all_domain_stats_ret { remote_domain_stats_record retStats<REMOTE_DOMAIN_LIST_MAX>; }; + +struct remote_domain_fsinfo { + remote_nonnull_string mountpoint; + remote_nonnull_string name; + remote_nonnull_string type; + remote_nonnull_string dev_aliases<REMOTE_DOMAIN_FSINFO_DISKS_MAX>; /* (const char **) */ +}; + +struct remote_domain_get_fsinfo_args { + remote_nonnull_domain dom; + unsigned int flags; +}; + +struct remote_domain_get_fsinfo_ret { + remote_domain_fsinfo info<REMOTE_DOMAIN_FSINFO_MAX>; + unsigned int ret; +}; + /*----- Protocol. -----*/

/* Define the program number, protocol version and procedure numbers here. */ @@ -5506,5 +5530,11 @@ enum remote_procedure { * @generate: none * @acl: connect:write */ - REMOTE_PROC_NODE_ALLOC_PAGES = 347 + REMOTE_PROC_NODE_ALLOC_PAGES = 347, + + /** + * @generate: none + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_GET_FSINFO = 348 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 362baf9..6419102 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2579,6 +2579,26 @@ struct remote_connect_get_all_domain_stats_ret { remote_domain_stats_record * retStats_val; } retStats; }; +struct remote_domain_fsinfo { + remote_nonnull_string mountpoint; + remote_nonnull_string name; + remote_nonnull_string type; + struct { + u_int dev_aliases_len; + remote_nonnull_string * dev_aliases_val; + } dev_aliases; +}; +struct remote_domain_get_fsinfo_args { + remote_nonnull_domain dom; + u_int flags; +}; +struct remote_domain_get_fsinfo_ret { + struct { + u_int info_len; + remote_domain_fsinfo * info_val; + } info; + u_int ret; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -2927,4 +2947,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_BLOCK_COPY = 345, REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TUNABLE = 346, REMOTE_PROC_NODE_ALLOC_PAGES = 347, + REMOTE_PROC_DOMAIN_GET_FSINFO = 348, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index b38d5bb..80f35b3 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -66,6 +66,7 @@ sub fixup_name { $name =~ s/Fstrim$/FSTrim/; $name =~ s/Fsfreeze$/FSFreeze/; $name =~ s/Fsthaw$/FSThaw/; + $name =~ s/Fsinfo$/FSInfo/; $name =~ s/Scsi/SCSI/; $name =~ s/Wwn$/WWN/; $name =~ s/Dhcp$/DHCP/;

On 11/20/14, 14:12 , "Eric Blake" <eblake@redhat.com> wrote:

On 11/17/2014 04:26 PM, Tomoki Sekiyama wrote:

...

Add daemon and driver code to (de-)serialize virDomainFSInfo.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- daemon/remote.c | 117 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 92 +++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 32 +++++++++++ src/remote_protocol-structs | 21 ++++++++ src/rpc/gendispatch.pl | 1 5 files changed, 262 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 1d7082e..9b89fd0 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6336,6 +6336,123 @@ remoteDispatchNodeAllocPages(virNetServerPtr server ATTRIBUTE_UNUSED, }

+static int +remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_get_fsinfo_args *args, + remote_domain_get_fsinfo_ret *ret)

Are we sure we have to write this by hand? [1]

...

@@ -8171,6 +8262,7 @@ static virHypervisorDriver hypervisor_driver = { .connectGetDomainCapabilities = remoteConnectGetDomainCapabilities, /* 1.2.7 */ .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8 */ .nodeAllocPages = remoteNodeAllocPages, /* 1.2.9 */ + .domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.10 */

1.2.10 is wrong; the next release is 1.2.11.

...

};

static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index ebf4530..10c8068 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -250,6 +250,12 @@ const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 4096; /* Upper limit of message size for tunable event. */ const REMOTE_DOMAIN_EVENT_TUNABLE_MAX = 2048;

+/* Upper limit on number of mountpoints in fsinfo */ +const REMOTE_DOMAIN_FSINFO_MAX = 256; + +/* Upper limit on number of disks per mountpoint in fsinfo */ +const REMOTE_DOMAIN_FSINFO_DISKS_MAX = 256; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];

@@ -3111,6 +3117,24 @@ struct remote_connect_get_all_domain_stats_args { struct remote_connect_get_all_domain_stats_ret { remote_domain_stats_record retStats<REMOTE_DOMAIN_LIST_MAX>; }; + +struct remote_domain_fsinfo { + remote_nonnull_string mountpoint; + remote_nonnull_string name; + remote_nonnull_string type; + remote_nonnull_string dev_aliases<REMOTE_DOMAIN_FSINFO_DISKS_MAX>; /* (const char **) */

Can any of these values ever be NULL because the guest didn't provide them? If so, you want remote_string instead of remote_nonnull_string. It wasn't obvious to me in the 1/5 documentation whether values are guaranteed to be non-NULL.

These are always filled. If guest agent doesn¹t pass these values, the qemu driver will return an error.

...

+ + /** + * @generate: none + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_GET_FSINFO = 348

[1] Did you try any other values of @generate to see if things get transferred correctly without writing it by hand? Then again, looking at the structure you are transferring, it consists of mallocing an array of structures which themselves malloc an array of names, so I guess you are right that you have to manage it by hand (the generator probably doesn't do that).

Right, I¹ve tried code generation, but it couldn¹t handle dynamically allocated arrays in an array...

On 11/17/2014 06:27 PM, Tomoki Sekiyama wrote:

Get mounted filesystems list, which contains hardware info of disks and its controllers, from QEMU guest agent 2.2+. Then, convert the hardware info to corresponding device aliases for the disks.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- src/conf/domain_conf.c | 71 ++++++++++++++++++ src/conf/domain_conf.h | 6 ++ src/libvirt_private.syms | 1 src/qemu/qemu_agent.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 2 + src/qemu/qemu_driver.c | 48 ++++++++++++ 6 files changed, 306 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5f4b9f6..5972d7a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11114,6 +11114,60 @@ virDomainHostdevFind(virDomainDefPtr def, return *found ? i : -1; }

+static bool +virDomainDiskControllerMatch(int controller_type, int disk_bus) +{ + if (controller_type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && + disk_bus == VIR_DOMAIN_DISK_BUS_SCSI) + return true; + + if (controller_type == VIR_DOMAIN_CONTROLLER_TYPE_FDC && + disk_bus == VIR_DOMAIN_DISK_BUS_FDC) + return true; + + if (controller_type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && + disk_bus == VIR_DOMAIN_DISK_BUS_IDE) + return true; + + if (controller_type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && + disk_bus == VIR_DOMAIN_DISK_BUS_SATA) + return true; + + return false; +} + +int +virDomainDiskIndexByAddress(virDomainDefPtr def, + virDevicePCIAddressPtr pci_address, + unsigned int bus, unsigned int target, + unsigned int unit) +{ + virDomainDiskDefPtr vdisk; + virDomainControllerDefPtr controller = NULL; + size_t i; + int cidx; + + if ((cidx = virDomainControllerFindByPCIAddress(def, pci_address)) >= 0) + controller = def->controllers[cidx]; + + for (i = 0; i < def->ndisks; i++) { + vdisk = def->disks[i]; + if (vdisk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + virDevicePCIAddressEqual(&vdisk->info.addr.pci, pci_address)) + return i; + if (vdisk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + virDomainDeviceDriveAddressPtr drive = &vdisk->info.addr.drive; + if (controller && + virDomainDiskControllerMatch(controller->type, vdisk->bus) && + drive->controller == controller->idx && + drive->bus == bus && drive->target == target && + drive->unit == unit) + return i; + } + } + return -1; +} + int virDomainDiskIndexByName(virDomainDefPtr def, const char *name, bool allow_ambiguous) @@ -11403,6 +11457,23 @@ virDomainControllerFind(virDomainDefPtr def, return -1; }

+int +virDomainControllerFindByPCIAddress(virDomainDefPtr def, + virDevicePCIAddressPtr addr) +{ + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + virDomainDeviceInfoPtr info = &def->controllers[i]->info; + + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + virDevicePCIAddressEqual(&info->addr.pci, addr)) + return i; + } + + return -1; +} + virDomainControllerDefPtr virDomainControllerRemove(virDomainDefPtr def, size_t i) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 530a3ca..6164588 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2461,6 +2461,10 @@ int virDomainEmulatorPinDel(virDomainDefPtr def);

void virDomainRNGDefFree(virDomainRNGDefPtr def);

+int virDomainDiskIndexByAddress(virDomainDefPtr def, + virDevicePCIAddressPtr pci_controller, + unsigned int bus, unsigned int target, + unsigned int unit); int virDomainDiskIndexByName(virDomainDefPtr def, const char *name, bool allow_ambiguous); const char *virDomainDiskPathByName(virDomainDefPtr, const char *name); @@ -2529,6 +2533,8 @@ int virDomainControllerInsert(virDomainDefPtr def, void virDomainControllerInsertPreAlloced(virDomainDefPtr def, virDomainControllerDefPtr controller); int virDomainControllerFind(virDomainDefPtr def, int type, int idx); +int virDomainControllerFindByPCIAddress(virDomainDefPtr def, + virDevicePCIAddressPtr addr); virDomainControllerDefPtr virDomainControllerRemove(virDomainDefPtr def, size_t i);

int virDomainLeaseIndex(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0864618..16d4311 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -234,6 +234,7 @@ virDomainDiskGetDriver; virDomainDiskGetFormat; virDomainDiskGetSource; virDomainDiskGetType; +virDomainDiskIndexByAddress; virDomainDiskIndexByName; virDomainDiskInsert; virDomainDiskInsertPreAlloced; diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 8df1330..f6432cc 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1777,3 +1777,181 @@ qemuAgentSetTime(qemuAgentPtr mon, virJSONValueFree(reply); return ret; } + + +int +qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, + virDomainDefPtr vmdef) +{ + size_t i, j, k; + int ret = -1; + int ndata = 0, ndisk; + char **alias; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + virDomainFSInfoPtr *info_ret = NULL; + virDevicePCIAddress pci_address; + + cmd = qemuAgentMakeCommand("guest-get-fsinfo", NULL); + if (!cmd) + return ret; + + if (qemuAgentCommand(mon, cmd, &reply, true, + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) + goto cleanup; + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s' missing in reply of guest-get-fsinfo"), + "return");

Not exactly what I had in mind, but I'll fix these... There's also an indent issue here (cut-n-paste from some other error message I assume) There was also a check-syntax error or two with those multiline messages...

+ goto cleanup; + } + + if (data->type != VIR_JSON_TYPE_ARRAY) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("guest-get-fsinfo '%s' data was not an array"), + "return"); + goto cleanup; + } + + ndata = virJSONValueArraySize(data); + if (!ndata) { + ret = 0; + goto cleanup; + } + if (VIR_ALLOC_N(info_ret, ndata) < 0) + goto cleanup; + + for (i = 0; i < ndata; i++) { + /* Reverse the order to arrange in mount order */ + virJSONValuePtr entry = virJSONValueArrayGet(data, ndata - 1 - i); + + if (!entry) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("array element '%zd' of '%d' missing in" + "guest-get-fsinfo '%s' data"), + i, ndata, "return"); + goto cleanup; + } + + if (VIR_ALLOC(info_ret[i]) < 0) + goto cleanup; + + if (VIR_STRDUP(info_ret[i]->mountpoint, + virJSONValueObjectGetString(entry, "mountpoint")) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s' missing in reply of guest-get-fsinfo"), + "mountpoint"); + goto cleanup; + } + + if (VIR_STRDUP(info_ret[i]->name, + virJSONValueObjectGetString(entry, "name")) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s' missing in reply of guest-get-fsinfo"), + "name"); + goto cleanup; + } + + if (VIR_STRDUP(info_ret[i]->type, + virJSONValueObjectGetString(entry, "type")) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s' missing in reply of guest-get-fsinfo"), + "type"); + goto cleanup; + } + + if (!(entry = virJSONValueObjectGet(entry, "disk"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s' missing in reply of guest-get-fsinfo"), + "disk"); + goto cleanup; + } + + if (entry->type != VIR_JSON_TYPE_ARRAY) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("guest-get-fsinfo '%s' data was not an array"), + "disk"); + goto cleanup; + } + + ndisk = virJSONValueArraySize(entry); + if (!ndisk) + continue; + if (VIR_ALLOC_N(info_ret[i]->devAlias, ndisk + 1) < 0) + goto cleanup; + + alias = info_ret[i]->devAlias; + for (j = 0; j < ndisk; j++) { + virJSONValuePtr disk = virJSONValueArrayGet(entry, j); + virJSONValuePtr pci; + int diskaddr[3], pciaddr[4], idx; + const char *diskaddr_comp[] = {"bus", "target", "unit"}; + const char *pciaddr_comp[] = {"domain", "bus", "slot", "function"}; + + if (!disk) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("array element '%zd' of '%d' missing in" + "guest-get-fsinfo '%s' data"), + j, ndisk, "disk"); + goto cleanup; + } + + if (!(pci = virJSONValueObjectGet(disk, "pci-controller"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s' missing in guest-get-fsinfo " + "'disk' data"), "pci-controller"); + goto cleanup; + } + + for (k = 0; k < 3; k++) { + if (virJSONValueObjectGetNumberInt( + disk, diskaddr_comp[k], &diskaddr[k]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s' missing in guest-get-fsinfo " + "'disk' data"), diskaddr_comp[k]); + goto cleanup; + } + } + for (k = 0; k < 4; k++) { + if (virJSONValueObjectGetNumberInt( + pci, pciaddr_comp[k], &pciaddr[k]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s' missing in guest-get-fsinfo " + "'pci-address' data"), pciaddr_comp[k]); + goto cleanup; + } + } + + pci_address.domain = pciaddr[0]; + pci_address.bus = pciaddr[1]; + pci_address.slot = pciaddr[2]; + pci_address.function = pciaddr[3]; + if ((idx = virDomainDiskIndexByAddress( + vmdef, &pci_address, + diskaddr[0], diskaddr[1], diskaddr[2])) < 0) + continue; + + if (VIR_STRDUP(*alias, vmdef->disks[idx]->dst) < 0) + goto cleanup; + + if (*alias) + alias++; + } + } + + *info = info_ret; + info_ret = NULL; + ret = ndata; + + cleanup: + if (info_ret) { + for (i = 0; i < ndata; i++) + virDomainFSInfoFree(info_ret[i]); + VIR_FREE(info_ret); + } + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +}

I'll squash the following in (hopefully I got the cut-n-paste right): diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index f6432cc..dcbeee8 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1802,16 +1802,15 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr goto cleanup; if (!(data = virJSONValueObjectGet(reply, "return"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%s' missing in reply of guest-get-fsinfo"), - "return"); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest-get-fsinfo reply was missing return data")) goto cleanup; } if (data->type != VIR_JSON_TYPE_ARRAY) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("guest-get-fsinfo '%s' data was not an array"), - "return"); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest-get-fsinfo return information was not " + "an array")); goto cleanup; } @@ -1830,8 +1829,8 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr ** if (!entry) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("array element '%zd' of '%d' missing in" - "guest-get-fsinfo '%s' data"), - i, ndata, "return"); + _("array element '%zd' of '%d' missing in " + "guest-get-fsinfo return data"), + i, ndata); goto cleanup; } @@ -1840,39 +1839,35 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr if (VIR_STRDUP(info_ret[i]->mountpoint, virJSONValueObjectGetString(entry, "mountpoint")) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%s' missing in reply of guest-get-fsinfo"), - "mountpoint"); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'mountpoint' missing in reply of " + "guest-get-fsinfo")); goto cleanup; } if (VIR_STRDUP(info_ret[i]->name, virJSONValueObjectGetString(entry, "name")) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%s' missing in reply of guest-get-fsinfo"), - "name"); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'name' missing in reply of guest-get-fsinfo")); goto cleanup; } if (VIR_STRDUP(info_ret[i]->type, virJSONValueObjectGetString(entry, "type")) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%s' missing in reply of guest-get-fsinfo"), - "type"); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'type' missing in reply of guest-get-fsinfo")); goto cleanup; } if (!(entry = virJSONValueObjectGet(entry, "disk"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%s' missing in reply of guest-get-fsinfo"), - "disk"); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'disk' missing in reply of guest-get-fsinfo")); goto cleanup; } if (entry->type != VIR_JSON_TYPE_ARRAY) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("guest-get-fsinfo '%s' data was not an array"), - "disk"); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest-get-fsinfo 'disk' data was not an array")); goto cleanup; } @@ -1893,15 +1888,15 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr if (!disk) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("array element '%zd' of '%d' missing in" - "guest-get-fsinfo '%s' data"), - j, ndisk, "disk"); + _("array element '%zd' of '%d' missing in " + "guest-get-fsinfo 'disk' data"), + j, ndisk); goto cleanup; } if (!(pci = virJSONValueObjectGet(disk, "pci-controller"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%s' missing in guest-get-fsinfo " - "'disk' data"), "pci-controller"); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'pci-controller' missing in guest-get-fsinfo + "'disk' data")); goto cleanup; }

diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index 6cd6b49..c983828 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -73,6 +73,8 @@ int qemuAgentShutdown(qemuAgentPtr mon, int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints, unsigned int nmountpoints); int qemuAgentFSThaw(qemuAgentPtr mon); +int qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info, + virDomainDefPtr vmdef);

int qemuAgentSuspend(qemuAgentPtr mon, unsigned int target); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a84fd47..f655302 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18767,6 +18767,53 @@ qemuNodeAllocPages(virConnectPtr conn, }

+static int +qemuDomainGetFSInfo(virDomainPtr dom, + virDomainFSInfoPtr **info, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + qemuDomainObjPrivatePtr priv; + virDomainObjPtr vm; + int ret = -1; + + virCheckFlags(0, ret); + + if (!(vm = qemuDomObjFromDomain(dom))) + return ret; + + if (virDomainGetFSInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + priv = vm->privateData; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup;

Again - this is a QUERY not a MODIFY as far as I understand things, correct? remote_protocol.x has: + /** + * @generate: none + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_GET_FSINFO = 348 Although everything that happens here is not my specialty, so hopefully someone else reads this review and can comment...

+ + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + if (!qemuDomainAgentAvailable(priv, true)) + goto endjob; + + qemuDomainObjEnterAgent(vm); + ret = qemuAgentGetFSInfo(priv->agent, info, vm->def); + qemuDomainObjExitAgent(vm); + + endjob: + if (!qemuDomainObjEndJob(driver, vm)) + vm = NULL; + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + + static virHypervisorDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -18967,6 +19014,7 @@ static virHypervisorDriver qemuDriver = { .connectGetDomainCapabilities = qemuConnectGetDomainCapabilities, /* 1.2.7 */ .connectGetAllDomainStats = qemuConnectGetAllDomainStats, /* 1.2.8 */ .nodeAllocPages = qemuNodeAllocPages, /* 1.2.9 */ + .domainGetFSInfo = qemuDomainGetFSInfo, /* 1.2.11 */ };

I'll squash the following in: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9f7d951..2263b59 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18787,7 +18787,7 @@ qemuDomainGetFSInfo(virDomainPtr dom, priv = vm->privateData; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; if (!virDomainObjIsActive(vm)) {

On 11/17/2014 06:27 PM, Tomoki Sekiyama wrote:

Add test cases for qemuAgentGetFSInfo, with a sample agent response for the qemu-get-fsinfo command and a configuration xml.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- tests/Makefile.am | 1 tests/qemuagentdata/qemuagent-fsinfo.xml | 39 ++++++++ tests/qemuagenttest.c | 143 ++++++++++++++++++++++++++++++ 3 files changed, 183 insertions(+) create mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml

ACK - thanks for the more than 1 alias test too!

diff --git a/tests/Makefile.am b/tests/Makefile.am index a3e3ab3..e9418ea 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -103,6 +103,7 @@ EXTRA_DIST = \ nwfilterxml2xmlin \ nwfilterxml2xmlout \ oomtrace.pl \ + qemuagentdata \ qemucapabilitiesdata \ qemucaps2xmldata \ qemuhelpdata \ diff --git a/tests/qemuagentdata/qemuagent-fsinfo.xml b/tests/qemuagentdata/qemuagent-fsinfo.xml new file mode 100644 index 0000000..9638feb --- /dev/null +++ b/tests/qemuagentdata/qemuagent-fsinfo.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <source file='/tmp/idedisk.img'/> + <target dev='hdc' bus='ide'/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/tmp/virtio-blk1.qcow2'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/tmp/virtio-blk2.qcow2'/> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </disk> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index bc649b4..4b6d950 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -164,6 +164,148 @@ testQemuAgentFSTrim(const void *data)

static int +testQemuAgentGetFSInfo(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + virCapsPtr caps = testQemuCapsInit(); + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + char *domain_filename = NULL; + char *domain_xml = NULL; + virDomainDefPtr def = NULL; + virDomainFSInfoPtr *info = NULL; + int ret = -1, ninfo = 0, i; + + if (!test) + return -1; + + if (virAsprintf(&domain_filename, "%s/qemuagentdata/qemuagent-fsinfo.xml", + abs_srcdir) < 0) + goto cleanup; + + if (virtTestLoadFile(domain_filename, &domain_xml) < 0) + goto cleanup; + + if (!(def = virDomainDefParseString(domain_xml, caps, xmlopt, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-get-fsinfo", + "{\"return\": [" + " {\"name\": \"sda1\", \"mountpoint\": \"/\"," + " \"disk\": [" + " {\"bus-type\": \"ide\"," + " \"bus\": 1, \"unit\": 0," + " \"pci-controller\": {" + " \"bus\": 0, \"slot\": 1," + " \"domain\": 0, \"function\": 1}," + " \"target\": 0}]," + " \"type\": \"ext4\"}," + " {\"name\": \"dm-1\"," + " \"mountpoint\": \"/opt\"," + " \"disk\": [" + " {\"bus-type\": \"virtio\"," + " \"bus\": 0, \"unit\": 0," + " \"pci-controller\": {" + " \"bus\": 0, \"slot\": 6," + " \"domain\": 0, \"function\": 0}," + " \"target\": 0}," + " {\"bus-type\": \"virtio\"," + " \"bus\": 0, \"unit\": 0," + " \"pci-controller\": {" + " \"bus\": 0, \"slot\": 7," + " \"domain\": 0, \"function\": 0}," + " \"target\": 0}]," + " \"type\": \"vfat\"}," + " {\"name\": \"sdb1\"," + " \"mountpoint\": \"/mnt/disk\"," + " \"disk\": [], \"type\": \"xfs\"}]}") < 0) + goto cleanup; + + if ((ninfo = qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test), + &info, def)) < 0) + goto cleanup; + + if (ninfo != 3) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 3 filesystems information, got %d", ninfo); + ret = -1; + goto cleanup; + } + if (STRNEQ(info[2]->name, "sda1") || + STRNEQ(info[2]->mountpoint, "/") || + STRNEQ(info[2]->type, "ext4") || + !info[2]->devAlias || !info[2]->devAlias[0] || info[2]->devAlias[1] || + STRNEQ(info[2]->devAlias[0], "hdc")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "unexpected filesystems information returned for sda1 (%s,%s)", + info[2]->name, info[2]->devAlias ? info[2]->devAlias[0] : "null"); + ret = -1; + goto cleanup; + } + if (STRNEQ(info[1]->name, "dm-1") || + STRNEQ(info[1]->mountpoint, "/opt") || + STRNEQ(info[1]->type, "vfat") || + !info[1]->devAlias || !info[1]->devAlias[0] || + !info[1]->devAlias[1] || info[1]->devAlias[2] || + STRNEQ(info[1]->devAlias[0], "vda") || + STRNEQ(info[1]->devAlias[1], "vdb")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "unexpected filesystems information returned for dm-1 (%s,%s)", + info[0]->name, info[0]->devAlias ? info[0]->devAlias[0] : "null"); + ret = -1; + goto cleanup; + } + if (STRNEQ(info[0]->name, "sdb1") || + STRNEQ(info[0]->mountpoint, "/mnt/disk") || + STRNEQ(info[0]->type, "xfs") || + (info[0]->devAlias && info[0]->devAlias[0])) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "unexpected filesystems information returned for sdb1 (%s,%s)", + info[0]->name, info[0]->devAlias ? info[0]->devAlias[0] : "null"); + ret = -1; + goto cleanup; + } + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-get-fsinfo", + "{\"error\":" + " {\"class\":\"CommandDisabled\"," + " \"desc\":\"The command guest-get-fsinfo " + "has been disabled for " + "this instance\"," + " \"data\":{\"name\":\"guest-get-fsinfo\"}" + " }" + "}") < 0) + goto cleanup; + + if (qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test), &info, def) != -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "agent get-fsinfo command should have failed"); + goto cleanup; + } + + ret = 0; + + cleanup: + for (i = 0; i < ninfo; i++) + virDomainFSInfoFree(info[i]); + VIR_FREE(info); + VIR_FREE(domain_filename); + VIR_FREE(domain_xml); + virObjectUnref(caps); + virDomainDefFree(def); + qemuMonitorTestFree(test); + return ret; +} + + +static int testQemuAgentSuspend(const void *data) { virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; @@ -605,6 +747,7 @@ mymain(void) DO_TEST(FSFreeze); DO_TEST(FSThaw); DO_TEST(FSTrim); + DO_TEST(GetFSInfo); DO_TEST(Suspend); DO_TEST(Shutdown); DO_TEST(CPU);

On 11/17/2014 06:27 PM, Tomoki Sekiyama wrote:

Add a "domfsinfo" command that shows a list of filesystems info mounted in the guest. For example:

virsh # domfsinfo vm1 Mountpoint Name Type Target ------------------------------------------------------------------- / sda1 ext4 hdc /opt dm-2 vfat vda,vdb /mnt/test sdb1 xfs sda

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- tools/virsh-domain.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 9 ++++++ 2 files changed, 83 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a7e9151..ab8ebab 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12120,6 +12120,74 @@ cmdDomFSThaw(vshControl *ctl, const vshCmd *cmd) return ret >= 0; }

+static const vshCmdInfo info_domfsinfo[] = { + {.name = "help", + .data = N_("Get information of domain's mounted filesystems.") + }, + {.name = "desc", + .data = N_("Get information of domain's mounted filesystems.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_domfsinfo[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = NULL} +}; + +static bool +cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + int ret = -1; + size_t i; + virDomainFSInfoPtr *info; + char **alias; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + ret = virDomainGetFSInfo(dom, &info, 0); + if (ret < 0) { + vshError(ctl, _("Unable to get filesystems information"));

You missed the "get domain filesystem", but I see you're mostly following other examples. So I'll just change this to: "Unable to get filesystem information" I could have gone with "filesystem(s)", but just didn't read correctly

+ goto cleanup; + } + if (ret == 0) { + vshError(ctl, _("No filesystems are mounted in the domain")); + goto cleanup; + } + + if (info) { + vshPrintExtra(ctl, "%-36s %-8s %-8s %s\n", + _("Mountpoint"), _("Name"), _("Type"), _("Target")); + vshPrintExtra(ctl, "-------------------------------------------------------------------\n"); + for (i = 0; i < ret; i++) { + vshPrintExtra(ctl, "%-36s %-8s %-8s ", + info[i]->mountpoint, info[i]->name, info[i]->type); + alias = info[i]->devAlias; + if (alias) { + while (*alias) { + vshPrintExtra(ctl, "%s", *alias++); + if (*alias) + vshPrint(ctl, ","); + } + } + vshPrint(ctl, "\n"); + + virDomainFSInfoFree(info[i]); + } + VIR_FREE(info); + } + + cleanup: + virDomainFree(dom); + return ret >= 0; +} + const vshCmdDef domManagementCmds[] = { {.name = "attach-device", .handler = cmdAttachDevice, @@ -12279,6 +12347,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_domfsthaw, .flags = 0 }, + {.name = "domfsinfo", + .handler = cmdDomFSInfo, + .opts = opts_domfsinfo, + .info = info_domfsinfo, + .flags = 0 + }, {.name = "domfstrim", .handler = cmdDomFSTrim, .opts = opts_domfstrim, diff --git a/tools/virsh.pod b/tools/virsh.pod index d5608cc..de8f2c3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1144,6 +1144,15 @@ Output a URI which can be used to connect to the graphical display of the domain via VNC, SPICE or RDP. If I<--include-password> is specified, the SPICE channel password will be included in the URI.

+=item B<domfsinfo> I<domain> + +Show a list of mounted filesystems within the running domain. The list contains +mountpoints, names of a mounted device in the guest, filesystem types, and +unique target names used in the domain XML (<target dev='name'/>). + +Note that this command requires a guest agent configured and running in the +domain's guest OS. + =item B<domfsfreeze> I<domain> [[I<--mountpoint>] B<mountpoint>...]

Freeze mounted filesystems within a running domain to prepare for consistent

On 11/17/2014 06:29 PM, Tomoki Sekiyama wrote:

Implement the function which returns a list of tuples, that contains members of virDomainFSInfo struct.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- generator.py | 5 +++ libvirt-override-api.xml | 6 ++++ libvirt-override.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++ sanitytest.py | 5 +++ 4 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/generator.py b/generator.py index c66c6d4..39ba2f7 100755 --- a/generator.py +++ b/generator.py @@ -476,6 +476,7 @@ skip_impl = ( 'virNetworkGetDHCPLeases', 'virDomainBlockCopy', 'virNodeAllocPages', + 'virDomainGetFSInfo', )

lxc_skip_impl = ( @@ -586,6 +587,7 @@ skip_function = (

'virNetworkDHCPLeaseFree', # only useful in C, python code uses list 'virDomainStatsRecordListFree', # only useful in C, python uses dict + 'virDomainFSInfoFree', # only useful in C, python code uses list )

lxc_skip_function = ( @@ -1107,6 +1109,9 @@ def nameFixup(name, classe, type, file): elif name[0:20] == "virDomainGetCPUStats": func = name[9:] func = func[0:1].lower() + func[1:] + elif name[0:20] == "virDomainGetFSInfo":

Shouldn't this be [0:18]

+ func = name[12:] + func = func[0:2].lower() + func[2:] elif name[0:12] == "virDomainGet": func = name[12:] func = func[0:1].lower() + func[1:] diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml index 4fe3c4d..2e807ba 100644 --- a/libvirt-override-api.xml +++ b/libvirt-override-api.xml @@ -658,5 +658,11 @@ <arg name='flags' type='unsigned int' info='an OR'ed set of virNodeAllocPagesFlags'/> <return type='int' info='the number of nodes successfully adjusted or -1 in case of an error'/> </function> + <function name="virDomainGetFSInfo" file='python'> + <info>Get a list of mapping informaiton for each mounted file systems within the specified guest and the disks.</info>

Again the 'informaiton'

+ <arg name='domain' type='virDomainPtr' info='pointer to domain object'/> + <arg name='flags' type='unsigned int' info='unused, pass 0'/> + <return type='char *' info="list of mounted filesystems information"/> + </function> </symbols> </api> diff --git a/libvirt-override.c b/libvirt-override.c index 8895289..bd6321f 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -8266,6 +8266,73 @@ libvirt_virNodeAllocPages(PyObject *self ATTRIBUTE_UNUSED, } #endif /* LIBVIR_CHECK_VERSION(1, 2, 8) */

+#if LIBVIR_CHECK_VERSION(1, 2, 11) + +static PyObject * +libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { + virDomainPtr domain; + PyObject *pyobj_domain; + unsigned int flags; + virDomainFSInfoPtr *fsinfo = NULL; + char **dev; + int c_retval, i; + PyObject *py_retval; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainFSInfo", + &pyobj_domain, &flags)) + return NULL; + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainGetFSInfo(domain, &fsinfo, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (c_retval < 0) + goto cleanup; + + /* convert to a Python list */ + if ((py_retval = PyList_New(c_retval)) == NULL) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + virDomainFSInfoPtr fs = fsinfo[i]; + PyObject *info, *alias; + + if (fs == NULL) + goto cleanup; + info = PyTuple_New(4); + if (info == NULL) + goto cleanup; + PyList_SetItem(py_retval, i, info); + alias = PyList_New(0); + if (alias == NULL) + goto cleanup; + + PyTuple_SetItem(info, 0, libvirt_constcharPtrWrap(fs->mountpoint)); + PyTuple_SetItem(info, 1, libvirt_constcharPtrWrap(fs->name)); + PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(fs->type)); + PyTuple_SetItem(info, 3, alias); + + for (dev = fs->devAlias; dev && *dev; dev++) + if (PyList_Append(alias, libvirt_constcharPtrWrap(*dev)) < 0) + goto cleanup; + } + + for (i = 0; i < c_retval; i++) + virDomainFSInfoFree(fsinfo[i]); + VIR_FREE(fsinfo); + return py_retval; + + cleanup: + for (i = 0; i < c_retval; i++) + virDomainFSInfoFree(fsinfo[i]); + VIR_FREE(fsinfo); + Py_DECREF(py_retval); + return VIR_PY_NONE; +} + +#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */ +

Hopefully phrdina gets a look here - to me things seem fine; however, I know there's been issues recently with return values, so I just want to be sure before an ACK Posting this shouldn't have been a 'followup' (eg, reply to .0) of the existing stream of patches, rather it should be its own patch. Otherwise, it can get "lost" in the shuffle of things I don't mind adjusting the patch in-line before pushing once the other part of the series goes in. John

/************************************************************************ * * * The registration stuff * @@ -8459,6 +8526,9 @@ static PyMethodDef libvirtMethods[] = { #if LIBVIR_CHECK_VERSION(1, 2, 9) {(char *) "virNodeAllocPages", libvirt_virNodeAllocPages, METH_VARARGS, NULL}, #endif /* LIBVIR_CHECK_VERSION(1, 2, 9) */ +#if LIBVIR_CHECK_VERSION(1, 2, 11) + {(char *) "virDomainGetFSInfo", libvirt_virDomainGetFSInfo, METH_VARARGS, NULL}, +#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */ {NULL, NULL, 0, NULL} };

diff --git a/sanitytest.py b/sanitytest.py index b161696..f5337fc 100644 --- a/sanitytest.py +++ b/sanitytest.py @@ -137,6 +137,9 @@ for cname in wantfunctions: if name[0:28] == "virDomainStatsRecordListFree": continue

+ if name[0:19] == "virDomainFSInfoFree": + continue + if name[0:21] == "virDomainListGetStats": name = "virConnectDomainListGetStats"

@@ -269,7 +272,7 @@ for name in sorted(basicklassmap): func = func[0:1].lower() + func[1:] if func[0:8] == "nWFilter": func = "nwfilter" + func[8:] - if func[0:8] == "fSFreeze" or func[0:6] == "fSThaw": + if func[0:8] == "fSFreeze" or func[0:6] == "fSThaw" or func[0:6] == "fSInfo": func = "fs" + func[2:]

if klass == "virNetwork":

On 11/20/2014 01:22 PM, Pavel Hrdina wrote:

On 11/18/2014 12:29 AM, Tomoki Sekiyama wrote:

...

Implement the function which returns a list of tuples, that contains members of virDomainFSInfo struct.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- generator.py | 5 +++ libvirt-override-api.xml | 6 ++++ libvirt-override.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++ sanitytest.py | 5 +++ 4 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/generator.py b/generator.py index c66c6d4..39ba2f7 100755 --- a/generator.py +++ b/generator.py @@ -476,6 +476,7 @@ skip_impl = ( 'virNetworkGetDHCPLeases', 'virDomainBlockCopy', 'virNodeAllocPages', + 'virDomainGetFSInfo', )

lxc_skip_impl = ( @@ -586,6 +587,7 @@ skip_function = (

'virNetworkDHCPLeaseFree', # only useful in C, python code uses list 'virDomainStatsRecordListFree', # only useful in C, python uses dict + 'virDomainFSInfoFree', # only useful in C, python code uses list )

lxc_skip_function = ( @@ -1107,6 +1109,9 @@ def nameFixup(name, classe, type, file): elif name[0:20] == "virDomainGetCPUStats": func = name[9:] func = func[0:1].lower() + func[1:] + elif name[0:20] == "virDomainGetFSInfo":

There definitely must be name[0:18] as John pointed out.

...

+ func = name[12:] + func = func[0:2].lower() + func[2:] elif name[0:12] == "virDomainGet": func = name[12:] func = func[0:1].lower() + func[1:] diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml index 4fe3c4d..2e807ba 100644 --- a/libvirt-override-api.xml +++ b/libvirt-override-api.xml @@ -658,5 +658,11 @@ <arg name='flags' type='unsigned int' info='an OR'ed set of virNodeAllocPagesFlags'/> <return type='int' info='the number of nodes successfully adjusted or -1 in case of an error'/> </function> + <function name="virDomainGetFSInfo" file='python'> + <info>Get a list of mapping informaiton for each mounted file systems within the specified guest and the disks.</info> + <arg name='domain' type='virDomainPtr' info='pointer to domain object'/> + <arg name='flags' type='unsigned int' info='unused, pass 0'/> + <return type='char *' info="list of mounted filesystems information"/> + </function> </symbols> </api> diff --git a/libvirt-override.c b/libvirt-override.c index 8895289..bd6321f 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -8266,6 +8266,73 @@ libvirt_virNodeAllocPages(PyObject *self ATTRIBUTE_UNUSED, } #endif /* LIBVIR_CHECK_VERSION(1, 2, 8) */

+#if LIBVIR_CHECK_VERSION(1, 2, 11) + +static PyObject * +libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { + virDomainPtr domain; + PyObject *pyobj_domain; + unsigned int flags; + virDomainFSInfoPtr *fsinfo = NULL; + char **dev; + int c_retval, i; + PyObject *py_retval; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainFSInfo", + &pyobj_domain, &flags)) + return NULL; + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainGetFSInfo(domain, &fsinfo, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (c_retval < 0) + goto cleanup; + + /* convert to a Python list */ + if ((py_retval = PyList_New(c_retval)) == NULL) + goto cleanup;

The PyList_New on success return new reference ... [1]

...

+ + for (i = 0; i < c_retval; i++) { + virDomainFSInfoPtr fs = fsinfo[i]; + PyObject *info, *alias; + + if (fs == NULL) + goto cleanup; + info = PyTuple_New(4); + if (info == NULL)

Wrong indentation, use spaces instead of tabs.

...

+ goto cleanup; + PyList_SetItem(py_retval, i, info); + alias = PyList_New(0); + if (alias == NULL)

The same wrong indentation.

...

+ goto cleanup; + + PyTuple_SetItem(info, 0, libvirt_constcharPtrWrap(fs->mountpoint)); + PyTuple_SetItem(info, 1, libvirt_constcharPtrWrap(fs->name)); + PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(fs->type)); + PyTuple_SetItem(info, 3, alias); + + for (dev = fs->devAlias; dev && *dev; dev++) + if (PyList_Append(alias, libvirt_constcharPtrWrap(*dev)) < 0) + goto cleanup; + } + + for (i = 0; i < c_retval; i++) + virDomainFSInfoFree(fsinfo[i]); + VIR_FREE(fsinfo); + return py_retval; + + cleanup: + for (i = 0; i < c_retval; i++) + virDomainFSInfoFree(fsinfo[i]); + VIR_FREE(fsinfo); + Py_DECREF(py_retval);

[1] ... there you correctly dereference it, but you should use PY_XDECREF because the py_retval could be NULL.

...

+ return VIR_PY_NONE; +} + +#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */ +

/************************************************************************ * * * The registration stuff * @@ -8459,6 +8526,9 @@ static PyMethodDef libvirtMethods[] = { #if LIBVIR_CHECK_VERSION(1, 2, 9) {(char *) "virNodeAllocPages", libvirt_virNodeAllocPages, METH_VARARGS, NULL}, #endif /* LIBVIR_CHECK_VERSION(1, 2, 9) */ +#if LIBVIR_CHECK_VERSION(1, 2, 11) + {(char *) "virDomainGetFSInfo", libvirt_virDomainGetFSInfo, METH_VARARGS, NULL}, +#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */ {NULL, NULL, 0, NULL} };

diff --git a/sanitytest.py b/sanitytest.py index b161696..f5337fc 100644 --- a/sanitytest.py +++ b/sanitytest.py @@ -137,6 +137,9 @@ for cname in wantfunctions: if name[0:28] == "virDomainStatsRecordListFree": continue

+ if name[0:19] == "virDomainFSInfoFree": + continue + if name[0:21] == "virDomainListGetStats": name = "virConnectDomainListGetStats"

@@ -269,7 +272,7 @@ for name in sorted(basicklassmap): func = func[0:1].lower() + func[1:] if func[0:8] == "nWFilter": func = "nwfilter" + func[8:] - if func[0:8] == "fSFreeze" or func[0:6] == "fSThaw": + if func[0:8] == "fSFreeze" or func[0:6] == "fSThaw" or func[0:6] == "fSInfo": func = "fs" + func[2:]

if klass == "virNetwork":

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Returning VIR_PY_NONE is safe there as none of the used function sets the python exception and the error is handled in the python wrapper.

ACK

Pavel

The ACK is of course with the changes mentioned above. Pavel