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

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



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 @@
              &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>



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