[libvirt] [PATCH] Added timestamps to storage volumes

Hendrik Schwartke hendrik at os-t.de
Mon Jul 16 07:55:41 UTC 2012


On 16.07.2012 09:45, Hendrik Schwartke wrote:
> On 13.07.2012 17:14, Eric Blake wrote:
>> On 07/13/2012 08:38 AM, Hendrik Schwartke wrote:
>>> !!! DON'T PUSH until stat-time lgpl 3 issue is fixed
>>> !!! To tests this change lgpl version to 3 in bootstrap.conf:176
>>>
>>> The access, birth, modification and change times are added to
>>> storage volumes and corresponding xml representations.
>>> ---
>>>   bootstrap.conf                |    1 +
>>>   docs/formatstorage.html.in    |   13 +++++++++++++
>>>   docs/schemas/storagevol.rng   |   36 
>>> ++++++++++++++++++++++++++++++++++++
>>>   src/conf/storage_conf.c       |   18 ++++++++++++++++++
>>>   src/conf/storage_conf.h       |   13 +++++++++++++
>>>   src/storage/storage_backend.c |    6 ++++++
>>>   6 files changed, 87 insertions(+)
>>>
>>> diff --git a/bootstrap.conf b/bootstrap.conf
>>> index 9b42cbf..da0b960 100644
>>> --- a/bootstrap.conf
>>> +++ b/bootstrap.conf
>>> @@ -115,6 +115,7 @@ vc-list-files
>>>   vsnprintf
>>>   waitpid
>>>   warnings
>>> +stat-time
>>>   '
>> Insert in sorted order.
>>
>>
>>> @@ -172,6 +177,14 @@
>>>           contains the MAC (eg SELinux) label string.
>>> <span class="since">Since 0.4.1</span>
>>> </dd>
>>> +<dt><code>timestamps</code></dt>
>>> +<dd>Provides timing information about the volume. The four sub 
>>> elements
>> since btime is omitted on Linux, maybe this would read better as 'Up to
>> four sub-elements are present, where'
>>
>>> +<code>atime</code>,<code>btime</code>,<code>ctime</code>  
>>> and<code>mtime</code>
>>> +        hold the access, birth, change and modification time of the 
>>> volume, where known.
>>> +        The used time format is<seconds>.<nanoseconds> 
>>> since the beginning
>>> +        of the epoch.  This is a readonly attribute and is ignored 
>>> when creating
>>> +        a volume.<span class="since">Since 0.10.0</span>
>>
>>> +<define name='timestamps'>
>>> +<optional>
>>> +<element name='timestamps'>
>>> +<optional>
>>> +<element name='atime'>
>>> +<data type="string">
>>> +<param name="pattern">[0-9]+\.[0-9]+</param>
>>> +</data>
>> It might be worth writing the regex to permit eliding the sub-second
>> resolution, on file systems that only have 1 second resolution.  Given
> Well, the problem here is that stat-time doesn't offer a way to 
> determine if sub-second resolution is available. If the system doesn't 
> support it then tv_nsec is simply zero. So there is always a 
> sub-second part in the timestamp and such an regex could be slightly 
> misleading. I will change it anyway and add a comment to the schema.
>> that we are repeating this<data>  four times, it might be worth defining
>> it, for a shorter diff:
>>
>> <element name='atime'>
>> <ref name='timestamp'/>
>> </element>
>>
>> ...
>> <define name='timestamp'>
>> <data type='string'>
>> <param name='pattern'>[0-9]+(\.[0-9]+)?</param>
>> </data>
>> </define>
>>
>>> +++ b/src/conf/storage_conf.c
>>> @@ -1277,6 +1277,24 @@ 
>>> virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
>>>
>>>       virBufferAddLit(buf,"</permissions>\n");
>>>
>>> +    virBufferAddLit(buf, "<timestamps>\n");
>>> +    virBufferAsprintf(buf, "<atime>%llu.%ld</atime>\n",
>>> +                      (unsigned long long) 
>>> def->timestamps.atime.tv_sec,
>>> +                      def->timestamps.atime.tv_nsec);
>> Eliding a sub-second suffix when tv_nsec == 0 would be easier with a
>> helper function:
>>
>> void
>> virStorageVolTimestampFormat(virBufferPtr buf, const char *name,
>>                               struct timespec *ts)
>> {
>>      if (ts->tv_nsec<  0)
> That's never the case. See above.
Yups, wrong line. Of course that could be the case. But again I prefer 
to check tv_sec also.
>>          return;
>>      virBufferAsprintf(buf, "<%s>%llu", name,
>>                        (unsigned long long) ts->tv_sec);
>>      if (ts->tv_nsec)
That the line I wanted to comment. I'm not sure if it's such a good idea 
to omit the sub second part. Although it's very unlikely that this 
happends on systems that support tv_nsec it could be misleading.
>>          virBufferAsprintf(buf, ".%ld", tv->tv_nsec);
>>      virBufferAsprintf(buf, "</%s>\n", name);
>> }
>>
>> called as:
>>
>> virStorageVolTimestampFormat(buf, "atime",&def->timestamps.atime);
>> virStorageVolTimestampFormat(buf, "atime",&def->timestamps.btime);
>>
>> and so on.
>>
>> Actually, I'd list atime, mtime, ctime, btime - in that order - rather
>> than trying to sort the names alphabetically (that is, match typical
>> 'struct stat' ordering).
> Well I thought about that and I think it's better to sort it 
> alphabetically, because everyone who doesn't know 'struct stat' could 
> be very puzzled about atime, mtime, ctime, btime.
>>
>>> +typedef virStorageTimestamps *virStorageTimestampsPtr;
>>> +struct _virStorageTimestamps {
>>> +    struct timespec atime;
>>> +    /* if btime.tv_sec == -1&&  btime.tv_nsec == -1 than
>>> +     * birth time is unknown
>> Doesn't gnulib guarantee that tv_nsec == -1 in isolation is sufficient
>> to point out an unknown value?  That is, checking tv_sec == -1 is 
>> overhead.
> Well, actually yes, but the the description on get_stat_birthtime 
> says: "Return *ST's birth time, if available; otherwise return a value 
> with tv_sec and tv_nsec both equal to -1.". So to be sure I prefer to 
> check both.
>> Looking nicer.  I'll have to ping upstream on gnulib about the last
>> holdout on the relicensing of stat-time; and I'm also still waiting for
>> the security fix in updated automake to hit Fedora.
>>
> Ok, please let me know if there are some changes here. Meanwhile I 
> will adapt my patch.




More information about the libvir-list mailing list