8:13 p.m.
On 04/02/2010 11:33 AM, Matthias Bolte wrote:
2010/4/2 Chris Lalancette <clalance(a)redhat.com>:
> On 04/02/2010 09:25 AM, Matthias Bolte wrote:
>> 2010/4/2 Chris Lalancette <clalance(a)redhat.com>:
>>> Signed-off-by: Chris Lalancette <clalance(a)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