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

Re: [libvirt] [RFC PATCH 4/8] Snapshot internal methods.



On 04/02/2010 09:25 AM, Matthias Bolte wrote:
> 2010/4/2 Chris Lalancette <clalance redhat com>:
>> Signed-off-by: Chris Lalancette <clalance redhat com>
>> ---
>>  src/conf/domain_conf.c   |  402 ++++++++++++++++++++++++++++++++++++++++++++--
>>  src/conf/domain_conf.h   |   53 ++++++
>>  src/libvirt_private.syms |   10 ++
>>  3 files changed, 455 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index e260dce..1971b9a 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
> 
> 
>> @@ -6096,22 +6101,25 @@ static virDomainObjPtr virDomainLoadConfig(virCapsPtr caps,
>>
>>     if ((configFile = virDomainConfigFile(configDir, name)) == NULL)
>>         goto error;
>> -    if ((autostartLink = virDomainConfigFile(autostartDir, name)) == NULL)
>> -        goto error;
>> -
>> -    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
>> -        goto error;
>> -
>>     if (!(def = virDomainDefParseFile(caps, configFile,
>>                                       VIR_DOMAIN_XML_INACTIVE)))
>>         goto error;
>>
>> -    if ((dom = virDomainFindByName(doms, def->name))) {
>> -        virDomainObjUnlock(dom);
>> -        dom = NULL;
>> -        newVM = 0;
>> +    /* if the domain is already in our hashtable, we don't need to do
>> +     * anything further
>> +     */
>> +    if ((dom = virDomainFindByUUID(doms, def->uuid))) {
>> +        VIR_FREE(configFile);
>> +        virDomainDefFree(def);
>> +        return dom;
>>     }
>>
>> +    if ((autostartLink = virDomainConfigFile(autostartDir, name)) == NULL)
>> +        goto error;
>> +
>> +    if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0)
>> +        goto error;
>> +
>>     if (!(dom = virDomainAssignDef(caps, doms, def, false)))
>>         goto error;
> 
> What's the reason for this reordering? It seems to be independent from
> snapshots. Maybe it should go into a separate patch?

Oops, yeah, I forgot to split that out.  It's either a feature of def/newDef
that I don't understand, or it's a bugfix.  That being said, I'm not sure I
even need this bugfix anymore, so I'll retest and split it into a separate
patch if I still need it.

> 
>> +virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr,
>> +                                                        int newSnapshot)
>> +{
>> +    xmlXPathContextPtr ctxt = NULL;
>> +    xmlDocPtr xml = NULL;
>> +    xmlNodePtr root;
>> +    virDomainSnapshotDefPtr def = NULL;
>> +    virDomainSnapshotDefPtr ret = NULL;
>> +    char *creation = NULL, *state = NULL;
>> +    struct timeval tv;
>> +    struct tm time_info;
>> +    char timestr[100];
>> +
>> +    xml = virXMLParse(NULL, xmlStr, "domainsnapshot.xml");
>> +    if (!xml) {
>> +        virDomainReportError(VIR_ERR_XML_ERROR,
>> +                             "%s",_("failed to parse snapshot xml document"));
>> +        return NULL;
>> +    }
>> +
>> +    if ((root = xmlDocGetRootElement(xml)) == NULL) {
>> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>> +                              "%s", _("missing root element"));
>> +        goto cleanup;
>> +    }
>> +
>> +    if (!xmlStrEqual(root->name, BAD_CAST "domainsnapshot")) {
>> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>> +                              "%s", _("incorrect root element"));
>> +        goto cleanup;
>> +    }
>> +
>> +    ctxt = xmlXPathNewContext(xml);
>> +    if (ctxt == NULL) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +
>> +    if (VIR_ALLOC(def) < 0) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +
>> +    ctxt->node = root;
>> +
>> +    def->name = virXPathString("string(./name)", ctxt);
>> +    if (def->name == NULL) {
>> +        /* make up a name */
>> +        gettimeofday(&tv, NULL);
>> +        localtime_r(&tv.tv_sec, &time_info);
>> +        strftime(timestr, sizeof(timestr), "%F_%T", &time_info);
>> +        def->name = strdup(timestr);
>> +    }
>> +    if (def->name == NULL) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +
>> +    def->description = virXPathString("string(./description)", ctxt);
>> +
>> +    if (!newSnapshot) {
>> +        creation = virXPathString("string(./creationTime)", ctxt);
> 
> I think it should be creationtime or creation_time, but not creationTime.

Taking the domain XML as an example, all 3 styles are used (e.g. currentMemory,
on_poweroff, and seclabel).  I don't really care too much, so I'll do whatever
is more comfortable.

<snip>

>> +char *virDomainSnapshotDefFormat(char *domain_uuid,
>> +                                 virDomainSnapshotDefPtr def)
>> +{
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +    char timestr[100];
>> +    struct tm time_info;
>> +
>> +    virBufferAddLit(&buf, "<domainsnapshot>\n");
>> +    virBufferVSprintf(&buf, "  <name>%s</name>\n", def->name);
>> +    if (def->description)
>> +        virBufferVSprintf(&buf, "  <description>%s</description>\n",
>> +                          def->description);
>> +    virBufferVSprintf(&buf, "  <state>%s</state>\n",
>> +                      virDomainStateTypeToString(def->state));
>> +    if (def->parent) {
>> +        virBufferAddLit(&buf, "  <parent>\n");
>> +        virBufferVSprintf(&buf, "    <name>%s</name>\n", def->parent);
>> +        virBufferAddLit(&buf, "  </parent>\n");
>> +    }
>> +    localtime_r(&def->creationTime, &time_info);
>> +    strftime(timestr, sizeof(timestr), "%F_%T", &time_info);
> 
> Again, you handle the time in local time. You could use %F_%T%z to
> include the local time zone and then handle that in the parsing
> function to get a correct UTC time back.
> 
> Maybe a better solution is to just store the Unix time in seconds in
> UTC (as returned by the time() function) in the XML and let
> applications do the conversion into a more human readable format and
> take care of timezone stuff. This way we get rid of the timezone
> problem at the libvirt level at all.

Well, I was sort of shooting for that by defining the field to be UTC
(which I obviously mis-implemented with localtime_r).  It would be nice
to have a human-readable string in the XML, though, which is why I didn't
just use seconds since the Epoch.  Can I just use gmtime_r()
here to get the time in UTC, and then strptime and mktime above will do the right thing?

-- 
Chris Lalancette


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