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

On 07/25/2012 01:43 AM, Hendrik Schwartke wrote: > The access, birth, modification and change times are added to > storage volumes and corresponding xml representations. Listing the actual output XML in the commit message will help future readers. > --- > bootstrap.conf | 1 + > docs/formatstorage.html.in | 16 ++++++++++++++++ > docs/schemas/storagevol.rng | 34 ++++++++++++++++++++++++++++++++++ > src/conf/storage_conf.c | 20 ++++++++++++++++++++ > src/conf/storage_conf.h | 13 +++++++++++++ > src/storage/storage_backend.c | 6 ++++++ > 6 files changed, 90 insertions(+) We should really update at least one test to validate our rng changes. For that matter, 'make check' fails without at least some updates, because your code blindly outputs<timestamps> with contents of 0 for a default-initialized struct, and with no way to parse input, the xml2xml round-trip test can't validate our output code. > +++ b/docs/formatstorage.html.in > @@ -141,6 +141,11 @@ > &lt;mode&gt;0744&lt;/mode&gt; > &lt;label&gt;virt_image_t&lt;/label&gt; > &lt;/permissions&gt; > +&lt;timestamps&gt; > +&lt;atime&gt;1341933637.27319099&lt;/atime&gt; That's only 8 digits for subsecond resolution. Are you truncating a trailing 0, or are you missing a leading 0? Thinking about it more, it's easier for end users to parse a fixed-length 9-digit number with trailing zeros and always have it be scaled correctly than it is to make them parse an arbitrary length number and then scale it to 9 digits, so I'd prefer leaving trailing zeros intact and only omit the subsecond resolution when it is exactly 0, at least on output. > @@ -172,6 +177,17 @@ > 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. 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. If > + nanosecond resolution isn't supported by the host OS or filesystem then the > + nanoseconds part is omitted. It is also omitted when zero. This is a > + readonly attribute and is ignored when creating a volume. > +<span class="since">Since 0.10.0</span> Long lines; I reformatted to fit in 80 columns. Technically, while<btime> and<ctime> must be ignored (as we can't really fake them), we could use futimens during creation to honor <atime> and<mtime> as a future extension, if we thought it was worth the ability to let people create volumes with hand-controlled timestamps. Doesn't affect this patch, though. > +++ b/docs/schemas/storagevol.rng > @@ -63,6 +63,39 @@ > </optional> > </define> > > +<define name='timestamps'> > +<optional> > +<element name='timestamps'> > +<optional> > +<element name='atime'> > +<ref name='timestamp'/> > +</element> > +</optional> If we want to allow creation to specify timestamps, then we should allow <interleave> of these subelements. In fact, I see no harm in allowing that now. > + > +<define name='timestamp'> > +<data type='string'> > +<param name="pattern">[0-9]+(\.[0-9]+)?</param> On output, we could enforce {9} instead of + in this regex. But if we ever allow input, then this is too strict (we want {0,9} to allow the user to give a shortened form, and deal with scaling the value appropriately on our input parse). > @@ -1277,6 +1290,13 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, > > virBufferAddLit(buf,"</permissions>\n"); > > + virBufferAddLit(buf, "<timestamps>\n"); > + virStorageVolTimestampFormat(buf, "atime",&def->timestamps.atime); > + virStorageVolTimestampFormat(buf, "btime",&def->timestamps.btime); > + virStorageVolTimestampFormat(buf, "ctime",&def->timestamps.ctime); > + virStorageVolTimestampFormat(buf, "mtime",&def->timestamps.mtime); I really do think laying this out in 'struct stat' order makes more sense; atim, mtim, ctim, then btim. XPath notation will be able to find it regardless of our ordering. I don't know why we use -Waggregate-return; gcc doesn't like stat-time.h as a result; so I had to disable it for now; maybe upstream gnulib will let us change the signature to something that modifies a pointer argument instead of returning an aggregate, but that's a change for down the road. Another nice followup patch would be adding timestamps to directory storagepool output XML. Everything else looked good. Thanks again for your patience. Here's what I squashed in, before pushing: diff --git i/docs/formatstorage.html.in w/docs/formatstorage.html.in index 2a578e9..9f93db8 100644 --- i/docs/formatstorage.html.in +++ w/docs/formatstorage.html.in @@ -142,9 +142,9 @@ &lt;label&gt;virt_image_t&lt;/label&gt; &lt;/permissions&gt; &lt;timestamps&gt; -&lt;atime&gt;1341933637.27319099&lt;/atime&gt; -&lt;ctime&gt;1341930622.47245868&lt;/ctime&gt; -&lt;mtime&gt;1341930622.47245868&lt;/mtime&gt; +&lt;atime&gt;1341933637.273190990&lt;/atime&gt; +&lt;mtime&gt;1341930622.047245868&lt;/mtime&gt; +&lt;ctime&gt;1341930622.047245868&lt;/ctime&gt; &lt;/timestamps&gt; &lt;encryption type='...'&gt; ... @@ -178,14 +178,16 @@ <span class="since">Since 0.4.1</span> </dd> <dt><code>timestamps</code></dt> -<dd>Provides timing information about the volume. 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. If - nanosecond resolution isn't supported by the host OS or filesystem then the - nanoseconds part is omitted. It is also omitted when zero. This is a - readonly attribute and is ignored when creating a volume. +<dd>Provides timing information about the volume. 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 (1 Jan 1970). If nanosecond resolution + is 0 or otherwise unsupported by the host OS or filesystem, + then the nanoseconds part is omitted. This is a readonly + attribute and is ignored when creating a volume. <span class="since">Since 0.10.0</span> </dd> <dt><code>encryption</code></dt> diff --git i/docs/schemas/storagevol.rng w/docs/schemas/storagevol.rng index 7b35520..8335b61 100644 --- i/docs/schemas/storagevol.rng +++ w/docs/schemas/storagevol.rng @@ -66,33 +66,35 @@ <define name='timestamps'> <optional> <element name='timestamps'> -<optional> -<element name='atime'> -<ref name='timestamp'/> -</element> -</optional> -<optional> -<element name='btime'> -<ref name='timestamp'/> -</element> -</optional> -<optional> -<element name='ctime'> -<ref name='timestamp'/> -</element> -</optional> -<optional> -<element name='mtime'> -<ref name='timestamp'/> -</element> -</optional> +<interleave> +<optional> +<element name='atime'> +<ref name='timestamp'/> +</element> +</optional> +<optional> +<element name='btime'> +<ref name='timestamp'/> +</element> +</optional> +<optional> +<element name='ctime'> +<ref name='timestamp'/> +</element> +</optional> +<optional> +<element name='mtime'> +<ref name='timestamp'/> +</element> +</optional> +</interleave> </element> </optional> </define> <define name='timestamp'> <data type='string'> -<param name="pattern">[0-9]+(\.[0-9]+)?</param> +<param name="pattern">[0-9]+(\.[0-9]{0,9})?</param> </data> </define> diff --git i/m4/virt-compile-warnings.m4 w/m4/virt-compile-warnings.m4 index 1817047..9dee000 100644 --- i/m4/virt-compile-warnings.m4 +++ w/m4/virt-compile-warnings.m4 @@ -55,6 +55,8 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ dontwarn="$dontwarn -Wunsafe-loop-optimizations" # Things like virAsprintf mean we can't use this dontwarn="$dontwarn -Wformat-nonliteral" + # Gnulib's stat-time.h violates this + dontwarn="$dontwarn -Waggregate-return" # We might fundamentally need some of these disabled forever, but # ideally we'd turn many of them on diff --git i/src/conf/storage_conf.c w/src/conf/storage_conf.c index 66b5682..3132aae 100644 --- i/src/conf/storage_conf.c +++ w/src/conf/storage_conf.c @@ -286,9 +286,11 @@ virStorageVolDefFree(virStorageVolDefPtr def) { VIR_FREE(def->target.path); VIR_FREE(def->target.perms.label); + VIR_FREE(def->target.timestamps); virStorageEncryptionFree(def->target.encryption); VIR_FREE(def->backingStore.path); VIR_FREE(def->backingStore.perms.label); + VIR_FREE(def->backingStore.timestamps); virStorageEncryptionFree(def->backingStore.encryption); VIR_FREE(def); } @@ -1301,12 +1303,14 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf,"</permissions>\n"); - virBufferAddLit(buf, "<timestamps>\n"); - virStorageVolTimestampFormat(buf, "atime",&def->timestamps.atime); - virStorageVolTimestampFormat(buf, "btime",&def->timestamps.btime); - virStorageVolTimestampFormat(buf, "ctime",&def->timestamps.ctime); - virStorageVolTimestampFormat(buf, "mtime",&def->timestamps.mtime); - virBufferAddLit(buf, "</timestamps>\n"); + if (def->timestamps) { + virBufferAddLit(buf, "<timestamps>\n"); + virStorageVolTimestampFormat(buf, "atime", &def->timestamps->atime); + virStorageVolTimestampFormat(buf, "mtime", &def->timestamps->mtime); + virStorageVolTimestampFormat(buf, "ctime", &def->timestamps->ctime); + virStorageVolTimestampFormat(buf, "btime", &def->timestamps->btime); + virBufferAddLit(buf, "</timestamps>\n"); + } if (def->encryption) { virBufferAdjustIndent(buf, 4); diff --git i/src/conf/storage_conf.h w/src/conf/storage_conf.h index 4477195..4fb99df 100644 --- i/src/conf/storage_conf.h +++ w/src/conf/storage_conf.h @@ -89,7 +89,7 @@ struct _virStorageVolTarget { char *path; int format; virStoragePerms perms; - virStorageTimestamps timestamps; + virStorageTimestampsPtr timestamps; int type; /* only used by disk backend for partition type */ /* Currently used only in virStorageVolDef.target, not in .backingstore. */ virStorageEncryptionPtr encryption; diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c index e05cadd..4a2109e 100644 --- i/src/storage/storage_backend.c +++ w/src/storage/storage_backend.c @@ -1,7 +1,7 @@ /* * storage_backend.c: internal storage driver backend contract * - * Copyright (C) 2007-2011 Red Hat, Inc. + * Copyright (C) 2007-2012 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -1215,10 +1215,14 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, target->perms.uid = sb.st_uid; target->perms.gid = sb.st_gid; - target->timestamps.atime = get_stat_atime(&sb); - target->timestamps.btime = get_stat_birthtime(&sb); - target->timestamps.ctime = get_stat_ctime(&sb); - target->timestamps.mtime = get_stat_mtime(&sb); + if (!target->timestamps&& VIR_ALLOC(target->timestamps)< 0) { + virReportOOMError(); + return -1; + } + target->timestamps->atime = get_stat_atime(&sb); + target->timestamps->btime = get_stat_birthtime(&sb); + target->timestamps->ctime = get_stat_ctime(&sb); + target->timestamps->mtime = get_stat_mtime(&sb); VIR_FREE(target->perms.label); diff --git i/tests/storagevolxml2xmlin/vol-file.xml w/tests/storagevolxml2xmlin/vol-file.xml index fdca510..d3f65f6 100644 --- i/tests/storagevolxml2xmlin/vol-file.xml +++ w/tests/storagevolxml2xmlin/vol-file.xml @@ -11,5 +11,10 @@ <group>0</group> <label>virt_image_t</label> </permissions> +<timestamps> +<atime>1341933637.273190990</atime> +<mtime>1341930622.047245868</mtime> +<ctime>1341930622.047245868</ctime> +</timestamps> </target> </volume>