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>