Re: [libvirt] [PATCH] Added timestamps to storage volumes

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&lt;seconds&gt;.&lt;nanoseconds&gt; >> 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.

> return; > virBufferAsprintf(buf, "<%s>%llu", name, > (unsigned long long) ts->tv_sec); > if (ts->tv_nsec)

> 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.