Hi John, thanks for your review.
On 10/23/14, 9:16 , "John Ferlan" <jferlan@xxxxxxxxxx> 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@xxxxxxx>
>> ---
>> 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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list