Re: [libvirt] [PATCH 1/5] snapshot: Implement reverting for external disk snapshots

2018-11-18 Thread Povilas Kanapickas 
Hi Peter,

On 09/11/2018 20:58, Povilas Kanapickas wrote:
> Hi Peter,
> 
> Many thanks for the reviews. I replied inline below.
> 
> On 07/11/2018 17:40, Peter Krempa wrote:
> [...]

Just a friendly ping :-)

Best regards,
Povilas

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/5] snapshot: Implement reverting for external disk snapshots

2018-11-09 Thread Povilas Kanapickas 
Hi Peter,

Many thanks for the reviews. I replied inline below.

On 07/11/2018 17:40, Peter Krempa wrote:
> On Sun, Oct 21, 2018 at 19:38:48 +0300, Povilas Kanapickas wrote:
> [...]
>> +   int* out_mapping)
>> +{
>> +size_t i, j;
>> +int found_idx;
>> +virDomainSnapshotDiskDefPtr snap_disk;
>> +
>> +for (i = 0; i < snap_def->ndisks; ++i) {
>> +snap_disk = &(snap_def->disks[i]);
>> +if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
>> +continue;
>> +
>> +found_idx = -1;
>> +for (j = 0; j < dom_def->ndisks; ++j) {
>> +if (STREQ(dom_def->disks[j]->dst, snap_disk->name)) {
>> +found_idx = j;
>> +break;
>> +}
>> +}
>> +out_mapping[i] = found_idx;
>> +}
>> +}
>> +
>> +/* This function reverts an external snapshot disk state. With external 
>> disks
>> +   we can't just revert to the disks listed in the domain stored within the
>> +   snapshot, because it's read-only and might be used by other VMs in 
>> different
>> +   backing chains. Since the contents of the current disks will be discarded
>> +   in favor of data in the snapshot, we reuse them by resetting them and
>> +   pointing the backing image link to the disks listed within the snapshot.
>> +
>> +   The domain is expected to be inactive.
>> +
>> +   new_def is the new domain definition that will be stored to vm sometime 
>> in
>> +   the future.
>> + */
>> +static int
>> +qemuDomainSnapshotRevertInactiveExternal(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm,
>> + virDomainSnapshotObjPtr snap,
>> + virDomainDefPtr new_def)
>> +{
>> +/* We have the following disks recorded in the snapshot and the current
>> +   domain definition:
>> +
>> +- the current disk state before the revert (vm->def->disks). We'll
>> +  discard and reuse them.
> 
> So this has multiple problems:
> 
> 1) You can have a chain of snapshots and revert in middle of that:
> - this will destroy any snapshots which depend on the one you are
>   reverting to and that is not acceptable

The intention was to drop just the most current disk, the one that does
not have a snapshot yet. Then we can create another file and point its
backing image to the disk storing the snapshot that we want to revert
to. As far as I understand, this does not affect any later snapshots in
the chain.

> 2) The snapshot can have a different mapping of disks. When reverting
> the old topology should be kept as it existed at the time when the
> snapshot was created.

Agreed. But what would be the best outcome in situations when a snapshot
does not include a certain disk? My thinking is that if user does not
include a disk into a snapshot then he doesn't care about the change of
its contents since the last snapshot. Thus we could iterate the parent
snapshots until we find one that includes that disk and then revert the
state to that disk. If neither the snapshot or its parents include that
disk then we could probably leave the current disk state unchanged.

> 3) you can't return to the state that you are leaving (might be
> desirable but needs to be opt-in)
>
> This basically means that we need a new reversion API which will allow
> to take an XML and will basically fork the snapshot into an alternate
> history and also will mark the state you are leaving so that we can
> return to that state.
> 
> Reverting to that "new" state will result into just swithcing the
> definitions and continuing to use the images and make changes. (Which
> should be done for any leaf snapshot in the snapshot tree).
> 
> Deleting the state as you do is IMO not acceptable.

I think we could simplify that a lot if we accepted that revert is by
definition a lossy operation and any changes since last snapshot would
be lost.

Otherwise storing the previous state would just complicate things:
consider the situation where we are repeatedly reverting back and forth
between two snapshots. Either we will need to save all states between
the reverts and implement ability to store potentially more than one
previous state as a child of any snapshot. Or we will need to accept
that at some point data loss is acceptable and, say, limit the number of
state children of each snapshot to one.

I suggest that if we accepted snapshot operation as lossy, then your use
case could be implemented by a pair of  and  ope

Re: [libvirt] External disk snapshot paths and the revert operation

2018-11-02 Thread Povilas Kanapickas 
On 02/11/2018 15:58, John Ferlan wrote:
> 
> 
> On 11/2/18 8:49 AM, Povilas Kanapickas wrote:
>> On 21/10/2018 21:31, Povilas Kanapickas wrote:
>>> Hey,
>>>
>>> It seems that there's an issue of how external disk snapshots currently
>>> work. The snapshot definition xml[1] that is supplied to the API has a
>>> list of disks each of which specify where the new overlay disks should
>>> be placed.
>>>
>>> This leads to ambiguities when we want to revert the domain to a
>>> snapshot with children. Consider the following situation:
>>>
>>> 0. Initial state:
>>> Domain definition: disk at disk-base.qcow2
>>>
>>> 1. We create a snapshot snap1 placing the overlay at disk-snap1.qcow2
>>>
>>> Domain definition: disk at disk-snap1.qcow2
>>> Disks:
>>>   - disk-base.qcow2 (read-only)
>>>   - disk-snap1.qcow2 (overlay, base: disk-base.qcow2)
>>>
>>> Snapshots:
>>>   - snap1 (snapshot domain disk list: disk-base.qcow2,
>>>snapshot disk list: disk-snap1.qcow2)
>>>
>>> 2. We create another snapshot snap2 placing the overlay at disk-snap2.qcow2
>>>
>>> VM definition: disk at disk-snap2.qcow2
>>> Disks:
>>>   - disk-base.qcow2 (read-only)
>>>   - disk-snap1.qcow2 (overlay, base: disk-base.qcow2, read-only)
>>>   - disk-snap2.qcow2 (overlay, base: disk-snap1.qcow2)
>>>
>>> Snapshots:
>>>   - snap1 (snapshot domain disk list: disk-base.qcow2,
>>>snapshot disk list: disk-snap1.qcow2)
>>>   - snap2 (snapshot domain disk list: disk-snap1.qcow2,
>>>snapshot disk list: disk-snap2.qcow2)
>>>
>>> Now what happens if we want to revert to snap1? We can't use any of the
>>> paths that snap1 itself references: both disk-base.qcow2 and
>>> disk-snap1.qcow2 are read-only as part of the backing chain for disks
>>> used by snap2. Possible options are these:
>>>
>>>  - we can reuse disk-snap2.qcow2 which we no longer need as it contains
>>> overlay on top of snap2. This is non-optimal because now the path of the
>>> disk after revert depends on the disk path before it.
>>>
>>>  - we can think of some name with the expectation that the user will
>>> change it later anyway.
>>>
>>> I think that both of the above are non-optimal because we don't actually
>>> revert to the exact state we took the snapshot of. The user is
>>> effectively no longer in control of the disk path after the revert.
>>>
>>> One of possible solutions would be add additional flag during the
>>> snapshot creation that would change the current behavior. Instead of
>>> creating a new disk for the overlay we could move the base disk to the
>>> specified path and create the overlay in its place. The advantages of
>>> this behavior would be the following:
>>>
>>>  - the path to the current disk used in the domain would not change when
>>> taking snapshots. This way we could avoid the problem listed above and
>>> easily revert to any snapshot.
>>>
>>>  - the confusion in naming of the overlay disks would be reduced.
>>> Currently the path specified when creating snapshot refers to the
>>> overlay image. Most of the time user does not know what will be changed
>>> in the domain until the next snapshot. So any path he chooses will
>>> likely not reflect what will end up in the overlay at the time of the
>>> next snapshot. If it was possible to specify the path where the current
>>> disk should be put instead, then the user could just name it after the
>>> snapshot and it would correctly represent the contents of that disk.
>>>
>>> The major disadvantage of the approach is that it would introduce extra
>>> complexity in the presently simple interface and also the underlying
>>> code. I've completely disregarded the case of taking snapshots while the
>>> domain is running, so there might be hidden complications there as well.
>>>
>>> Personally, I don't mind the current situation too much, but if libvirt
>>> developers think it's a thing worth fixing I would be willing to work on it.
>>>
>>> Any opinions?
>>>
>>
>> Friendly ping :-)
>>
> 
> Please be aware that the typical Red Hat reviewers of the list were away
> at KVM Forum from Oct 22-26 and some took some extended vacation time
> after that. In particular, Peter Krempa who generally understand the
> snapshot code best is away. So, it may take longer than usual for the
> patches to be considered especially since beyond the Forum/Vacation time
> we had other rather major news to disrupt our normal work flow.

Thank you for explanation.

Regards,
Povilas


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] External disk snapshot paths and the revert operation

2018-11-02 Thread Povilas Kanapickas 
On 21/10/2018 21:31, Povilas Kanapickas wrote:
> Hey,
> 
> It seems that there's an issue of how external disk snapshots currently
> work. The snapshot definition xml[1] that is supplied to the API has a
> list of disks each of which specify where the new overlay disks should
> be placed.
> 
> This leads to ambiguities when we want to revert the domain to a
> snapshot with children. Consider the following situation:
> 
> 0. Initial state:
> Domain definition: disk at disk-base.qcow2
> 
> 1. We create a snapshot snap1 placing the overlay at disk-snap1.qcow2
> 
> Domain definition: disk at disk-snap1.qcow2
> Disks:
>   - disk-base.qcow2 (read-only)
>   - disk-snap1.qcow2 (overlay, base: disk-base.qcow2)
> 
> Snapshots:
>   - snap1 (snapshot domain disk list: disk-base.qcow2,
>snapshot disk list: disk-snap1.qcow2)
> 
> 2. We create another snapshot snap2 placing the overlay at disk-snap2.qcow2
> 
> VM definition: disk at disk-snap2.qcow2
> Disks:
>   - disk-base.qcow2 (read-only)
>   - disk-snap1.qcow2 (overlay, base: disk-base.qcow2, read-only)
>   - disk-snap2.qcow2 (overlay, base: disk-snap1.qcow2)
> 
> Snapshots:
>   - snap1 (snapshot domain disk list: disk-base.qcow2,
>snapshot disk list: disk-snap1.qcow2)
>   - snap2 (snapshot domain disk list: disk-snap1.qcow2,
>snapshot disk list: disk-snap2.qcow2)
> 
> Now what happens if we want to revert to snap1? We can't use any of the
> paths that snap1 itself references: both disk-base.qcow2 and
> disk-snap1.qcow2 are read-only as part of the backing chain for disks
> used by snap2. Possible options are these:
> 
>  - we can reuse disk-snap2.qcow2 which we no longer need as it contains
> overlay on top of snap2. This is non-optimal because now the path of the
> disk after revert depends on the disk path before it.
> 
>  - we can think of some name with the expectation that the user will
> change it later anyway.
> 
> I think that both of the above are non-optimal because we don't actually
> revert to the exact state we took the snapshot of. The user is
> effectively no longer in control of the disk path after the revert.
> 
> One of possible solutions would be add additional flag during the
> snapshot creation that would change the current behavior. Instead of
> creating a new disk for the overlay we could move the base disk to the
> specified path and create the overlay in its place. The advantages of
> this behavior would be the following:
> 
>  - the path to the current disk used in the domain would not change when
> taking snapshots. This way we could avoid the problem listed above and
> easily revert to any snapshot.
> 
>  - the confusion in naming of the overlay disks would be reduced.
> Currently the path specified when creating snapshot refers to the
> overlay image. Most of the time user does not know what will be changed
> in the domain until the next snapshot. So any path he chooses will
> likely not reflect what will end up in the overlay at the time of the
> next snapshot. If it was possible to specify the path where the current
> disk should be put instead, then the user could just name it after the
> snapshot and it would correctly represent the contents of that disk.
> 
> The major disadvantage of the approach is that it would introduce extra
> complexity in the presently simple interface and also the underlying
> code. I've completely disregarded the case of taking snapshots while the
> domain is running, so there might be hidden complications there as well.
> 
> Personally, I don't mind the current situation too much, but if libvirt
> developers think it's a thing worth fixing I would be willing to work on it.
> 
> Any opinions?
> 

Friendly ping :-)

Regards,
Povilas

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC/WIP] [PATCH 0/5] Add support for revert and delete operations to external disk snapshots

2018-11-02 Thread Povilas Kanapickas 
On 21/10/2018 19:38, Povilas Kanapickas wrote:
> Hey all,
> 
> Currently libvirt only supports creation of external disk snapshots, but not 
> reversion and deletion which are essential for any serious use of this 
> feature. 
> I've looked into implementing removal and reversion of external disk 
> snapshots 
> and came up with some prototype code that works with my simple test VMs (see 
> attached patches).
> 
> I'd like to discuss about how these features could be implemented properly. 
> As 
> I've never significantly contributed to libvirt yet, I wanted to delay the 
> discussion until I understand the problem space myself so that the discussion
> could be productive.
> 
> My current approach is relatively simple. For snapshot deletion we either
> simply remove the disk or use `qemu-img rebase` to reparent a snapshot on top 
> of the parent of the snapshot that is being deleted. For reversion we delete 
> the current overlay disk and create another that uses the image of the
> snapshot we want to revert to as the backing disk.
> 
> Are the attached patches good in principle? Are there any major blockers 
> aside 
> from lack of tests, code formatting, bugs and so on? Are there any design
> issues which prevent a simple implementation of external disk snapshot 
> support that I didn't see?
> 
> If there aren't significant blockers, my plan would be to continue work on 
> the 
> feature until I have something that could actually be reviewed and possibly
> merged.
> 

Friendly ping :-)

Regards,
Povilas

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] External disk snapshot paths and the revert operation

2018-10-21 Thread Povilas Kanapickas 
Hey,

It seems that there's an issue of how external disk snapshots currently
work. The snapshot definition xml[1] that is supplied to the API has a
list of disks each of which specify where the new overlay disks should
be placed.

This leads to ambiguities when we want to revert the domain to a
snapshot with children. Consider the following situation:

0. Initial state:
Domain definition: disk at disk-base.qcow2

1. We create a snapshot snap1 placing the overlay at disk-snap1.qcow2

Domain definition: disk at disk-snap1.qcow2
Disks:
  - disk-base.qcow2 (read-only)
  - disk-snap1.qcow2 (overlay, base: disk-base.qcow2)

Snapshots:
  - snap1 (snapshot domain disk list: disk-base.qcow2,
   snapshot disk list: disk-snap1.qcow2)

2. We create another snapshot snap2 placing the overlay at disk-snap2.qcow2

VM definition: disk at disk-snap2.qcow2
Disks:
  - disk-base.qcow2 (read-only)
  - disk-snap1.qcow2 (overlay, base: disk-base.qcow2, read-only)
  - disk-snap2.qcow2 (overlay, base: disk-snap1.qcow2)

Snapshots:
  - snap1 (snapshot domain disk list: disk-base.qcow2,
   snapshot disk list: disk-snap1.qcow2)
  - snap2 (snapshot domain disk list: disk-snap1.qcow2,
   snapshot disk list: disk-snap2.qcow2)

Now what happens if we want to revert to snap1? We can't use any of the
paths that snap1 itself references: both disk-base.qcow2 and
disk-snap1.qcow2 are read-only as part of the backing chain for disks
used by snap2. Possible options are these:

 - we can reuse disk-snap2.qcow2 which we no longer need as it contains
overlay on top of snap2. This is non-optimal because now the path of the
disk after revert depends on the disk path before it.

 - we can think of some name with the expectation that the user will
change it later anyway.

I think that both of the above are non-optimal because we don't actually
revert to the exact state we took the snapshot of. The user is
effectively no longer in control of the disk path after the revert.

One of possible solutions would be add additional flag during the
snapshot creation that would change the current behavior. Instead of
creating a new disk for the overlay we could move the base disk to the
specified path and create the overlay in its place. The advantages of
this behavior would be the following:

 - the path to the current disk used in the domain would not change when
taking snapshots. This way we could avoid the problem listed above and
easily revert to any snapshot.

 - the confusion in naming of the overlay disks would be reduced.
Currently the path specified when creating snapshot refers to the
overlay image. Most of the time user does not know what will be changed
in the domain until the next snapshot. So any path he chooses will
likely not reflect what will end up in the overlay at the time of the
next snapshot. If it was possible to specify the path where the current
disk should be put instead, then the user could just name it after the
snapshot and it would correctly represent the contents of that disk.

The major disadvantage of the approach is that it would introduce extra
complexity in the presently simple interface and also the underlying
code. I've completely disregarded the case of taking snapshots while the
domain is running, so there might be hidden complications there as well.

Personally, I don't mind the current situation too much, but if libvirt
developers think it's a thing worth fixing I would be willing to work on it.

Any opinions?

Regards,
Povilas

[1]: https://libvirt.org/formatsnapshot.html

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 5/5] snapshot: Support reparenting external disk snapshots when deleting

2018-10-21 Thread Povilas Kanapickas 
Signed-off-by: Povilas Kanapickas 
---
 src/qemu/qemu_driver.c | 89 ++
 1 file changed, 89 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 227ec1c6d9..a3ffc19122 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16590,6 +16590,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 typedef struct _virQEMUSnapReparent virQEMUSnapReparent;
 typedef virQEMUSnapReparent *virQEMUSnapReparentPtr;
 struct _virQEMUSnapReparent {
+virQEMUDriverPtr driver;
 virQEMUDriverConfigPtr cfg;
 virDomainSnapshotObjPtr parent;
 virDomainObjPtr vm;
@@ -16617,6 +16618,88 @@ 
qemuDomainSnapshotReparentChildrenMetadata(virDomainSnapshotObjPtr snap,
rep->cfg->snapshotDir);
 }
 
+static int
+qemuDomainSnapshotReparentDiskExternal(virQEMUDriverPtr driver,
+   virDomainSnapshotObjPtr parent_snap,
+   virDomainSnapshotDiskDefPtr disk)
+{
+const char* qemu_img_path = NULL;
+virCommandPtr cmd = NULL;
+int i;
+int ret = -1;
+const char* parent_disk_path = NULL;
+
+// Find the path to the disk we should use as the base when reparenting
+// FIXME: what if there's no parent snapshot? i.e. we need to reparent on
+// "empty" disk?
+for (i = 0; i < parent_snap->def->dom->ndisks; ++i) {
+if (STREQ(parent_snap->def->dom->disks[i]->dst, disk->name)) {
+parent_disk_path = parent_snap->def->dom->disks[i]->src->path;
+break;
+}
+}
+
+if (parent_disk_path == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Could not find disk to reparent '%s' to"),
+   disk->src->path);
+goto cleanup;
+}
+
+if (!(qemu_img_path = qemuFindQemuImgBinary(driver))) {
+goto cleanup;
+}
+
+if (!(cmd = virCommandNewArgList(qemu_img_path,
+ "rebase", "-b", parent_disk_path,
+ disk->src->path,
+ NULL))) {
+goto cleanup;
+}
+
+if (virCommandRun(cmd, NULL) < 0)
+goto cleanup;
+
+ret = 0;
+cleanup:
+virCommandFree(cmd);
+return ret;
+}
+
+static int
+qemuDomainSnapshotReparentDisks(virDomainSnapshotObjPtr snap,
+virQEMUSnapReparentPtr rep)
+{
+int i;
+virDomainSnapshotDiskDefPtr snap_disk;
+
+if (!snap->def->dom) {
+VIR_WARN("Any external disk snapshots in a snapshot created with "
+ "pre-0.9.5 libvirt will not be correctly reparented.");
+// In snapshots created with pre-0.9.5 libvirt we don't have 
information
+// needed to correctly reparent external disk snapshots but we also
+// don't even know whether external disk snapshots were used so that
+// we could report this as an error. We can only emit a warning in that
+// case.
+return 0;
+}
+
+for (i = 0; i < snap->def->ndisks; ++i) {
+snap_disk = &(snap->def->disks[i]);
+
+// FIXME: do we need to explicitly reparent internal snapshots to
+// e.g. prevent long non-visible snapshot chains that might affect
+// performance?
+if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+continue;
+
+if (qemuDomainSnapshotReparentDiskExternal(rep->driver, rep->parent,
+   snap_disk) < 0)
+return -1;
+}
+return 0;
+}
+
 static int
 qemuDomainSnapshotReparentChildren(void *payload,
const void *name ATTRIBUTE_UNUSED,
@@ -16628,6 +16711,11 @@ qemuDomainSnapshotReparentChildren(void *payload,
 if (rep->err < 0)
 return 0;
 
+if (qemuDomainSnapshotReparentDisks(snap, rep) < 0) {
+rep->err = -1;
+goto cleanup;
+}
+
 if (qemuDomainSnapshotReparentChildrenMetadata(snap, rep) < 0) {
 rep->err = -1;
 goto cleanup;
@@ -16718,6 +16806,7 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
 vm->current_snapshot = snap;
 }
 } else if (snap->nchildren) {
+rep.driver = driver;
 rep.cfg = cfg;
 rep.parent = snap->parent;
 rep.vm = vm;
-- 
2.17.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 4/5] snapshot: Extract qemuDomainSnapshotReparentChildrenMetadata()

2018-10-21 Thread Povilas Kanapickas 
Signed-off-by: Povilas Kanapickas 
---
 src/qemu/qemu_driver.c | 30 --
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7cd44d6957..227ec1c6d9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16600,6 +16600,23 @@ struct _virQEMUSnapReparent {
 };
 
 
+static int
+qemuDomainSnapshotReparentChildrenMetadata(virDomainSnapshotObjPtr snap,
+   virQEMUSnapReparentPtr rep)
+{
+VIR_FREE(snap->def->parent);
+snap->parent = rep->parent;
+
+if (rep->parent->def &&
+VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
+return -1;
+}
+
+return qemuDomainSnapshotWriteMetadata(rep->vm, snap,
+   rep->caps, rep->xmlopt,
+   rep->cfg->snapshotDir);
+}
+
 static int
 qemuDomainSnapshotReparentChildren(void *payload,
const void *name ATTRIBUTE_UNUSED,
@@ -16611,21 +16628,14 @@ qemuDomainSnapshotReparentChildren(void *payload,
 if (rep->err < 0)
 return 0;
 
-VIR_FREE(snap->def->parent);
-snap->parent = rep->parent;
-
-if (rep->parent->def &&
-VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
+if (qemuDomainSnapshotReparentChildrenMetadata(snap, rep) < 0) {
 rep->err = -1;
-return 0;
+goto cleanup;
 }
 
+cleanup:
 if (!snap->sibling)
 rep->last = snap;
-
-rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap,
-   rep->caps, rep->xmlopt,
-   rep->cfg->snapshotDir);
 return 0;
 }
 
-- 
2.17.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [RFC/WIP] [PATCH 0/5] Add support for revert and delete operations to external disk snapshots

2018-10-21 Thread Povilas Kanapickas 
Hey all,

Currently libvirt only supports creation of external disk snapshots, but not 
reversion and deletion which are essential for any serious use of this feature. 
I've looked into implementing removal and reversion of external disk snapshots 
and came up with some prototype code that works with my simple test VMs (see 
attached patches).

I'd like to discuss about how these features could be implemented properly. As 
I've never significantly contributed to libvirt yet, I wanted to delay the 
discussion until I understand the problem space myself so that the discussion
could be productive.

My current approach is relatively simple. For snapshot deletion we either
simply remove the disk or use `qemu-img rebase` to reparent a snapshot on top 
of the parent of the snapshot that is being deleted. For reversion we delete 
the current overlay disk and create another that uses the image of the
snapshot we want to revert to as the backing disk.

Are the attached patches good in principle? Are there any major blockers aside 
from lack of tests, code formatting, bugs and so on? Are there any design
issues which prevent a simple implementation of external disk snapshot 
support that I didn't see?

If there aren't significant blockers, my plan would be to continue work on the 
feature until I have something that could actually be reviewed and possibly
merged.

Regards,
Povilas

Povilas Kanapickas (5):
  snapshot: Implement reverting for external disk snapshots
  snapshot: Add VIR_DEBUG to qemuDomainSnapshotCreateXML()
  snapshot: Support deleting external disk snapshots when deleting
  snapshot: Extract qemuDomainSnapshotReparentChildrenMetadata()
  snapshot: Support reparenting external disk snapshots when deleting

 src/qemu/qemu_domain.c |  45 -
 src/qemu/qemu_driver.c | 372 +
 2 files changed, 379 insertions(+), 38 deletions(-)

-- 
2.17.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/5] snapshot: Add VIR_DEBUG to qemuDomainSnapshotCreateXML()

2018-10-21 Thread Povilas Kanapickas 
Signed-off-by: Povilas Kanapickas 
---
 src/qemu/qemu_driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 279e5d93aa..eb104d306b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15298,6 +15298,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
 const char *xmlDesc,
 unsigned int flags)
 {
+VIR_DEBUG("domain=%p, flags=0x%x, xmlDesc=%s", domain, flags, xmlDesc);
+
 virQEMUDriverPtr driver = domain->conn->privateData;
 virDomainObjPtr vm = NULL;
 char *xml = NULL;
-- 
2.17.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/5] snapshot: Support deleting external disk snapshots when deleting

2018-10-21 Thread Povilas Kanapickas 
Signed-off-by: Povilas Kanapickas 
---
 src/qemu/qemu_domain.c | 45 ++
 src/qemu/qemu_driver.c |  7 ---
 2 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ba3fff607a..73c41788f8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8244,6 +8244,38 @@ qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver,
  op, try_all, def->ndisks);
 }
 
+static int
+qemuDomainSnapshotDiscardExternal(virDomainSnapshotObjPtr snap)
+{
+int i;
+virDomainSnapshotDiskDefPtr snap_disk;
+
+// FIXME: libvirt stores the VM definition and the list of disks that
+// snapshot has been taken of since 0.9.5. Before that we must assume that
+// all disks from within the VM definition have been snapshotted and that
+// they all were internal disks. Did libvirt support external snapshots
+// before 0.9.5? If so, if we ever end up in this code path we may incur
+// data loss as we'll delete whole QEMU file thinking it's an external
+// snapshot.
+if (!snap->def->dom) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("Snapshots created using libvirt 9.5 and containing "
+ "external disk snapshots cannot be safely "
+ "discarded."));
+return -1;
+}
+
+for (i = 0; i < snap->def->ndisks; ++i) {
+snap_disk = &(snap->def->disks[i]);
+if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+continue;
+if (unlink(snap_disk->src->path) < 0)
+VIR_WARN("Failed to unlink snapshot disk '%s'",
+ snap_disk->src->path);
+}
+return 0;
+}
+
 /* Discard one snapshot (or its metadata), without reparenting any children.  
*/
 int
 qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
@@ -8260,10 +8292,15 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
 
 if (!metadata_only) {
 if (!virDomainObjIsActive(vm)) {
-/* Ignore any skipped disks */
-if (qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-d",
-   true) < 0)
-goto cleanup;
+if (virDomainSnapshotIsExternal(snap)) {
+if (qemuDomainSnapshotDiscardExternal(snap) < 0)
+goto cleanup;
+} else {
+/* Ignore any skipped disks */
+if (qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-d",
+   true) < 0)
+goto cleanup;
+}
 } else {
 priv = vm->privateData;
 qemuDomainObjEnterMonitor(driver, vm);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index eb104d306b..7cd44d6957 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16662,7 +16662,7 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
 if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
 goto endjob;
 
-if (!metadata_only) {
+if (!metadata_only && virDomainObjIsActive(vm)) {
 if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) &&
 virDomainSnapshotIsExternal(snap))
 external++;
@@ -16673,8 +16673,9 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
);
 if (external) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("deletion of %d external disk snapshots not "
- "supported yet"), external);
+   _("deletion of %d external disk snapshots while the 
"
+ "domain is active is not supported yet"),
+   external);
 goto endjob;
 }
 }
-- 
2.17.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/5] snapshot: Implement reverting for external disk snapshots

2018-10-21 Thread Povilas Kanapickas 
Signed-off-by: Povilas Kanapickas 
---
 src/qemu/qemu_driver.c | 246 +
 1 file changed, 224 insertions(+), 22 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e2495d5..279e5d93aa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15952,19 +15952,194 @@ qemuDomainSnapshotHasMetadata(virDomainSnapshotPtr 
snapshot,
 return ret;
 }
 
+static int
+qemuDomainSnapshotRevertExternalSingleDisk(virQEMUDriverPtr driver,
+   virDomainDiskDefPtr revert_disk,
+   virDomainDiskDefPtr backing_disk)
+{
+virCommandPtr cmd = NULL;
+const char *qemuImgPath;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+int ret = -1;
+
+if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
+goto cleanup;
+
+if (unlink(revert_disk->src->path) < 0)
+VIR_WARN("Failed to overwrite current image for snapshot '%s'",
+ revert_disk->src->path);
+
+/* TODO: maybe figure out the format from the backing_disk? */
+revert_disk->src->format = VIR_STORAGE_FILE_QCOW2;
+/* FIXME: what about revert_disk->src->backingStore ? */
+
+/* creates cmd line args: qemu-img create -f qcow2 -o */
+if (!(cmd = virCommandNewArgList(qemuImgPath,
+ "create",
+ "-f",
+ 
virStorageFileFormatTypeToString(revert_disk->src->format),
+ "-o",
+ NULL)))
+goto cleanup;
+
+/* adds cmd line arg: 
backing_file=/path/to/backing/file,backing_fmt=format */
+virCommandAddArgFormat(cmd, "backing_file=%s,backing_fmt=%s",
+   backing_disk->src->path,
+   
virStorageFileFormatTypeToString(backing_disk->src->format));
+
+/* adds cmd line args: /path/to/target/file */
+virCommandAddArg(cmd, revert_disk->src->path);
+
+if (virCommandRun(cmd, NULL) < 0)
+goto cleanup;
+
+virCommandFree(cmd);
+cmd = NULL;
+ret = 0;
+
+ cleanup:
+virCommandFree(cmd);
+virObjectUnref(cfg);
+
+return ret;
+}
+
+static void
+qemuComputeSnapshotDiskToDomainDiskMapping(virDomainSnapshotDefPtr snap_def,
+   virDomainDefPtr dom_def,
+   int* out_mapping)
+{
+size_t i, j;
+int found_idx;
+virDomainSnapshotDiskDefPtr snap_disk;
+
+for (i = 0; i < snap_def->ndisks; ++i) {
+snap_disk = &(snap_def->disks[i]);
+if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+continue;
+
+found_idx = -1;
+for (j = 0; j < dom_def->ndisks; ++j) {
+if (STREQ(dom_def->disks[j]->dst, snap_disk->name)) {
+found_idx = j;
+break;
+}
+}
+out_mapping[i] = found_idx;
+}
+}
+
+/* This function reverts an external snapshot disk state. With external disks
+   we can't just revert to the disks listed in the domain stored within the
+   snapshot, because it's read-only and might be used by other VMs in different
+   backing chains. Since the contents of the current disks will be discarded
+   in favor of data in the snapshot, we reuse them by resetting them and
+   pointing the backing image link to the disks listed within the snapshot.
+
+   The domain is expected to be inactive.
+
+   new_def is the new domain definition that will be stored to vm sometime in
+   the future.
+ */
+static int
+qemuDomainSnapshotRevertInactiveExternal(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainSnapshotObjPtr snap,
+ virDomainDefPtr new_def)
+{
+/* We have the following disks recorded in the snapshot and the current
+   domain definition:
+
+- the current disk state before the revert (vm->def->disks). We'll
+  discard and reuse them.
+- the lists of disks that were snapshotted (snap->def->disks). We'll
+  use this information to figure out which disks to actually revert.
+- the original disk state stored in the snapshot
+  (snap->def->dom->disks). We'll point reverted disks to use these
+  disks as backing images.
+
+FIXME: what to do with disks that weren't included in all snapshots
+within the hierrachy?
+*/
+size_t i;
+int* snap_disk_to_vm_def_disk_idx_map = NULL;
+int* snap_disk_to_new_def_disk_idx_map = NULL;
+int ret = -1;
+virDomainSnapshotDiskDefPtr snap_disk;
+virDomainDiskDefPtr backing_disk;
+virDom

Re: [libvirt] Virsh reference is out of date, should it be updated from the man page?

2018-09-04 Thread Povilas Kanapickas 
On 09/03/2018 01:09 PM, Ján Tomko wrote:
> On Mon, Sep 03, 2018 at 10:04:16AM +0200, Michal Prívozník wrote:
>> On 09/03/2018 09:38 AM, Povilas Kanapickas wrote:
>>> Hi!
>>>
>>> The online virsh command reference at [1] seems to be very out of date
>>> according to [2]. There's much more recent documentation at
>>> tools/virsh.pod in the main libvirt repository.
>>
>> Yes, this is because of lacking manpower. As usual O:-)
>>
>>>
>>> I believe it would be great to update the web docs as they are much more
>>> accessible and convenient to use (especially the one-command-per-page
>>> version).
>>>
> 
> AFAIK the "Virsh Command Reference" was intended to provide more
> information than the man page, especially practical usage of the
> commands, not just to have the manpage in HTML.
> 
> So providing this extra content with examples would be more worthwhile
> than just a copy of the manpage.
> 
>>> What would be the best way to approach that? I suppose we could look
>>> into automatically converting the current manpages into docbook and
>>> using that to update the website? What do you think?
>>
> 
> Generating parts of the document automatically would help it stay
> current, but it should not get in the way of adding more 'human touch'
> to it. But a manpage copy might be better than nothing.
> 
> We already generate the API documentation automatically, we have a
> custom C parser in docs/apibuild.py and docs/hvsupport.pl
> that generate some intermediate files - similar approach could be used
> to generate a table of options and the availablity sections.
> 
>> This sounds like the best way to go. virsh is changed virtually with
>> each release so an automated tool that would regenerate the docs which
>> could run at every release is certainly desired.>
> 
> libvirt development goes much faster than that - it can be generated
> hourly just like the rest of the autogenerated docs.
> 
> Jano
> 
>> However, I'm no docbook/manpages master, so I'll let somebody else look
>> into it (perhaps yourself? ;-))
>>
>> Michal
>>

Thanks for replies, I will come back again asking more questions when I
start working in this.

Regards,
Povilas



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] Virsh reference is out of date, should it be updated from the man page?

2018-09-03 Thread Povilas Kanapickas 
Hi!

The online virsh command reference at [1] seems to be very out of date
according to [2]. There's much more recent documentation at
tools/virsh.pod in the main libvirt repository.

I believe it would be great to update the web docs as they are much more
accessible and convenient to use (especially the one-command-per-page
version).

What would be the best way to approach that? I suppose we could look
into automatically converting the current manpages into docbook and
using that to update the website? What do you think?

Regards,
Povilas

[1] - https://libvirt.org/virshcmdref.html
[2] - https://libvirt.org/git/?p=libvirt-virshcmdref.git;a=summary

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] Virsh reference is out of date, should it be updated from the man page?

2018-09-03 Thread Povilas Kanapickas 
Hi!

The online virsh command reference at [1] seems to be very out of date
according to [2]. There's much more recent documentation at
tools/virsh.pod in the main libvirt repository.

I believe it would be great to update the web docs as they are much more
accessible and convenient to use (especially the one-command-per-page
version).

What would be the best way to approach that? I suppose we could look
into automatically converting the current manpages into docbook and
using that to update the website? What do you think?

Regards,
Povilas

[1] - https://libvirt.org/virshcmdref.html
[2] - https://libvirt.org/git/?p=libvirt-virshcmdref.git;a=summary

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files

2018-08-16 Thread Povilas Kanapickas 
On 16/08/2018 10:38, Peter Krempa wrote:
> To fix this you should record the backing format [1] into your overlay
> image. If we'd relax the code we'd face the regression in the security
> fix we've done.
> 
> [1] qemu-img creage -f qcow2 -F qcow2 -b backing-qcow2 overlay.qcow2
> 
> -F option specifies the format of the backing file
> 

Thanks a lot for your explanation, now I see that my proposal does not
make any sense. Your suggestion works fine and virt-aa-helper produces
correct output.

Do you think this situation should ideally be diagnosed by higher-level
tools such as virt-manager which right now emit a generic permission
denied error?

Maybe virt-aa-helper could also emit a comment into the apparmor profile
saying something like "image.img has a backing image xyz.img but it was
not probed because its format is not recorded into the overlay image"?

Regards,
Povilas

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files

2018-08-16 Thread Povilas Kanapickas 
Hi,

I've looked into why apparmor profiles do not contain exceptions for
backing files of images which later leads to permission errors due to
apparmor containment. As of newest libvirt git master, only the first
level backing image is included, the subsequent images are omitted.

Below is my investigation of why this issue happens. It is based on
libvirt git master as of 2018-08-15 (e3e48d7cb82d58). The possible fix
is easy, but I don't have the background how to write tests for it, so
it's best that someone who knows the project well completes the fix

In my case I have the following file hierarchy:
 - image-master.qcow2 (has backing file image-backing-1.qcow2)
 - image-backing-1.qcow2 (has backing file image-backing-2.qcow2
 - image-backing-2.qcow2

The apparmor profiles are created by the virt-aa-helper tool. The
get_files function (src/security/virt-aa-helper.c:949) creates a list of
files that need to be accessed by the virtual machine. The call to
virDomainDiskDefForeachPath() is responsible for creating the list of
files required to access each disk given the disk metadata.

The disk->src argument contains the source file. The expected file
hierarchy would be this:

disk->src->path == path/to/image-master.qcow2
disk->src->backingStore->path == path/to/image-backing-1.qcow2
disk->src->backingStore->backingStore->path == path/to/image-backing-2.qcow2

Unfortunately only the first two levels are present and
disk->src->backingStore->backingStore points to a dummy object.

The backing store details are filled in virStorageFileGetMetadata()
call. It calls into virStorageFileGetMetadataRecurse
(src/util/virstoragefile.c:4789) which will collect metadata for a
single image and recurse into itself for backing files.

For us, the following part of virStorageFileGetMetadataRecurse is
important (simplified for brevity):

```
virStorageFileGetMetadataInternal(src, ..., );

if (src->backingStoreRaw) {
backingStore = ...

if (backingFormat == VIR_STORAGE_FILE_AUTO)
backingStore->format = VIR_STORAGE_FILE_RAW; [1]
else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE)
backingStore->format = VIR_STORAGE_FILE_AUTO;
else
backingStore->format = backingFormat;

virStorageFileGetMetadataRecurse(backingStore, ...) [2]
}
```

The crux of the issue seems that the call to
virStorageFileGetMetadataInternal() for the image-master.qcow2 image
will set the `backingFormat` return value  to
VIR_STORAGE_FILE_AUTO. The code responsible is in qcowXGetBackingStore
(src/util/virstoragefile.c:491) called via
`fileTypeInfo[meta->format].getBackingStore(...)` at
src/util/virstoragefile.c:1042.

It backingFormat is then downgraded to VIR_STORAGE_FILE_RAW at
src/util/virstoragefile.c:4854 ([1] above). This causes the next recurse
call to
virStorageFileGetMetadataRecurse() ([2] above) to not investigate the
backing images at all in virStorageFileGetMetadataInternal() as
fileTypeInfo[VIR_STORAGE_FILE_RAW].getBackingStore will be NULL.

The possible solution is to return VIR_STORAGE_FILE_AUTO_SAFE from
qcowXGetBackingStore.  It probably does not make much sense to prevent
probing of qcow2 backing images as we probe the first level of backing
images anyway, so that return value only affect second and subsequent
levels of backing images. Looking into the implementation of
qcowXGetBackingStore it also does not seem that it performs any
operations that are unsafe by design.

Currently VIR_STORAGE_FILE_AUTO is used only in qedGetBackingStore and
it does not seem that probing of qcow images is unsafe enough

What do the developers think about this? Could someone complete the fix
with tests or advise me about the testing frameworks if there's not
enough time?

Thanks a lot!
Povilas

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] Investigation and possible fix of 1361592 - apparmor profiles do not include backing files

2018-08-15 Thread Povilas Kanapickas 
Hi,

I've looked into why apparmor profiles do not contain exceptions for
backing files of images which later leads to permission errors due to
apparmor containment. As of newest libvirt git master, only the first
level backing image is included, the subsequent images are omitted.

Below is my investigation of why this issue happens. It is based on
libvirt git master as of 2018-08-15 (e3e48d7cb82d58). The possible fix
is easy, but I don't have the background how to write tests for it, so
it's best that someone who knows the project well completes the fix

In my case I have the following file hierarchy:
 - image-master.qcow2 (has backing file image-backing-1.qcow2)
 - image-backing-1.qcow2 (has backing file image-backing-2.qcow2
 - image-backing-2.qcow2

The apparmor profiles are created by the virt-aa-helper tool. The
get_files function (src/security/virt-aa-helper.c:949) creates a list of
files that need to be accessed by the virtual machine. The call to
virDomainDiskDefForeachPath() is responsible for creating the list of
files required to access each disk given the disk metadata.

The disk->src argument contains the source file. The expected file
hierarchy would be this:

disk->src->path == path/to/image-master.qcow2
disk->src->backingStore->path == path/to/image-backing-1.qcow2
disk->src->backingStore->backingStore->path == path/to/image-backing-2.qcow2

Unfortunately only the first two levels are present and
disk->src->backingStore->backingStore points to a dummy object.

The backing store details are filled in virStorageFileGetMetadata()
call. It calls into virStorageFileGetMetadataRecurse
(src/util/virstoragefile.c:4789) which will collect metadata for a
single image and recurse into itself for backing files.

For us, the following part of virStorageFileGetMetadataRecurse is
important (simplified for brevity):

```
virStorageFileGetMetadataInternal(src, ..., );

if (src->backingStoreRaw) {
backingStore = ...

if (backingFormat == VIR_STORAGE_FILE_AUTO)
backingStore->format = VIR_STORAGE_FILE_RAW; [1]
else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE)
backingStore->format = VIR_STORAGE_FILE_AUTO;
else
backingStore->format = backingFormat;

virStorageFileGetMetadataRecurse(backingStore, ...) [2]
}
```

The crux of the issue seems that the call to
virStorageFileGetMetadataInternal() for the image-master.qcow2 image
will set the `backingFormat` return value to VIR_STORAGE_FILE_AUTO. The
code responsible is in qcowXGetBackingStore
(src/util/virstoragefile.c:491) called via
`fileTypeInfo[meta->format].getBackingStore(...)` at
src/util/virstoragefile.c:1042.

It backingFormat is then downgraded to VIR_STORAGE_FILE_RAW at
src/util/virstoragefile.c:4854 ([1] above). This causes the next recurse
call to virStorageFileGetMetadataRecurse() ([2] above) to not
investigate the backing images at all in
virStorageFileGetMetadataInternal() as
fileTypeInfo[VIR_STORAGE_FILE_RAW].getBackingStore will be NULL.

The possible solution is to return VIR_STORAGE_FILE_AUTO_SAFE from
qcowXGetBackingStore.  It probably does not make much sense to prevent
probing of qcow2 backing images as we probe the first level of backing
images anyway, so that return value only affect second and subsequent
levels of backing images. Looking into the implementation of
qcowXGetBackingStore it also does not seem that it performs any
operations that are unsafe by design.

Currently VIR_STORAGE_FILE_AUTO is used only in qedGetBackingStore and
it does not seem that probing of qcow images is unsafe enough

What do the developers think about this? Could someone complete the fix
with tests or advise me about the testing frameworks if there's not
enough time?

Thanks a lot!
Povilas


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list