[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 @@
>              &lt;mode&gt;0744&lt;/mode&gt;
>              &lt;label&gt;virt_image_t&lt;/label&gt;
>            &lt;/permissions&gt;
> +          &lt;timestamps&gt;
> +            &lt;atime&gt;1341933637.27319099&lt;/atime&gt;

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
> +        &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.
> +        <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 @@
             &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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]