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

On 07/25/2012 01:43 AM, Hendrik Schwartke wrote: Listing the actual output XML in the commit message will help future readers. 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. 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. 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. 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. 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). 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> -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org