Re: [libvirt] [PATCH v4 11/20] wip: backup: Parse and output checkpoint XML

[John Ferlan]

On 2/25/19 4:06 PM, Eric Blake wrote:
> On 2/12/19 11:23 AM, John Ferlan wrote:
>>
>>
>> On 2/6/19 2:18 PM, Eric Blake wrote:
>>> Work in progress - the checkpoint code is not quite passing
>>> tests (part of that is figuring out the minimal XML that is
>>> still valid as a  element, or just use --no-domain flag).
>>>
>>> Signed-off-by: Eric Blake 
>>> ---
>>>  src/conf/checkpoint_conf.h  |  150 
>>>  src/conf/domain_conf.h  |   11 +-
>>>  po/POTFILES |1 +
>>>  src/conf/Makefile.inc.am|2 +
>>>  src/conf/checkpoint_conf.c  | 1030 +++
>>>  src/conf/domain_conf.c  |7 +-
>>>  src/libvirt_private.syms|   21 +
>>>  tests/Makefile.am   |9 +-
>>>  tests/domaincheckpointxml2xmltest.c |  231 ++
>>>  9 files changed, 1458 insertions(+), 4 deletions(-)
>>>  create mode 100644 src/conf/checkpoint_conf.h
>>>  create mode 100644 src/conf/checkpoint_conf.c
>>>  create mode 100644 tests/domaincheckpointxml2xmltest.c
>>>
>>
>> Starting to lose some steam - seeing wip means I don't want to spend too
>> much time on some algorithms for fear they'll change, but then again I
>> know you're looking for feedback...
> 
> I understand. The 'wip' tags should be gone for v5.
> 
>>
>>> diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
>>> new file mode 100644
>>> index 00..994a8bd083
>>> --- /dev/null
>>> +++ b/src/conf/checkpoint_conf.h
>>> @@ -0,0 +1,150 @@
>>> +/*
>>> + * checkpoint_conf.h: domain checkpoint XML processing
>>> + *
>>> + * Copyright (C) 2006-2019 Red Hat, Inc.
>>> + * Copyright (C) 2006-2008 Daniel P. Berrange
>>
>> This is new, right?  Not a copy of...
> 
> New file, but so heavily copied from snapshot_conf.h that I felt safer
> preserving all existing copyrights.
> 

See src/conf/{vir*obj}.c for some examples, e.g.:

 * virinterfaceobj.c: interface object handling
 *(derived from interface_conf.c)

that's what I ended up doing when splitting apart XML handling from
Object handling code.

> 
>>> +struct _virDomainCheckpointDef {
>>> +/* Public XML.  */
>>> +char *name;
>>> +char *description;
>>> +char *parent;
>>> +long long creationTime; /* in seconds */
>>> +
>>> +size_t ndisks; /* should not exceed dom->ndisks */
>>> +virDomainCheckpointDiskDef *disks;
>>> +
>>> +virDomainDefPtr dom;
>>> +
>>> +/* Internal use.  */
>>> +bool current; /* At most one checkpoint in the list should have this 
>>> set */
>>
>> Typically these are then in the *Obj...
> 
> Copy-and-paste from virDomainSnapshotDef, but my recent work there has
> made me want to reconsider where the current object is tracked.
> 

Yeah, understandable. Like I noted earlier - when I did the work to
split out object code and make it more common, I skipped snapshots for
various reasons, but mostly because I feared that code and wasn't sure
if changing it would cause issues with any blockdev work Peter was
doing. So I want the safer route.

I do think snapshot_conf.c could be split up with virsnapshotobjlist.c
being created, but that's future work.  Maybe something you can consider
for checkpoints though.

> The problem is that the current way we store things on disk is via a
> separate  XML per snapshot, with no other obvious place
> for the internal representation of a  to track which snapshot is
> current.  BUT, as I have just posted patches to add a bulk dump/redefine
> with VIR_DOMAIN_XML_SNAPSHOTS, we COULD just ditch our current
> separate-file storage of snapshots, and instead store the snapshots
> directly with the  (for all new saves; when loading libvirtd,
> we'd still have to keep the code to load from older separate snapshot
> files for back-compat reasons, even if we no longer track separate files
> after the one-time conversion).  And if I make snapshots cleaner like
> that, then checkpoints can just start life with the saner approach,
> rather than being stuck with copying the older one-file-per-checkpoint
> approach that I implemented using copy-and-paste.
> 

Hey, I recognize that explanation!

>>
>>> +};
>>> +
>>> +struct _virDomainCheckpointObj {
>>> +virDomainCheckpointDefPtr def; /* non-NULL except for metaroot */
>>> +
>>> +virDomainCheckpointObjPtr parent; /* non-NULL except for metaroot, 
>>> before
>>> + 
>>> virDomainCheckpointUpdateRelations, or
>>> + after 
>>> virDomainCheckpointDropParent */
>>> +virDomainCheckpointObjPtr sibling; /* NULL if last child of parent */
>>> +size_t nchildren;
>>> +virDomainCheckpointObjPtr first_child; /* NULL if no children */
>>
>> The whole relationship thing is I think overly complex and a bit
>> difficult to follow. Having @sibling and @first_child seems to make the
>> code even more complex. I would think you have a parent and maybe a
>> child. If this is 

Re: [libvirt] [PATCH v4 11/20] wip: backup: Parse and output checkpoint XML

[Eric Blake]

On 2/12/19 11:23 AM, John Ferlan wrote:
> 
> 
> On 2/6/19 2:18 PM, Eric Blake wrote:
>> Work in progress - the checkpoint code is not quite passing
>> tests (part of that is figuring out the minimal XML that is
>> still valid as a  element, or just use --no-domain flag).
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  src/conf/checkpoint_conf.h  |  150 
>>  src/conf/domain_conf.h  |   11 +-
>>  po/POTFILES |1 +
>>  src/conf/Makefile.inc.am|2 +
>>  src/conf/checkpoint_conf.c  | 1030 +++
>>  src/conf/domain_conf.c  |7 +-
>>  src/libvirt_private.syms|   21 +
>>  tests/Makefile.am   |9 +-
>>  tests/domaincheckpointxml2xmltest.c |  231 ++
>>  9 files changed, 1458 insertions(+), 4 deletions(-)
>>  create mode 100644 src/conf/checkpoint_conf.h
>>  create mode 100644 src/conf/checkpoint_conf.c
>>  create mode 100644 tests/domaincheckpointxml2xmltest.c
>>
> 
> Starting to lose some steam - seeing wip means I don't want to spend too
> much time on some algorithms for fear they'll change, but then again I
> know you're looking for feedback...

I understand. The 'wip' tags should be gone for v5.

> 
>> diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
>> new file mode 100644
>> index 00..994a8bd083
>> --- /dev/null
>> +++ b/src/conf/checkpoint_conf.h
>> @@ -0,0 +1,150 @@
>> +/*
>> + * checkpoint_conf.h: domain checkpoint XML processing
>> + *
>> + * Copyright (C) 2006-2019 Red Hat, Inc.
>> + * Copyright (C) 2006-2008 Daniel P. Berrange
> 
> This is new, right?  Not a copy of...

New file, but so heavily copied from snapshot_conf.h that I felt safer
preserving all existing copyrights.


>> +struct _virDomainCheckpointDef {
>> +/* Public XML.  */
>> +char *name;
>> +char *description;
>> +char *parent;
>> +long long creationTime; /* in seconds */
>> +
>> +size_t ndisks; /* should not exceed dom->ndisks */
>> +virDomainCheckpointDiskDef *disks;
>> +
>> +virDomainDefPtr dom;
>> +
>> +/* Internal use.  */
>> +bool current; /* At most one checkpoint in the list should have this 
>> set */
> 
> Typically these are then in the *Obj...

Copy-and-paste from virDomainSnapshotDef, but my recent work there has
made me want to reconsider where the current object is tracked.

The problem is that the current way we store things on disk is via a
separate  XML per snapshot, with no other obvious place
for the internal representation of a  to track which snapshot is
current.  BUT, as I have just posted patches to add a bulk dump/redefine
with VIR_DOMAIN_XML_SNAPSHOTS, we COULD just ditch our current
separate-file storage of snapshots, and instead store the snapshots
directly with the  (for all new saves; when loading libvirtd,
we'd still have to keep the code to load from older separate snapshot
files for back-compat reasons, even if we no longer track separate files
after the one-time conversion).  And if I make snapshots cleaner like
that, then checkpoints can just start life with the saner approach,
rather than being stuck with copying the older one-file-per-checkpoint
approach that I implemented using copy-and-paste.

> 
>> +};
>> +
>> +struct _virDomainCheckpointObj {
>> +virDomainCheckpointDefPtr def; /* non-NULL except for metaroot */
>> +
>> +virDomainCheckpointObjPtr parent; /* non-NULL except for metaroot, 
>> before
>> + 
>> virDomainCheckpointUpdateRelations, or
>> + after 
>> virDomainCheckpointDropParent */
>> +virDomainCheckpointObjPtr sibling; /* NULL if last child of parent */
>> +size_t nchildren;
>> +virDomainCheckpointObjPtr first_child; /* NULL if no children */
> 
> The whole relationship thing is I think overly complex and a bit
> difficult to follow. Having @sibling and @first_child seems to make the
> code even more complex. I would think you have a parent and maybe a
> child. If this is the "first" checkpoint, then there is no parent. I
> would think that means it's the root.
> 
> There's just much more to be see when it comes to how these
> relationships play out as checkpoints are made and removed. I guess I
> just have a very linear model in mind, but reading the code seems to go
> beyond linearality.
> 
> The use of hash tables just makes it easier to ensure no def->name is
> reused. Not having locks in the object means some parent lock is
> managing to make sure two checkpoint operations cannot occur at the same
> time from different threads (e.g. a domain object lock), but that is not
> 100% clear.

Again, heavy copy-and-paste from snapshots, which DO lend themselves to
non-linear DAGs of related snapshots. (If you create snapshot A, then
run the guest, then create B, then revert to A, then run the guest and
create C, then both B and C are children of A).  I don't readily know if
checkpoints 

Re: [libvirt] [PATCH v4 11/20] wip: backup: Parse and output checkpoint XML

[John Ferlan]

On 2/6/19 2:18 PM, Eric Blake wrote:
> Work in progress - the checkpoint code is not quite passing
> tests (part of that is figuring out the minimal XML that is
> still valid as a  element, or just use --no-domain flag).
> 
> Signed-off-by: Eric Blake 
> ---
>  src/conf/checkpoint_conf.h  |  150 
>  src/conf/domain_conf.h  |   11 +-
>  po/POTFILES |1 +
>  src/conf/Makefile.inc.am|2 +
>  src/conf/checkpoint_conf.c  | 1030 +++
>  src/conf/domain_conf.c  |7 +-
>  src/libvirt_private.syms|   21 +
>  tests/Makefile.am   |9 +-
>  tests/domaincheckpointxml2xmltest.c |  231 ++
>  9 files changed, 1458 insertions(+), 4 deletions(-)
>  create mode 100644 src/conf/checkpoint_conf.h
>  create mode 100644 src/conf/checkpoint_conf.c
>  create mode 100644 tests/domaincheckpointxml2xmltest.c
> 

Starting to lose some steam - seeing wip means I don't want to spend too
much time on some algorithms for fear they'll change, but then again I
know you're looking for feedback...

> diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
> new file mode 100644
> index 00..994a8bd083
> --- /dev/null
> +++ b/src/conf/checkpoint_conf.h
> @@ -0,0 +1,150 @@
> +/*
> + * checkpoint_conf.h: domain checkpoint XML processing
> + *
> + * Copyright (C) 2006-2019 Red Hat, Inc.
> + * Copyright (C) 2006-2008 Daniel P. Berrange

This is new, right?  Not a copy of...

> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + */
> +
> +#ifndef LIBVIRT_CHECKPOINT_CONF_H
> +# define LIBVIRT_CHECKPOINT_CONF_H
> +
> +# include "internal.h"
> +# include "domain_conf.h"
> +
> +/* Items related to checkpoint state */
> +
> +typedef enum {
> +VIR_DOMAIN_CHECKPOINT_TYPE_DEFAULT = 0,
> +VIR_DOMAIN_CHECKPOINT_TYPE_NONE,
> +VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP,
> +
> +VIR_DOMAIN_CHECKPOINT_TYPE_LAST
> +} virDomainCheckpointType;
> +
> +/* Stores disk-checkpoint information */
> +typedef struct _virDomainCheckpointDiskDef virDomainCheckpointDiskDef;
> +typedef virDomainCheckpointDiskDef *virDomainCheckpointDiskDefPtr;
> +struct _virDomainCheckpointDiskDef {
> +char *name; /* name matching the  +int idx;/* index within checkpoint->dom->disks that matches name 
> */
> +int type;   /* virDomainCheckpointType */
> +char *bitmap;   /* bitmap name, if type is bitmap */
> +unsigned long long size; /* current checkpoint size in bytes */

Recall earlier query in RNG file about unsigned long...

> +};
> +
> +/* Stores the complete checkpoint metadata */
> +typedef struct _virDomainCheckpointDef virDomainCheckpointDef;
> +typedef virDomainCheckpointDef *virDomainCheckpointDefPtr;
> +struct _virDomainCheckpointDef {
> +/* Public XML.  */
> +char *name;
> +char *description;
> +char *parent;
> +long long creationTime; /* in seconds */
> +
> +size_t ndisks; /* should not exceed dom->ndisks */
> +virDomainCheckpointDiskDef *disks;
> +
> +virDomainDefPtr dom;
> +
> +/* Internal use.  */
> +bool current; /* At most one checkpoint in the list should have this set 
> */

Typically these are then in the *Obj...

> +};
> +
> +struct _virDomainCheckpointObj {
> +virDomainCheckpointDefPtr def; /* non-NULL except for metaroot */
> +
> +virDomainCheckpointObjPtr parent; /* non-NULL except for metaroot, before
> + virDomainCheckpointUpdateRelations, 
> or
> + after virDomainCheckpointDropParent 
> */
> +virDomainCheckpointObjPtr sibling; /* NULL if last child of parent */
> +size_t nchildren;
> +virDomainCheckpointObjPtr first_child; /* NULL if no children */

The whole relationship thing is I think overly complex and a bit
difficult to follow. Having @sibling and @first_child seems to make the
code even more complex. I would think you have a parent and maybe a
child. If this is the "first" checkpoint, then there is no parent. I
would think that means it's the root.

There's just much more to be see when it comes to how these
relationships play out as checkpoints are made and removed. I guess I
just have a very linear model in mind, but reading the code seems to go
beyond 

[libvirt] [PATCH v4 11/20] wip: backup: Parse and output checkpoint XML

[Eric Blake]

Work in progress - the checkpoint code is not quite passing
tests (part of that is figuring out the minimal XML that is
still valid as a  element, or just use --no-domain flag).

Signed-off-by: Eric Blake 
---
 src/conf/checkpoint_conf.h  |  150 
 src/conf/domain_conf.h  |   11 +-
 po/POTFILES |1 +
 src/conf/Makefile.inc.am|2 +
 src/conf/checkpoint_conf.c  | 1030 +++
 src/conf/domain_conf.c  |7 +-
 src/libvirt_private.syms|   21 +
 tests/Makefile.am   |9 +-
 tests/domaincheckpointxml2xmltest.c |  231 ++
 9 files changed, 1458 insertions(+), 4 deletions(-)
 create mode 100644 src/conf/checkpoint_conf.h
 create mode 100644 src/conf/checkpoint_conf.c
 create mode 100644 tests/domaincheckpointxml2xmltest.c

diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
new file mode 100644
index 00..994a8bd083
--- /dev/null
+++ b/src/conf/checkpoint_conf.h
@@ -0,0 +1,150 @@
+/*
+ * checkpoint_conf.h: domain checkpoint XML processing
+ *
+ * Copyright (C) 2006-2019 Red Hat, Inc.
+ * Copyright (C) 2006-2008 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#ifndef LIBVIRT_CHECKPOINT_CONF_H
+# define LIBVIRT_CHECKPOINT_CONF_H
+
+# include "internal.h"
+# include "domain_conf.h"
+
+/* Items related to checkpoint state */
+
+typedef enum {
+VIR_DOMAIN_CHECKPOINT_TYPE_DEFAULT = 0,
+VIR_DOMAIN_CHECKPOINT_TYPE_NONE,
+VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP,
+
+VIR_DOMAIN_CHECKPOINT_TYPE_LAST
+} virDomainCheckpointType;
+
+/* Stores disk-checkpoint information */
+typedef struct _virDomainCheckpointDiskDef virDomainCheckpointDiskDef;
+typedef virDomainCheckpointDiskDef *virDomainCheckpointDiskDefPtr;
+struct _virDomainCheckpointDiskDef {
+char *name; /* name matching the dom->disks that matches name */
+int type;   /* virDomainCheckpointType */
+char *bitmap;   /* bitmap name, if type is bitmap */
+unsigned long long size; /* current checkpoint size in bytes */
+};
+
+/* Stores the complete checkpoint metadata */
+typedef struct _virDomainCheckpointDef virDomainCheckpointDef;
+typedef virDomainCheckpointDef *virDomainCheckpointDefPtr;
+struct _virDomainCheckpointDef {
+/* Public XML.  */
+char *name;
+char *description;
+char *parent;
+long long creationTime; /* in seconds */
+
+size_t ndisks; /* should not exceed dom->ndisks */
+virDomainCheckpointDiskDef *disks;
+
+virDomainDefPtr dom;
+
+/* Internal use.  */
+bool current; /* At most one checkpoint in the list should have this set */
+};
+
+struct _virDomainCheckpointObj {
+virDomainCheckpointDefPtr def; /* non-NULL except for metaroot */
+
+virDomainCheckpointObjPtr parent; /* non-NULL except for metaroot, before
+ virDomainCheckpointUpdateRelations, or
+ after virDomainCheckpointDropParent */
+virDomainCheckpointObjPtr sibling; /* NULL if last child of parent */
+size_t nchildren;
+virDomainCheckpointObjPtr first_child; /* NULL if no children */
+};
+
+virDomainCheckpointObjListPtr virDomainCheckpointObjListNew(void);
+void virDomainCheckpointObjListFree(virDomainCheckpointObjListPtr checkpoints);
+
+typedef enum {
+VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE = 1 << 0,
+VIR_DOMAIN_CHECKPOINT_PARSE_DISKS= 1 << 1,
+VIR_DOMAIN_CHECKPOINT_PARSE_INTERNAL = 1 << 2,
+} virDomainCheckpointParseFlags;
+
+virDomainCheckpointDefPtr virDomainCheckpointDefParseString(const char *xmlStr,
+virCapsPtr caps,
+
virDomainXMLOptionPtr xmlopt,
+unsigned int 
flags);
+virDomainCheckpointDefPtr virDomainCheckpointDefParseNode(xmlDocPtr xml,
+  xmlNodePtr root,
+  virCapsPtr caps,
+  
virDomainXMLOptionPtr xmlopt,
+  unsigned int flags);
+void virDomainCheckpointDefFree(virDomainCheckpointDefPtr