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

Re: [libvirt] [PATCH] esx: Add domain snapshot support



2010/4/7 Daniel Veillard <veillard redhat com>:
> On Wed, Apr 07, 2010 at 12:00:01PM +0200, Matthias Bolte wrote:
>> Fix invalid code generating in esx_vi_generator.py regarding deep copy
>> types that contain enum properties.
>>
>> Add strptime and timegm to bootstrap.conf. Both are used to convert a
>> xsd:dateTime to calendar time.
>> ---
>>  bootstrap.conf                 |    2 +
>>  src/esx/esx_driver.c           |  468 +++++++++++++++++++++++++++++++++++++---
>>  src/esx/esx_vi.c               |  290 +++++++++++++++++++++++++
>>  src/esx/esx_vi.h               |   27 +++
>>  src/esx/esx_vi_generator.input |   12 +
>>  src/esx/esx_vi_generator.py    |   25 ++-
>>  src/esx/esx_vi_methods.c       |   86 ++++++++
>>  src/esx/esx_vi_methods.h       |   14 ++
>>  src/esx/esx_vi_types.c         |   99 +++++++++
>>  src/esx/esx_vi_types.h         |   12 +
>>  10 files changed, 990 insertions(+), 45 deletions(-)
>>
>> diff --git a/bootstrap.conf b/bootstrap.conf
>> index ac2f8e6..ca9332d 100644
>> --- a/bootstrap.conf
>> +++ b/bootstrap.conf
>> @@ -52,9 +52,11 @@ stpcpy
>>  strchrnul
>>  strndup
>>  strerror
>> +strptime
>>  strsep
>>  sys_stat
>>  time_r
>> +timegm
>>  useless-if-before-free
>>  vasprintf
>>  verify
>
>  Okay, IIRC the environment checks for LGPL licence compat

Yes, but no problem here, both are LGPLv2+.

>> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
>> index eb06555..5272654 100644
>> --- a/src/esx/esx_driver.c
>> +++ b/src/esx/esx_driver.c
>
>  pure formatting changes on this module
>
> [...]
>> +static virDomainSnapshotPtr
>> +esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc,
>> +                           unsigned int flags ATTRIBUTE_UNUSED)
>> +{
> [...]
>> +}
>> +
>
>  Looks fine
>
>> +
>> +static char *
>> +esxDomainSnapshotDumpXML(virDomainSnapshotPtr snapshot,
>> +                         unsigned int flags ATTRIBUTE_UNUSED)
>> +{
>> +    esxPrivate *priv = snapshot->domain->conn->privateData;
>> +    esxVI_VirtualMachineSnapshotTree *rootSnapshotList = NULL;
>> +    esxVI_VirtualMachineSnapshotTree *snapshotTree = NULL;
>> +    esxVI_VirtualMachineSnapshotTree *snapshotTreeParent = NULL;
>> +    virDomainSnapshotDef def;
>> +    char uuid_string[VIR_UUID_STRING_BUFLEN] = "";
>> +    char *xml = NULL;
>> +
>> +    memset(&def, 0, sizeof (virDomainSnapshotDef));
>> +
>> +    if (esxVI_EnsureSession(priv->host) < 0) {
>> +        goto failure;
>> +    }
>> +
>> +    if (esxVI_LookupRootSnapshotTreeList(priv->host, snapshot->domain->uuid,
>> +                                         &rootSnapshotList) < 0 ||
>> +        esxVI_GetSnapshotTreeByName(rootSnapshotList, snapshot->name,
>> +                                    &snapshotTree, &snapshotTreeParent,
>> +                                    esxVI_Occurrence_RequiredItem) < 0) {
>> +        goto failure;
>> +    }
>> +
>> +    def.name = snapshot->name;
>> +    def.description = snapshotTree->description;
>> +    def.parent = snapshotTreeParent != NULL ? snapshotTreeParent->name : NULL;
>> +
>> +    if (esxVI_DateTime_ConvertToCalendarTime(snapshotTree->createTime,
>> +                                             &def.creationTime) < 0) {
>> +        goto failure;
>> +    }
>> +
>> +    def.state = esxVI_VirtualMachinePowerState_ConvertToLibvirt
>> +                  (snapshotTree->state);
>> +
>> +    virUUIDFormat(snapshot->domain->uuid, uuid_string);
>> +
>> +    xml = virDomainSnapshotDefFormat(uuid_string, &def, 0);
>> +
>> +  cleanup:
>> +    esxVI_VirtualMachineSnapshotTree_Free(&rootSnapshotList);
>> +
>> +    return xml;
>> +
>> +  failure:
>> +    VIR_FREE(xml);
>> +
>> +    goto cleanup;
>> +}
>> +
>
>  Okay, I we will need to check if virDomainSnapshotDef ever grow to get
> new fields, but the memset should prevent problems anyway.
>
>> +
>> +static int
>> +esxDomainSnapshotNum(virDomainPtr domain, unsigned int flags ATTRIBUTE_UNUSED)
>> +{
> [...]
>> +}
>> +
>
>  looks fine but we should probably raise an error if flags != 0 since
>  this is not supported in this API level

Okay, added those checks now.

>> +
>> +static int
>> +esxDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen,
>> +                           unsigned int flags ATTRIBUTE_UNUSED)
>> +{
> [..]
>> +}
>> +
>
>  same here
>
>> +
>> +static virDomainSnapshotPtr
>> +esxDomainSnapshotLookupByName(virDomainPtr domain, const char *name,
>> +                              unsigned int flags ATTRIBUTE_UNUSED)
>> +{
>> +    esxPrivate *priv = domain->conn->privateData;
>> +    esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL;
>> +    esxVI_VirtualMachineSnapshotTree *snapshotTree = NULL;
>> +    esxVI_VirtualMachineSnapshotTree *snapshotTreeParent = NULL;
>> +    virDomainSnapshotPtr snapshot = NULL;
>> +
>> +    if (esxVI_EnsureSession(priv->host) < 0) {
>> +        goto failure;
>> +    }
>> +
>> +    if (esxVI_LookupRootSnapshotTreeList(priv->host, domain->uuid,
>> +                                         &rootSnapshotTreeList) < 0 ||
>> +        esxVI_GetSnapshotTreeByName(rootSnapshotTreeList, name, &snapshotTree,
>> +                                    &snapshotTreeParent,
>> +                                    esxVI_Occurrence_RequiredItem) < 0) {
>> +        goto failure;
>> +    }
>> +
>> +    snapshot = virGetDomainSnapshot(domain, name);
>> +
>> +  cleanup:
>> +    esxVI_VirtualMachineSnapshotTree_Free(&rootSnapshotTreeList);
>> +
>> +    return snapshot;
>> +
>> +  failure:
>> +    snapshot = NULL;
>> +
>> +    goto cleanup;
>> +}
>> +
>
>  seems the failure path does a unecessary write to snapshot which may
>  get pointed out by automatic tools

True, I removed the failure label here now.

>> +    virDomainSnapshotPtr snapshot = NULL;
>> +    esxPrivate *priv = domain->conn->privateData;
>> +    esxVI_VirtualMachineSnapshotTree *currentSnapshotTree = NULL;
>> +
>> +    if (esxVI_EnsureSession(priv->host) < 0) {
>> +        goto failure;
>> +    }
>> +
>> +    if (esxVI_LookupCurrentSnapshotTree(priv->host, domain->uuid,
>> +                                        &currentSnapshotTree,
>> +                                        esxVI_Occurrence_RequiredItem) < 0) {
>> +        goto failure;
>> +    }
>> +
>> +    snapshot = virGetDomainSnapshot(domain, currentSnapshotTree->name);
>> +
>> +  cleanup:
>> +    esxVI_VirtualMachineSnapshotTree_Free(&currentSnapshotTree);
>> +
>> +    return snapshot;
>> +
>> +  failure:
>> +    snapshot = NULL;
>
>  seems redundant here too

Removed here too.

> [...]
>> +int
>> +esxVI_DateTime_ConvertToCalendarTime(esxVI_DateTime *dateTime,
>> +                                     time_t *secondsSinceEpoch)
>> +{
>> +    char value[64] = "";
>> +    char *tmp;
>> +    struct tm tm;
>> +    int milliseconds;
>> +    char sign;
>> +    int tz_hours;
>> +    int tz_minutes;
>> +    int tz_offset = 0;
>> +
>> +    if (dateTime == NULL || secondsSinceEpoch == NULL) {
>> +        ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
>> +        return -1;
>> +    }
>> +
>> +    if (virStrcpyStatic(value, dateTime->value) == NULL) {
>> +        ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR,
>> +                     _("xsd:dateTime value '%s' too long for destination"),
>> +                     dateTime->value);
>> +        return -1;
>> +    }
>
>  Ouch, they are using XSD dateTime type !
>
>> +    /* expected format: 2010-04-05T12:13:55.316789+02:00 */
>> +    tmp = strptime(value, "%Y-%m-%dT%H:%M:%S", &tm);
>
>  well timezone and fractional second are optional in XSD dateTime
> http://www.w3.org/TR/xmlschema-2/#dateTime
> so theorically some of those last fields my be missing and break this
> code :-\ the optional leading '-' should not be used here but ...

I've made this code more robust now. It handles the optional parts
correct now and I've added a test case for this function.

> [...]
>> +    /*
>> +     * xsd:dateTime represents local time relative to the timezone given
>> +     * as offset. pretend the local time is in UTC and use timegm in order
>
>   and optional :-)
>
>> +     * to avoid interference with the timezone to this computer.
>> +     * apply timezone correction afterwards, because it's simpler than
>> +     * handling all the possible over- and underflows when trying to apply
>> +     * it to the tm struct.
>> +     */
>> +    *secondsSinceEpoch = timegm(&tm) - tz_offset;
>> +
>> +    return 0;
>> +}
>> +
>
>  Nothing major, the flags attribute should be checked since we do that
>  now, and a couple of cleanups. Once done, ACK
>
>   thanks for getting there so fast :-)
>
> Daniel
>

Thanks, pushed the improved patch.

Matthias


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