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

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



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

> +    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 veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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