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 @@ <mode>0744</mode> <label>virt_image_t</label> </permissions> + <timestamps> + <atime>1341933637.27319099</atime>
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 + <seconds>.<nanoseconds> 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 @@ <label>virt_image_t</label> </permissions> <timestamps> - <atime>1341933637.27319099</atime> - <ctime>1341930622.47245868</ctime> - <mtime>1341930622.47245868</mtime> + <atime>1341933637.273190990</atime> + <mtime>1341930622.047245868</mtime> + <ctime>1341930622.047245868</ctime> </timestamps> <encryption type='...'> ... @@ -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 - <seconds>.<nanoseconds> 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 <seconds>.<nanoseconds> 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> -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org