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, + ¤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