2010/4/7 Daniel Veillard <veillard@xxxxxxxxxx>:
> 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,
>> + ¤tSnapshotTree,
>> + esxVI_Occurrence_RequiredItem) < 0) {
>> + goto failure;
>> + }
>> +
>> + snapshot = virGetDomainSnapshot(domain, currentSnapshotTree->name);
>> +
>> + cleanup:
>> + esxVI_VirtualMachineSnapshotTree_Free(¤tSnapshotTree);
>> +
>> + 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list