Hi John, thanks for your review. On 10/23/14, 9:16 , "John Ferlan" <jferlan@redhat.com> wrote:

On 09/30/2014 08:20 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.h.in | 21 +++++++++++++ src/driver.h | 6 ++++ src/libvirt.c | 68 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++++ 4 files changed, 101 insertions(+)

Since this is a new API you might need to make some changes in libvirt-python (as well as others) in order to ensure the API is in each as well.

OK, I¹ll add a patch for libvirt-python in v2.

...

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 5217ab3..c60c3d7 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -5651,6 +5651,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.h b/src/driver.h

FYI: Recent upstream changes will cause you to find a new home for things in driver.h. See commit-id 'd21d35e3' - this will now be in driver-hypervisor.h.

You may want to watch Daniel's series of patches because his patches will probably modify your libvirt.c changes as well

Thanks for the info ‹ I¹ll rebase it after the Daniel¹s series is merged.

...

index dc62a8e..33e3bd9 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1195,6 +1195,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, @@ -1445,6 +1450,7 @@ struct _virDriver { virDrvConnectGetDomainCapabilities connectGetDomainCapabilities; virDrvConnectGetAllDomainStats connectGetAllDomainStats; virDrvNodeAllocPages nodeAllocPages; + virDrvDomainGetFSInfo domainGetFSInfo; };

diff --git a/src/libvirt.c b/src/libvirt.c index 245c373..77ec851 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21200,6 +21200,74 @@ virDomainFSThaw(virDomainPtr dom, return -1; }

+ +/** + * 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

s/informaiton/information

...

+ * specified guest and the disks. + * + * Returns the number of returned mount points, or -1 and sets @info to + * NULL in case of error. On success, the array stored into @info is + * guaranteed to have an extra allocated element set to NULL but not included + * in the return count, to make iteration easier. The caller is responsible + * for calling virDomainFSInfoFree() on each array element, then calling + * free() on @info.

NOTE: See comment in patch 3 - I see no reason to allocate an extra element for @info. Since you are returning the count of elements, that's all the caller should access.

This design was copied from virNetworkGetDHCPLeases. If it isn¹t a standard way to return an array, I¹ll remove the extra element.

...

+ */ +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);

Since it's touched and assumed later on:

virCheckNonNullArgGoto(info, error);

and since on error you are indicating you guarantee it being NULL on error, you may as well initialize it

*info = NULL;

John

OK, I¹ll add the check and initialization. Thanks, Tomoki Sekiyama

...

+ + 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); +} + + /** * virDomainGetTime: * @dom: a domain object diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 5f95802..f688e76 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.10 { + global: + virDomainFSInfoFree; + virDomainGetFSInfo; +} LIBVIRT_1.2.9; + # .... define new API here using predicted next version number ....

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

On 09/30/2014 08:20 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 | 87 +++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 32 +++++++++++ src/remote_protocol-structs | 21 ++++++++ src/rpc/gendispatch.pl | 1 5 files changed, 257 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c index c93c97a..d775434 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6340,6 +6340,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; + + 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) {

Because you compare 'info' to NULL here, Coverity gets grumpy later on [1]

+ 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;

Because we won't get here if virDomainGetFSInfo() fails, then the loop below [2] will cause a problem... The good news is if you move this to above the virDomainGetFSInfo() call, then you avoid the Coverity complaint.

+ } + + ret->ret = ninfo; + + rv = 0; + + cleanup: + if (rv < 0) { + virNetMessageSaveError(rerr); + + if (ret->info.info_val) { + for (i = 0; i < ninfo; i++) {

[2] Coverity complains when virDomainGetFSInfo() fails leaving ninfo == -1

+ 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)

You'll need the { at the end (clarity) and because of [1], make it if (ninfo >= 0 && info) {

+ for (i = 0; i < ninfo; i++)

[1] And this is where Coverity gets grumpy indicating 'info' could be NULL and this would be a NULL deref. Yes, a false positive when you know the code, but nonetheless

+ virDomainFSInfoFree(info[i]);

and the corresponding }

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

/* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 6c49e49..aa1866a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7897,6 +7897,92 @@ 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->networkPrivateData; + 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 && + VIR_ALLOC_N(info_ret, ret.info.info_len + 1) < 0)

I see no reason for the + 1 since you're returning a count.

+ 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, @@ -8239,6 +8325,7 @@ static virDriver remote_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 db12cda..bf261e1 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. */ @@ -5505,5 +5529,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..e002b2a 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; + } leases; + u_int ret; +};

make check fails... s/leases/info resolves it. John

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 27093d2..d2cfffb 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/;

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

On 09/30/2014 08:20 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 | 165 ++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 2 + src/qemu/qemu_driver.c | 48 +++++++++++++ 6 files changed, 293 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b114737..f5c5e0c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10838,6 +10838,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) @@ -11127,6 +11181,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 ea201b3..5755eae 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2426,6 +2426,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); @@ -2493,6 +2497,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 2019ef5..db55abf 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 fe38f6d..5862c24 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1780,3 +1780,168 @@ 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;

Mainly curious - since it's stated that this returns data from qemu guest agent 2.2+ - what happens when run on something earlier?

+ + if (!(data = virJSONValueObjectGet(reply, "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, "%s", + _("guest-get-fsinfo return data was not an array")); + goto cleanup; + } + + ndata = virJSONValueArraySize(data);

Is a zero return valid here?

+ + if (VIR_ALLOC_N(info_ret, ndata + 1) < 0)

Does any code actually make use of this empty entry? Since you return the count callers should "honor" that instead (and it seems all your callers do so). Be sure to adjust your comments everywhere accordingly, such as patch 1 in the API comment.

+ 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, "%s", + _("array element missing in guest-get-fsinfo data")); + 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", + _("guest-get-fsinfo entry was missing mountpoint " + "data"));

For fields that are expected put single quotes around them. To be consistent with what I saw for guest-get-vcpus, consider: "'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", + _("guest-get-fsinfo entry was missing name data"));

likewise: "'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", + _("guest-get-fsinfo entry was missing type data"));

likewise: "'type' missing in reply of guest-get-fsinfo"

+ goto cleanup; + } + + if (!(entry = virJSONValueObjectGet(entry, "disk"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest-get-fsinfo entry was missing disk data"));

likewise: "'disk' missing in reply of guest-get-fsinfo"

+ goto cleanup; + } + + if (entry->type != VIR_JSON_TYPE_ARRAY) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest-get-fsinfo disk data was not an array"));

s/disk/'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, "%s", + _("array element missing in guest-get-fsinfo " + "disk data"));

similar to earlier : s/disk/'disk'/ You may want to consider adding '%d' (j) and 'ndisk' to the output as well as in "array elem 'j' of 'ndisk'"... Whether that'd be useful or not if something go wrong.

+ goto cleanup; + } + + if (!(pci = virJSONValueObjectGet(disk, "pci-controller"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest-get-fsinfo disk data was missing " + "pci-controller"));

likewise: "'pci-controller' missing in reply of guest-get-fsinfo"

+ goto cleanup; + } + + for (k = 0; k < 3; k++) { + if (virJSONValueObjectGetNumberInt( + disk, diskaddr_comp[k], &diskaddr[k]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("guest-get-fsinfo disk data was missing " + "%s"), diskaddr_comp[k]);

likewise: "'%s' missing in reply of guest-get-fsinfo 'disk' address data"

+ goto cleanup; + } + } + for (k = 0; k < 4; k++) { + if (virJSONValueObjectGetNumberInt( + pci, pciaddr_comp[k], &pciaddr[k]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("guest-get-fsinfo pci-address data was " + "missing %s"), pciaddr_comp[k]);

likewise: "'%s' missing in reply of guest-get-fsinfo 'pci-address' data"

+ 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;

The "somewhat" standard way to avoid the unwanted free in cleanup is: info_ret = NULL; That also makes it so Coverity doesn't get confused over the two conditions later on...

+ ret = ndata; + + cleanup: + if (ret < 0 && info_ret) {

Having two conditions confuses Coverity because it tries "both" paths and although in the failure case, ret == -1 and we'd go thru here, Coverity will generate the false positive and assume we can get here with ret >= 0, so if we do the info_ret = NULL trick above, then we don't need to check (ret < 0 &&), thus it just becomes: if (info_ret)

+ for (i = 0; i < ndata; i++) + virDomainFSInfoFree(info_ret[i]); + VIR_FREE(info_ret); + } + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} 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 e873d45..68eb4b3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18211,6 +18211,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)

This is a QEMU_JOB_QUERY John

+ goto cleanup; + + 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 virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -18411,6 +18458,7 @@ static virDriver qemuDriver = { .connectGetDomainCapabilities = qemuConnectGetDomainCapabilities, /* 1.2.7 */ .connectGetAllDomainStats = qemuConnectGetAllDomainStats, /* 1.2.8 */ .nodeAllocPages = qemuNodeAllocPages, /* 1.2.9 */ + .domainGetFSInfo = qemuDomainGetFSInfo, /* 1.2.10 */ };

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

On 09/30/2014 08:20 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 | 27 +++++++ tests/qemuagenttest.c | 118 ++++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+) create mode 100644 tests/qemuagentdata/qemuagent-fsinfo.xml

Great a test! Is there an option where you could list more than one alias? You have none and one. A simple ACK for v2... John

diff --git a/tests/Makefile.am b/tests/Makefile.am index 293611b..e3fbb66 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -102,6 +102,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..beae1f3 --- /dev/null +++ b/tests/qemuagentdata/qemuagent-fsinfo.xml @@ -0,0 +1,27 @@ +<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> + <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..f8ea1d5 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -164,6 +164,123 @@ 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, *i; + int ret = -1; + + 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\": \"sdb1\"," + " \"mountpoint\": \"/mnt/disk\"," + " \"disk\": [], \"type\": \"xfs\"}]}") < 0) + goto cleanup; + + if ((ret = qemuAgentGetFSInfo(qemuMonitorTestGetAgent(test), + &info, def)) < 0) + goto cleanup; + + if (ret != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 2 filesystems information, got %d", ret); + ret = -1; + goto cleanup; + } + if (STRNEQ(info[1]->name, "sda1") || + STRNEQ(info[1]->mountpoint, "/") || + STRNEQ(info[1]->type, "ext4") || + !info[1]->devAlias || !info[1]->devAlias[0] || info[1]->devAlias[1] || + STRNEQ(info[1]->devAlias[0], "hdc")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "unexpected filesystems information returned for sda1 (%s,%s)", + info[1]->name, info[1]->devAlias ? info[1]->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; + } + + for (i = info; i && *i; i++) + virDomainFSInfoFree(*i); + VIR_FREE(info); + + 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 = info; i && *i; i++) + virDomainFSInfoFree(*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 +722,7 @@ mymain(void) DO_TEST(FSFreeze); DO_TEST(FSThaw); DO_TEST(FSTrim); + DO_TEST(GetFSInfo); DO_TEST(Suspend); DO_TEST(Shutdown); DO_TEST(CPU);

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

On 09/30/2014 08:20 PM, Tomoki Sekiyama wrote:

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

Perhaps a small example of the command being run and expected output would be nice.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- tools/virsh-domain.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+)

You'll need to modify virsh.pod (e.g. man page) in order to complete this work. Be sure to note that the guest agent is required - there are examples within other commands.

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ce59406..cc07bc3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11999,6 +11999,70 @@ 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"));

s/get filesystems /get domain filesystem /

+ 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, ",");

Theoretically speaking this could be very wide... You may want to consider some sort of verbose option which would print all aliases beyond the first alias found. Not sure of the best way to handle this though and whether there are "other" examples. John

+ } + } + vshPrint(ctl, "\n"); + + virDomainFSInfoFree(info[i]); + } + VIR_FREE(info); + } + + cleanup: + virDomainFree(dom); + return ret >= 0; +} + const vshCmdDef domManagementCmds[] = { {.name = "attach-device", .handler = cmdAttachDevice, @@ -12158,6 +12222,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,

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