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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list