[libvirt] [PATCH] Added timestamps to storage volumes
Hendrik Schwartke
hendrik at os-t.de
Fri Aug 3 07:39:51 UTC 2012
Thanks for your support Eric!
Hendrik
On 03.08.2012 01:14, Eric Blake wrote:
> 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>
>
More information about the libvir-list
mailing list