[libvirt] [PATCH] esx: Add domain snapshot support
Daniel Veillard
veillard at redhat.com
Wed Apr 7 15:30:41 UTC 2010
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
> 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
> +
> +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
> +
> +static int
> +esxDomainHasCurrentSnapshot(virDomainPtr domain,
> + unsigned int flags ATTRIBUTE_UNUSED)
> +{
[...]
> +}
> +
flag check for 0
> +static virDomainSnapshotPtr
> +esxDomainSnapshotCurrent(virDomainPtr domain,
> + unsigned int flags ATTRIBUTE_UNUSED)
flag check for 0
> +{
> +
extra empty line here it seems :-)
> + 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
> + goto cleanup;
> +}
> +
> +
> +
> +static int
> +esxDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> + unsigned int flags ATTRIBUTE_UNUSED)
> +{
flag check :-)
> + int result = 0;
[...]
> +}
> +
> +
> +
> +static int
> +esxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags)
> +{
[...]
> +}
> +
> +
> +
[...]
> diff --git a/src/esx/esx_vi_generator.input b/src/esx/esx_vi_generator.input
> index 06dddbf..9c545eb 100644
> --- a/src/esx/esx_vi_generator.input
> +++ b/src/esx/esx_vi_generator.input
> @@ -424,3 +424,15 @@ object VirtualMachineQuestionInfo
> ChoiceOption choice r
> VirtualMachineMessage message i
> end
> +
> +
> +object VirtualMachineSnapshotTree
> + ManagedObjectReference snapshot r
> + ManagedObjectReference vm r
> + String name r
> + String description r
> + DateTime createTime r
> + VirtualMachinePowerState state r
> + Boolean quiesced r
> + VirtualMachineSnapshotTree childSnapshotList ol
> +end
very concice :-)
[...]
> +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 ...
[...]
> + /*
> + * 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
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
More information about the libvir-list
mailing list