Re: [PATCH 1/5] Implement public API for virDomainGetFSInfo

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]