[libvirt] [PATCH] Added timestamps to volumes
The access, modify and change times are added to volumes and corresponding xml representations. --- docs/schemas/storagevol.rng | 17 +++++++++++++++++ src/conf/storage_conf.c | 9 +++++++++ src/conf/storage_conf.h | 9 +++++++++ src/storage/storage_backend.c | 4 ++++ 4 files changed, 39 insertions(+) diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 7a74331..fc7eb09 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -63,6 +63,22 @@ </optional> </define> + <define name='timestamps'> + <optional> + <element name='timestamps'> + <element name='atime'> + <ref name='unsignedLong'/> + </element> + <element name='mtime'> + <ref name='unsignedLong'/> + </element> + <element name='ctime'> + <ref name='unsignedLong'/> + </element> + </element> + </optional> + </define> + <define name='target'> <element name='target'> <optional> @@ -72,6 +88,7 @@ </optional> <ref name='format'/> <ref name='permissions'/> + <ref name='timestamps'/> <optional> <ref name='encryption'/> </optional> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ab8df9e..a4cdac8 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1272,6 +1272,15 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf," </permissions>\n"); + virBufferAddLit(buf," <timestamps>\n"); + virBufferAsprintf(buf," <atime>%llu</atime>\n", + (unsigned long long) def->timestamps.atime); + virBufferAsprintf(buf," <mtime>%llu</mtime>\n", + (unsigned long long) def->timestamps.mtime); + virBufferAsprintf(buf," <ctime>%llu</ctime>\n", + (unsigned long long) def->timestamps.ctime); + virBufferAddLit(buf," </timestamps>\n"); + if (def->encryption) { virBufferAdjustIndent(buf, 4); if (virStorageEncryptionFormat(buf, def->encryption) < 0) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 5733b57..8cd1d63 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -46,6 +46,14 @@ struct _virStoragePerms { /* Storage volumes */ +typedef struct _virStorageTimestamps virStorageTimestamps; +typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + time_t atime; + time_t mtime; + time_t ctime; +}; + /* * How the volume's data is stored on underlying @@ -77,6 +85,7 @@ struct _virStorageVolTarget { char *path; int format; virStoragePerms perms; + virStorageTimestamps timestamps; int type; /* only used by disk backend for partition type */ /* Currently used only in virStorageVolDef.target, not in .backingstore. */ virStorageEncryptionPtr encryption; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e2e9b51..c827e3c 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1209,6 +1209,10 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, target->perms.uid = sb.st_uid; target->perms.gid = sb.st_gid; + target->timestamps.atime = sb.st_atime; + target->timestamps.mtime = sb.st_mtime; + target->timestamps.ctime = sb.st_ctime; + VIR_FREE(target->perms.label); #if HAVE_SELINUX -- 1.7.9.5
On 07/10/2012 05:52 AM, Hendrik Schwartke wrote:
The access, modify and change times are added to volumes and corresponding xml representations. --- docs/schemas/storagevol.rng | 17 +++++++++++++++++
Incomplete. You must also document this in docs/formatstorage.html.in before this patch can be applied.
src/conf/storage_conf.c | 9 +++++++++ src/conf/storage_conf.h | 9 +++++++++ src/storage/storage_backend.c | 4 ++++ 4 files changed, 39 insertions(+)
+++ b/docs/schemas/storagevol.rng @@ -63,6 +63,22 @@ </optional> </define>
+ <define name='timestamps'> + <optional> + <element name='timestamps'> + <element name='atime'> + <ref name='unsignedLong'/> + </element> + <element name='mtime'> + <ref name='unsignedLong'/> + </element> + <element name='ctime'> + <ref name='unsignedLong'/> + </element> + </element> + </optional>
Sounds interesting. Should we also list birth-time, which is available from some filesystems as a fourth timestamp? On BSD and Cygwin, birthtime is available as part of stat(); on Linux, you still have to use ioctl() or wait for the proposed xstat() interface to ever be finalized, and even then, not all file systems track that information, so it would have to be an optional element. I also think we need to track things to full precision, as in seconds + nanoseconds since epoch. That is, a proper timestamp would look like 1341925773.351768083 on a file system with full 9-digit sub-second resolution.
+ virBufferAddLit(buf," <timestamps>\n");
Space after comma, throughout the patch.
+typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + time_t atime;
Store this as struct timespec, to match the POSIX definition of st_atim having both seconds and nanoseconds as part of stat() (oh phooey - the gnulib module stat-time for portably getting at nanoseconds is currently LGPLv3+; I'll see about whether we can get that relaxed). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The access, modification and change times are added to storage volumes and corresponding xml representations. --- docs/formatstorage.html.in | 13 +++++++++++++ docs/schemas/storagevol.rng | 23 +++++++++++++++++++++++ src/conf/storage_conf.c | 12 ++++++++++++ src/conf/storage_conf.h | 9 +++++++++ src/storage/storage_backend.c | 20 ++++++++++++++++++++ 5 files changed, 77 insertions(+) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d0e4319..c4d6d85 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -141,6 +141,11 @@ <mode>0744</mode> <label>virt_image_t</label> </permissions> + <timestamps> + <atime>1341933637.27319099</atime> + <mtime>1341930622.47245868</mtime> + <ctime>1341930622.47245868</ctime> + </timestamps> <encryption type='...'> ... </encryption> @@ -172,6 +177,14 @@ 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. The three sub elements + <code>atime</code>, <code>mtime</code> and <code>ctime</code> hold the + access, modification and respectively the change time of the volume. The + used time format is <seconds>.<nanoseconds> since the beginning + of the epoch. 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> <dd>If present, specifies how the volume is encrypted. See the <a href="formatstorageencryption.html">Storage Encryption</a> page diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 7a74331..dafe918 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -63,6 +63,28 @@ </optional> </define> + <define name='timestamps'> + <optional> + <element name='timestamps'> + <element name='atime'> + <data type="string"> + <param name="pattern">[0-9]+\.[0-9]+</param> + </data> + </element> + <element name='mtime'> + <data type="string"> + <param name="pattern">[0-9]+\.[0-9]+</param> + </data> + </element> + <element name='ctime'> + <data type="string"> + <param name="pattern">[0-9]+\.[0-9]+</param> + </data> + </element> + </element> + </optional> + </define> + <define name='target'> <element name='target'> <optional> @@ -72,6 +94,7 @@ </optional> <ref name='format'/> <ref name='permissions'/> + <ref name='timestamps'/> <optional> <ref name='encryption'/> </optional> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ab8df9e..435ad00 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1272,6 +1272,18 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf," </permissions>\n"); + virBufferAddLit(buf, " <timestamps>\n"); + virBufferAsprintf(buf, " <atime>%llu.%lu</atime>\n", + (unsigned long long) def->timestamps.atime.tv_sec, + def->timestamps.atime.tv_nsec); + virBufferAsprintf(buf, " <mtime>%llu.%lu</mtime>\n", + (unsigned long long) def->timestamps.mtime.tv_sec, + def->timestamps.mtime.tv_nsec); + virBufferAsprintf(buf, " <ctime>%llu.%lu</ctime>\n", + (unsigned long long) def->timestamps.ctime.tv_sec, + def->timestamps.ctime.tv_nsec); + virBufferAddLit(buf, " </timestamps>\n"); + if (def->encryption) { virBufferAdjustIndent(buf, 4); if (virStorageEncryptionFormat(buf, def->encryption) < 0) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 5733b57..977b136 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -46,6 +46,14 @@ struct _virStoragePerms { /* Storage volumes */ +typedef struct _virStorageTimestamps virStorageTimestamps; +typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + struct timespec mtime; + struct timespec ctime; +}; + /* * How the volume's data is stored on underlying @@ -77,6 +85,7 @@ struct _virStorageVolTarget { char *path; int format; virStoragePerms perms; + virStorageTimestamps timestamps; int type; /* only used by disk backend for partition type */ /* Currently used only in virStorageVolDef.target, not in .backingstore. */ virStorageEncryptionPtr encryption; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e2e9b51..ce4d808 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1156,6 +1156,9 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, unsigned long long *capacity) { struct stat sb; + struct timespec *const atime=&target->timestamps.atime, + *const mtime=&target->timestamps.mtime, + *const catime=&target->timestamps.ctime; #if HAVE_SELINUX security_context_t filecon = NULL; #endif @@ -1209,6 +1212,23 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, target->perms.uid = sb.st_uid; target->perms.gid = sb.st_gid; + atime->tv_sec = sb.st_atime; + mtime->tv_sec = sb.st_mtime; + catime->tv_sec = sb.st_ctime; +#if _BSD_SOURCE || _SVID_SOURCE + atime->tv_nsec = sb.st_atim.tv_nsec; + mtime->tv_nsec = sb.st_mtim.tv_nsec; + catime->tv_nsec = sb.st_ctim.tv_nsec; +#elif _POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700 + atime->tv_nsec = sb.st_atimensec; + mtime->tv_nsec = sb.st_mtimensec; + catime->tv_nsec = sb.st_ctimensec; +#else + atime->tv_nsec = 0; + mtime->tv_nsec = 0; + catime->tv_nsec = 0; +#endif + VIR_FREE(target->perms.label); #if HAVE_SELINUX -- 1.7.9.5
On 07/10/2012 09:22 AM, Hendrik Schwartke wrote:
The access, modification and change times are added to storage volumes and corresponding xml representations. --- docs/formatstorage.html.in | 13 +++++++++++++ docs/schemas/storagevol.rng | 23 +++++++++++++++++++++++ src/conf/storage_conf.c | 12 ++++++++++++ src/conf/storage_conf.h | 9 +++++++++ src/storage/storage_backend.c | 20 ++++++++++++++++++++ 5 files changed, 77 insertions(+)
</dd> + <dt><code>timestamps</code></dt> + <dd>Provides timing information about the volume. The three sub elements + <code>atime</code>, <code>mtime</code> and <code>ctime</code> hold the + access, modification and respectively the change time of the volume. The
Grammar: hold the access, modification, and change times of the volume, where known. no need to mention 'respectively'.
+++ b/src/conf/storage_conf.c @@ -1272,6 +1272,18 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
virBufferAddLit(buf," </permissions>\n");
+ virBufferAddLit(buf, " <timestamps>\n"); + virBufferAsprintf(buf, " <atime>%llu.%lu</atime>\n", + (unsigned long long) def->timestamps.atime.tv_sec, + def->timestamps.atime.tv_nsec);
Technically, tv_nsec is a signed long, and you should be using %ld instead of %lu.
+++ b/src/storage/storage_backend.c @@ -1156,6 +1156,9 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, unsigned long long *capacity) { struct stat sb; + struct timespec *const atime=&target->timestamps.atime,
Space on either side of '='.
+ atime->tv_sec = sb.st_atime; + mtime->tv_sec = sb.st_mtime; + catime->tv_sec = sb.st_ctime; +#if _BSD_SOURCE || _SVID_SOURCE + atime->tv_nsec = sb.st_atim.tv_nsec;
Yuck. I've nearly got consensus to use the gnulib stat-time module, in which case this would instead be the much simpler: #include "stat-time.h" ... atime = get_stat_atime(sb); mtime = get_stat_mtime(sb); ctime = get_stat_mtime(sb); But before we can use the gnulib stat-time module, we have to update .gnulib and bootstrap.conf; and I'm holding off on the .gnulib update until after fixed Automake is available in at least Fedora 17 (right now, unless you are manually using automake 1.11.6 or newer, you are injecting a security bug into every other package that uses automake). Also, with gnulib's stat-time module, my earlier suggestion to include birthtime would be as simple as: btime = get_state_birthtime(sb); along with filtering the display: if (btime->tv_nsec == -1) { /* birth time not available */ } -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10.07.2012 17:36, Eric Blake wrote:
On 07/10/2012 09:22 AM, Hendrik Schwartke wrote:
The access, modification and change times are added to storage volumes and corresponding xml representations. --- docs/formatstorage.html.in | 13 +++++++++++++ docs/schemas/storagevol.rng | 23 +++++++++++++++++++++++ src/conf/storage_conf.c | 12 ++++++++++++ src/conf/storage_conf.h | 9 +++++++++ src/storage/storage_backend.c | 20 ++++++++++++++++++++ 5 files changed, 77 insertions(+) </dd> +<dt><code>timestamps</code></dt> +<dd>Provides timing information about the volume. The three sub elements +<code>atime</code>,<code>mtime</code> and<code>ctime</code> hold the + access, modification and respectively the change time of the volume. The Grammar:
hold the access, modification, and change times of the volume, where known.
no need to mention 'respectively'.
+++ b/src/conf/storage_conf.c @@ -1272,6 +1272,18 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
virBufferAddLit(buf,"</permissions>\n");
+ virBufferAddLit(buf, "<timestamps>\n"); + virBufferAsprintf(buf, "<atime>%llu.%lu</atime>\n", + (unsigned long long) def->timestamps.atime.tv_sec, + def->timestamps.atime.tv_nsec); Technically, tv_nsec is a signed long, and you should be using %ld instead of %lu.
+++ b/src/storage/storage_backend.c @@ -1156,6 +1156,9 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, unsigned long long *capacity) { struct stat sb; + struct timespec *const atime=&target->timestamps.atime, Space on either side of '='.
+ atime->tv_sec = sb.st_atime; + mtime->tv_sec = sb.st_mtime; + catime->tv_sec = sb.st_ctime; +#if _BSD_SOURCE || _SVID_SOURCE + atime->tv_nsec = sb.st_atim.tv_nsec;
Yuck. I've nearly got consensus to use the gnulib stat-time module, in which case this would instead be the much simpler: Ok, that sounds good. But what exactly does 'nearly' mean? #include "stat-time.h" ...
atime = get_stat_atime(sb); mtime = get_stat_mtime(sb); ctime = get_stat_mtime(sb); Of course, you are absolutely right. It is much cleaner to use stat-time. But before we can use the gnulib stat-time module, we have to update .gnulib and bootstrap.conf; and I'm holding off on the .gnulib update until after fixed Automake is available in at least Fedora 17 (right now, unless you are manually using automake 1.11.6 or newer, you are injecting a security bug into every other package that uses automake). So any idea how long this will take? Also, with gnulib's stat-time module, my earlier suggestion to include birthtime would be as simple as:
btime = get_state_birthtime(sb);
along with filtering the display:
if (btime->tv_nsec == -1) { /* birth time not available */ }
On 07/11/2012 01:12 AM, Hendrik Schwartke wrote:
+ atime->tv_sec = sb.st_atime; + mtime->tv_sec = sb.st_mtime; + catime->tv_sec = sb.st_ctime; +#if _BSD_SOURCE || _SVID_SOURCE + atime->tv_nsec = sb.st_atim.tv_nsec; Yuck. I've nearly got consensus to use the gnulib stat-time module, in which case this would instead be the much simpler: Ok, that sounds good. But what exactly does 'nearly' mean?
It means 3 of the 4 copyright holders have agreed, with less than 24-hour turnaround time; the 4th will likely respond sometime this week: https://lists.gnu.org/archive/html/bug-gnulib/2012-07/msg00141.html
But before we can use the gnulib stat-time module, we have to update .gnulib and bootstrap.conf; and I'm holding off on the .gnulib update until after fixed Automake is available in at least Fedora 17 (right now, unless you are manually using automake 1.11.6 or newer, you are injecting a security bug into every other package that uses automake). So any idea how long this will take?
Being a security patch that affects multiple packages, I'm sure it won't be too long in the works. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/11/2012 07:09 AM, Eric Blake wrote:
On 07/11/2012 01:12 AM, Hendrik Schwartke wrote:
+ atime->tv_sec = sb.st_atime; + mtime->tv_sec = sb.st_mtime; + catime->tv_sec = sb.st_ctime; +#if _BSD_SOURCE || _SVID_SOURCE + atime->tv_nsec = sb.st_atim.tv_nsec; Yuck. I've nearly got consensus to use the gnulib stat-time module, in which case this would instead be the much simpler: Ok, that sounds good. But what exactly does 'nearly' mean?
It means 3 of the 4 copyright holders have agreed, with less than 24-hour turnaround time; the 4th will likely respond sometime this week: https://lists.gnu.org/archive/html/bug-gnulib/2012-07/msg00141.html
Correction - there is only one copyright holder - the FSF. But 3 of the 4 original contributors have agreed to a relicense, and if all original contributors agree, then we don't have to bother the FSF with a much more formal and lengthy process of convincing the FSF to relicense. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11.07.2012 15:10, Eric Blake wrote:
On 07/11/2012 07:09 AM, Eric Blake wrote:
On 07/11/2012 01:12 AM, Hendrik Schwartke wrote:
+ atime->tv_sec = sb.st_atime; + mtime->tv_sec = sb.st_mtime; + catime->tv_sec = sb.st_ctime; +#if _BSD_SOURCE || _SVID_SOURCE + atime->tv_nsec = sb.st_atim.tv_nsec; Yuck. I've nearly got consensus to use the gnulib stat-time module, in which case this would instead be the much simpler: Ok, that sounds good. But what exactly does 'nearly' mean? It means 3 of the 4 copyright holders have agreed, with less than 24-hour turnaround time; the 4th will likely respond sometime this week: https://lists.gnu.org/archive/html/bug-gnulib/2012-07/msg00141.html
Correction - there is only one copyright holder - the FSF. But 3 of the 4 original contributors have agreed to a relicense, and if all original contributors agree, then we don't have to bother the FSF with a much more formal and lengthy process of convincing the FSF to relicense.
Oh, cool. I didn't thought that this could be done in such a short term. So I will update my patch using stat-time this week so that it is ready if the license has changed. (I will also add the birth time.) Thanks Hendrik
!!! DON'T PUSH until stat-time lgpl 3 issue is fixed !!! To tests this change lgpl version to 3 in bootstrap.conf:176 The access, birth, modification and change times are added to storage volumes and corresponding xml representations. --- bootstrap.conf | 1 + docs/formatstorage.html.in | 13 +++++++++++++ docs/schemas/storagevol.rng | 36 ++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.c | 18 ++++++++++++++++++ src/conf/storage_conf.h | 13 +++++++++++++ src/storage/storage_backend.c | 6 ++++++ 6 files changed, 87 insertions(+) diff --git a/bootstrap.conf b/bootstrap.conf index 9b42cbf..da0b960 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -115,6 +115,7 @@ vc-list-files vsnprintf waitpid warnings +stat-time ' # Additional xgettext options to use. Use "\\\newline" to break lines. diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d0e4319..ec7ec90 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -141,6 +141,11 @@ <mode>0744</mode> <label>virt_image_t</label> </permissions> + <timestamps> + <atime>1341933637.27319099</atime> + <ctime>1341930622.47245868</ctime> + <mtime>1341930622.47245868</mtime> + </timestamps> <encryption type='...'> ... </encryption> @@ -172,6 +177,14 @@ 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. The four sub elements + <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. 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> <dd>If present, specifies how the volume is encrypted. See the <a href="formatstorageencryption.html">Storage Encryption</a> page diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 7a74331..a20c7a5 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -63,6 +63,41 @@ </optional> </define> + <define name='timestamps'> + <optional> + <element name='timestamps'> + <optional> + <element name='atime'> + <data type="string"> + <param name="pattern">[0-9]+\.[0-9]+</param> + </data> + </element> + </optional> + <optional> + <element name='btime'> + <data type="string"> + <param name="pattern">[0-9]+\.[0-9]+</param> + </data> + </element> + </optional> + <optional> + <element name='ctime'> + <data type="string"> + <param name="pattern">[0-9]+\.[0-9]+</param> + </data> + </element> + </optional> + <optional> + <element name='mtime'> + <data type="string"> + <param name="pattern">[0-9]+\.[0-9]+</param> + </data> + </element> + </optional> + </element> + </optional> + </define> + <define name='target'> <element name='target'> <optional> @@ -72,6 +107,7 @@ </optional> <ref name='format'/> <ref name='permissions'/> + <ref name='timestamps'/> <optional> <ref name='encryption'/> </optional> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 36a3bb9..b1ec8da 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1277,6 +1277,24 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf," </permissions>\n"); + virBufferAddLit(buf, " <timestamps>\n"); + virBufferAsprintf(buf, " <atime>%llu.%ld</atime>\n", + (unsigned long long) def->timestamps.atime.tv_sec, + def->timestamps.atime.tv_nsec); + if(def->timestamps.btime.tv_sec != -1 || + def->timestamps.btime.tv_nsec != -1) { + virBufferAsprintf(buf, " <btime>%llu.%ld</btime>\n", + (unsigned long long) def->timestamps.btime.tv_sec, + def->timestamps.btime.tv_nsec); + } + virBufferAsprintf(buf, " <ctime>%llu.%ld</ctime>\n", + (unsigned long long) def->timestamps.ctime.tv_sec, + def->timestamps.ctime.tv_nsec); + virBufferAsprintf(buf, " <mtime>%llu.%ld</mtime>\n", + (unsigned long long) def->timestamps.mtime.tv_sec, + def->timestamps.mtime.tv_nsec); + virBufferAddLit(buf, " </timestamps>\n"); + if (def->encryption) { virBufferAdjustIndent(buf, 4); if (virStorageEncryptionFormat(buf, def->encryption) < 0) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 5733b57..706e874 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -46,6 +46,18 @@ struct _virStoragePerms { /* Storage volumes */ +typedef struct _virStorageTimestamps virStorageTimestamps; +typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + /* if btime.tv_sec == -1 && btime.tv_nsec == -1 than + * birth time is unknown + */ + struct timespec btime; + struct timespec ctime; + struct timespec mtime; +}; + /* * How the volume's data is stored on underlying @@ -77,6 +89,7 @@ struct _virStorageVolTarget { char *path; int format; virStoragePerms perms; + virStorageTimestamps timestamps; int type; /* only used by disk backend for partition type */ /* Currently used only in virStorageVolDef.target, not in .backingstore. */ virStorageEncryptionPtr encryption; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 6ea0881..c665711 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -57,6 +57,7 @@ #include "storage_backend.h" #include "logging.h" #include "virfile.h" +#include "stat-time.h" #if WITH_STORAGE_LVM # include "storage_backend_logical.h" @@ -1208,6 +1209,11 @@ 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); + VIR_FREE(target->perms.label); #if HAVE_SELINUX -- 1.7.9.5
On 07/13/2012 08:38 AM, Hendrik Schwartke wrote:
!!! DON'T PUSH until stat-time lgpl 3 issue is fixed !!! To tests this change lgpl version to 3 in bootstrap.conf:176
The access, birth, modification and change times are added to storage volumes and corresponding xml representations. --- bootstrap.conf | 1 + docs/formatstorage.html.in | 13 +++++++++++++ docs/schemas/storagevol.rng | 36 ++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.c | 18 ++++++++++++++++++ src/conf/storage_conf.h | 13 +++++++++++++ src/storage/storage_backend.c | 6 ++++++ 6 files changed, 87 insertions(+)
diff --git a/bootstrap.conf b/bootstrap.conf index 9b42cbf..da0b960 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -115,6 +115,7 @@ vc-list-files vsnprintf waitpid warnings +stat-time '
Insert in sorted order.
@@ -172,6 +177,14 @@ 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. The four sub elements
since btime is omitted on Linux, maybe this would read better as '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. This is a readonly attribute and is ignored when creating + a volume. <span class="since">Since 0.10.0</span>
+ <define name='timestamps'> + <optional> + <element name='timestamps'> + <optional> + <element name='atime'> + <data type="string"> + <param name="pattern">[0-9]+\.[0-9]+</param> + </data>
It might be worth writing the regex to permit eliding the sub-second resolution, on file systems that only have 1 second resolution. Given that we are repeating this <data> four times, it might be worth defining it, for a shorter diff: <element name='atime'> <ref name='timestamp'/> </element> ... <define name='timestamp'> <data type='string'> <param name='pattern'>[0-9]+(\.[0-9]+)?</param> </data> </define>
+++ b/src/conf/storage_conf.c @@ -1277,6 +1277,24 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
virBufferAddLit(buf," </permissions>\n");
+ virBufferAddLit(buf, " <timestamps>\n"); + virBufferAsprintf(buf, " <atime>%llu.%ld</atime>\n", + (unsigned long long) def->timestamps.atime.tv_sec, + def->timestamps.atime.tv_nsec);
Eliding a sub-second suffix when tv_nsec == 0 would be easier with a helper function: void virStorageVolTimestampFormat(virBufferPtr buf, const char *name, struct timespec *ts) { if (ts->tv_nsec < 0) return; virBufferAsprintf(buf, " <%s>%llu", name, (unsigned long long) ts->tv_sec); if (ts->tv_nsec) virBufferAsprintf(buf, ".%ld", tv->tv_nsec); virBufferAsprintf(buf, "</%s>\n", name); } called as: virStorageVolTimestampFormat(buf, "atime", &def->timestamps.atime); virStorageVolTimestampFormat(buf, "atime", &def->timestamps.btime); and so on. Actually, I'd list atime, mtime, ctime, btime - in that order - rather than trying to sort the names alphabetically (that is, match typical 'struct stat' ordering).
+typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + /* if btime.tv_sec == -1 && btime.tv_nsec == -1 than + * birth time is unknown
Doesn't gnulib guarantee that tv_nsec == -1 in isolation is sufficient to point out an unknown value? That is, checking tv_sec == -1 is overhead. Looking nicer. I'll have to ping upstream on gnulib about the last holdout on the relicensing of stat-time; and I'm also still waiting for the security fix in updated automake to hit Fedora. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/13/2012 08:38 AM, Hendrik Schwartke wrote:
!!! DON'T PUSH until stat-time lgpl 3 issue is fixed !!! To tests this change lgpl version to 3 in bootstrap.conf:176
The access, birth, modification and change times are added to storage volumes and corresponding xml representations. --- bootstrap.conf | 1 + docs/formatstorage.html.in | 13 +++++++++++++ docs/schemas/storagevol.rng | 36 ++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.c | 18 ++++++++++++++++++ src/conf/storage_conf.h | 13 +++++++++++++ src/storage/storage_backend.c | 6 ++++++ 6 files changed, 87 insertions(+)
diff --git a/bootstrap.conf b/bootstrap.conf index 9b42cbf..da0b960 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -115,6 +115,7 @@ vc-list-files vsnprintf waitpid warnings +stat-time ' Insert in sorted order.
@@ -172,6 +177,14 @@ 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. The four sub elements since btime is omitted on Linux, maybe this would read better as '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. This is a readonly attribute and is ignored when creating + a volume.<span class="since">Since 0.10.0</span>
+<define name='timestamps'> +<optional> +<element name='timestamps'> +<optional> +<element name='atime'> +<data type="string"> +<param name="pattern">[0-9]+\.[0-9]+</param> +</data> It might be worth writing the regex to permit eliding the sub-second resolution, on file systems that only have 1 second resolution. Given Well, the problem here is that stat-time doesn't offer a way to determine if sub-second resolution is available. If the system doesn't support it then tv_nsec is simply zero. So there is always a sub-second
On 13.07.2012 17:14, Eric Blake wrote: part in the timestamp and such an regex could be slightly misleading. I will change it anyway and add a comment to the schema.
that we are repeating this<data> four times, it might be worth defining it, for a shorter diff:
<element name='atime'> <ref name='timestamp'/> </element>
... <define name='timestamp'> <data type='string'> <param name='pattern'>[0-9]+(\.[0-9]+)?</param> </data> </define>
+++ b/src/conf/storage_conf.c @@ -1277,6 +1277,24 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
virBufferAddLit(buf,"</permissions>\n");
+ virBufferAddLit(buf, "<timestamps>\n"); + virBufferAsprintf(buf, "<atime>%llu.%ld</atime>\n", + (unsigned long long) def->timestamps.atime.tv_sec, + def->timestamps.atime.tv_nsec); Eliding a sub-second suffix when tv_nsec == 0 would be easier with a helper function:
void virStorageVolTimestampFormat(virBufferPtr buf, const char *name, struct timespec *ts) { if (ts->tv_nsec< 0) That's never the case. See above. return; virBufferAsprintf(buf, "<%s>%llu", name, (unsigned long long) ts->tv_sec); if (ts->tv_nsec) virBufferAsprintf(buf, ".%ld", tv->tv_nsec); virBufferAsprintf(buf, "</%s>\n", name); }
called as:
virStorageVolTimestampFormat(buf, "atime",&def->timestamps.atime); virStorageVolTimestampFormat(buf, "atime",&def->timestamps.btime);
and so on.
Actually, I'd list atime, mtime, ctime, btime - in that order - rather than trying to sort the names alphabetically (that is, match typical 'struct stat' ordering). Well I thought about that and I think it's better to sort it alphabetically, because everyone who doesn't know 'struct stat' could be very puzzled about atime, mtime, ctime, btime.
+typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + /* if btime.tv_sec == -1&& btime.tv_nsec == -1 than + * birth time is unknown Doesn't gnulib guarantee that tv_nsec == -1 in isolation is sufficient to point out an unknown value? That is, checking tv_sec == -1 is overhead. Well, actually yes, but the the description on get_stat_birthtime says: "Return *ST's birth time, if available; otherwise return a value with tv_sec and tv_nsec both equal to -1.". So to be sure I prefer to check both. Looking nicer. I'll have to ping upstream on gnulib about the last holdout on the relicensing of stat-time; and I'm also still waiting for the security fix in updated automake to hit Fedora.
Ok, please let me know if there are some changes here. Meanwhile I will adapt my patch.
On 16.07.2012 09:45, Hendrik Schwartke wrote:
On 13.07.2012 17:14, Eric Blake wrote:
On 07/13/2012 08:38 AM, Hendrik Schwartke wrote:
!!! DON'T PUSH until stat-time lgpl 3 issue is fixed !!! To tests this change lgpl version to 3 in bootstrap.conf:176
The access, birth, modification and change times are added to storage volumes and corresponding xml representations. --- bootstrap.conf | 1 + docs/formatstorage.html.in | 13 +++++++++++++ docs/schemas/storagevol.rng | 36 ++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.c | 18 ++++++++++++++++++ src/conf/storage_conf.h | 13 +++++++++++++ src/storage/storage_backend.c | 6 ++++++ 6 files changed, 87 insertions(+)
diff --git a/bootstrap.conf b/bootstrap.conf index 9b42cbf..da0b960 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -115,6 +115,7 @@ vc-list-files vsnprintf waitpid warnings +stat-time ' Insert in sorted order.
@@ -172,6 +177,14 @@ 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. The four sub elements since btime is omitted on Linux, maybe this would read better as '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. This is a readonly attribute and is ignored when creating + a volume.<span class="since">Since 0.10.0</span>
+<define name='timestamps'> +<optional> +<element name='timestamps'> +<optional> +<element name='atime'> +<data type="string"> +<param name="pattern">[0-9]+\.[0-9]+</param> +</data> It might be worth writing the regex to permit eliding the sub-second resolution, on file systems that only have 1 second resolution. Given Well, the problem here is that stat-time doesn't offer a way to determine if sub-second resolution is available. If the system doesn't support it then tv_nsec is simply zero. So there is always a sub-second part in the timestamp and such an regex could be slightly misleading. I will change it anyway and add a comment to the schema. that we are repeating this<data> four times, it might be worth defining it, for a shorter diff:
<element name='atime'> <ref name='timestamp'/> </element>
... <define name='timestamp'> <data type='string'> <param name='pattern'>[0-9]+(\.[0-9]+)?</param> </data> </define>
+++ b/src/conf/storage_conf.c @@ -1277,6 +1277,24 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
virBufferAddLit(buf,"</permissions>\n");
+ virBufferAddLit(buf, "<timestamps>\n"); + virBufferAsprintf(buf, "<atime>%llu.%ld</atime>\n", + (unsigned long long) def->timestamps.atime.tv_sec, + def->timestamps.atime.tv_nsec); Eliding a sub-second suffix when tv_nsec == 0 would be easier with a helper function:
void virStorageVolTimestampFormat(virBufferPtr buf, const char *name, struct timespec *ts) { if (ts->tv_nsec< 0) That's never the case. See above. Yups, wrong line. Of course that could be the case. But again I prefer to check tv_sec also. return; virBufferAsprintf(buf, "<%s>%llu", name, (unsigned long long) ts->tv_sec); if (ts->tv_nsec) That the line I wanted to comment. I'm not sure if it's such a good idea to omit the sub second part. Although it's very unlikely that this happends on systems that support tv_nsec it could be misleading. virBufferAsprintf(buf, ".%ld", tv->tv_nsec); virBufferAsprintf(buf, "</%s>\n", name); }
called as:
virStorageVolTimestampFormat(buf, "atime",&def->timestamps.atime); virStorageVolTimestampFormat(buf, "atime",&def->timestamps.btime);
and so on.
Actually, I'd list atime, mtime, ctime, btime - in that order - rather than trying to sort the names alphabetically (that is, match typical 'struct stat' ordering). Well I thought about that and I think it's better to sort it alphabetically, because everyone who doesn't know 'struct stat' could be very puzzled about atime, mtime, ctime, btime.
+typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + /* if btime.tv_sec == -1&& btime.tv_nsec == -1 than + * birth time is unknown Doesn't gnulib guarantee that tv_nsec == -1 in isolation is sufficient to point out an unknown value? That is, checking tv_sec == -1 is overhead. Well, actually yes, but the the description on get_stat_birthtime says: "Return *ST's birth time, if available; otherwise return a value with tv_sec and tv_nsec both equal to -1.". So to be sure I prefer to check both. Looking nicer. I'll have to ping upstream on gnulib about the last holdout on the relicensing of stat-time; and I'm also still waiting for the security fix in updated automake to hit Fedora.
Ok, please let me know if there are some changes here. Meanwhile I will adapt my patch.
I reconsidered the way timestamps are represented. I think that an event at 100.91 happened before 100.200 is misleading. So I changed that. Furthermore I would appreciate if the timestamps are available in 0.10.0, so I splitted the patch. The first patch doesn't use stat-time and the second patches the first and does use stat-time. I would be nice if at least the first one could be commited before the next version is tagged. Thanks Hendrik On 13.07.2012 17:14, Eric Blake wrote:
On 07/13/2012 08:38 AM, Hendrik Schwartke wrote:
!!! DON'T PUSH until stat-time lgpl 3 issue is fixed !!! To tests this change lgpl version to 3 in bootstrap.conf:176
The access, birth, modification and change times are added to storage volumes and corresponding xml representations. --- bootstrap.conf | 1 + docs/formatstorage.html.in | 13 +++++++++++++ docs/schemas/storagevol.rng | 36 ++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.c | 18 ++++++++++++++++++ src/conf/storage_conf.h | 13 +++++++++++++ src/storage/storage_backend.c | 6 ++++++ 6 files changed, 87 insertions(+)
diff --git a/bootstrap.conf b/bootstrap.conf index 9b42cbf..da0b960 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -115,6 +115,7 @@ vc-list-files vsnprintf waitpid warnings +stat-time ' Insert in sorted order.
@@ -172,6 +177,14 @@ 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. The four sub elements since btime is omitted on Linux, maybe this would read better as '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. This is a readonly attribute and is ignored when creating + a volume.<span class="since">Since 0.10.0</span>
+<define name='timestamps'> +<optional> +<element name='timestamps'> +<optional> +<element name='atime'> +<data type="string"> +<param name="pattern">[0-9]+\.[0-9]+</param> +</data> It might be worth writing the regex to permit eliding the sub-second resolution, on file systems that only have 1 second resolution. Given that we are repeating this<data> four times, it might be worth defining it, for a shorter diff:
<element name='atime'> <ref name='timestamp'/> </element>
... <define name='timestamp'> <data type='string'> <param name='pattern'>[0-9]+(\.[0-9]+)?</param> </data> </define>
+++ b/src/conf/storage_conf.c @@ -1277,6 +1277,24 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
virBufferAddLit(buf,"</permissions>\n");
+ virBufferAddLit(buf, "<timestamps>\n"); + virBufferAsprintf(buf, "<atime>%llu.%ld</atime>\n", + (unsigned long long) def->timestamps.atime.tv_sec, + def->timestamps.atime.tv_nsec); Eliding a sub-second suffix when tv_nsec == 0 would be easier with a helper function:
void virStorageVolTimestampFormat(virBufferPtr buf, const char *name, struct timespec *ts) { if (ts->tv_nsec< 0) return; virBufferAsprintf(buf, "<%s>%llu", name, (unsigned long long) ts->tv_sec); if (ts->tv_nsec) virBufferAsprintf(buf, ".%ld", tv->tv_nsec); virBufferAsprintf(buf, "</%s>\n", name); }
called as:
virStorageVolTimestampFormat(buf, "atime",&def->timestamps.atime); virStorageVolTimestampFormat(buf, "atime",&def->timestamps.btime);
and so on.
Actually, I'd list atime, mtime, ctime, btime - in that order - rather than trying to sort the names alphabetically (that is, match typical 'struct stat' ordering).
+typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + /* if btime.tv_sec == -1&& btime.tv_nsec == -1 than + * birth time is unknown Doesn't gnulib guarantee that tv_nsec == -1 in isolation is sufficient to point out an unknown value? That is, checking tv_sec == -1 is overhead.
Looking nicer. I'll have to ping upstream on gnulib about the last holdout on the relicensing of stat-time; and I'm also still waiting for the security fix in updated automake to hit Fedora.
The access, birth, modification and change times are added to storage volumes and corresponding xml representations. --- docs/formatstorage.html.in | 27 +++++++++++++++++++++++++++ docs/schemas/storagevol.rng | 39 +++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.c | 21 +++++++++++++++++++++ src/conf/storage_conf.h | 13 +++++++++++++ src/storage/storage_backend.c | 21 +++++++++++++++++++++ 5 files changed, 121 insertions(+) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d0e4319..d8ffbea 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -141,6 +141,20 @@ <mode>0744</mode> <label>virt_image_t</label> </permissions> + <timestamps> + <atime> + <sec>1341933637</sec> + <nsec>27319099</nsec> + </atime> + <ctime> + <sec>1341930622</sec> + <nsec>47245868</nsec> + </ctime> + <mtime> + <sec>1341930622</sec> + <nsec>47245868</nsec> + </mtime> + </timestamps> <encryption type='...'> ... </encryption> @@ -172,6 +186,19 @@ 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. Each timestamp is represented by an attribute + <code>sec</code> and and optional attribute <code>nsec</code>. The first + attribute holds the seconds since the beginning of the epoch and the second + attribute the additional nanoseconds. If nanosecond resolution isn't supported + by the host OS or filesystem then the <code>nsec</code> attribute is omitted. + For technical reasons the attribute is also omitted when zero. All timestamps + are readonly attributes and are ignored when creating a volume. + <span class="since">Since 0.10.0</span> + </dd> <dt><code>encryption</code></dt> <dd>If present, specifies how the volume is encrypted. See the <a href="formatstorageencryption.html">Storage Encryption</a> page diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 7a74331..af48bd2 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -63,6 +63,44 @@ </optional> </define> + <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> + </element> + </optional> + </define> + + <define name='timestamp'> + <element name='sec'> + <ref name='unsignedLong'/> + </element> + <optional> + <element name='nsec'> + <ref name='unsignedLong'/> + </element> + </optional> + </define> + <define name='target'> <element name='target'> <optional> @@ -72,6 +110,7 @@ </optional> <ref name='format'/> <ref name='permissions'/> + <ref name='timestamps'/> <optional> <ref name='encryption'/> </optional> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 36a3bb9..5651b76 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1241,6 +1241,20 @@ virStorageVolDefParseFile(virStoragePoolDefPtr pool, return virStorageVolDefParse(pool, NULL, filename); } +static void +virStorageVolTimestampFormat(virBufferPtr buf, const char *name, + struct timespec *ts) +{ + if (ts->tv_nsec < 0) + return; + virBufferAsprintf(buf, " <%s>\n", name); + virBufferAsprintf(buf, " <sec>%llu</sec>\n", + (unsigned long long) ts->tv_sec); + if (ts->tv_nsec) + virBufferAsprintf(buf, " <nsec>%ld</nsec>\n", ts->tv_nsec); + virBufferAsprintf(buf, " </%s>\n", name); +} + static int virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferPtr buf, @@ -1277,6 +1291,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); + virBufferAddLit(buf, " </timestamps>\n"); + if (def->encryption) { virBufferAdjustIndent(buf, 4); if (virStorageEncryptionFormat(buf, def->encryption) < 0) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 5733b57..b67ef64 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -46,6 +46,18 @@ struct _virStoragePerms { /* Storage volumes */ +typedef struct _virStorageTimestamps virStorageTimestamps; +typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + /* if btime.tv_nsec == -1 then + * birth time is unknown + */ + struct timespec btime; + struct timespec ctime; + struct timespec mtime; +}; + /* * How the volume's data is stored on underlying @@ -77,6 +89,7 @@ struct _virStorageVolTarget { char *path; int format; virStoragePerms perms; + virStorageTimestamps timestamps; int type; /* only used by disk backend for partition type */ /* Currently used only in virStorageVolDef.target, not in .backingstore. */ virStorageEncryptionPtr encryption; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 6ea0881..1ef6df9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1155,6 +1155,11 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, unsigned long long *capacity) { struct stat sb; + struct timespec *const atime = &target->timestamps.atime, + *const btime = &target->timestamps.btime, + *const catime = &target->timestamps.ctime, + *const mtime = &target->timestamps.mtime; + #if HAVE_SELINUX security_context_t filecon = NULL; #endif @@ -1208,6 +1213,22 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, target->perms.uid = sb.st_uid; target->perms.gid = sb.st_gid; + atime->tv_sec = sb.st_atime; + mtime->tv_sec = sb.st_mtime; + catime->tv_sec = sb.st_ctime; +#if _BSD_SOURCE || _SVID_SOURCE || \ + _POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700 + atime->tv_nsec = sb.st_atim.tv_nsec; + mtime->tv_nsec = sb.st_mtim.tv_nsec; + catime->tv_nsec = sb.st_ctim.tv_nsec; +#else + atime->tv_nsec = sb.st_atimensec; + mtime->tv_nsec = sb.st_mtimensec; + catime->tv_nsec = sb.st_ctimensec; +#endif + btime->tv_sec = -1; + btime->tv_nsec = -1; + VIR_FREE(target->perms.label); #if HAVE_SELINUX -- 1.7.9.5
!!! DON'T PUSH until stat-time lgpl 3 issue is fixed !!! To tests this change lgpl version to 3 in bootstrap.conf:176 stat-time offers a much cleaner way of getting timestamps than the current implementation. Therefor the implementation is changed. --- bootstrap.conf | 1 + src/storage/storage_backend.c | 25 +++++-------------------- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 9b42cbf..d80d92d 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -90,6 +90,7 @@ sigaction sigpipe snprintf socket +stat-time stdarg stpcpy strchrnul diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 1ef6df9..c665711 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -57,6 +57,7 @@ #include "storage_backend.h" #include "logging.h" #include "virfile.h" +#include "stat-time.h" #if WITH_STORAGE_LVM # include "storage_backend_logical.h" @@ -1155,11 +1156,6 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, unsigned long long *capacity) { struct stat sb; - struct timespec *const atime = &target->timestamps.atime, - *const btime = &target->timestamps.btime, - *const catime = &target->timestamps.ctime, - *const mtime = &target->timestamps.mtime; - #if HAVE_SELINUX security_context_t filecon = NULL; #endif @@ -1213,21 +1209,10 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, target->perms.uid = sb.st_uid; target->perms.gid = sb.st_gid; - atime->tv_sec = sb.st_atime; - mtime->tv_sec = sb.st_mtime; - catime->tv_sec = sb.st_ctime; -#if _BSD_SOURCE || _SVID_SOURCE || \ - _POSIX_C_SOURCE >= 200809L || _XOPEN_SOURCE >= 700 - atime->tv_nsec = sb.st_atim.tv_nsec; - mtime->tv_nsec = sb.st_mtim.tv_nsec; - catime->tv_nsec = sb.st_ctim.tv_nsec; -#else - atime->tv_nsec = sb.st_atimensec; - mtime->tv_nsec = sb.st_mtimensec; - catime->tv_nsec = sb.st_ctimensec; -#endif - btime->tv_sec = -1; - btime->tv_nsec = -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); -- 1.7.9.5
On 07/19/2012 01:13 AM, Hendrik Schwartke wrote:
The access, birth, modification and change times are added to storage volumes and corresponding xml representations. --- docs/formatstorage.html.in | 27 +++++++++++++++++++++++++++ docs/schemas/storagevol.rng | 39 +++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.c | 21 +++++++++++++++++++++ src/conf/storage_conf.h | 13 +++++++++++++ src/storage/storage_backend.c | 21 +++++++++++++++++++++ 5 files changed, 121 insertions(+)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d0e4319..d8ffbea 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -141,6 +141,20 @@ <mode>0744</mode> <label>virt_image_t</label> </permissions> + <timestamps> + <atime> + <sec>1341933637</sec> + <nsec>27319099</nsec>
Just because C can't represent struct timespec as a single number doesn't mean that our XML has to suffer; I still think %d.%09lld is a better format than splitting <sec> and <nsec> into separate elements. On the other hand, will XPath queries of the XML be easier if it is a floating point number compared to two different elements? Does anyone else have an opinion on which color to paint this bikeshed? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hi Eric, I changed the timestamp format again. I hope the following patch is ok now. Is stat-time ready to be added in bootstrap.conf or is there anything to do? regards Hendrik
The access, birth, modification and change times are added to storage volumes and corresponding xml representations. --- 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(+) diff --git a/bootstrap.conf b/bootstrap.conf index 9b42cbf..d80d92d 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -90,6 +90,7 @@ sigaction sigpipe snprintf socket +stat-time stdarg stpcpy strchrnul diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d0e4319..2a578e9 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -141,6 +141,11 @@ <mode>0744</mode> <label>virt_image_t</label> </permissions> + <timestamps> + <atime>1341933637.27319099</atime> + <ctime>1341930622.47245868</ctime> + <mtime>1341930622.47245868</mtime> + </timestamps> <encryption type='...'> ... </encryption> @@ -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> + </dd> <dt><code>encryption</code></dt> <dd>If present, specifies how the volume is encrypted. See the <a href="formatstorageencryption.html">Storage Encryption</a> page diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 7a74331..f981b47 100644 --- a/docs/schemas/storagevol.rng +++ 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> + <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> + </element> + </optional> + </define> + + <define name='timestamp'> + <data type='string'> + <param name="pattern">[0-9]+(\.[0-9]+)?</param> + </data> + </define> + <define name='target'> <element name='target'> <optional> @@ -72,6 +105,7 @@ </optional> <ref name='format'/> <ref name='permissions'/> + <ref name='timestamps'/> <optional> <ref name='encryption'/> </optional> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 36a3bb9..bc8e8be 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1241,6 +1241,19 @@ virStorageVolDefParseFile(virStoragePoolDefPtr pool, return virStorageVolDefParse(pool, NULL, filename); } +static void +virStorageVolTimestampFormat(virBufferPtr buf, const char *name, + struct timespec *ts) +{ + if (ts->tv_nsec < 0) + return; + virBufferAsprintf(buf, " <%s>%llu", name, + (unsigned long long) ts->tv_sec); + if (ts->tv_nsec) + virBufferAsprintf(buf, ".%09ld", ts->tv_nsec); + virBufferAsprintf(buf, "</%s>\n", name); +} + static int virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferPtr buf, @@ -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); + virBufferAddLit(buf, " </timestamps>\n"); + if (def->encryption) { virBufferAdjustIndent(buf, 4); if (virStorageEncryptionFormat(buf, def->encryption) < 0) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 5733b57..b67ef64 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -46,6 +46,18 @@ struct _virStoragePerms { /* Storage volumes */ +typedef struct _virStorageTimestamps virStorageTimestamps; +typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + /* if btime.tv_nsec == -1 then + * birth time is unknown + */ + struct timespec btime; + struct timespec ctime; + struct timespec mtime; +}; + /* * How the volume's data is stored on underlying @@ -77,6 +89,7 @@ struct _virStorageVolTarget { char *path; int format; virStoragePerms perms; + virStorageTimestamps timestamps; int type; /* only used by disk backend for partition type */ /* Currently used only in virStorageVolDef.target, not in .backingstore. */ virStorageEncryptionPtr encryption; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 6ea0881..c665711 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -57,6 +57,7 @@ #include "storage_backend.h" #include "logging.h" #include "virfile.h" +#include "stat-time.h" #if WITH_STORAGE_LVM # include "storage_backend_logical.h" @@ -1208,6 +1209,11 @@ 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); + VIR_FREE(target->perms.label); #if HAVE_SELINUX -- 1.7.9.5
Hi Eric, could you give me a short feedback on the status of my patch? Thanks Hendrik On 25.07.2012 09:43, Hendrik Schwartke wrote:
The access, birth, modification and change times are added to storage volumes and corresponding xml representations. --- 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(+)
diff --git a/bootstrap.conf b/bootstrap.conf index 9b42cbf..d80d92d 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -90,6 +90,7 @@ sigaction sigpipe snprintf socket +stat-time stdarg stpcpy strchrnul diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d0e4319..2a578e9 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -141,6 +141,11 @@ <mode>0744</mode> <label>virt_image_t</label> </permissions> +<timestamps> +<atime>1341933637.27319099</atime> +<ctime>1341930622.47245868</ctime> +<mtime>1341930622.47245868</mtime> +</timestamps> <encryption type='...'> ... </encryption> @@ -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> +</dd> <dt><code>encryption</code></dt> <dd>If present, specifies how the volume is encrypted. See the<a href="formatstorageencryption.html">Storage Encryption</a> page diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 7a74331..f981b47 100644 --- a/docs/schemas/storagevol.rng +++ 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> +<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> +</element> +</optional> +</define> + +<define name='timestamp'> +<data type='string'> +<param name="pattern">[0-9]+(\.[0-9]+)?</param> +</data> +</define> + <define name='target'> <element name='target'> <optional> @@ -72,6 +105,7 @@ </optional> <ref name='format'/> <ref name='permissions'/> +<ref name='timestamps'/> <optional> <ref name='encryption'/> </optional> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 36a3bb9..bc8e8be 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1241,6 +1241,19 @@ virStorageVolDefParseFile(virStoragePoolDefPtr pool, return virStorageVolDefParse(pool, NULL, filename); }
+static void +virStorageVolTimestampFormat(virBufferPtr buf, const char *name, + struct timespec *ts) +{ + if (ts->tv_nsec< 0) + return; + virBufferAsprintf(buf, "<%s>%llu", name, + (unsigned long long) ts->tv_sec); + if (ts->tv_nsec) + virBufferAsprintf(buf, ".%09ld", ts->tv_nsec); + virBufferAsprintf(buf, "</%s>\n", name); +} + static int virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferPtr buf, @@ -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); + virBufferAddLit(buf, "</timestamps>\n"); + if (def->encryption) { virBufferAdjustIndent(buf, 4); if (virStorageEncryptionFormat(buf, def->encryption)< 0) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 5733b57..b67ef64 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -46,6 +46,18 @@ struct _virStoragePerms {
/* Storage volumes */
+typedef struct _virStorageTimestamps virStorageTimestamps; +typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + /* if btime.tv_nsec == -1 then + * birth time is unknown + */ + struct timespec btime; + struct timespec ctime; + struct timespec mtime; +}; +
/* * How the volume's data is stored on underlying @@ -77,6 +89,7 @@ struct _virStorageVolTarget { char *path; int format; virStoragePerms perms; + virStorageTimestamps timestamps; int type; /* only used by disk backend for partition type */ /* Currently used only in virStorageVolDef.target, not in .backingstore. */ virStorageEncryptionPtr encryption; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 6ea0881..c665711 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -57,6 +57,7 @@ #include "storage_backend.h" #include "logging.h" #include "virfile.h" +#include "stat-time.h"
#if WITH_STORAGE_LVM # include "storage_backend_logical.h" @@ -1208,6 +1209,11 @@ 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); + VIR_FREE(target->perms.label);
#if HAVE_SELINUX
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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>
On 07/19/2012 01:13 AM, Hendrik Schwartke wrote:
I reconsidered the way timestamps are represented. I think that an event at 100.91 happened before 100.200 is misleading.
Yep, definite bug - you have to zero-pad the subsecond resolution, and also consider whether to strip trailing zeros.
So I changed that. Furthermore I would appreciate if the timestamps are available in 0.10.0, so I splitted the patch. The first patch doesn't use stat-time and the second patches the first and does use stat-time.
Not necessary to do two implementations. stat-time has now been relicensed in gnulib, so all we are waiting on now is for updated automake to hit Fedora 17 (it has already hit rawhide): https://bugzilla.redhat.com/show_bug.cgi?id=838660 Given the security bug in current automake, I will actively protest releasing the next version of libvirt until fixed automake is available. Don't worry - I plan to include this feature in the next libvirt build; just a few more days of waiting. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 19.07.2012 16:08, Eric Blake wrote:
On 07/19/2012 01:13 AM, Hendrik Schwartke wrote:
I reconsidered the way timestamps are represented. I think that an event at 100.91 happened before 100.200 is misleading. Yep, definite bug - you have to zero-pad the subsecond resolution, and also consider whether to strip trailing zeros.
So I changed that. Furthermore I would appreciate if the timestamps are available in 0.10.0, so I splitted the patch. The first patch doesn't use stat-time and the second patches the first and does use stat-time. Not necessary to do two implementations. stat-time has now been relicensed in gnulib, so all we are waiting on now is for updated automake to hit Fedora 17 (it has already hit rawhide): https://bugzilla.redhat.com/show_bug.cgi?id=838660 Wow, that was fast. Many thanks for your help.
Given the security bug in current automake, I will actively protest releasing the next version of libvirt until fixed automake is available.
Don't worry - I plan to include this feature in the next libvirt build; just a few more days of waiting.
participants (2)
-
Eric Blake -
Hendrik Schwartke