3:07 p.m.
2010/4/7 Daniel Veillard <veillard(a)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