diff mbox series

[02/13] qcow2: introduce icount field for snapshots

Message ID 159316679154.10508.16814264064541947914.stgit@pasha-ThinkPad-X280 (mailing list archive)
State New, archived
Headers show
Series Reverse debugging | expand

Commit Message

Pavel Dovgalyuk June 26, 2020, 10:19 a.m. UTC
From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch introduces the icount field for saving within the snapshot.
It is required for navigation between the snapshots in record/replay mode.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Acked-by: Kevin Wolf <kwolf@redhat.com>
---
 0 files changed

Comments

Eric Blake July 6, 2020, 8:17 p.m. UTC | #1
On 6/26/20 5:19 AM, Pavel Dovgalyuk wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> 
> This patch introduces the icount field for saving within the snapshot.
> It is required for navigation between the snapshots in record/replay mode.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> Acked-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   0 files changed
> 

That's an odd diffstat; you may want to investigate why git isn't 
showing the usual diffstat that makes it easier to see which files are 
touched and the relative size of the changes.


> +++ b/docs/interop/qcow2.txt
> @@ -645,6 +645,11 @@ Snapshot table entry:
>   
>                       Byte 48 - 55:   Virtual disk size of the snapshot in bytes
>   
> +                    Byte 56 - 63:   icount value which corresponds to
> +                                    the record/replay instruction count
> +                                    when the snapshot was taken. Set to -1
> +                                    if icount was disabled
> +
>                       Version 3 images must include extra data at least up to
>                       byte 55.

Should we have additional text here, similar to what was added to the 
overall header in 3ae3fcfa, about how to properly add additional 
optional fields while maintaining back-compat considerations?  Maybe 
just a one sentence reference that the rules in that section apply here too?
Pavel Dovgalyuk July 16, 2020, 6:43 a.m. UTC | #2
On 06.07.2020 23:17, Eric Blake wrote:
> On 6/26/20 5:19 AM, Pavel Dovgalyuk wrote:
>> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>>
>> This patch introduces the icount field for saving within the snapshot.
>> It is required for navigation between the snapshots in record/replay 
>> mode.
>>
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
>> Acked-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   0 files changed
>>
> 
> That's an odd diffstat; you may want to investigate why git isn't 
> showing the usual diffstat that makes it easier to see which files are 
> touched and the relative size of the changes.

Thanks for noticing, that was a stgit bug.

>> +++ b/docs/interop/qcow2.txt
>> @@ -645,6 +645,11 @@ Snapshot table entry:
>>                       Byte 48 - 55:   Virtual disk size of the 
>> snapshot in bytes
>> +                    Byte 56 - 63:   icount value which corresponds to
>> +                                    the record/replay instruction count
>> +                                    when the snapshot was taken. Set 
>> to -1
>> +                                    if icount was disabled
>> +
>>                       Version 3 images must include extra data at 
>> least up to
>>                       byte 55.
> 
> Should we have additional text here, similar to what was added to the 
> overall header in 3ae3fcfa, about how to properly add additional 
> optional fields while maintaining back-compat considerations?  Maybe 
> just a one sentence reference that the rules in that section apply here 
> too?

Your proposal is adding more details about header extension in that patch?
But I didn't get what exactly is needed, because there is already
the following text in the beginning of "variable" part:

variable:   Extra data for future extensions. Unknown fields must be
             ignored. Currently defined are (offset relative to snapshot
             table entry):


Pavel Dovgalyuk
diff mbox series

Patch

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 2756b37d24..d14e7be1aa 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -164,6 +164,12 @@  static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
             sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
         }
 
+        if (sn->extra_data_size >= endof(QCowSnapshotExtraData, icount)) {
+            sn->icount = be64_to_cpu(extra.icount);
+        } else {
+            sn->icount = -1ULL;
+        }
+
         if (sn->extra_data_size > sizeof(extra)) {
             uint64_t extra_data_end;
             size_t unknown_extra_data_size;
@@ -333,6 +339,7 @@  int qcow2_write_snapshots(BlockDriverState *bs)
         memset(&extra, 0, sizeof(extra));
         extra.vm_state_size_large = cpu_to_be64(sn->vm_state_size);
         extra.disk_size = cpu_to_be64(sn->disk_size);
+        extra.icount = cpu_to_be64(sn->icount);
 
         id_str_size = strlen(sn->id_str);
         name_size = strlen(sn->name);
diff --git a/block/qcow2.h b/block/qcow2.h
index 7ce2c23bdb..9c81086991 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -179,6 +179,7 @@  typedef struct QEMU_PACKED QCowSnapshotHeader {
 typedef struct QEMU_PACKED QCowSnapshotExtraData {
     uint64_t vm_state_size_large;
     uint64_t disk_size;
+    uint64_t icount;
 } QCowSnapshotExtraData;
 
 
@@ -192,6 +193,8 @@  typedef struct QCowSnapshot {
     uint32_t date_sec;
     uint32_t date_nsec;
     uint64_t vm_clock_nsec;
+    /* icount value for the moment when snapshot was taken */
+    uint64_t icount;
     /* Size of all extra data, including QCowSnapshotExtraData if available */
     uint32_t extra_data_size;
     /* Data beyond QCowSnapshotExtraData, if any */
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index cb723463f2..06014c0e90 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -645,6 +645,11 @@  Snapshot table entry:
 
                     Byte 48 - 55:   Virtual disk size of the snapshot in bytes
 
+                    Byte 56 - 63:   icount value which corresponds to
+                                    the record/replay instruction count
+                                    when the snapshot was taken. Set to -1
+                                    if icount was disabled
+
                     Version 3 images must include extra data at least up to
                     byte 55.