Re: [Qemu-devel] [RFC] qcow2: Add snapshot inherantance and uuid identification

[Blue Swirl]

On Tue, Aug 3, 2010 at 5:19 PM, Miguel Di Ciurcio Filho
<address@hidden> wrote:
> As discussed on today's call, here is a prototype to support snapshots
> inherantance in qcow2 and to use uuid as identification mechanism.
>
> Bugs/Limitations:
> * 'info snapshots' output is huge
> Displaying one item per line seams cumbersome. Maybe we should have two 
> commands,
> like lvscan and lvdisplay. One very simple, the other one show full 
> information.
>
> * Better distinguish ids and names
> I would like to suggest split functions like bdrv_snapshot_find() to something
> more strict, like bdrv_snapshot_find_by_id() and bdrv_snapshot_find_by_name()
>
> Signed-off-by: Miguel Di Ciurcio Filho <address@hidden>
> ---
>  block.c                |    8 +++---
>  block.h                |    1 +
>  block/qcow2-snapshot.c |   58 
> +++++++++++++++++++++++++++++++++++++++++++++++-
>  block/qcow2.h          |    1 +
>  savevm.c               |   17 ++++++++++++++
>  5 files changed, 80 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index 452ae94..f305744 100644
> --- a/block.c
> +++ b/block.c
> @@ -1941,8 +1941,8 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, 
> QEMUSnapshotInfo *sn)
>
>     if (!sn) {
>         snprintf(buf, buf_size,
> -                 "%-10s%-20s%7s%20s%15s",
> -                 "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
> +                 "%-37s%-37s%-20s%7s%20s%15s",
> +                 "ID", "PARENT ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
>     } else {
>         ti = sn->date_sec;
>  #ifdef _WIN32
> @@ -1962,8 +1962,8 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, 
> QEMUSnapshotInfo *sn)
>                  (int)(secs % 60),
>                  (int)((sn->vm_clock_nsec / 1000000) % 1000));
>         snprintf(buf, buf_size,
> -                 "%-10s%-20s%7s%20s%15s",
> -                 sn->id_str, sn->name,
> +                 "%-37s%-37s%-20s%7s%20s%15s",
> +                 sn->id_str, sn->parent_id_str, sn->name,
>                  get_human_readable_size(buf1, sizeof(buf1), 
> sn->vm_state_size),
>                  date_buf,
>                  clock_buf);
> diff --git a/block.h b/block.h
> index db131a3..e3c040a 100644
> --- a/block.h
> +++ b/block.h
> @@ -25,6 +25,7 @@ typedef struct QEMUSnapshotInfo {
>     uint32_t date_sec; /* UTC date of the snapshot */
>     uint32_t date_nsec;
>     uint64_t vm_clock_nsec; /* VM clock relative to boot */
> +    char parent_id_str[128];
>  } QEMUSnapshotInfo;
>
>  #define BDRV_O_RDWR        0x0002
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 6228612..97877dd 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -54,6 +54,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
>     for(i = 0; i < s->nb_snapshots; i++) {
>         qemu_free(s->snapshots[i].name);
>         qemu_free(s->snapshots[i].id_str);
> +        qemu_free(s->snapshots[i].parent_id_str);
>     }
>     qemu_free(s->snapshots);
>     s->snapshots = NULL;
> @@ -69,6 +70,8 @@ int qcow2_read_snapshots(BlockDriverState *bs)
>     int64_t offset;
>     uint32_t extra_data_size;
>
> +    extra_data_size = 0;
> +
>     if (!s->nb_snapshots) {
>         s->snapshots = NULL;
>         s->snapshots_size = 0;
> @@ -94,7 +97,15 @@ int qcow2_read_snapshots(BlockDriverState *bs)
>         id_str_size = be16_to_cpu(h.id_str_size);
>         name_size = be16_to_cpu(h.name_size);
>
> -        offset += extra_data_size;
> +        if (extra_data_size > 0) {
> +            sn->parent_id_str = qemu_malloc(extra_data_size);
> +            if (bdrv_pread(bs->file, offset, sn->parent_id_str, 
> extra_data_size) != extra_data_size) {
> +                goto fail;
> +            }
> +
> +            offset += extra_data_size;
> +            sn->parent_id_str[extra_data_size] = '\0';
> +        }
>
>         sn->id_str = qemu_malloc(id_str_size + 1);
>         if (bdrv_pread(bs->file, offset, sn->id_str, id_str_size) != 
> id_str_size)
> @@ -125,6 +136,9 @@ static int qcow_write_snapshots(BlockDriverState *bs)
>     uint64_t data64;
>     uint32_t data32;
>     int64_t offset, snapshots_offset;
> +    uint32_t extra_data_size;
> +
> +    extra_data_size = 0;
>
>     /* compute the size of the snapshots */
>     offset = 0;
> @@ -132,6 +146,9 @@ static int qcow_write_snapshots(BlockDriverState *bs)
>         sn = s->snapshots + i;
>         offset = align_offset(offset, 8);
>         offset += sizeof(h);
> +        if (sn->parent_id_str) {
> +            offset += strlen(sn->parent_id_str);
> +        }
>         offset += strlen(sn->id_str);
>         offset += strlen(sn->name);
>     }
> @@ -153,6 +170,11 @@ static int qcow_write_snapshots(BlockDriverState *bs)
>         h.date_nsec = cpu_to_be32(sn->date_nsec);
>         h.vm_clock_nsec = cpu_to_be64(sn->vm_clock_nsec);
>
> +        if (sn->parent_id_str) {
> +            extra_data_size = strlen(sn->parent_id_str);
> +            h.extra_data_size = cpu_to_be32(extra_data_size);
> +        }
> +
>         id_str_size = strlen(sn->id_str);
>         name_size = strlen(sn->name);
>         h.id_str_size = cpu_to_be16(id_str_size);
> @@ -161,6 +183,15 @@ static int qcow_write_snapshots(BlockDriverState *bs)
>         if (bdrv_pwrite_sync(bs->file, offset, &h, sizeof(h)) < 0)
>             goto fail;
>         offset += sizeof(h);
> +
> +        if (sn->parent_id_str) {
> +            if (bdrv_pwrite_sync(bs->file, offset, sn->parent_id_str, 
> extra_data_size) < 0) {
> +                        goto fail;
> +            }
> +
> +            offset += extra_data_size;
> +        }
> +
>         if (bdrv_pwrite_sync(bs->file, offset, sn->id_str, id_str_size) < 0)
>             goto fail;
>         offset += id_str_size;
> @@ -257,6 +288,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info)
>     sn->name = qemu_strdup(sn_info->name);
>     if (!sn->name)
>         goto fail;
> +
> +    sn->parent_id_str = qemu_strdup(sn_info->parent_id_str);
> +
>     sn->vm_state_size = sn_info->vm_state_size;
>     sn->date_sec = sn_info->date_sec;
>     sn->date_nsec = sn_info->date_nsec;
> @@ -352,6 +386,19 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
> *snapshot_id)
>     return -EIO;
>  }
>
> +static void qcow2_update_parent_id_references(BlockDriverState *bs, 
> QCowSnapshot *sn)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int i;
> +
> +    for(i = 0; i < s->nb_snapshots; i++) {
> +        if (!strcmp(s->snapshots[i].parent_id_str, sn->id_str)) {
> +            pstrcpy(s->snapshots[i].parent_id_str, strlen(sn->parent_id_str),
> +            sn->parent_id_str);
> +        }
> +    }
> +}
> +
>  int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>  {
>     BDRVQcowState *s = bs->opaque;
> @@ -372,7 +419,10 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const 
> char *snapshot_id)
>         return ret;
>     qcow2_free_clusters(bs, sn->l1_table_offset, sn->l1_size * 
> sizeof(uint64_t));
>
> +    qcow2_update_parent_id_references(bs, sn);
> +
>     qemu_free(sn->id_str);
> +    qemu_free(sn->parent_id_str);
>     qemu_free(sn->name);
>     memmove(sn, sn + 1, (s->nb_snapshots - snapshot_index - 1) * sizeof(*sn));
>     s->nb_snapshots--;
> @@ -407,6 +457,12 @@ int qcow2_snapshot_list(BlockDriverState *bs, 
> QEMUSnapshotInfo **psn_tab)
>                 sn->id_str);
>         pstrcpy(sn_info->name, sizeof(sn_info->name),
>                 sn->name);
> +
> +        if (sn->parent_id_str) {
> +            pstrcpy(sn_info->parent_id_str, sizeof(sn_info->parent_id_str),
> +                    sn->parent_id_str);
> +        }
> +
>         sn_info->vm_state_size = sn->vm_state_size;
>         sn_info->date_sec = sn->date_sec;
>         sn_info->date_nsec = sn->date_nsec;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 3ff162e..d40e6bf 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -76,6 +76,7 @@ typedef struct QCowSnapshot {
>     uint32_t date_sec;
>     uint32_t date_nsec;
>     uint64_t vm_clock_nsec;
> +    char *parent_id_str;
>  } QCowSnapshot;
>
>  typedef struct BDRVQcowState {
> diff --git a/savevm.c b/savevm.c
> index 4c0e5d3..6ed839b 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -28,6 +28,7 @@
>  #include <errno.h>
>  #include <sys/time.h>
>  #include <zlib.h>
> +#include <uuid/uuid.h>

This header is not available on all systems.

>
>  /* Needed early for CONFIG_BSD etc. */
>  #include "config-host.h"
> @@ -86,6 +87,8 @@
>
>  #define SELF_ANNOUNCE_ROUNDS 5
>
> +static char current_snapshot_id[128];
> +
>  #ifndef ETH_P_RARP
>  #define ETH_P_RARP 0x8035
>  #endif
> @@ -1797,6 +1800,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>     QEMUFile *f;
>     int saved_vm_running;
>     uint32_t vm_state_size;
> +    uuid_t uuid_buf;
>  #ifdef _WIN32
>     struct _timeb tb;
>  #else
> @@ -1841,6 +1845,16 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>         }
>     }
>
> +    if (strlen(current_snapshot_id) > 0) {
> +        pstrcpy(sn->parent_id_str, sizeof(sn->parent_id_str), 
> current_snapshot_id);
> +    } else {
> +        pstrcpy(sn->parent_id_str, sizeof(sn->parent_id_str), 
> "00000000-0000-0000-0000-000000000000");
> +    }
> +
> +    uuid_generate(uuid_buf);
> +    uuid_unparse(uuid_buf, sn->id_str);

We have qemu_uuid which is defined with command line switch, can you use that?

> +    pstrcpy(current_snapshot_id, sizeof(current_snapshot_id), sn->id_str);
> +
>     /* fill auxiliary fields */
>  #ifdef _WIN32
>     _ftime(&tb);
> @@ -1969,6 +1983,9 @@ int load_vmstate(const char *name)
>         error_report("Error %d while loading VM state", ret);
>         return ret;
>     }
> +
> +    pstrcpy(current_snapshot_id, sizeof(current_snapshot_id), sn.id_str);
> +
>     return 0;
>  }
>
> --
> 1.7.1
>
>
>