[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 11:33 AM, Matthias Bolte wrote:
> 2010/4/2 Chris Lalancette <clalance redhat com>:
>> 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
> 
>>>
>>>> +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.
> 
> I missed currentMemory and since it's already a mixture of all styles
> lets just keep creationTime.
> 
>>>> +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?
>>
> 
> I think the problem is that struct tm doesn't contain a timezone
> filed. Therefore, strptime parses a time that is not fixed in a
> timezone. So even If we would use a time format like
> 
>   2010-04-02_12:30:58+0200
> 
> it won't help with strptime. strptime knows %z for timezone part, but
> just ignores it.

Ah, I see.  That does make it a problem.

> 
> mktime operates in local time. The man page says:
> 
> "The  mktime() function converts a broken-down time structure,
> expressed as local time, to calendar time representation"
> 
> So gmtime_r doesn't help.
> 
> I think we could use seconds since the Epoch from time() as the actual
> value, and attach a human readable version (from strftime with "%a, %d
> %b %Y %H:%M:%S %z" in RFC 2822 format) in a comment like this:
> 
>   <creationTime>1270221648</creationTime><!-- Fri, 02 Apr 2010
> 17:20:48 +0200 -->
> 
> That way we don't need to deal with the subtle timezone stuff as
> seconds since the Epoch is in UTC and we have a human readable version
> in virsh snapshot-dumpxml for example.
> 
> Applications then can take the seconds since the Epoch value and
> format it as they like to local time or what ever they prefer.

Yeah, I'll change it over to seconds since the epoch.  Thanks.

-- 
Chris Lalancette


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