Re: [libvirt][RFC PATCH] add a new 'default' option for attribute mode in numatune

[Zhong, Luyao]

On 8/3/2020 7:00 PM, Martin Kletzander wrote:

On Mon, Aug 03, 2020 at 05:31:56PM +0800, Luyao Zhong wrote:

Hi Libvirt experts,

I would like enhence the numatune snippet configuration. Given a 
example snippet:



 ...
 
   
   
   
 
 ...


Currently, attribute mode is either 'interleave', 'strict', or 
'preferred',

I propose to add a new 'default'  option. I give the reason as following.

Presume we are using cgroups v1, Libvirt sets cpuset.mems for all vcpu 
threads
according to 'nodeset' in memory element. And translate the memnode 
element to
qemu config options (--object memory-backend-ram) for per numa cell, 
which

invoking mbind() system call at the end.[1]

But what if we want using default memory policy and request each guest 
numa cell
pinned to different host memory nodes? We can't use mbind via qemu 
config options,
because (I quoto here) "For MPOL_DEFAULT, the nodemask and maxnode 
arguments must

be specify the empty set of nodes." [2]

So my solution is introducing a new 'default' option for attribute 
mode. e.g.



 ...
 
   
   
   
 
 ...


If the mode is 'default', libvirt should avoid generating qemu command 
line
'--object memory-backend-ram', and invokes cgroups to set cpuset.mems 
for per guest numa

combining with numa topology config. Presume the numa topology is :


 ...
 
   
   
 
 ...


Then libvirt should set cpuset.mems to '1' for vcpus 0-3, and '2' for 
vcpus 4-7.



Is this reasonable and feasible? Welcome any comments.



There are couple of problems here.  The memory is not (always) allocated 
by the
vCPU threads.  I also remember it to not be allocated by the process, 
but in KVM
in a way that was not affected by the cgroup settings.  
Thanks for your reply. Maybe I don't get what you mean, could you give 
me more context? But what I proposed will have no effect on other memory 
allocation.
That might be 
fixed now,

however.

But basically what we have against is all the reasons why we started using
QEMU's command line arguments for all that.

I'm not proposing use QEMU's command line arguments, on contrary I want 
using cgroups setting to support a new config/requirement. I give a 
solution about if we require default memory policy and memory numa pinning.


Thanks,
Luyao
Sorry, but I think it will more likely break rather than fix stuff.  
Maybe this
could be dealt with by a switch in `qemu.conf` with a huge warning above 
it.


I'm not trying to fix something, I propose how to support a new 
requirement just like I stated above.

Have a nice day,
Martin


Regards,
Luyao

[1]https://github.com/qemu/qemu/blob/f2a1cf9180f63e88bb38ff21c169da97c3f2bad5/backends/hostmem.c#L379 


[2]https://man7.org/linux/man-pages/man2/mbind.2.html

--
2.25.1

Re: qemu modularization of qemu-5.1 vs libvirt domcapabilities cache?

[Mark Mielke]

On Wed, Aug 5, 2020 at 4:31 AM Mark Mielke  wrote:

> On Wed, Aug 5, 2020 at 4:19 AM Andrea Bolognani 
> wrote:
>
>> That said, if upgrading QEMU results in losing features, even though
>> you can recover them through additional steps I would argue that's a
>> bug in the packaging that should be addressed on the QEMU side.
>>
>
> I had the same thought. However, I'm expecting this will be part of Fedora
> 33 (not yet out), and the QXL display driver is possibly becoming optional?
> Within the same release of Fedora, I expect the default module list should
> be stable, but between releases it might not be? But, what about long-lived
> major releases like RHEL / CentOS? Or people who "upgrade" from the distro
> release to the "advanced virtualization" release (RHEL / CentOS 8)? I also
> would expect functionality which seems pretty default - to stay default,
> although perhaps it could be a weak package dependency or similar, to
> permit people to uninstall it?
>
> They also moved qemu-device-usb-smartcart to optional along with the
> already mentioned qemu-device-display-qxl and qemu-device-usb-redirect, and
> I believe this has happened in the past with some of the block device
> drivers, only I might not use them, so they might not have affected me? I
> think everything in the module directory is really in scope.
>

The Fedora package owner agreed, and will be correcting it so that the
default will include these packages. This addresses the upgrade case, and
the surprise factor for users merely upgrading from Fedora 32 to Fedora 33
resulting in libvirt breaking for them.

However, it does not address the exposed problem - which is that I can add
or remove individual Qemu module packages at any time, and libvirt will not
be aware of this change until some other event occurs which might not ever
occur in this workflow (even reboot!).

So, I think it is important to include the Qemu module directory in the
list of timestamps to check to determine if the domcapabilities cache
should be invalidated or not. If a module gets added or removed, the
directory timestamp should change.

I think the idea of libvirt probing the system and guessing when to
invalidate the cache based upon only a few select data points, including
ones like "qemu binary timestamp" are embedded with assumptions, is going
to continue to be a problem. But, adding the above check would close an
additional set of scenarios as covered.


-- 
Mark Mielke 

[libvirt PATCH] Adds e1000e/vmxnet3 Vnet_hdr suuport

[Patrick Magauran]

Libvirt bases its decision about whether to apply the vnet_hdr flag to the tap 
interface on whether or not the selected model is VirtIO. Originally, VirtIO 
was the only model to support the vnet_hdr in QEMU; however, the e1000e & 
vmxnet3 adapters also support it(seemingly from introduction based on commits). 
This passes the whole packet to the host, reducing emulation overhead and 
improving performance.

Signed-off-by: Patrick Magauran 
---
 src/conf/domain_conf.c| 8 +++-
 src/conf/domain_conf.h| 1 +
 src/libvirt_private.syms  | 1 +
 src/qemu/qemu_interface.c | 8 
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 69e0439e7e..cb184110f7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30981,7 +30981,13 @@ virDomainNetIsVirtioModel(const virDomainNetDef *net)
 net->model == VIR_DOMAIN_NET_MODEL_VIRTIO_TRANSITIONAL ||
 net->model == VIR_DOMAIN_NET_MODEL_VIRTIO_NON_TRANSITIONAL);
 }
-
+bool
+virDomainNetIsVnetCompatModel(const virDomainNetDef *net)
+{
+return (virDomainNetIsVirtioModel(net) ||
+net->model == VIR_DOMAIN_NET_MODEL_E1000E ||
+net->model == VIR_DOMAIN_NET_MODEL_VMXNET3);
+}
 
 /* Return listens[i] from the appropriate union for the graphics
  * type, or NULL if this is an unsuitable type, or the index is out of
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 011bf66cb4..cbc46fdf78 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3371,6 +3371,7 @@ const char *virDomainNetGetModelString(const 
virDomainNetDef *net);
 int virDomainNetSetModelString(virDomainNetDefPtr et,
const char *model);
 bool virDomainNetIsVirtioModel(const virDomainNetDef *net);
+bool virDomainNetIsVnetCompatModel(const virDomainNetDef *net);
 int virDomainNetAppendIPAddress(virDomainNetDefPtr def,
 const char *address,
 int family,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 01c2e710cd..2b64042dd2 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -525,6 +525,7 @@ virDomainNetGetActualVlan;
 virDomainNetGetModelString;
 virDomainNetInsert;
 virDomainNetIsVirtioModel;
+virDomainNetIsVnetCompatModel;
 virDomainNetModelTypeFromString;
 virDomainNetModelTypeToString;
 virDomainNetNotifyActualDevice;
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index ffec992596..8397ed8645 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -255,7 +255,7 @@ qemuInterfaceDirectConnect(virDomainDefPtr def,
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
 
-if (virDomainNetIsVirtioModel(net))
+if (virDomainNetIsVnetCompatModel(net))
 macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR;
 
 if (virNetDevMacVLanCreateWithVPortProfile(net->ifname,
@@ -417,7 +417,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
 }
 }
 
-if (virDomainNetIsVirtioModel(net))
+if (virDomainNetIsVnetCompatModel(net))
 tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
 
 if (net->managed_tap == VIR_TRISTATE_BOOL_NO) {
@@ -436,7 +436,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
 if (virNetDevMacVLanTapOpen(net->ifname, tapfd, tapfdSize) < 0)
 goto cleanup;
 if (virNetDevMacVLanTapSetup(tapfd, tapfdSize,
- virDomainNetIsVirtioModel(net)) < 0) {
+ virDomainNetIsVnetCompatModel(net)) < 
0) {
 goto cleanup;
 }
 } else {
@@ -559,7 +559,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
 template_ifname = true;
 }
 
-if (virDomainNetIsVirtioModel(net))
+if (virDomainNetIsVnetCompatModel(net))
 tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
 
 if (driver->privileged) {
-- 
2.26.2

Re: [PATCH 1/3] qemu: implementation of transient option for qcow2 file

[Masayoshi Mizuma]

On Thu, Aug 06, 2020 at 12:09:20PM +0200, Peter Krempa wrote:
> On Sun, Jul 19, 2020 at 22:56:50 -0400, Masayoshi Mizuma wrote:
> > On Sat, Jul 18, 2020 at 08:06:00AM +0200, Peter Krempa wrote:
> > > On Thu, Jul 16, 2020 at 20:55:29 -0400, Masayoshi Mizuma wrote:
> > > > Thank you for your review.
> > > > 
> > > > On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
> > > > > On 7/7/20 2:12 AM, Peter Krempa wrote:
> > > > > 
> > > > > > You can install a qcow2 overlay on top of a raw file too. IMO the
> > > > > > implications of using  allow that.
> > > > > > 
> > > > > > As said above I'd strongly prefer if the overlay is created in qemu
> > > > > > using the blockdev-create blockjob (there is already infrastructure 
> > > > > > in
> > > > > > libvirt to achieve that).
> > > > > 
> > > > > Agreed.  At this point, any time we call out to qemu-img as a separate
> > > > > process, we are probably doing it wrong.
> > > > 
> > > > Got it. I'm thinking about the procedure such as followings.
> > > > Does that make sense?
> > > > 
> > > >   1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(), 
> > > >  and connect it.
> > > 
> > > Starting a new qemu process just to format an image is extreme overkill
> > > and definitely not what we want to do.
> > 
> > I see, thanks.
> > 
> > > 
> > > >   2) Setup the transient disk with 
> > > > qemuDomainPrepareStorageSourceBlockdev(),
> > > >  qemuBlockStorageSourceAttachApplyStorage(), 
> > > > qemuBlockStorageSourceCreateGetFormatProps()
> > > >  and something...
> > > > 
> > > >   3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then
> > > >  close the monitor.
> > > 
> > > These two steps should be exectued in the qemu process which already
> > > will run the VM prior to starting the guest CPUs.
> > > 
> > > >   4) Switch the original disk to the transient disk.
> > > > 
> > > >   5) Build the blockdev argument for qemu.
> > > 
> > > And instead of this step, you use the external snapshot infrastructure
> > > to install the overlays via 'blockdev-snapshot' QMP command
> > 
> > OK. I suppose qemuDomainSnapshotDiskPrepare() and
> > qemuDomainSnapshotDiskUpdateSource() maybe helpful to implement the
> > setup steps of transient disk.
> > 
> > > 
> > > > 
> > > >   6) Run qemu
> > > 
> > > And instead of this the VM cpus will be started.
> > 
> > Got it, I think the appropriate place is after virCommandRun() at 
> > qemuProcessLaunch(),
> > and before qemuProcessFinishStartup().
> > 
> > > 
> > > 
> > > The above steps require factoring out snapshot code a bit. I have a few
> > > patches in that direction so I'll try posting them next week hopefully.
> 
> Sorry this took longer than expected, but we were dealing with the build
> system change.
> 
> The refactor is here:
> 
> https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html
> 
> You can now create an equivalent of 'qemuSnapshotDiskPrepare' which will
> go through the disks and find the 'transient' ones. It will then create
> snapshot data by a call to 'qemuSnapshotDiskPrepareOne' with a faked
> snapshot disk object.
> 
> 'qemuSnapshotDiskPrepareOne' ensures that the files are created and
> formatted properly.
> 
> You then use same algorithm as 'qemuSnapshotCreateDiskActive'
> (e.g. by extracting the common internals (basically everything except
> call to 'qemuSnapshotDiskPrepare') into a separate function) and reuse
> it when starting the VM as you've described above.
> 
> Note that all of the above can work only when QEMU_CAPS_BLOCKDEV is
> supported.
> 
> The caveats/limitations with blockjobs and snapshots still apply though.
> It depends on how you approach it. It's okay to limit/block the features
> if transient disk is used. Alternatively you can implement some form of
> private data to mark which image was transient and allow those
> operations.

Thank you for the helpful advice and the patches!
I'll try to implement the transient disk procedure with the refactor.

Thanks!
Masa

Re: Is the default discard mode to ignore?

[Martin Kletzander]

On Thu, Aug 06, 2020 at 01:35:45PM -0600, Chris Murphy wrote:

virt-manager 'Discard Mode" defaults to "Hypervisor default" but I'm
not sure if there's ever a case where this could translate into
on/unmap?

What problems could result if the guest OS file system mount option
were to default to 'discard'? Is it reasonable to expect discards will
be inhibited by default because of Discard Mode's default setting?

What I'm concerned about is the use case of either raw or qcow2
backing files that have been fully preallocated. If discards pass
through to the backing file, they become sparse files, which may be
suboptimal due to ensuing fragmentation. Of course it's a trade off,
you also gain efficient storage of the backing file - it consumes only
what the guest uses. But I think the more compatible behavior is to
not do discards out of the box. Someone who wants sparse backing
files, I expect, will (a) create sparse backing files and (b) set
discard mode to unmap.



And similarly, who wants fully allocated files will fully allocate them and set
the mode to `ignore`.

What we mean by "hypervisor default" is that we let the underlying hypervisor
(in this case only QEMU) decide.  The reason for that is that we could not pass
any option to the hypervisor before we introduced this option in libvirt and XML
definitions from that time must keep working the same way they did before due to
our back-compatibility promise. Sometimes we can fill in the default from how it
worked before, but most of the time it is more work, mostly because you need to
prove that the option you selected was always the case etc.

HTH,
Martin


Background and genesis of all of this:
RFE: kickstart option to control discard configuration
https://bugzilla.redhat.com/show_bug.cgi?id=1860720


Thanks,


--
Chris Murphy



signature.asc
Description: PGP signature

Is the default discard mode to ignore?

[Chris Murphy]

virt-manager 'Discard Mode" defaults to "Hypervisor default" but I'm
not sure if there's ever a case where this could translate into
on/unmap?

What problems could result if the guest OS file system mount option
were to default to 'discard'? Is it reasonable to expect discards will
be inhibited by default because of Discard Mode's default setting?

What I'm concerned about is the use case of either raw or qcow2
backing files that have been fully preallocated. If discards pass
through to the backing file, they become sparse files, which may be
suboptimal due to ensuing fragmentation. Of course it's a trade off,
you also gain efficient storage of the backing file - it consumes only
what the guest uses. But I think the more compatible behavior is to
not do discards out of the box. Someone who wants sparse backing
files, I expect, will (a) create sparse backing files and (b) set
discard mode to unmap.

Background and genesis of all of this:
RFE: kickstart option to control discard configuration
https://bugzilla.redhat.com/show_bug.cgi?id=1860720


Thanks,


-- 
Chris Murphy

[PATCH 1/2] testutilsqemuschema: Add template checker for schema entries

[Peter Krempa]

We'll need to match that a certain part of the qemu schema hasn't grown
new properties unexpectedly. Add a helper which matches an 'object' QMP
schema entry against a template and reports errors if expected types
don't match or new entries are added.

Signed-off-by: Peter Krempa 
---
 tests/testutilsqemuschema.c | 115 
 tests/testutilsqemuschema.h |   5 ++
 2 files changed, 120 insertions(+)

diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c
index d9bc00903d..21a58cb4c6 100644
--- a/tests/testutilsqemuschema.c
+++ b/tests/testutilsqemuschema.c
@@ -607,6 +607,121 @@ testQEMUSchemaValidateCommand(const char *command,
 }


+/**
+ * testQEMUSchemaEntryMatchTemplate:
+ *
+ * @schemaentry: a JSON object representing a 'object' node in the QAPI schema
+ * ...: a NULL terminated list of strings representing the template of 
properties
+ *  which the QMP object needs to have.
+ *
+ *  The strings have following format:
+ *
+ *  "type:name"
+ *  "?type:name"
+ *
+ *  "type" corresponds to the 'type' property of the member to check (str, 
bool, any ...)
+ *  "name" corresponds to the name of the member to check
+ *
+ *  If the query string starts with an '?' and member 'name' may be 
missing.
+ *
+ * This function matches that @schemaentry has all expected members and the
+ * members have expected types. @schemaentry also must not have any unknown
+ * members.
+ */
+int
+testQEMUSchemaEntryMatchTemplate(virJSONValuePtr schemaentry,
+ ...)
+{
+g_autoptr(virJSONValue) members = NULL;
+va_list ap;
+const char *next;
+int ret = -1;
+
+if (STRNEQ_NULLABLE(virJSONValueObjectGetString(schemaentry, "meta-type"), 
"object")) {
+VIR_TEST_VERBOSE("schemaentry is not an object");
+return -1;
+}
+
+if (!(members = virJSONValueCopy(virJSONValueObjectGetArray(schemaentry, 
"members" {
+VIR_TEST_VERBOSE("failed to copy 'members'");
+return -1;
+}
+
+va_start(ap, schemaentry);
+
+/* pass 1 */
+
+while ((next = va_arg(ap, const char *))) {
+char modifier = *next;
+g_autofree char *type = NULL;
+char *name;
+size_t i;
+bool found = false;
+bool optional = false;
+
+if (!g_ascii_isalpha(modifier))
+next++;
+
+if (modifier == '?')
+optional = true;
+
+type = g_strdup(next);
+
+if ((name = strchr(type, ':'))) {
+*(name++) = '\0';
+} else {
+VIR_TEST_VERBOSE("malformed template string '%s'", next);
+goto cleanup;
+}
+
+for (i = 0; i < virJSONValueArraySize(members); i++) {
+virJSONValuePtr member = virJSONValueArrayGet(members, i);
+const char *membername = virJSONValueObjectGetString(member, 
"name");
+const char *membertype = virJSONValueObjectGetString(member, 
"type");
+
+if (STRNEQ_NULLABLE(name, membername))
+continue;
+
+if (STRNEQ_NULLABLE(membertype, type)) {
+VIR_TEST_VERBOSE("member '%s' is of unexpected type '%s' 
(expected '%s')",
+ NULLSTR(membername), NULLSTR(membertype), 
type);
+goto cleanup;
+}
+
+found = true;
+break;
+}
+
+if (found) {
+virJSONValueFree(virJSONValueArraySteal(members, i));
+} else {
+if (!optional) {
+VIR_TEST_VERBOSE("mandatory member '%s' not found", name);
+goto cleanup;
+}
+}
+}
+
+/* pass 2 - check any unexpected members */
+if (virJSONValueArraySize(members) > 0) {
+size_t i;
+
+for (i = 0; i < virJSONValueArraySize(members); i++) {
+VIR_TEST_VERBOSE("unexpected member '%s'",
+ 
NULLSTR(virJSONValueObjectGetString(virJSONValueArrayGet(members, i), "name")));
+}
+
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+va_end(ap);
+return ret;
+}
+
+
 static virJSONValuePtr
 testQEMUSchemaLoadReplies(const char *filename)
 {
diff --git a/tests/testutilsqemuschema.h b/tests/testutilsqemuschema.h
index c90a6b626d..1649ad78b5 100644
--- a/tests/testutilsqemuschema.h
+++ b/tests/testutilsqemuschema.h
@@ -37,6 +37,11 @@ testQEMUSchemaValidateCommand(const char *command,
   bool allowRemoved,
   virBufferPtr debug);

+int
+testQEMUSchemaEntryMatchTemplate(virJSONValuePtr schemaentry,
+ ...);
+
+
 virJSONValuePtr
 testQEMUSchemaGetLatest(const char* arch);

-- 
2.26.2

[PATCH 2/2] qemumonitorjsontest: Add a last-resort warning if object-add/device_add are QAPIfied

[Peter Krempa]

When netdev-add was qapified it took us by surprise and we had to
scramble to fix the internals to format conformant monitor arguments.

Add a last-resort early warning system if this happens to object-add or
device_add. Hopefully qemu developers notify us sooner than this.

Signed-off-by: Peter Krempa 
---
 tests/qemumonitorjsontest.c | 53 +
 1 file changed, 53 insertions(+)

diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 77e70c1dc4..bc25958e70 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2888,6 +2888,55 @@ testQAPISchemaValidate(const void *opaque)
 }


+/**
+ * testQAPISchemaObjectDeviceAdd:
+ *
+ * Purpose of this test is to add a last-resort notification that 'object-add'
+ * and 'device_add' are not covered by the QMP schema by surprise. Ideally QEMU
+ * developers will notify us before they switch so we have time to adapt our
+ * generators first. This didn't work out when netdev-add was converted.
+ *
+ * We validate that the QMP schema describes only the expected types and 
nothing
+ * else assuming that no new field will be added until final conversion.
+ */
+static int
+testQAPISchemaObjectDeviceAdd(const void *opaque)
+{
+virHashTablePtr schema = (virHashTablePtr) opaque;
+virJSONValuePtr entry;
+
+if (virQEMUQAPISchemaPathGet("device_add/arg-type", schema, ) < 0) {
+fprintf(stderr, "schema for 'device_add' not found\n");
+return -1;
+}
+
+if (testQEMUSchemaEntryMatchTemplate(entry,
+ "str:driver",
+ "str:bus",
+ "str:id",
+ NULL) < 0) {
+VIR_TEST_VERBOSE("device_add has unexpected members in schema");
+return -1;
+}
+
+if (virQEMUQAPISchemaPathGet("object-add/arg-type", schema, ) < 0) {
+fprintf(stderr, "schema for 'objectadd' not found\n");
+return -1;
+}
+
+if (testQEMUSchemaEntryMatchTemplate(entry,
+ "str:qom-type",
+ "str:id",
+ "any:props",
+ NULL) < 0) {
+VIR_TEST_VERBOSE("object-add has unexpected members in schema");
+return -1;
+}
+
+return 0;
+}
+
+
 static void
 testQueryJobsPrintJob(virBufferPtr buf,
   qemuMonitorJobInfoPtr job)
@@ -3380,6 +3429,10 @@ mymain(void)

 #undef DO_TEST_QAPI_VALIDATE

+if (virTestRun("validate that object-add and device_add don't have schema",
+   testQAPISchemaObjectDeviceAdd, qapiData.schema) < 0)
+ret = -1;
+
 #define DO_TEST_QUERY_JOBS(name) \
 do { \
 struct testQueryJobsData data = { name, driver.xmlopt}; \
-- 
2.26.2

[PATCH 0/2] qemu: Add a last-resort warning if object-add/device_add are QAPIfied

[Peter Krempa]

See patch 2/2.

Peter Krempa (2):
  testutilsqemuschema: Add template checker for schema entries
  qemumonitorjsontest: Add a last-resort warning if
object-add/device_add are QAPIfied

 tests/qemumonitorjsontest.c |  53 +
 tests/testutilsqemuschema.c | 115 
 tests/testutilsqemuschema.h |   5 ++
 3 files changed, 173 insertions(+)

-- 
2.26.2

Re: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies

[Andrea Bolognani]

On Thu, 2020-08-06 at 15:45 +0200, Marc Hartmayer wrote:
> On Tue, Aug 04, 2020 at 11:39 PM +0200, Andrea Bolognani 
>  wrote:
> > This patch broke libvirt in Debian for certain setups.
> > 
> > With AppArmor enabled (the default), the error is
> > 
> >   $ virsh start cirros
> >   error: Failed to start domain cirros
> >   error: internal error: Process exited prior to exec: libvirt:
> >   error : Cannot delete directory '/run/libvirt/qemu/1-cirros.dev':
> >   Device or resource busy
> > 
> > If I disable AppArmor by passing security='' on the kernel command
> > line, the error message changes to
> > 
> >   $ virsh start cirros
> >   error: Failed to start domain cirros
> >   error: internal error: Process exited prior to exec: libvirt:
> >   QEMU Driver error : Unable to get devmapper targets for
> >   /var/lib/libvirt/images/cirros.qcow2: Success
> > 
> > An effective workaround is to set namespaces=[] in qemu.conf, but
> > that's of course not something that we want users doing :)
> > 
> > The underlying issue seems to be caused by the fact that, on a Debian
> > installation that uses plain partitions instead of LVM, /proc/devices
> > doesn't contain an entry for device-mapper right after boot, which...
> > ... this code expects.
> > 
> > Running
> > 
> >   $ sudo dmsetup info
> >   No devices found
> 
> We see the same problem as mentioned by Andrea. The host kernel
> configuration used:
> 
> …
> CONFIG_BLK_DEV_DM_BUILTIN=y
> CONFIG_BLK_DEV_DM=m
> …
> 
> As soon as we load the kernel module ‘dm-mod‘ everything works because
> then ‘device-mapper‘ is listed in /proc/devices.

Thanks Marc! I have confirmed that Debian also uses the same kernel
configuration as the one you have reported above, and that running
dmsetup(8) causes the dm-mod kernel module to be loaded.

For comparison Fedora, where everything works fine, uses

  CONFIG_BLK_DEV_DM_BUILTIN=y
  CONFIG_BLK_DEV_DM=y

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v2 00/13] resolve hangs/crashes on libvirtd shutdown

[Nikolay Shirokovskiy]

polite ping

On 23.07.2020 13:14, Nikolay Shirokovskiy wrote:
> I keep qemu VM event loop exiting synchronously but add code to avoid deadlock
> that can be caused by this approach. I guess it is worth having synchronous
> exiting of threads in this case to avoid crashes.
> 
> Patches that are already positively reviewed has appropriate 'Reviewed-by' 
> lines.
> 
> Changes from v1:
> - rename stateShutdown to state stateShutdownPrepare
> - introduce net daemon shutdown callbacks
> - make some adjustments in terms of qemu per VM's event loop thread
>   finishing
> - factor out net server shutdown facilities into distinct patch
> - increase shutdown timeout from 15s to 30s
> 
> Nikolay Shirokovskiy (13):
>   libvirt: add stateShutdownPrepare/stateShutdownWait to drivers
>   util: always initialize priority condition
>   util: add stop/drain functions to thread pool
>   rpc: don't unref service ref on socket behalf twice
>   rpc: add virNetDaemonSetShutdownCallbacks
>   rpc: add shutdown facilities to netserver
>   rpc: finish all threads before exiting main loop
>   qemu: don't shutdown event thread in monitor EOF callback
>   vireventthread: exit thread synchronously on finalize
>   qemu: avoid deadlock in qemuDomainObjStopWorker
>   qemu: implement driver's shutdown/shutdown wait methods
>   rpc: cleanup virNetDaemonClose method
>   util: remove unused virThreadPoolNew macro
> 
>  scripts/check-drivername.py   |   2 +
>  src/driver-state.h|   8 
>  src/libvirt.c |  42 
>  src/libvirt_internal.h|   2 +
>  src/libvirt_private.syms  |   4 ++
>  src/libvirt_remote.syms   |   2 +-
>  src/qemu/qemu_domain.c|  18 +--
>  src/qemu/qemu_driver.c|  32 +
>  src/qemu/qemu_process.c   |   3 --
>  src/remote/remote_daemon.c|   6 +--
>  src/rpc/virnetdaemon.c| 109 
> --
>  src/rpc/virnetdaemon.h|   8 +++-
>  src/rpc/virnetserver.c|   8 
>  src/rpc/virnetserver.h|   1 +
>  src/rpc/virnetserverservice.c |   1 -
>  src/util/vireventthread.c |   1 +
>  src/util/virthreadpool.c  |  65 +
>  src/util/virthreadpool.h  |   6 +--
>  18 files changed, 267 insertions(+), 51 deletions(-)
> 

Re: [PATCH 3/4] virdevmapper: Don't use libdevmapper to obtain dependencies

[Marc Hartmayer]

On Tue, Aug 04, 2020 at 11:39 PM +0200, Andrea Bolognani  
wrote:
> On Mon, 2020-07-27 at 10:31 +0200, Michal Privoznik wrote:
>> CVE-2020-14339
>> 
>> When building domain's private /dev in a namespace, libdevmapper
>> is consulted for getting full dependency tree of domain's disks.
>> The reason is that for a multipath devices all dependent devices
>> must be created in the namespace and allowed in CGroups.
>> 
>> However, this approach is very fragile as building of namespace
>> happens in the forked off child process, after mass close of FDs
>> and just before dropping privileges and execing QEMU. And it so
>> happens that when calling libdevmapper APIs, one of them opens
>> /dev/mapper/control and saves the FD into a global variable. The
>> FD is kept open until the lib is unlinked or dm_lib_release() is
>> called explicitly. We are doing neither.
>> 
>> However, the virDevMapperGetTargets() function is called also
>> from libvirtd (when setting up CGroups) and thus has to be thread
>> safe. Unfortunately, libdevmapper APIs are not thread safe (nor
>> async signal safe) and thus we can't use them. Reimplement what
>> libdevmapper would do using plain C (ioctl()-s, /proc/devices
>> parsing, /dev/mapper dirwalking, and so on).
>> 
>> Fixes: a30078cb832646177defd256e77c632905f1e6d0
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1858260
>> 
>> Signed-off-by: Michal Privoznik 
>> Reviewed-by: Daniel P. Berrangé 
>> ---
>>  po/POTFILES.in  |   1 +
>>  src/util/virdevmapper.c | 304 
>>  2 files changed, 219 insertions(+), 86 deletions(-)
>
> This patch broke libvirt in Debian for certain setups.
>
> With AppArmor enabled (the default), the error is
>
>   $ virsh start cirros
>   error: Failed to start domain cirros
>   error: internal error: Process exited prior to exec: libvirt:
>   error : Cannot delete directory '/run/libvirt/qemu/1-cirros.dev':
>   Device or resource busy
>
> If I disable AppArmor by passing security='' on the kernel command
> line, the error message changes to
>
>   $ virsh start cirros
>   error: Failed to start domain cirros
>   error: internal error: Process exited prior to exec: libvirt:
>   QEMU Driver error : Unable to get devmapper targets for
>   /var/lib/libvirt/images/cirros.qcow2: Success
>
> An effective workaround is to set namespaces=[] in qemu.conf, but
> that's of course not something that we want users doing :)
>
> The underlying issue seems to be caused by the fact that, on a Debian
> installation that uses plain partitions instead of LVM, /proc/devices
> doesn't contain an entry for device-mapper right after boot, which...
>
>>  static int
>>  virDevMapperOnceInit(void)
>>  {
>> -/* Ideally, we would not need this. But libdevmapper prints
>> - * error messages to stderr by default. Sad but true. */
>> -dm_log_with_errno_init(virDevMapperDummyLogger);
>> +g_autofree char *buf = NULL;
>> +VIR_AUTOSTRINGLIST lines = NULL;
>> +size_t i;
>> +
>> +if (virFileReadAll(PROC_DEVICES, BUF_SIZE, ) < 0)
>> +return -1;
>> +
>> +lines = virStringSplit(buf, "\n", 0);
>> +if (!lines)
>> +return -1;
>> +
>> +for (i = 0; lines[i]; i++) {
>> +g_autofree char *dev = NULL;
>> +unsigned int maj;
>> +
>> +if (sscanf(lines[i], "%u %ms\n", , ) == 2 &&
>> +STREQ(dev, DM_NAME)) {
>> +virDMMajor = maj;
>> +break;
>> +}
>> +}
>> +
>> +if (!lines[i]) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("Unable to find major for %s"),
>> +   DM_NAME);
>> +return -1;
>> +}
>
> ... this code expects.
>
> Running
>
>   $ sudo dmsetup info
>   No devices found

We see the same problem as mentioned by Andrea. The host kernel
configuration used:

…
CONFIG_BLK_DEV_DM_BUILTIN=y
CONFIG_BLK_DEV_DM=m
…

As soon as we load the kernel module ‘dm-mod‘ everything works because
then ‘device-mapper‘ is listed in /proc/devices.

>
> causes the entry to appear, and from that moment on guest startup
> will work as expected regardless of whether or not AppArmor is
> enabled.
>
> I hope the information above can help someone who's familiar with the
> code figure out a fix. I'll provide more if needed, just ask! I can
> also provide prebuilt .deb files for 6.6.0 that consistently trigger
> the issue when added to a bog standard Debian testing installation.
>
> -- 
> Andrea Bolognani / Red Hat / Virtualization
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] kbase: Add knowledge base for libvirt systemtap

[Daniel Henrique Barboza]

On 8/6/20 3:34 AM, Han Han wrote:

Signed-off-by: Han Han 
---
  docs/kbase.rst   |   3 ++
  docs/kbase/meson.build   |   1 +
  docs/kbase/systemtap.rst | 113 +++
  3 files changed, 117 insertions(+)
  create mode 100644 docs/kbase/systemtap.rst

diff --git a/docs/kbase.rst b/docs/kbase.rst
index 78daaa5989..d8bff5d41e 100644
--- a/docs/kbase.rst
+++ b/docs/kbase.rst
@@ -36,6 +36,9 @@ Knowledge base
Examination of the security protections used for QEMU and how they need
configuring to allow use of QEMU passthrough with host files/devices.
  
+   `Systemtap `__

+  Explanation of how to use systemtap for libvirt tracing.
+
 `Virtio-FS `__
Share a filesystem between the guest and the host
  
diff --git a/docs/kbase/meson.build b/docs/kbase/meson.build

index d7f254e163..ca032a4b9b 100644
--- a/docs/kbase/meson.build
+++ b/docs/kbase/meson.build
@@ -13,6 +13,7 @@ docs_kbase_files = [
'rpm-deployment',
's390_protected_virt',
'secureusage',
+  'systemtap',
'virtiofs',
  ]
  
diff --git a/docs/kbase/systemtap.rst b/docs/kbase/systemtap.rst

new file mode 100644
index 00..34420efbb2
--- /dev/null
+++ b/docs/kbase/systemtap.rst
@@ -0,0 +1,113 @@
+===
+Systemtap of Libvirt
+===
+
+.. contents::
+
+`Systemtap `__ is a scripting
+language and tool for dynamically probing or tracing in Linux kernel
+space or user space. This page is about the usage of systemtap
+in libvirt tracing.
+
+Preparation
+===
+
+Libvirt
+-
+
+Libvirt should be configured with the systemtap option to support libvirt
+probing events in systemtap.
+
+For libvirt before **6.7.0**, it can be configured by:
+
+::
+
+mkdir build
+cd build
+   ../configure --with-dtrace
+
+For libvirt **6.7.0** or later, configure it by the ``meson`` (seeing
+`libvirt compiling `__):
+
+::
+
+   meson build -Ddtrace=enabled
+
+For the libvirt binaries installed by the package manager like ``dnf`` or
+``apt``, if libvirt systemtap tapset ``/usr/share/systemtap/tapset/libvirt_*``
+exists, it means the libvirt enables the systemtap feature.
+
+Systemtap
+
+
+For most of linux distributions, execute ``stap-prep`` by root to prepare the
+environment for systemtap after installing the systemtap. If your distribution
+doesn't have ``stap-prep``, install the ``kernel debuginfo`` packages manually.
+
+After these above, run this test command to confirm the systemtap works well:
+
+::
+
+   stap -e 'probe oneshot{ printf("hello world\n")}'
+
+
+Tracing events
+===
+
+The libvirt systemtap tracing events are defined in tapset
+``/usr/share/systemtap/tapset/libvirt_*``. Libvirt support these type of 
tracing


s/type/types


With this nit fixed:


Reviewed-by: Daniel Henrique Barboza 




+events: ``dbus``, ``event_glib``, ``object``, ``qemu``, ``rpc``.
+
+List all tracing events in libvirt:
+
+::
+
+   grep 'probe libvirt.[a-z_0-9.]*' /usr/share/systemtap/tapset/libvirt_* 
-o|cut -f 2 -d :
+
+
+Tracing examples
+==
+
+Here is an example of the systemtap script to trace the QMP messages sent from 
libvirtd
+daemon to the qemu process.
+``qmp.stp``:
+
+::
+
+   probe begin
+   {
+ printf("Start tracing\n")
+   }
+   probe libvirt.qemu.monitor_send_msg
+   {
+ printf("QMPs: %s", msg);
+   }
+
+Then run the systemtap script attaching to the libvirtd process:
+
+::
+
+   stap qmp.stp -x `pidof libvirtd`
+
+
+To trace a libvirtd started from command line, use the option ``-c``
+
+::
+
+   stap qmp.stp -c "/usr/sbin/libvirtd"
+
+
+Then after seeing the welcome message "Start tracing" from systemtap, then 
execute a virsh
+command associated with QMP, for example ``virsh domstats``. Then get the QMP 
tracing logs
+from systemtap. For example, the result from ``virsh domstats``
+
+::
+
+   QMPs: {"execute":"query-balloon","id":"libvirt-393"}
+   QMPs: 
{"execute":"qom-get","arguments":{"path":"/machine/peripheral/balloon0","property":"guest-stats"},"id":"libvirt-394"}
+   QMPs: {"execute":"query-blockstats","id":"libvirt-395"}
+   QMPs: {"execute":"query-named-block-nodes","id":"libvirt-396"}
+   QMPs: {"execute":"query-iothreads","id":"libvirt-397"}
+
+For more examples of libvirt systemtap scripts, see the scripts in 
``/usr/share/doc/libvirt-docs/examples/systemtap``
+For more details of systemtap language, see `document of systemtap 
`__

[PATCH 1/4] docs: get rid of 'make check' references

[Daniel Henrique Barboza]

Update the remaining 'make check' references after the
switch to meson/ninja.

The reference in testsuites.html.in was kept with a note that it is
the process for Libvirt 6.6.0 and older.

Signed-off-by: Daniel Henrique Barboza 
---
 docs/advanced-tests.rst| 2 +-
 docs/api_extension.html.in | 9 -
 docs/testsuites.html.in| 3 ++-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/docs/advanced-tests.rst b/docs/advanced-tests.rst
index bc20bbf4d9..458a9105b6 100644
--- a/docs/advanced-tests.rst
+++ b/docs/advanced-tests.rst
@@ -90,7 +90,7 @@ location where the file is stored.
   $ VIR_TEST_FILE_ACCESS=1 VIR_TEST_FILE_ACCESS_OUTPUT="/tmp/file_access.txt" 
./qemuxml2argvtest
 
 #. The Valgrind test should produce similar output to
-``make check``. If the output has traces within libvirt API's,
+``ninja test``. If the output has traces within libvirt API's,
 then investigation is required in order to determine the cause
 of the issue. Output such as the following indicates some sort
 of leak:
diff --git a/docs/api_extension.html.in b/docs/api_extension.html.in
index 79e7913c1a..6c64e83314 100644
--- a/docs/api_extension.html.in
+++ b/docs/api_extension.html.in
@@ -361,11 +361,10 @@
 
 
 
-  Once you have working functionality, run make check and make
-  syntax-check on each patch of the series before submitting
-  patches.  It may also be worth writing tests for the libvirt-TCK
-  testsuite to exercise your new API, although those patches are
-  not kept in the libvirt repository.
+  Once you have working functionality, run ninja test on each patch
+  of the series before submitting patches. It may also be worth
+  writing tests for the libvirt-TCK testsuite to exercise your new API,
+  although those patches are not kept in the libvirt repository.
 
   
 
diff --git a/docs/testsuites.html.in b/docs/testsuites.html.in
index a619e6d000..cd9cad9160 100644
--- a/docs/testsuites.html.in
+++ b/docs/testsuites.html.in
@@ -10,7 +10,8 @@
   by developers before submitting patches upstream, it is also
   suggested to have it run and pass as part of the packaging
   process for distributions. It is run by launching:
-  make check
+  make check (libvirt 6.6.0 and older)
+  ninja test (libvirt 6.7.0 and newer)
   in a source tree after compilation has finished. It doesn't
   really make functional testing but checks that large portions
   of the code not interacting directly with virtualization
-- 
2.26.2

[PATCH 0/4] remove 'make check' references from code

[Daniel Henrique Barboza]

Simple series that removes the references of the now former
'make check', replacing it to 'ninja test'.


Daniel Henrique Barboza (4):
  docs: get rid of 'make check' references
  ci/Makefile: get rid of 'make check' references
  tests: get rid of 'make check' references
  vbox_XPCOMCGlue.c: get rid of 'make check' reference

 ci/Makefile  | 6 +++---
 docs/advanced-tests.rst  | 2 +-
 docs/api_extension.html.in   | 9 -
 docs/testsuites.html.in  | 3 ++-
 src/vbox/vbox_XPCOMCGlue.c   | 2 +-
 tests/domaincapstest.c   | 2 +-
 tests/qemucapabilitiestest.c | 2 +-
 tests/test-lib.sh| 2 +-
 8 files changed, 14 insertions(+), 14 deletions(-)

-- 
2.26.2

[PATCH 2/4] ci/Makefile: get rid of 'make check' references

[Daniel Henrique Barboza]

Update the remaining 'make check' references after the
switch to meson/ninja.

Signed-off-by: Daniel Henrique Barboza 
---
 ci/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ci/Makefile b/ci/Makefile
index 65d8fcca07..c7c8eb9a45 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -85,7 +85,7 @@ endif
 
 # IDs you run as do not need to exist in
 # the container's /etc/passwd & /etc/group files, but
-# if they do not, then libvirt's 'make check' will fail
+# if they do not, then libvirt's 'ninja test' will fail
 # many tests.
 
 # We do not directly mount /etc/{passwd,group} as Docker
@@ -255,8 +255,8 @@ ci-help:
@echo
@echo "Available targets:"
@echo
-   @echo "ci-build@\$$IMAGE - run a default 'make'"
-   @echo "ci-check@\$$IMAGE - run a 'make check'"
+   @echo "ci-build@\$$IMAGE - run a default 'ninja' build"
+   @echo "ci-check@\$$IMAGE - run a 'ninja test'"
@echo "ci-shell@\$$IMAGE - run an interactive shell"
@echo "ci-list-images  - list available images"
@echo "ci-help - show this help message"
-- 
2.26.2

[PATCH 3/4] tests: get rid of 'make check' references

[Daniel Henrique Barboza]

Update the remaining 'make check' references after the
switch to meson/ninja.

Signed-off-by: Daniel Henrique Barboza 
---
 tests/domaincapstest.c   | 2 +-
 tests/qemucapabilitiestest.c | 2 +-
 tests/test-lib.sh| 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index c3a9f4ef91..674594ff8b 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -448,7 +448,7 @@ mymain(void)
  * programs will automatically pick it up.
  *
  * To generate the corresponding output files after a new replies
- * file has been added, run "VIR_TEST_REGENERATE_OUTPUT=1 make check".
+ * file has been added, run "VIR_TEST_REGENERATE_OUTPUT=1 ninja test".
  */
 
 virObjectUnref(cfg);
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 82309b44bb..77fe716c20 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -225,7 +225,7 @@ mymain(void)
  * programs will automatically pick it up.
  *
  * To generate the corresponding output files after a new replies
- * file has been added, run "VIR_TEST_REGENERATE_OUTPUT=1 make check".
+ * file has been added, run "VIR_TEST_REGENERATE_OUTPUT=1 ninja test".
  */
 
 testQemuDataReset();
diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index c19005a371..cc3924c07a 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -200,7 +200,7 @@ test_expensive()
   if test "$VIR_TEST_EXPENSIVE" != 1; then
 skip_test_ '
 This test is very expensive, so it is disabled by default.
-To run it anyway, rerun: make check VIR_TEST_EXPENSIVE=1
+To run it anyway, rerun: VIR_TEST_EXPENSIVE=1 ninja test
 '
   fi
 }
-- 
2.26.2

[PATCH 4/4] vbox_XPCOMCGlue.c: get rid of 'make check' reference

[Daniel Henrique Barboza]

Change the 'make check' reference after the switch to meson/ninja.

Signed-off-by: Daniel Henrique Barboza 
---
 src/vbox/vbox_XPCOMCGlue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vbox/vbox_XPCOMCGlue.c b/src/vbox/vbox_XPCOMCGlue.c
index fa5a56ede6..3cbb679110 100644
--- a/src/vbox/vbox_XPCOMCGlue.c
+++ b/src/vbox/vbox_XPCOMCGlue.c
@@ -114,7 +114,7 @@ tryLoadOne(const char *dir, bool setAppHome, bool 
ignoreMissing,
 
 if (hVBoxXPCOMC == NULL) {
 /*
- * FIXME: Don't warn in this case as it currently breaks make check
+ * FIXME: Don't warn in this case as it currently breaks ninja test
  *on systems without VirtualBox.
  */
 if (dir != NULL)
-- 
2.26.2

Re: [PATCH v3 4/5] doc: rng schemas and html doc for rbd namespace attribute

[Peter Krempa]

On Thu, Aug 06, 2020 at 19:41:45 +0800, Han Han wrote:
> Signed-off-by: Han Han 
> ---
>  docs/formatdomain.rst | 5 -
>  docs/schemas/domaincommon.rng | 3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)

I suggest you combine this with patch 2.

Re: [PATCH v3 3/5] qemu: Implement rbd namespace attribute

[Peter Krempa]

On Thu, Aug 06, 2020 at 19:41:44 +0800, Han Han wrote:
> Ceph Nautilus supports separate image namespaces within a pool for
> tenant isolation and QEMU added it as a rbd blockdev options from 5.0.0.
> This optional attribute is used to access a image with namespace.
> 
> Add unit tests for this attribute.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1816909
> 
> Signed-off-by: Han Han 
> ---
>  src/qemu/qemu_block.c |  1 +
>  src/qemu/qemu_command.c   |  7 +++-
>  ...k-network-rbd-namespace.x86_64-latest.args | 41 +++
>  .../disk-network-rbd-namespace.xml| 33 +++
>  tests/qemuxml2argvtest.c  |  1 +
>  ...sk-network-rbd-namespace.x86_64-latest.xml | 41 +++
>  tests/qemuxml2xmltest.c   |  1 +
>  7 files changed, 124 insertions(+), 1 deletion(-)
>  create mode 100644 
> tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/disk-network-rbd-namespace.x86_64-latest.xml

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 01812cd39b..b6288d1308 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -980,7 +980,12 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
>  return NULL;
>  }
>  
> -virBufferStrcat(, "rbd:", src->volume, "/", src->path, NULL);
> +virBufferStrcat(, "rbd:", src->volume, "/", NULL);
> +/* The filename of image with namespace: 
> rbd:POOL/NAMESPACE/IMAGE... */
> +if (src->namespace)
> +virBufferStrcat(, src->namespace, "/", NULL);
> +
> +virBufferStrcat(, src->path, NULL);
>  

This is dead code now. qemu-5.0 will be started with -blockdev only.


>  if (src->snapshot)
>  virBufferEscape(, '\\', ":", "@%s", src->snapshot);

[...]

> diff --git a/tests/qemuxml2argvdata/disk-network-rbd-namespace.xml 
> b/tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
> new file mode 100644
> index 00..8b526c4a20
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
> @@ -0,0 +1,33 @@
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219136
> +  219136
> +  1
> +  
> +hvm
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/bin/qemu-system-x86_64
> +
> +  
> +  
> +
> +
> +
> +  
> +  
> +
> +
> +
> +
> +
> +
> +
> +  
> +

virschematest fails after adding this file in this patch. You must add
the RNG schema prior to any file using it as we invoke the schema test
on all example files.

Re: [PATCH v3 2/5] conf: Support to parse rbd namespace attribute

[Peter Krempa]

On Thu, Aug 06, 2020 at 19:41:43 +0800, Han Han wrote:
> Signed-off-by: Han Han 
> ---
>  src/conf/domain_conf.c| 4 
>  src/util/virstoragefile.h | 1 +
>  2 files changed, 5 insertions(+)

[...]

> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index f73b3ee005..0364fe5e7e 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -284,6 +284,7 @@ struct _virStorageSource {
>  char *snapshot; /* for storage systems supporting internal snapshots */
>  char *configFile; /* some storage systems use config file as part of
>   the source definition */
> +char *namespace; /* for the storage systems supporting namespace */

'namespace' is a c++ keyword. Consider using a different identifier.

Re: [PATCH v3 1/5] qemu_capabilities: Add QEMU_CAPS_RBD_NAMESPACE

[Peter Krempa]

On Thu, Aug 06, 2020 at 19:41:42 +0800, Han Han wrote:
> Add rbd namespace in aarch64 capability replies.
> 
> The capability flag will be used for rbd namespace option. The rbd namespace
> is introduced since ceph Nautilus and qemu v5.0.0.
> 
> Signed-off-by: Han Han 
> ---

[...]

> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 5d08941538..ef7ea9aa20 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -578,6 +578,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
> syntax-check */
>  QEMU_CAPS_NUMA_HMAT, /* -numa hmat */
>  QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI, /* -blockdev used for (i)SCSI hostdevs 
> */
>  
> +/* 380 */
> +QEMU_CAPS_RBD_NAMESPACE, /* -blockdev 
> '{"driver":"rbd",...,"namespace":str}' */

The series doesn't actually use this capability.

[PATCH v3 2/5] conf: Support to parse rbd namespace attribute

[Han Han]

Signed-off-by: Han Han 
---
 src/conf/domain_conf.c| 4 
 src/util/virstoragefile.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ef67efa1da..163c934428 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9601,6 +9601,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
 
 /* config file currently only works with remote disks */
 src->configFile = virXPathString("string(./config/@file)", ctxt);
+src->namespace = virXPathString("string(./@namespace)", ctxt);
 
 if (src->protocol == VIR_STORAGE_NET_PROTOCOL_HTTP ||
 src->protocol == VIR_STORAGE_NET_PROTOCOL_HTTPS)
@@ -25083,6 +25084,9 @@ virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf,
 path = g_strdup_printf("%s/%s", src->volume, src->path);
 
 virBufferEscapeString(attrBuf, " name='%s'", path ? path : src->path);
+if (src->namespace)
+virBufferEscapeString(attrBuf, " namespace='%s'", src->namespace);
+
 virBufferEscapeString(attrBuf, " query='%s'", src->query);
 
 if (src->haveTLS != VIR_TRISTATE_BOOL_ABSENT &&
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index f73b3ee005..0364fe5e7e 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -284,6 +284,7 @@ struct _virStorageSource {
 char *snapshot; /* for storage systems supporting internal snapshots */
 char *configFile; /* some storage systems use config file as part of
  the source definition */
+char *namespace; /* for the storage systems supporting namespace */
 char *query; /* query string for HTTP based protocols */
 size_t nhosts;
 virStorageNetHostDefPtr hosts;
-- 
2.27.0

[PATCH v3 1/5] qemu_capabilities: Add QEMU_CAPS_RBD_NAMESPACE

[Han Han]

Add rbd namespace in aarch64 capability replies.

The capability flag will be used for rbd namespace option. The rbd namespace
is introduced since ceph Nautilus and qemu v5.0.0.

Signed-off-by: Han Han 
---
 src/qemu/qemu_capabilities.c  | 4 
 src/qemu/qemu_capabilities.h  | 3 +++
 tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml   | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 +
 tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml  | 1 +
 7 files changed, 12 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index ff6ba8c9e9..7656d5f8cb 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -597,6 +597,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
   "spapr-tpm-proxy",
   "numa.hmat",
   "blockdev-hostdev-scsi",
+
+  /* 380 */
+  "rbd.namespace",
 );
 
 
@@ -1525,6 +1528,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsQMPSchemaQueries[] = {
 { "migrate-set-parameters/arg-type/downtime-limit", 
QEMU_CAPS_MIGRATION_PARAM_DOWNTIME },
 { "migrate-set-parameters/arg-type/xbzrle-cache-size", 
QEMU_CAPS_MIGRATION_PARAM_XBZRLE_CACHE_SIZE },
 { "set-numa-node/arg-type/+hmat-lb", QEMU_CAPS_NUMA_HMAT },
+{ "blockdev-add/arg-type/+rbd/namespace", QEMU_CAPS_RBD_NAMESPACE },
 };
 
 typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 5d08941538..ef7ea9aa20 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -578,6 +578,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 QEMU_CAPS_NUMA_HMAT, /* -numa hmat */
 QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI, /* -blockdev used for (i)SCSI hostdevs */
 
+/* 380 */
+QEMU_CAPS_RBD_NAMESPACE, /* -blockdev 
'{"driver":"rbd",...,"namespace":str}' */
+
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
index 928af2a01c..75622ca5fd 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
@@ -199,6 +199,7 @@
   
   
   
+  
   500
   0
   61700241
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
index e8668a25a9..1780c8a5bc 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
@@ -208,6 +208,7 @@
   
   
   
+  
   500
   0
   42900241
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
index 85a8a46dac..d885bd9bb3 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
@@ -195,6 +195,7 @@
   
   
   
+  
   500
   0
   0
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
index 546b9b0422..e93483f0c6 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
@@ -243,6 +243,7 @@
   
   
   
+  
   500
   0
   43100241
diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
index a9d82661e3..387a11a0c6 100644
--- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
@@ -242,6 +242,7 @@
   
   
   
+  
   592
   0
   43100242
-- 
2.27.0

[PATCH v3 5/5] news: qemu: Support rbd namespace

[Han Han]

Signed-off-by: Han Han 
---
 NEWS.rst | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 63ca689b43..57d8a40731 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -13,6 +13,12 @@ v6.7.0 (unreleased)
 
 * **New features**
 
+  * qemu: Support rbd namespace attribute
+
+The namespaces is for the tenant isolation within a rbd pool, introduced
+from Ceph Nautilus, supported since QEMU 5.0.0. In libvirt, using it by
+the namespace attribute in the source element of rbd disk.
+
 * **Improvements**
 
 * **Bug fixes**
-- 
2.27.0

[PATCH v3 3/5] qemu: Implement rbd namespace attribute

[Han Han]

Ceph Nautilus supports separate image namespaces within a pool for
tenant isolation and QEMU added it as a rbd blockdev options from 5.0.0.
This optional attribute is used to access a image with namespace.

Add unit tests for this attribute.

https://bugzilla.redhat.com/show_bug.cgi?id=1816909

Signed-off-by: Han Han 
---
 src/qemu/qemu_block.c |  1 +
 src/qemu/qemu_command.c   |  7 +++-
 ...k-network-rbd-namespace.x86_64-latest.args | 41 +++
 .../disk-network-rbd-namespace.xml| 33 +++
 tests/qemuxml2argvtest.c  |  1 +
 ...sk-network-rbd-namespace.x86_64-latest.xml | 41 +++
 tests/qemuxml2xmltest.c   |  1 +
 7 files changed, 124 insertions(+), 1 deletion(-)
 create mode 100644 
tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
 create mode 100644 
tests/qemuxml2xmloutdata/disk-network-rbd-namespace.x86_64-latest.xml

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 26c1b42428..beb5639707 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -872,6 +872,7 @@ qemuBlockStorageSourceGetRBDProps(virStorageSourcePtr src,
  "s:pool", src->volume,
  "s:image", src->path,
  "S:snapshot", src->snapshot,
+ "S:namespace", src->namespace,
  "S:conf", src->configFile,
  "A:server", ,
  "S:user", username,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 01812cd39b..b6288d1308 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -980,7 +980,12 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
 return NULL;
 }
 
-virBufferStrcat(, "rbd:", src->volume, "/", src->path, NULL);
+virBufferStrcat(, "rbd:", src->volume, "/", NULL);
+/* The filename of image with namespace: 
rbd:POOL/NAMESPACE/IMAGE... */
+if (src->namespace)
+virBufferStrcat(, src->namespace, "/", NULL);
+
+virBufferStrcat(, src->path, NULL);
 
 if (src->snapshot)
 virBufferEscape(, '\\', ":", "@%s", src->snapshot);
diff --git 
a/tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args 
b/tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
new file mode 100644
index 00..a744805d74
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
@@ -0,0 +1,41 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object secret,id=masterKey0,format=raw,\
+file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off \
+-cpu qemu64 \
+-m 214 \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot strict=on \
+-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
+-blockdev '{"driver":"rbd","pool":"pool","image":"image","namespace":"ns",\
+"server":[{"host":"mon1.example.org","port":"6321"},{"host":"mon2.example.org",\
+"port":"6322"},{"host":"mon3.example.org","port":"6322"}],\
+"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
+-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\
+"file":"libvirt-1-storage"}' \
+-device virtio-blk-pci,bus=pci.0,addr=0x2,drive=libvirt-1-format,\
+id=virtio-disk0,bootindex=1 \
+-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
+resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/disk-network-rbd-namespace.xml 
b/tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
new file mode 100644
index 00..8b526c4a20
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
@@ -0,0 +1,33 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-x86_64
+
+  
+  
+
+
+
+  
+  
+
+
+
+
+
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 01839cb88c..c6c18c4335 100644
--- a/tests/qemuxml2argvtest.c
+++ 

[PATCH v3 4/5] doc: rng schemas and html doc for rbd namespace attribute

[Han Han]

Signed-off-by: Han Han 
---
 docs/formatdomain.rst | 5 -
 docs/schemas/domaincommon.rng | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 218f0c1718..431abc6f56 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -2256,7 +2256,7 @@ paravirtualized driver is specified via the ``disk`` 
element.
  
  

-   
+   
  
  
  
@@ -2497,6 +2497,9 @@ paravirtualized driver is specified via the ``disk`` 
element.
   For protocols ``http`` and ``https`` an optional attribute ``query``
   specifies the query string. ( :since:`Since 6.2.0` )
 
+  For protocol ``rbd``, an optional attribute ``namespace`` specifies the
+  namespace of a rbd pool. ( :since:`Since 6.7.0 and QEMU 5.0.0` )
+
   For "iscsi" ( :since:`since 1.0.4` ), the ``name`` attribute may include 
a
   logical unit number, separated from the target's name by a slash (e.g.,
   ``iqn.2013-07.com.example:iscsi-pool/1``). If not specified, the default
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 0d0dcbc5ce..ee09da3c7c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1762,6 +1762,9 @@
 
   
 
+
+  
+
 
   
 
-- 
2.27.0

[PATCH v3 0/5] qemu: Support rbd namespace attribute

[Han Han]

Diff from v2:
- rebase to master
- update some qemu capabilities tests

Test results of this patch series on qemu-5.0.0 and librbd1-14.2.8:
disk xml:

  
  

  
  

  
  


Live attach and detach the disk:
➜  ~ virsh attach-device new /tmp/ceph-ns.xml
Device attached successfully

➜  ~ virsh dumpxml new|xmllint --xpath //disk -

  
  
  
  
  WD-WMAP9A966149
  
  

  
  

  
  

  
  
  
  


➜  ~ virsh detach-device new /tmp/ceph-ns.xml
Device detached successfully

➜  ~ virsh dumpxml new|xmllint --xpath //disk -

  
  
  
  
  WD-WMAP9A966149
  
  



It also works when start VM with this disk.
And tests pass when it is tested with -blockdev disabled:

   [...]
  


  



v2: https://www.redhat.com/archives/libvir-list/2020-April/msg00972.html


Han Han (5):
  qemu_capabilities: Add QEMU_CAPS_RBD_NAMESPACE
  conf: Support to parse rbd namespace attribute
  qemu: Implement rbd namespace attribute
  doc: rng schemas and html doc for rbd namespace attribute
  news: qemu: Support rbd namespace

 NEWS.rst  |  6 +++
 docs/formatdomain.rst |  5 ++-
 docs/schemas/domaincommon.rng |  3 ++
 src/conf/domain_conf.c|  4 ++
 src/qemu/qemu_block.c |  1 +
 src/qemu/qemu_capabilities.c  |  4 ++
 src/qemu/qemu_capabilities.h  |  3 ++
 src/qemu/qemu_command.c   |  7 +++-
 src/util/virstoragefile.h |  1 +
 .../caps_5.0.0.aarch64.xml|  1 +
 .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml |  1 +
 .../caps_5.0.0.riscv64.xml|  1 +
 .../caps_5.0.0.x86_64.xml |  1 +
 .../caps_5.1.0.x86_64.xml |  1 +
 ...k-network-rbd-namespace.x86_64-latest.args | 41 +++
 .../disk-network-rbd-namespace.xml| 33 +++
 tests/qemuxml2argvtest.c  |  1 +
 ...sk-network-rbd-namespace.x86_64-latest.xml | 41 +++
 tests/qemuxml2xmltest.c   |  1 +
 19 files changed, 154 insertions(+), 2 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/disk-network-rbd-namespace.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/disk-network-rbd-namespace.xml
 create mode 100644 
tests/qemuxml2xmloutdata/disk-network-rbd-namespace.x86_64-latest.xml

-- 
2.27.0

[libvirt PATCH v3 03/10] remote: split off enums into separate source file

[Daniel P . Berrangé]

The remoteDriverTransport and remoteDriverMode enums are going to be
needed by source files beyond the remote driver client.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/meson.build  |  1 +
 src/remote/remote_driver.c  | 41 +-
 src/remote/remote_sockets.c | 39 +
 src/remote/remote_sockets.h | 50 +
 4 files changed, 91 insertions(+), 40 deletions(-)
 create mode 100644 src/remote/remote_sockets.c
 create mode 100644 src/remote/remote_sockets.h

diff --git a/src/remote/meson.build b/src/remote/meson.build
index 5983238a0a..91dd587cba 100644
--- a/src/remote/meson.build
+++ b/src/remote/meson.build
@@ -1,5 +1,6 @@
 remote_driver_sources = [
   'remote_driver.c',
+  'remote_sockets.c',
 ]
 
 remote_driver_generated = []
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index f511a9bc33..b214632bdf 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -38,6 +38,7 @@
 #include "virbuffer.h"
 #include "remote_driver.h"
 #include "remote_protocol.h"
+#include "remote_sockets.h"
 #include "lxc_protocol.h"
 #include "qemu_protocol.h"
 #include "viralloc.h"
@@ -54,46 +55,6 @@
 
 VIR_LOG_INIT("remote.remote_driver");
 
-typedef enum {
-REMOTE_DRIVER_TRANSPORT_TLS,
-REMOTE_DRIVER_TRANSPORT_UNIX,
-REMOTE_DRIVER_TRANSPORT_SSH,
-REMOTE_DRIVER_TRANSPORT_LIBSSH2,
-REMOTE_DRIVER_TRANSPORT_EXT,
-REMOTE_DRIVER_TRANSPORT_TCP,
-REMOTE_DRIVER_TRANSPORT_LIBSSH,
-
-REMOTE_DRIVER_TRANSPORT_LAST,
-} remoteDriverTransport;
-
-VIR_ENUM_DECL(remoteDriverTransport);
-VIR_ENUM_IMPL(remoteDriverTransport,
-  REMOTE_DRIVER_TRANSPORT_LAST,
-  "tls",
-  "unix",
-  "ssh",
-  "libssh2",
-  "ext",
-  "tcp",
-  "libssh");
-
-typedef enum {
-/* Try to figure out the "best" choice magically */
-REMOTE_DRIVER_MODE_AUTO,
-/* Always use the legacy libvirtd */
-REMOTE_DRIVER_MODE_LEGACY,
-/* Always use the per-driver virt*d daemons */
-REMOTE_DRIVER_MODE_DIRECT,
-
-REMOTE_DRIVER_MODE_LAST
-} remoteDriverMode;
-
-VIR_ENUM_DECL(remoteDriverMode);
-VIR_ENUM_IMPL(remoteDriverMode,
-  REMOTE_DRIVER_MODE_LAST,
-  "auto",
-  "legacy",
-  "direct");
 
 #if SIZEOF_LONG < 8
 # define HYPER_TO_TYPE(_type, _to, _from) \
diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
new file mode 100644
index 00..0662cbad14
--- /dev/null
+++ b/src/remote/remote_sockets.c
@@ -0,0 +1,39 @@
+/*
+ * remote_sockets.c: helpers for getting remote driver socket paths
+ *
+ * Copyright (C) 2007-2019 Red Hat, Inc.
+ *
+ * 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
+ * .
+ */
+
+#include 
+
+#include "remote_sockets.h"
+
+VIR_ENUM_IMPL(remoteDriverTransport,
+  REMOTE_DRIVER_TRANSPORT_LAST,
+  "tls",
+  "unix",
+  "ssh",
+  "libssh2",
+  "ext",
+  "tcp",
+  "libssh");
+
+VIR_ENUM_IMPL(remoteDriverMode,
+  REMOTE_DRIVER_MODE_LAST,
+  "auto",
+  "legacy",
+  "direct");
diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h
new file mode 100644
index 00..1d4ae3f9c1
--- /dev/null
+++ b/src/remote/remote_sockets.h
@@ -0,0 +1,50 @@
+/*
+ * remote_sockets.h: helpers for getting remote driver socket paths
+ *
+ * Copyright (C) 2007-2020 Red Hat, Inc.
+ *
+ * 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
+ * .
+ */
+
+#pragma once
+
+#include "virenum.h"
+
+typedef enum {
+REMOTE_DRIVER_TRANSPORT_TLS,
+

[libvirt PATCH v3 09/10] rpc: switch order of args in virNetClientNewSSH

[Daniel P . Berrangé]

Switch keyfile and netcat parameters, since the netcat path and
socket path are a logical pair that belong together. This patches
the other constructors.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c | 2 +-
 src/rpc/virnetclient.c | 2 +-
 src/rpc/virnetclient.h | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index df10cfaef6..d56f4b7260 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1010,8 +1010,8 @@ doRemoteOpen(virConnectPtr conn,
 username,
 !tty,
 !verify,
-netcat,
 keyfile,
+netcat,
 sockname)))
 goto failed;
 
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index f65cda0d7f..72ece28f59 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -444,8 +444,8 @@ virNetClientPtr virNetClientNewSSH(const char *nodename,
const char *username,
bool noTTY,
bool noVerify,
-   const char *netcatPath,
const char *keyfile,
+   const char *netcatPath,
const char *socketPath)
 {
 virNetSocketPtr sock;
diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h
index 0005de46f3..6fdc370083 100644
--- a/src/rpc/virnetclient.h
+++ b/src/rpc/virnetclient.h
@@ -48,9 +48,9 @@ virNetClientPtr virNetClientNewSSH(const char *nodename,
const char *username,
bool noTTY,
bool noVerify,
-   const char *netcat,
const char *keyfile,
-   const char *path);
+   const char *netcat,
+   const char *socketPath);
 
 virNetClientPtr virNetClientNewLibSSH2(const char *host,
const char *port,
-- 
2.26.2

[libvirt PATCH v3 05/10] remote: parse the remote transport string earlier

[Daniel P . Berrangé]

We delay converting the remote transport string to enum form until
fairly late. As a result we're doing string comparisons when we
could be just doing enum comparisons.

Signed-off-by: Daniel P. Berrangé 
---
 po/POTFILES.in  |  1 +
 src/remote/remote_driver.c  | 51 ++---
 src/remote/remote_sockets.c | 35 +
 src/remote/remote_sockets.h |  2 +-
 4 files changed, 45 insertions(+), 44 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index c5b43df7b5..c4197604ef 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -181,6 +181,7 @@
 @SRCDIR@src/remote/remote_daemon_dispatch.c
 @SRCDIR@src/remote/remote_daemon_stream.c
 @SRCDIR@src/remote/remote_driver.c
+@SRCDIR@src/remote/remote_sockets.c
 @SRCDIR@src/rpc/virkeepalive.c
 @SRCDIR@src/rpc/virnetclient.c
 @SRCDIR@src/rpc/virnetclientprogram.c
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 8952484b8e..66d66c1284 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -863,12 +863,11 @@ static int
 doRemoteOpen(virConnectPtr conn,
  struct private_data *priv,
  const char *driver_str,
- const char *transport_str,
+ remoteDriverTransport transport,
  virConnectAuthPtr auth G_GNUC_UNUSED,
  virConfPtr conf,
  unsigned int flags)
 {
-int transport;
 #ifndef WIN32
 g_autofree char *daemonPath = NULL;
 #endif
@@ -903,34 +902,6 @@ doRemoteOpen(virConnectPtr conn,
 /* We handle *ALL* URIs here. The caller has rejected any
  * URIs we don't care about */
 
-if (conn->uri) {
-if (!transport_str) {
-if (conn->uri->server)
-transport = REMOTE_DRIVER_TRANSPORT_TLS;
-else
-transport = REMOTE_DRIVER_TRANSPORT_UNIX;
-} else {
-if ((transport = 
remoteDriverTransportTypeFromString(transport_str)) < 0) {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
-   _("remote_open: transport in URL not recognised 
"
- "(should be 
tls|unix|ssh|ext|tcp|libssh2|libssh)"));
-return VIR_DRV_OPEN_ERROR;
-}
-
-if (transport == REMOTE_DRIVER_TRANSPORT_UNIX &&
-conn->uri->server) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("using unix socket and remote "
- "server '%s' is not supported."),
-   conn->uri->server);
-return VIR_DRV_OPEN_ERROR;
-}
-}
-} else {
-/* No URI, then must be probing so use UNIX socket */
-transport = REMOTE_DRIVER_TRANSPORT_UNIX;
-}
-
 /* Remote server defaults to "localhost" if not specified. */
 if (conn->uri && conn->uri->port != 0) {
 port = g_strdup_printf("%d", conn->uri->port);
@@ -1352,11 +1323,16 @@ remoteConnectOpen(virConnectPtr conn,
 int rflags = 0;
 const char *autostart = getenv("LIBVIRT_AUTOSTART");
 char *driver = NULL;
-char *transport = NULL;
+remoteDriverTransport transport;
+
+if (conn->uri) {
+if (remoteSplitURIScheme(conn->uri, , ) < 0)
+goto cleanup;
+} else {
+/* No URI, then must be probing so use UNIX socket */
+transport = REMOTE_DRIVER_TRANSPORT_UNIX;
+}
 
-if (conn->uri &&
-remoteSplitURIScheme(conn->uri, , ) < 0)
-goto cleanup;
 
 if (inside_daemon) {
 if (!conn->uri) {
@@ -1398,12 +1374,12 @@ remoteConnectOpen(virConnectPtr conn,
 rflags |= VIR_DRV_OPEN_REMOTE_USER;
 
 /*
- * Furthermore if no servername is given, and no +XXX
- * transport is listed, or transport is unix,
+ * Furthermore if no servername is given,
+ * and the transport is unix,
  * and uid is unprivileged then auto-spawn a daemon.
  */
 if (!conn->uri->server &&
-(transport == NULL || STREQ(transport, "unix")) &&
+(transport == REMOTE_DRIVER_TRANSPORT_UNIX) &&
 (!autostart ||
  STRNEQ(autostart, "0"))) {
 VIR_DEBUG("Try daemon autostart");
@@ -1438,7 +1414,6 @@ remoteConnectOpen(virConnectPtr conn,
 
  cleanup:
 VIR_FREE(driver);
-VIR_FREE(transport);
 return ret;
 }
 
diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
index 976124d0ed..cdc0a00293 100644
--- a/src/remote/remote_sockets.c
+++ b/src/remote/remote_sockets.c
@@ -21,6 +21,9 @@
 #include 
 
 #include "remote_sockets.h"
+#include "virerror.h"
+
+#define VIR_FROM_THIS VIR_FROM_REMOTE
 
 VIR_ENUM_IMPL(remoteDriverTransport,
   REMOTE_DRIVER_TRANSPORT_LAST,
@@ -42,25 +45,47 @@ VIR_ENUM_IMPL(remoteDriverMode,
 int
 remoteSplitURIScheme(virURIPtr uri,
  char **driver,
- char **transport)
+ 

[libvirt PATCH v3 00/10] remote: introduce a custom netcat impl for ssh tunnelling

[Daniel P . Berrangé]

We have long had a problem with use of netcat for ssh tunnelling because
there's no guarantee the UNIX socket path the client builds will match
the UNIX socket path the remote host uses. We don't even allow session
mode SSH tunnelling for this reason. We also can't easily auto-spawn
libvirtd in session mode.

With the introduction of modular daemons we also have potential for two
completely different UNIX socket paths even for system mode, and the
client can't know which to use.

The solution to all these problems is to introduce a custom netcat impl.
Instead passing the UNIX socket path, we pass the libvirt driver URI.
The custom netcat then decides which socket path to use based on the
remote build host environment.

We still have to support netcat for interoperability with legacy libvirt
versions, but we can default to the new virt-nc.

v3: Now with more meson and less autotools !

Daniel P. Berrangé (10):
  rpc: merge logic for generating remote SSH shell script
  remote: push logic for default netcat binary into common helper
  remote: split off enums into separate source file
  remote: split out function for parsing URI scheme
  remote: parse the remote transport string earlier
  remote: split out function for constructing socket path
  remote: extract logic for determining daemon to connect to
  remote: introduce virt-ssh-helper binary
  rpc: switch order of args in virNetClientNewSSH
  rpc: use new virt-ssh-helper binary for remote tunnelling

 build-aux/syntax-check.mk  |   2 +-
 docs/uri.html.in   |  24 +-
 libvirt.spec.in|   2 +
 po/POTFILES.in |   2 +
 src/libvirt_remote.syms|   1 +
 src/remote/meson.build |  18 ++
 src/remote/remote_driver.c | 323 -
 src/remote/remote_sockets.c| 277 +
 src/remote/remote_sockets.h|  70 ++
 src/remote/remote_ssh_helper.c | 425 +
 src/rpc/virnetclient.c | 170 -
 src/rpc/virnetclient.h |  29 ++-
 src/rpc/virnetsocket.c |  37 +--
 src/rpc/virnetsocket.h |   4 +-
 tests/virnetsockettest.c   |  12 +-
 15 files changed, 1024 insertions(+), 372 deletions(-)
 create mode 100644 src/remote/remote_sockets.c
 create mode 100644 src/remote/remote_sockets.h
 create mode 100644 src/remote/remote_ssh_helper.c

-- 
2.26.2

[libvirt PATCH v3 08/10] remote: introduce virt-ssh-helper binary

[Daniel P . Berrangé]

When accessing libvirtd over a SSH tunnel, the remote driver needs a way
to proxy the SSH input/output stream to a suitable libvirt daemon. Tihs
is currently done by spawning netcat, pointing it to the libvirtd socket
path. This is problematic for a number of reasons:

 - The socket path varies according to the --prefix chosen at build
   time. The remote client is seeing the local prefix, but what we
   need is the remote prefix

 - The socket path varies according to remote env variables, such as
   the XDG_RUNTIME_DIR location. Again we see the local XDG_RUNTIME_DIR
   value, but what we need is the remote value (if any)

 - The remote driver doesn't know whether it must connect to the legacy
   libvirtd or the modular daemons, so must always assume legacy
   libvirtd for back-compat. This means we'll always end up using the
   virtproxyd daemon adding an extra hop in the RPC layer.

 - We can not able to autospawn the libvirtd daemon for session mode
   access

To address these problems this patch introduces the 'virtd-ssh-helper'
program which takes the URI for the remote driver as a CLI parameter.
It then figures out which daemon to connect to and its socket path,
using the same code that the remote driver client would on the remote
host's build of libvirt.

Signed-off-by: Daniel P. Berrangé 
---
 build-aux/syntax-check.mk  |   2 +-
 libvirt.spec.in|   2 +
 po/POTFILES.in |   1 +
 src/remote/meson.build |  17 ++
 src/remote/remote_ssh_helper.c | 425 +
 src/rpc/virnetsocket.h |   1 +
 6 files changed, 447 insertions(+), 1 deletion(-)
 create mode 100644 src/remote/remote_ssh_helper.c

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 6eb59cf90e..151f7a4767 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1864,7 +1864,7 @@ sc_group-qemu-caps:
 # List all syntax-check exemptions:
 exclude_file_name_regexp--sc_avoid_strcase = ^tools/vsh\.h$$
 
-_src1=libvirt-stream|qemu/qemu_monitor|util/vir(command|file|fdstream)|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon
+_src1=libvirt-stream|qemu/qemu_monitor|util/vir(command|file|fdstream)|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon|remote/remote_ssh_helper
 
_test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock|commandhelper
 exclude_file_name_regexp--sc_avoid_write = \
   ^(src/($(_src1))|tools/virsh-console|tests/($(_test1)))\.c$$
diff --git a/libvirt.spec.in b/libvirt.spec.in
index e64cfdb561..35125e4f8e 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1561,6 +1561,8 @@ exit 0
 
 %attr(0755, root, root) %{_libexecdir}/libvirt_iohelper
 
+%attr(0755, root, root) %{_bindir}/virt-ssh-helper
+
 %attr(0755, root, root) %{_sbindir}/libvirtd
 %attr(0755, root, root) %{_sbindir}/virtproxyd
 %attr(0755, root, root) %{_sbindir}/virtlogd
diff --git a/po/POTFILES.in b/po/POTFILES.in
index c4197604ef..1ab94972c7 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -182,6 +182,7 @@
 @SRCDIR@src/remote/remote_daemon_stream.c
 @SRCDIR@src/remote/remote_driver.c
 @SRCDIR@src/remote/remote_sockets.c
+@SRCDIR@src/remote/remote_ssh_helper.c
 @SRCDIR@src/rpc/virkeepalive.c
 @SRCDIR@src/rpc/virnetclient.c
 @SRCDIR@src/rpc/virnetclientprogram.c
diff --git a/src/remote/meson.build b/src/remote/meson.build
index 91dd587cba..9ad2f6ab1c 100644
--- a/src/remote/meson.build
+++ b/src/remote/meson.build
@@ -51,6 +51,15 @@ remote_daemon_sources = files(
 
 remote_daemon_generated = []
 
+virt_ssh_helper_sources = files(
+  'remote_sockets.c',
+  'remote_ssh_helper.c',
+)
+
+virt_ssh_helper_dep = [
+  src_dep,
+]
+
 foreach name : [ 'remote', 'qemu', 'lxc' ]
   protocol_x = '@0@_protocol.x'.format(name)
   dispatch_h = '@0@_daemon_dispatch_stubs.h'.format(name)
@@ -278,6 +287,14 @@ if conf.has('WITH_REMOTE')
 rename: [ '50-libvirt.rules' ],
   )
 endif
+
+virt_helpers += {
+  'name': 'virt-ssh-helper',
+  'sources': [
+virt_ssh_helper_sources
+  ],
+  'install_dir': bindir,
+}
   endif
 endif
 
diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c
new file mode 100644
index 00..0da55c1d1f
--- /dev/null
+++ b/src/remote/remote_ssh_helper.c
@@ -0,0 +1,425 @@
+/*
+ * remote_ssh_helper.c: a netcat replacement for proxying ssh tunnel to daemon
+ *
+ * Copyright (C) 2020 Red Hat, Inc.
+ *
+ * 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 

[libvirt PATCH v3 10/10] rpc: use new virt-ssh-helper binary for remote tunnelling

[Daniel P . Berrangé]

This wires up support for using the new virt-ssh-helper binary with the ssh,
libssh and libssh2 protocols.

The new binary will be used preferentially if it is available in $PATH,
otherwise we fall back to traditional netcat.

The "proxy" URI parameter can be used to force use of netcat e.g.

  qemu+ssh://host/system?proxy=netcat

or the disable fallback e.g.

  qemu+ssh://host/system?proxy=native

With use of virt-ssh-helper, we can now support remote session URIs

  qemu+ssh://host/session

and this will only use virt-ssh-helper, with no fallback. This also lets
the libvirtd process be auto-started, and connect directly to the
modular daemons, avoiding use of virtproxyd back-compat tunnelling.

Signed-off-by: Daniel P. Berrangé 
---
 docs/uri.html.in| 24 +-
 src/remote/remote_driver.c  | 30 +++-
 src/remote/remote_sockets.c |  8 
 src/rpc/virnetclient.c  | 91 ++---
 src/rpc/virnetclient.h  | 30 ++--
 tests/virnetsockettest.c|  7 ++-
 6 files changed, 156 insertions(+), 34 deletions(-)

diff --git a/docs/uri.html.in b/docs/uri.html.in
index 49f92773f8..4cce2d6a48 100644
--- a/docs/uri.html.in
+++ b/docs/uri.html.in
@@ -259,6 +259,24 @@ Note that parameter values must be
 
  Example: mode=direct 
   
+  
+
+  proxy
+
+auto, virt, generic 
+
+  
+autotry native, fallback to netcat
+netcatonly use netcat
+nativeonly use native
+  
+  Can also be set in libvirt.conf as 
remote_proxy
+
+  
+  
+
+ Example: proxy=native 
+  
   
 
   command
@@ -296,8 +314,10 @@ Note that parameter values must be
  ssh, libssh2, libssh 
 
   The name of the netcat command on the remote machine.
-  The default is nc.  For ssh transport, libvirt
-  constructs an ssh command which looks like:
+  The default is nc. This is not permitted
+  when using the native proxy mode. For ssh
+  transport, libvirt constructs an ssh command which looks
+  like:
 
 command -p port [-l username] hostname 
netcat -U socket
 
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index d56f4b7260..3058849153 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -761,12 +761,14 @@ doRemoteOpen(virConnectPtr conn,
 g_autofree char *knownHosts = NULL;
 g_autofree char *mode_str = NULL;
 g_autofree char *daemon_name = NULL;
+g_autofree char *proxy_str = NULL;
 bool sanity = true;
 bool verify = true;
 #ifndef WIN32
 bool tty = true;
 #endif
 int mode;
+int proxy;
 
 if (inside_daemon && !conn->uri->server) {
 mode = REMOTE_DRIVER_MODE_DIRECT;
@@ -774,6 +776,14 @@ doRemoteOpen(virConnectPtr conn,
 mode = REMOTE_DRIVER_MODE_AUTO;
 }
 
+/* Historically we didn't allow ssh tunnel with session mode,
+ * since we can't construct the accurate path remotely,
+ * so we can default to modern virt-ssh-helper */
+if (flags & VIR_DRV_OPEN_REMOTE_USER)
+proxy = VIR_NET_CLIENT_PROXY_NATIVE;
+else
+proxy = VIR_NET_CLIENT_PROXY_NETCAT;
+
 /* We handle *ALL* URIs here. The caller has rejected any
  * URIs we don't care about */
 
@@ -813,6 +823,7 @@ doRemoteOpen(virConnectPtr conn,
 EXTRACT_URI_ARG_STR("known_hosts_verify", knownHostsVerify);
 EXTRACT_URI_ARG_STR("tls_priority", tls_priority);
 EXTRACT_URI_ARG_STR("mode", mode_str);
+EXTRACT_URI_ARG_STR("proxy", proxy_str);
 EXTRACT_URI_ARG_BOOL("no_sanity", sanity);
 EXTRACT_URI_ARG_BOOL("no_verify", verify);
 #ifndef WIN32
@@ -865,6 +876,14 @@ doRemoteOpen(virConnectPtr conn,
 (mode = remoteDriverModeTypeFromString(mode_str)) < 0)
 goto failed;
 
+if (conf && !proxy_str &&
+virConfGetValueString(conf, "remote_proxy", _str) < 0)
+goto failed;
+
+if (proxy_str &&
+(proxy = virNetClientProxyTypeFromString(proxy_str)) < 0)
+goto failed;
+
 /* Sanity check that nothing requested !direct mode by mistake */
 if (inside_daemon && !conn->uri->server && mode != 
REMOTE_DRIVER_MODE_DIRECT) {
 virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -949,8 +968,11 @@ doRemoteOpen(virConnectPtr conn,
   knownHosts,
   knownHostsVerify,
   sshauth,
+  proxy,
   netcat,
   sockname,
+  name,
+  flags & VIR_DRV_OPEN_REMOTE_RO,
   auth,
   conn->uri);
 

[libvirt PATCH v3 07/10] remote: extract logic for determining daemon to connect to

[Daniel P . Berrangé]

We'll shortly want to reuse code for determining whether to connect to
the system or session daemon from places outside the remote driver
client. Pulling it out into a self contained function facilitates reuse.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c  | 51 
 src/remote/remote_sockets.c | 59 +
 src/remote/remote_sockets.h |  6 
 3 files changed, 71 insertions(+), 45 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 6ab0d67854..df10cfaef6 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1198,7 +1198,8 @@ remoteConnectOpen(virConnectPtr conn,
 struct private_data *priv;
 int ret = VIR_DRV_OPEN_ERROR;
 int rflags = 0;
-const char *autostart = getenv("LIBVIRT_AUTOSTART");
+bool user;
+bool autostart;
 char *driver = NULL;
 remoteDriverTransport transport;
 
@@ -1233,51 +1234,11 @@ remoteConnectOpen(virConnectPtr conn,
 if (flags & VIR_CONNECT_RO)
 rflags |= VIR_DRV_OPEN_REMOTE_RO;
 
-/*
- * User session daemon is used for
- *
- *  - Any URI with /session suffix
- *  - Test driver, if a protocol is given
- *
- * provided we are running non-root
- */
-if (conn->uri &&
-conn->uri->path &&
-conn->uri->scheme &&
-(STREQ(conn->uri->path, "/session") ||
- STRPREFIX(conn->uri->scheme, "test+")) &&
-geteuid() > 0) {
-VIR_DEBUG("User session daemon required");
+remoteGetURIDaemonInfo(conn->uri, transport, , );
+if (user)
 rflags |= VIR_DRV_OPEN_REMOTE_USER;
-
-/*
- * Furthermore if no servername is given,
- * and the transport is unix,
- * and uid is unprivileged then auto-spawn a daemon.
- */
-if (!conn->uri->server &&
-(transport == REMOTE_DRIVER_TRANSPORT_UNIX) &&
-(!autostart ||
- STRNEQ(autostart, "0"))) {
-VIR_DEBUG("Try daemon autostart");
-rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART;
-}
-}
-
-/*
- * If URI is NULL, then do a UNIX connection possibly auto-spawning
- * unprivileged server and probe remote server for URI.
- */
-if (!conn->uri) {
-VIR_DEBUG("Auto-probe remote URI");
-if (geteuid() > 0) {
-VIR_DEBUG("Auto-spawn user daemon instance");
-rflags |= VIR_DRV_OPEN_REMOTE_USER;
-if (!autostart ||
-STRNEQ(autostart, "0"))
-rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART;
-}
-}
+if (autostart)
+rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART;
 
 ret = doRemoteOpen(conn, priv, driver, transport, auth, conf, rflags);
 if (ret != VIR_DRV_OPEN_SUCCESS) {
diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
index 28e02e24d5..854775f401 100644
--- a/src/remote/remote_sockets.c
+++ b/src/remote/remote_sockets.c
@@ -224,3 +224,62 @@ remoteGetUNIXSocket(remoteDriverTransport transport,
   ro, session);
 return sock_name;
 }
+
+
+void
+remoteGetURIDaemonInfo(virURIPtr uri,
+   remoteDriverTransport transport,
+   bool *session,
+   bool *autostart)
+{
+const char *autostart_str = getenv("LIBVIRT_AUTOSTART");
+
+*session = false;
+*autostart = false;
+
+/*
+ * User session daemon is used for
+ *
+ *  - Any URI with /session suffix
+ *  - Test driver, if a protocol is given
+ *
+ * provided we are running non-root
+ */
+if (uri &&
+uri->path &&
+uri->scheme &&
+(STREQ(uri->path, "/session") ||
+ STRPREFIX(uri->scheme, "test+")) &&
+geteuid() > 0) {
+VIR_DEBUG("User session daemon required");
+*session = true;
+
+/*
+ * Furthermore if no servername is given,
+ * and the transport is unix,
+ * and uid is unprivileged then auto-spawn a daemon.
+ */
+if (!uri->server &&
+(transport == REMOTE_DRIVER_TRANSPORT_UNIX) &&
+(!autostart_str ||
+ STRNEQ(autostart_str, "0"))) {
+VIR_DEBUG("Try daemon autostart");
+*autostart = true;
+}
+}
+
+/*
+ * If URI is NULL, then do a UNIX connection possibly auto-spawning
+ * unprivileged server and probe remote server for URI.
+ */
+if (!uri) {
+VIR_DEBUG("Auto-probe remote URI");
+if (geteuid() > 0) {
+VIR_DEBUG("Auto-spawn user daemon instance");
+*session = true;
+if (!autostart_str ||
+STRNEQ(autostart_str, "0"))
+*autostart = true;
+}
+}
+}
diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h
index 64055f3d44..7526752835 100644
--- a/src/remote/remote_sockets.h
+++ b/src/remote/remote_sockets.h
@@ -62,3 

[libvirt PATCH v3 02/10] remote: push logic for default netcat binary into common helper

[Daniel P . Berrangé]

We don't want to repeat the choice of default netcat binary setting in
three different places. This will also make it possible to do better
error reporting in the helper.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c | 2 +-
 src/rpc/virnetclient.c | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 0331060a2d..f511a9bc33 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1226,7 +1226,7 @@ doRemoteOpen(virConnectPtr conn,
 username,
 !tty,
 !verify,
-netcat ? netcat : "nc",
+netcat,
 keyfile,
 sockname)))
 goto failed;
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index bd818df953..f65cda0d7f 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -419,6 +419,9 @@ virNetClientSSHHelperCommand(const char *netcatPath,
 {
 g_autofree char *netcatPathSafe = 
virNetClientDoubleEscapeShell(netcatPath);
 
+if (!netcatPath)
+netcatPath = "nc";
+
 return g_strdup_printf(
 "sh -c "
 "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; 
then "
@@ -505,7 +508,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
 DEFAULT_VALUE(host, "localhost");
 DEFAULT_VALUE(port, "22");
 DEFAULT_VALUE(username, "root");
-DEFAULT_VALUE(netcatPath, "nc");
 DEFAULT_VALUE(knownHostsVerify, "normal");
 
 command = virNetClientSSHHelperCommand(netcatPath, socketPath);
@@ -566,7 +568,6 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
 DEFAULT_VALUE(host, "localhost");
 DEFAULT_VALUE(port, "22");
 DEFAULT_VALUE(username, "root");
-DEFAULT_VALUE(netcatPath, "nc");
 DEFAULT_VALUE(knownHostsVerify, "normal");
 
 command = virNetClientSSHHelperCommand(netcatPath, socketPath);
-- 
2.26.2

[libvirt PATCH v3 01/10] rpc: merge logic for generating remote SSH shell script

[Daniel P . Berrangé]

Three parts of the code all build up the same SSH shell script
snippet for remote tunneling the RPC protocol, but in slightly
different ways. Combine them all into one helper method in the
virNetClient code, since this logic doesn't really belong in
the virNetSocket code.

Note that the this change means the shell snippet is passed to
the SSH binary as a single arg, instead of three separate args,
but this is functionally identical, as the three separate args
were combined into one already when passed to the remote system.

Signed-off-by: Daniel P. Berrangé 
---
 src/libvirt_remote.syms  |   1 +
 src/rpc/virnetclient.c   | 108 ---
 src/rpc/virnetclient.h   |   3 ++
 src/rpc/virnetsocket.c   |  37 +-
 src/rpc/virnetsocket.h   |   3 +-
 tests/virnetsockettest.c |   9 +++-
 6 files changed, 69 insertions(+), 92 deletions(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0018a0c41d..0b00bce1fa 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -42,6 +42,7 @@ virNetClientSendStream;
 virNetClientSendWithReply;
 virNetClientSetCloseCallback;
 virNetClientSetTLSSession;
+virNetClientSSHHelperCommand;
 
 
 # rpc/virnetclientprogram.h
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 441f1502a6..bd818df953 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -391,28 +391,74 @@ virNetClientPtr virNetClientNewTCP(const char *nodename,
 return virNetClientNew(sock, nodename);
 }
 
+
+/*
+ * The SSH Server uses shell to spawn the command we give
+ * it.  Our command then invokes shell again. Thus we need
+ * to apply two levels of escaping, so that commands with
+ * whitespace in their path get correctly interpreted.
+ */
+static char *
+virNetClientDoubleEscapeShell(const char *str)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+g_autofree char *tmp = NULL;
+
+virBufferEscapeShell(, str);
+
+tmp = virBufferContentAndReset();
+
+virBufferEscapeShell(, tmp);
+
+return virBufferContentAndReset();
+}
+
+char *
+virNetClientSSHHelperCommand(const char *netcatPath,
+ const char *socketPath)
+{
+g_autofree char *netcatPathSafe = 
virNetClientDoubleEscapeShell(netcatPath);
+
+return g_strdup_printf(
+"sh -c "
+"'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; 
then "
+  "ARG=-q0;"
+"else "
+  "ARG=;"
+"fi;"
+"'%s' $ARG -U %s'",
+netcatPathSafe, netcatPathSafe, socketPath);
+}
+
+
+#define DEFAULT_VALUE(VAR, VAL) \
+if (!VAR) \
+VAR = VAL;
+
 virNetClientPtr virNetClientNewSSH(const char *nodename,
const char *service,
const char *binary,
const char *username,
bool noTTY,
bool noVerify,
-   const char *netcat,
+   const char *netcatPath,
const char *keyfile,
-   const char *path)
+   const char *socketPath)
 {
 virNetSocketPtr sock;
+g_autofree char *command = NULL;
+
+DEFAULT_VALUE(netcatPath, "nc");
+
+command = virNetClientSSHHelperCommand(netcatPath, socketPath);
 
 if (virNetSocketNewConnectSSH(nodename, service, binary, username, noTTY,
-  noVerify, netcat, keyfile, path, ) < 0)
+  noVerify, keyfile, command, ) < 0)
 return NULL;
 
 return virNetClientNew(sock, NULL);
 }
 
-#define DEFAULT_VALUE(VAR, VAL) \
-if (!VAR) \
-VAR = VAL;
 virNetClientPtr virNetClientNewLibSSH2(const char *host,
const char *port,
int family,
@@ -427,11 +473,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
virURIPtr uri)
 {
 virNetSocketPtr sock = NULL;
-
-g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-g_autofree char *nc = NULL;
 g_autofree char *command = NULL;
-
 g_autofree char *homedir = NULL;
 g_autofree char *confdir = NULL;
 g_autofree char *knownhosts = NULL;
@@ -442,9 +484,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
 knownhosts = g_strdup(knownHostsPath);
 } else {
 confdir = virGetUserConfigDirectory();
-virBufferAsprintf(, "%s/known_hosts", confdir);
-if (!(knownhosts = virBufferContentAndReset()))
-return NULL;
+knownhosts = g_strdup_printf("%s/known_hosts", confdir);
 }
 
 if (privkeyPath) {
@@ -468,26 +508,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
 DEFAULT_VALUE(netcatPath, "nc");
 DEFAULT_VALUE(knownHostsVerify, "normal");
 
-

[libvirt PATCH v3 06/10] remote: split out function for constructing socket path

[Daniel P . Berrangé]

The remoteGetUNIXSocketHelper method will be needed by source files
beyond the remote driver client.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c  | 129 +-
 src/remote/remote_sockets.c | 134 
 src/remote/remote_sockets.h |   8 +++
 3 files changed, 145 insertions(+), 126 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 66d66c1284..6ab0d67854 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -697,131 +697,6 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn,
 }
 
 
-static char *
-remoteGetUNIXSocketHelper(remoteDriverTransport transport,
-  const char *sock_prefix,
-  unsigned int flags)
-{
-char *sockname = NULL;
-g_autofree char *userdir = NULL;
-
-if (flags & VIR_DRV_OPEN_REMOTE_USER) {
-if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-   _("Connecting to session instance without "
- "socket path is not supported by the %s "
- "transport"),
-   remoteDriverTransportTypeToString(transport));
-return NULL;
-}
-userdir = virGetUserRuntimeDirectory();
-
-sockname = g_strdup_printf("%s/%s-sock", userdir, sock_prefix);
-} else {
-/* Intentionally do *NOT* use RUNSTATEDIR here. We might
- * be connecting to a remote machine, and cannot assume
- * the remote host has /run. The converse is ok though,
- * any machine with /run will have a /var/run symlink.
- * The portable option is to thus use $LOCALSTATEDIR/run
- */
-sockname = g_strdup_printf("%s/run/libvirt/%s-%s", LOCALSTATEDIR,
-   sock_prefix,
-   flags & VIR_DRV_OPEN_REMOTE_RO ? "sock-ro" 
: "sock");
-}
-
-VIR_DEBUG("Built UNIX sockname %s for transport %s prefix %s flags=0x%x",
-  sockname, remoteDriverTransportTypeToString(transport),
-  sock_prefix, flags);
-return sockname;
-}
-
-
-static char *
-remoteGetUNIXSocket(remoteDriverTransport transport,
-remoteDriverMode mode,
-const char *driver,
-char **daemon,
-unsigned int flags)
-{
-char *sock_name = NULL;
-g_autofree char *direct_daemon = NULL;
-g_autofree char *legacy_daemon = NULL;
-g_autofree char *direct_sock_name = NULL;
-g_autofree char *legacy_sock_name = NULL;
-
-if (driver)
-direct_daemon = g_strdup_printf("virt%sd", driver);
-
-legacy_daemon = g_strdup("libvirtd");
-
-if (driver &&
-!(direct_sock_name = remoteGetUNIXSocketHelper(transport, 
direct_daemon, flags)))
-return NULL;
-
-if (!(legacy_sock_name = remoteGetUNIXSocketHelper(transport, "libvirt", 
flags)))
-return NULL;
-
-if (mode == REMOTE_DRIVER_MODE_AUTO) {
-if (transport == REMOTE_DRIVER_TRANSPORT_UNIX) {
-if (direct_sock_name && virFileExists(direct_sock_name)) {
-mode = REMOTE_DRIVER_MODE_DIRECT;
-} else if (virFileExists(legacy_sock_name)) {
-mode = REMOTE_DRIVER_MODE_LEGACY;
-} else if (driver) {
-/*
- * This constant comes from the configure script and
- * maps to either the direct or legacy mode constant
- */
-mode = REMOTE_DRIVER_MODE_DEFAULT;
-} else {
-mode = REMOTE_DRIVER_MODE_LEGACY;
-}
-} else {
-mode = REMOTE_DRIVER_MODE_LEGACY;
-}
-}
-
-switch ((remoteDriverMode)mode) {
-case REMOTE_DRIVER_MODE_LEGACY:
-sock_name = g_steal_pointer(_sock_name);
-*daemon = g_steal_pointer(_daemon);
-break;
-
-case REMOTE_DRIVER_MODE_DIRECT:
-if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-   _("Cannot use direct socket mode for %s transport"),
-   remoteDriverTransportTypeToString(transport));
-return NULL;
-}
-
-if (!direct_sock_name) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-   _("Cannot use direct socket mode if no URI is 
set"));
-return NULL;
-}
-
-sock_name = g_steal_pointer(_sock_name);
-*daemon = g_steal_pointer(_daemon);
-break;
-
-case REMOTE_DRIVER_MODE_AUTO:
-case REMOTE_DRIVER_MODE_LAST:
-default:
-virReportEnumRangeError(remoteDriverMode, mode);
-return NULL;
-}
-
-VIR_DEBUG("Chosen UNIX sockname %s daemon %s "
-  "for mode %s transport %s 

[libvirt PATCH v3 04/10] remote: split out function for parsing URI scheme

[Daniel P . Berrangé]

The remoteSplitURISCheme method will be needed by source files beyond
the remote driver client.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c  | 25 -
 src/remote/remote_sockets.c | 28 
 src/remote/remote_sockets.h |  6 ++
 3 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b214632bdf..8952484b8e 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -165,31 +165,6 @@ static void 
make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapsho
 /*--*/
 
 /* Helper functions for remoteOpen. */
-static int remoteSplitURIScheme(virURIPtr uri,
-char **driver,
-char **transport)
-{
-char *p = strchr(uri->scheme, '+');
-
-*driver = *transport = NULL;
-
-if (p)
-*driver = g_strndup(uri->scheme, p - uri->scheme);
-else
-*driver = g_strdup(uri->scheme);
-
-if (p) {
-*transport = g_strdup(p + 1);
-
-p = *transport;
-while (*p) {
-*p = g_ascii_tolower(*p);
-p++;
-}
-}
-
-return 0;
-}
 
 
 static int
diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
index 0662cbad14..976124d0ed 100644
--- a/src/remote/remote_sockets.c
+++ b/src/remote/remote_sockets.c
@@ -37,3 +37,31 @@ VIR_ENUM_IMPL(remoteDriverMode,
   "auto",
   "legacy",
   "direct");
+
+
+int
+remoteSplitURIScheme(virURIPtr uri,
+ char **driver,
+ char **transport)
+{
+char *p = strchr(uri->scheme, '+');
+
+*driver = *transport = NULL;
+
+if (p)
+*driver = g_strndup(uri->scheme, p - uri->scheme);
+else
+*driver = g_strdup(uri->scheme);
+
+if (p) {
+*transport = g_strdup(p + 1);
+
+p = *transport;
+while (*p) {
+*p = g_ascii_tolower(*p);
+p++;
+}
+}
+
+return 0;
+}
diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h
index 1d4ae3f9c1..bef3cdada9 100644
--- a/src/remote/remote_sockets.h
+++ b/src/remote/remote_sockets.h
@@ -21,6 +21,7 @@
 #pragma once
 
 #include "virenum.h"
+#include "viruri.h"
 
 typedef enum {
 REMOTE_DRIVER_TRANSPORT_TLS,
@@ -48,3 +49,8 @@ typedef enum {
 } remoteDriverMode;
 
 VIR_ENUM_DECL(remoteDriverMode);
+
+int
+remoteSplitURIScheme(virURIPtr uri,
+ char **driver,
+ char **transport);
-- 
2.26.2

Re: [PATCH 1/3] qemu: implementation of transient option for qcow2 file

[Peter Krempa]

On Sun, Jul 19, 2020 at 22:56:50 -0400, Masayoshi Mizuma wrote:
> On Sat, Jul 18, 2020 at 08:06:00AM +0200, Peter Krempa wrote:
> > On Thu, Jul 16, 2020 at 20:55:29 -0400, Masayoshi Mizuma wrote:
> > > Thank you for your review.
> > > 
> > > On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
> > > > On 7/7/20 2:12 AM, Peter Krempa wrote:
> > > > 
> > > > > You can install a qcow2 overlay on top of a raw file too. IMO the
> > > > > implications of using  allow that.
> > > > > 
> > > > > As said above I'd strongly prefer if the overlay is created in qemu
> > > > > using the blockdev-create blockjob (there is already infrastructure in
> > > > > libvirt to achieve that).
> > > > 
> > > > Agreed.  At this point, any time we call out to qemu-img as a separate
> > > > process, we are probably doing it wrong.
> > > 
> > > Got it. I'm thinking about the procedure such as followings.
> > > Does that make sense?
> > > 
> > >   1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(), 
> > >  and connect it.
> > 
> > Starting a new qemu process just to format an image is extreme overkill
> > and definitely not what we want to do.
> 
> I see, thanks.
> 
> > 
> > >   2) Setup the transient disk with 
> > > qemuDomainPrepareStorageSourceBlockdev(),
> > >  qemuBlockStorageSourceAttachApplyStorage(), 
> > > qemuBlockStorageSourceCreateGetFormatProps()
> > >  and something...
> > > 
> > >   3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then
> > >  close the monitor.
> > 
> > These two steps should be exectued in the qemu process which already
> > will run the VM prior to starting the guest CPUs.
> > 
> > >   4) Switch the original disk to the transient disk.
> > > 
> > >   5) Build the blockdev argument for qemu.
> > 
> > And instead of this step, you use the external snapshot infrastructure
> > to install the overlays via 'blockdev-snapshot' QMP command
> 
> OK. I suppose qemuDomainSnapshotDiskPrepare() and
> qemuDomainSnapshotDiskUpdateSource() maybe helpful to implement the
> setup steps of transient disk.
> 
> > 
> > > 
> > >   6) Run qemu
> > 
> > And instead of this the VM cpus will be started.
> 
> Got it, I think the appropriate place is after virCommandRun() at 
> qemuProcessLaunch(),
> and before qemuProcessFinishStartup().
> 
> > 
> > 
> > The above steps require factoring out snapshot code a bit. I have a few
> > patches in that direction so I'll try posting them next week hopefully.

Sorry this took longer than expected, but we were dealing with the build
system change.

The refactor is here:

https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html

You can now create an equivalent of 'qemuSnapshotDiskPrepare' which will
go through the disks and find the 'transient' ones. It will then create
snapshot data by a call to 'qemuSnapshotDiskPrepareOne' with a faked
snapshot disk object.

'qemuSnapshotDiskPrepareOne' ensures that the files are created and
formatted properly.

You then use same algorithm as 'qemuSnapshotCreateDiskActive'
(e.g. by extracting the common internals (basically everything except
call to 'qemuSnapshotDiskPrepare') into a separate function) and reuse
it when starting the VM as you've described above.

Note that all of the above can work only when QEMU_CAPS_BLOCKDEV is
supported.

The caveats/limitations with blockjobs and snapshots still apply though.
It depends on how you approach it. It's okay to limit/block the features
if transient disk is used. Alternatively you can implement some form of
private data to mark which image was transient and allow those
operations.

Re: [PATCH 5/5] qemu: Extract snapshot related code to a separate file

[Peter Krempa]

On Thu, Aug 06, 2020 at 11:55:16 +0200, Peter Krempa wrote:
> We've dumped all the snapshot helpers and related code into
> qemu_driver.c. It accounted for ~10% of overal size of qemu_driver.c.
> 
> Separate the code to qemu_snapshot.c/h.
> 
> Signed-off-by: Peter Krempa 
> ---

This patch is best viewed with --patience.

[PATCH 2/5] qemuOpenFile: Move to qemu_domain.c

[Peter Krempa]

Move the code to qemu_domain.c so that it can be reused in other parts
of the qemu driver. 'qemu_domain' was chosen as the permissions are
based on the domain configuration.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_domain.c | 42 +++
 src/qemu/qemu_domain.h |  7 ++
 src/qemu/qemu_driver.c | 50 +-
 3 files changed, 54 insertions(+), 45 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c440c79e1d..670db6ebfb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -64,6 +64,7 @@
 #include "virdomaincheckpointobjlist.h"
 #include "backup_conf.h"
 #include "virutil.h"
+#include "virqemu.h"

 #include 
 #include 
@@ -10679,3 +10680,44 @@ qemuDomainDiskBlockJobIsSupported(virDomainObjPtr vm,

 return true;
 }
+
+
+/**
+ * qemuDomainOpenFile:
+ * @driver: driver object
+ * @vm: domain object
+ * @path: path to file to open
+ * @oflags: flags for opening/creation of the file
+ * @needUnlink: set to true if file was created by this function
+ *
+ * Internal function to properly create or open existing files, with
+ * ownership affected by qemu driver setup and domain DAC label.
+ *
+ * Returns the file descriptor on success and negative errno on failure.
+ *
+ * This function should not be used on storage sources. Use
+ * qemuDomainStorageFileInit and storage driver APIs if possible.
+ **/
+int
+qemuDomainOpenFile(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   const char *path,
+   int oflags,
+   bool *needUnlink)
+{
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+uid_t user = cfg->user;
+gid_t group = cfg->group;
+bool dynamicOwnership = cfg->dynamicOwnership;
+virSecurityLabelDefPtr seclabel;
+
+/* TODO: Take imagelabel into account? */
+if (vm &&
+(seclabel = virDomainDefGetSecurityLabelDef(vm->def, "dac")) != NULL &&
+seclabel->label != NULL &&
+(virParseOwnershipIds(seclabel->label, , ) < 0))
+return -1;
+
+return virQEMUFileOpenAs(user, group, dynamicOwnership,
+ path, oflags, needUnlink);
+}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 3a1bcbbfa3..ef03702fa1 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1017,3 +1017,10 @@ qemuDomainDiskBlockJobIsSupported(virDomainObjPtr vm,
 int
 qemuDomainDefNumaCPUsRectify(virDomainDefPtr def,
  virQEMUCapsPtr qemuCaps);
+
+int
+qemuDomainOpenFile(virQEMUDriverPtr driver,
+   virDomainObjPtr vm,
+   const char *path,
+   int oflags,
+   bool *needUnlink);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a667eb21bf..0bc7eebe9a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3021,46 +3021,6 @@ qemuCompressGetCommand(virQEMUSaveFormat compression)
 return ret;
 }

-/**
- * qemuOpenFile:
- * @driver: driver object
- * @vm: domain object
- * @path: path to file to open
- * @oflags: flags for opening/creation of the file
- * @needUnlink: set to true if file was created by this function
- *
- * Internal function to properly create or open existing files, with
- * ownership affected by qemu driver setup and domain DAC label.
- *
- * Returns the file descriptor on success and negative errno on failure.
- *
- * This function should not be used on storage sources. Use
- * qemuDomainStorageFileInit and storage driver APIs if possible.
- **/
-static int
-qemuOpenFile(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- const char *path,
- int oflags,
- bool *needUnlink)
-{
-g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-uid_t user = cfg->user;
-gid_t group = cfg->group;
-bool dynamicOwnership = cfg->dynamicOwnership;
-virSecurityLabelDefPtr seclabel;
-
-/* TODO: Take imagelabel into account? */
-if (vm &&
-(seclabel = virDomainDefGetSecurityLabelDef(vm->def, "dac")) != NULL &&
-seclabel->label != NULL &&
-(virParseOwnershipIds(seclabel->label, , ) < 0))
-return -1;
-
-return virQEMUFileOpenAs(user, group, dynamicOwnership,
- path, oflags, needUnlink);
-}
-

 static int
 qemuFileWrapperFDClose(virDomainObjPtr vm,
@@ -3154,7 +3114,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
 if (qemuFileWrapperFDClose(vm, wrapperFd) < 0)
 goto cleanup;

-if ((fd = qemuOpenFile(driver, vm, path, O_WRONLY, NULL)) < 0 ||
+if ((fd = qemuDomainOpenFile(driver, vm, path, O_WRONLY, NULL)) < 0 ||
 virQEMUSaveDataFinish(data, , path) < 0)
 goto cleanup;

@@ -6593,7 +6553,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 oflags |= directFlag;
 }

-if ((fd = qemuOpenFile(driver, NULL, path, oflags, NULL)) < 

[PATCH 1/5] qemuOpenFileAs: Move into util/virqemu.c

[Peter Krempa]

Signed-off-by: Peter Krempa 
---
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_driver.c   | 138 ++-
 src/util/virqemu.c   | 130 
 src/util/virqemu.h   |   7 ++
 4 files changed, 144 insertions(+), 132 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 01c2e710cd..d737e4d9e4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2939,6 +2939,7 @@ virQEMUBuildDriveCommandlineFromJSON;
 virQEMUBuildNetdevCommandlineFromJSON;
 virQEMUBuildObjectCommandlineFromJSON;
 virQEMUBuildQemuImgKeySecretOpts;
+virQEMUFileOpenAs;


 # util/virrandom.h
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0f98243fe4..a667eb21bf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -148,11 +148,6 @@ static int qemuDomainObjStart(virConnectPtr conn,
 static int qemuDomainManagedSaveLoad(virDomainObjPtr vm,
  void *opaque);

-static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
-  bool dynamicOwnership,
-  const char *path, int oflags,
-  bool *needUnlink);
-
 static virQEMUDriverPtr qemu_driver;

 /* Looks up the domain object from snapshot and unlocks the
@@ -3062,129 +3057,8 @@ qemuOpenFile(virQEMUDriverPtr driver,
 (virParseOwnershipIds(seclabel->label, , ) < 0))
 return -1;

-return qemuOpenFileAs(user, group, dynamicOwnership,
-  path, oflags, needUnlink);
-}
-
-static int
-qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
-   bool dynamicOwnership,
-   const char *path, int oflags,
-   bool *needUnlink)
-{
-struct stat sb;
-bool is_reg = true;
-bool need_unlink = false;
-unsigned int vfoflags = 0;
-int fd = -1;
-int path_shared = virFileIsSharedFS(path);
-uid_t uid = geteuid();
-gid_t gid = getegid();
-
-/* path might be a pre-existing block dev, in which case
- * we need to skip the create step, and also avoid unlink
- * in the failure case */
-if (oflags & O_CREAT) {
-need_unlink = true;
-
-/* Don't force chown on network-shared FS
- * as it is likely to fail. */
-if (path_shared <= 0 || dynamicOwnership)
-vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
-
-if (stat(path, ) == 0) {
-/* It already exists, we don't want to delete it on error */
-need_unlink = false;
-
-is_reg = !!S_ISREG(sb.st_mode);
-/* If the path is regular file which exists
- * already and dynamic_ownership is off, we don't
- * want to change its ownership, just open it as-is */
-if (is_reg && !dynamicOwnership) {
-uid = sb.st_uid;
-gid = sb.st_gid;
-}
-}
-}
-
-/* First try creating the file as root */
-if (!is_reg) {
-if ((fd = open(path, oflags & ~O_CREAT)) < 0) {
-fd = -errno;
-goto error;
-}
-} else {
-if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid,
-vfoflags | VIR_FILE_OPEN_NOFORK)) < 0) {
-/* If we failed as root, and the error was permission-denied
-   (EACCES or EPERM), assume it's on a network-connected share
-   where root access is restricted (eg, root-squashed NFS). If the
-   qemu user is non-root, just set a flag to
-   bypass security driver shenanigans, and retry the operation
-   after doing setuid to qemu user */
-if ((fd != -EACCES && fd != -EPERM) || fallback_uid == geteuid())
-goto error;
-
-/* On Linux we can also verify the FS-type of the directory. */
-switch (path_shared) {
-case 1:
-/* it was on a network share, so we'll continue
- * as outlined above
- */
-break;
-
-case -1:
-virReportSystemError(-fd, oflags & O_CREAT
- ? _("Failed to create file "
- "'%s': couldn't determine fs 
type")
- : _("Failed to open file "
- "'%s': couldn't determine fs 
type"),
- path);
-goto cleanup;
-
-case 0:
-default:
-/* local file - log the error returned by virFileOpenAs */
-goto error;
-}
-
-/* If we created the file above, then we need to remove it;
- * otherwise, the next attempt to create will fail. If the
- * file had already existed before we 

[PATCH 0/5] qemu: split off snapshot code for cleanup and reuse

[Peter Krempa]

Peter Krempa (5):
  qemuOpenFileAs: Move into util/virqemu.c
  qemuOpenFile: Move to qemu_domain.c
  qemuFileWrapperFDClose: move to qemu_domain.c
  qemu: Split of code related to handling of the save image file
  qemu: Extract snapshot related code to a separate file

 po/POTFILES.in|2 +
 src/libvirt_private.syms  |1 +
 src/qemu/meson.build  |2 +
 src/qemu/qemu_domain.c|   70 +
 src/qemu/qemu_domain.h|   11 +
 src/qemu/qemu_driver.c| 3320 ++---
 src/qemu/qemu_saveimage.c |  764 +
 src/qemu/qemu_saveimage.h |  116 ++
 src/qemu/qemu_snapshot.c  | 2266 +
 src/qemu/qemu_snapshot.h  |   55 +
 src/util/virqemu.c|  130 ++
 src/util/virqemu.h|7 +
 12 files changed, 3520 insertions(+), 3224 deletions(-)
 create mode 100644 src/qemu/qemu_saveimage.c
 create mode 100644 src/qemu/qemu_saveimage.h
 create mode 100644 src/qemu/qemu_snapshot.c
 create mode 100644 src/qemu/qemu_snapshot.h

-- 
2.26.2

[PATCH 3/5] qemuFileWrapperFDClose: move to qemu_domain.c

[Peter Krempa]

Move the code to qemu_domain.c so that it can be reused in other parts
of the qemu driver. 'qemu_domain' was chosen as we check the domain
state after closing the wrapper.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_domain.c | 28 
 src/qemu/qemu_domain.h |  4 
 src/qemu/qemu_driver.c | 36 
 3 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 670db6ebfb..e28f704dba 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10721,3 +10721,31 @@ qemuDomainOpenFile(virQEMUDriverPtr driver,
 return virQEMUFileOpenAs(user, group, dynamicOwnership,
  path, oflags, needUnlink);
 }
+
+
+int
+qemuDomainFileWrapperFDClose(virDomainObjPtr vm,
+ virFileWrapperFdPtr fd)
+{
+int ret;
+
+/* virFileWrapperFd uses iohelper to write data onto disk.
+ * However, iohelper calls fdatasync() which may take ages to
+ * finish. Therefore, we shouldn't be waiting with the domain
+ * object locked. */
+
+/* XXX Currently, this function is intended for *Save() only
+ * as restore needs some reworking before it's ready for
+ * this. */
+
+virObjectUnlock(vm);
+ret = virFileWrapperFdClose(fd);
+virObjectLock(vm);
+if (!virDomainObjIsActive(vm)) {
+if (virGetLastErrorCode() == VIR_ERR_OK)
+virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+   _("domain is no longer running"));
+ret = -1;
+}
+return ret;
+}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index ef03702fa1..e4c22864dc 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1024,3 +1024,7 @@ qemuDomainOpenFile(virQEMUDriverPtr driver,
const char *path,
int oflags,
bool *needUnlink);
+
+int
+qemuDomainFileWrapperFDClose(virDomainObjPtr vm,
+ virFileWrapperFdPtr fd);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0bc7eebe9a..8f61759f53 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3022,34 +3022,6 @@ qemuCompressGetCommand(virQEMUSaveFormat compression)
 }


-static int
-qemuFileWrapperFDClose(virDomainObjPtr vm,
-   virFileWrapperFdPtr fd)
-{
-int ret;
-
-/* virFileWrapperFd uses iohelper to write data onto disk.
- * However, iohelper calls fdatasync() which may take ages to
- * finish. Therefore, we shouldn't be waiting with the domain
- * object locked. */
-
-/* XXX Currently, this function is intended for *Save() only
- * as restore needs some reworking before it's ready for
- * this. */
-
-virObjectUnlock(vm);
-ret = virFileWrapperFdClose(fd);
-virObjectLock(vm);
-if (!virDomainObjIsActive(vm)) {
-if (virGetLastErrorCode() == VIR_ERR_OK)
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("domain is no longer running"));
-ret = -1;
-}
-return ret;
-}
-
-
 /* Helper function to execute a migration to file with a correct save header
  * the caller needs to make sure that the processors are stopped and do all 
other
  * actions besides saving memory */
@@ -3111,7 +3083,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
 goto cleanup;
 }

-if (qemuFileWrapperFDClose(vm, wrapperFd) < 0)
+if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
 goto cleanup;

 if ((fd = qemuDomainOpenFile(driver, vm, path, O_WRONLY, NULL)) < 0 ||
@@ -3122,7 +3094,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,

  cleanup:
 VIR_FORCE_CLOSE(fd);
-if (qemuFileWrapperFDClose(vm, wrapperFd) < 0)
+if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
 ret = -1;
 virFileWrapperFdFree(wrapperFd);

@@ -3703,14 +3675,14 @@ doCoreDump(virQEMUDriverPtr driver,
  path);
 goto cleanup;
 }
-if (qemuFileWrapperFDClose(vm, wrapperFd) < 0)
+if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
 goto cleanup;

 ret = 0;

  cleanup:
 VIR_FORCE_CLOSE(fd);
-if (qemuFileWrapperFDClose(vm, wrapperFd) < 0)
+if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
 ret = -1;
 virFileWrapperFdFree(wrapperFd);
 if (ret != 0)
-- 
2.26.2

[PATCH 4/5] qemu: Split of code related to handling of the save image file

[Peter Krempa]

There's a lot of helper code related to the save image handling. Extract
it to qemu_saveimage.c/h.

Signed-off-by: Peter Krempa 
---
 po/POTFILES.in|   1 +
 src/qemu/meson.build  |   1 +
 src/qemu/qemu_driver.c| 843 +++---
 src/qemu/qemu_saveimage.c | 764 ++
 src/qemu/qemu_saveimage.h | 116 ++
 5 files changed, 928 insertions(+), 797 deletions(-)
 create mode 100644 src/qemu/qemu_saveimage.c
 create mode 100644 src/qemu/qemu_saveimage.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index c5b43df7b5..6f47371b01 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -170,6 +170,7 @@
 @SRCDIR@src/qemu/qemu_namespace.c
 @SRCDIR@src/qemu/qemu_process.c
 @SRCDIR@src/qemu/qemu_qapi.c
+@SRCDIR@src/qemu/qemu_saveimage.c
 @SRCDIR@src/qemu/qemu_slirp.c
 @SRCDIR@src/qemu/qemu_tpm.c
 @SRCDIR@src/qemu/qemu_validate.c
diff --git a/src/qemu/meson.build b/src/qemu/meson.build
index 7faba16049..7d5249978a 100644
--- a/src/qemu/meson.build
+++ b/src/qemu/meson.build
@@ -29,6 +29,7 @@ qemu_driver_sources = [
   'qemu_namespace.c',
   'qemu_process.c',
   'qemu_qapi.c',
+  'qemu_saveimage.c',
   'qemu_security.c',
   'qemu_slirp.c',
   'qemu_tpm.c',
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8f61759f53..a6b8c79168 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -51,6 +51,7 @@
 #include "qemu_checkpoint.h"
 #include "qemu_backup.h"
 #include "qemu_namespace.h"
+#include "qemu_saveimage.h"

 #include "virerror.h"
 #include "virlog.h"
@@ -135,6 +136,16 @@ VIR_LOG_INIT("qemu.qemu_driver");

 #define QEMU_NB_BANDWIDTH_PARAM 7

+VIR_ENUM_DECL(qemuDumpFormat);
+VIR_ENUM_IMPL(qemuDumpFormat,
+  VIR_DOMAIN_CORE_DUMP_FORMAT_LAST,
+  "elf",
+  "kdump-zlib",
+  "kdump-lzo",
+  "kdump-snappy",
+);
+
+
 static void qemuProcessEventHandler(void *data, void *opaque);

 static int qemuStateCleanup(void);
@@ -2771,339 +2782,6 @@ qemuDomainGetControlInfo(virDomainPtr dom,
 }


-/* It would be nice to replace 'Qemud' with 'Qemu' but
- * this magic string is ABI, so it can't be changed
- */
-#define QEMU_SAVE_MAGIC   "LibvirtQemudSave"
-#define QEMU_SAVE_PARTIAL "LibvirtQemudPart"
-#define QEMU_SAVE_VERSION 2
-
-G_STATIC_ASSERT(sizeof(QEMU_SAVE_MAGIC) == sizeof(QEMU_SAVE_PARTIAL));
-
-typedef enum {
-QEMU_SAVE_FORMAT_RAW = 0,
-QEMU_SAVE_FORMAT_GZIP = 1,
-QEMU_SAVE_FORMAT_BZIP2 = 2,
-/*
- * Deprecated by xz and never used as part of a release
- * QEMU_SAVE_FORMAT_LZMA
- */
-QEMU_SAVE_FORMAT_XZ = 3,
-QEMU_SAVE_FORMAT_LZOP = 4,
-/* Note: add new members only at the end.
-   These values are used in the on-disk format.
-   Do not change or re-use numbers. */
-
-QEMU_SAVE_FORMAT_LAST
-} virQEMUSaveFormat;
-
-VIR_ENUM_DECL(qemuSaveCompression);
-VIR_ENUM_IMPL(qemuSaveCompression,
-  QEMU_SAVE_FORMAT_LAST,
-  "raw",
-  "gzip",
-  "bzip2",
-  "xz",
-  "lzop",
-);
-
-VIR_ENUM_DECL(qemuDumpFormat);
-VIR_ENUM_IMPL(qemuDumpFormat,
-  VIR_DOMAIN_CORE_DUMP_FORMAT_LAST,
-  "elf",
-  "kdump-zlib",
-  "kdump-lzo",
-  "kdump-snappy",
-);
-
-typedef struct _virQEMUSaveHeader virQEMUSaveHeader;
-typedef virQEMUSaveHeader *virQEMUSaveHeaderPtr;
-struct _virQEMUSaveHeader {
-char magic[sizeof(QEMU_SAVE_MAGIC)-1];
-uint32_t version;
-uint32_t data_len;
-uint32_t was_running;
-uint32_t compressed;
-uint32_t cookieOffset;
-uint32_t unused[14];
-};
-
-typedef struct _virQEMUSaveData virQEMUSaveData;
-typedef virQEMUSaveData *virQEMUSaveDataPtr;
-struct _virQEMUSaveData {
-virQEMUSaveHeader header;
-char *xml;
-char *cookie;
-};
-
-
-static inline void
-bswap_header(virQEMUSaveHeaderPtr hdr)
-{
-hdr->version = GUINT32_SWAP_LE_BE(hdr->version);
-hdr->data_len = GUINT32_SWAP_LE_BE(hdr->data_len);
-hdr->was_running = GUINT32_SWAP_LE_BE(hdr->was_running);
-hdr->compressed = GUINT32_SWAP_LE_BE(hdr->compressed);
-hdr->cookieOffset = GUINT32_SWAP_LE_BE(hdr->cookieOffset);
-}
-
-
-static void
-virQEMUSaveDataFree(virQEMUSaveDataPtr data)
-{
-if (!data)
-return;
-
-VIR_FREE(data->xml);
-VIR_FREE(data->cookie);
-VIR_FREE(data);
-}
-
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUSaveData, virQEMUSaveDataFree);
-
-/**
- * This function steals @domXML on success.
- */
-static virQEMUSaveDataPtr
-virQEMUSaveDataNew(char *domXML,
-   qemuDomainSaveCookiePtr cookieObj,
-   bool running,
-   int compressed,
-   virDomainXMLOptionPtr xmlopt)
-{
-virQEMUSaveDataPtr data = NULL;
-virQEMUSaveHeaderPtr header;
-
-if (VIR_ALLOC(data) < 0)
-return NULL;
-
-data->xml = g_steal_pointer();
-
-if (cookieObj &&
-!(data->cookie = virSaveCookieFormat((virObjectPtr) 

[PATCH] kbase: Add knowledge base for libvirt systemtap

[Han Han]

Signed-off-by: Han Han 
---
 docs/kbase.rst   |   3 ++
 docs/kbase/meson.build   |   1 +
 docs/kbase/systemtap.rst | 113 +++
 3 files changed, 117 insertions(+)
 create mode 100644 docs/kbase/systemtap.rst

diff --git a/docs/kbase.rst b/docs/kbase.rst
index 78daaa5989..d8bff5d41e 100644
--- a/docs/kbase.rst
+++ b/docs/kbase.rst
@@ -36,6 +36,9 @@ Knowledge base
   Examination of the security protections used for QEMU and how they need
   configuring to allow use of QEMU passthrough with host files/devices.
 
+   `Systemtap `__
+  Explanation of how to use systemtap for libvirt tracing.
+
`Virtio-FS `__
   Share a filesystem between the guest and the host
 
diff --git a/docs/kbase/meson.build b/docs/kbase/meson.build
index d7f254e163..ca032a4b9b 100644
--- a/docs/kbase/meson.build
+++ b/docs/kbase/meson.build
@@ -13,6 +13,7 @@ docs_kbase_files = [
   'rpm-deployment',
   's390_protected_virt',
   'secureusage',
+  'systemtap',
   'virtiofs',
 ]
 
diff --git a/docs/kbase/systemtap.rst b/docs/kbase/systemtap.rst
new file mode 100644
index 00..34420efbb2
--- /dev/null
+++ b/docs/kbase/systemtap.rst
@@ -0,0 +1,113 @@
+===
+Systemtap of Libvirt
+===
+
+.. contents::
+
+`Systemtap `__ is a scripting
+language and tool for dynamically probing or tracing in Linux kernel
+space or user space. This page is about the usage of systemtap
+in libvirt tracing.
+
+Preparation
+===
+
+Libvirt
+-
+
+Libvirt should be configured with the systemtap option to support libvirt
+probing events in systemtap.
+
+For libvirt before **6.7.0**, it can be configured by:
+
+::
+
+mkdir build
+cd build
+   ../configure --with-dtrace
+
+For libvirt **6.7.0** or later, configure it by the ``meson`` (seeing
+`libvirt compiling `__):
+
+::
+
+   meson build -Ddtrace=enabled
+
+For the libvirt binaries installed by the package manager like ``dnf`` or
+``apt``, if libvirt systemtap tapset ``/usr/share/systemtap/tapset/libvirt_*``
+exists, it means the libvirt enables the systemtap feature.
+
+Systemtap
+
+
+For most of linux distributions, execute ``stap-prep`` by root to prepare the
+environment for systemtap after installing the systemtap. If your distribution
+doesn't have ``stap-prep``, install the ``kernel debuginfo`` packages manually.
+
+After these above, run this test command to confirm the systemtap works well:
+
+::
+
+   stap -e 'probe oneshot{ printf("hello world\n")}'
+
+
+Tracing events
+===
+
+The libvirt systemtap tracing events are defined in tapset
+``/usr/share/systemtap/tapset/libvirt_*``. Libvirt support these type of 
tracing
+events: ``dbus``, ``event_glib``, ``object``, ``qemu``, ``rpc``.
+
+List all tracing events in libvirt:
+
+::
+
+   grep 'probe libvirt.[a-z_0-9.]*' /usr/share/systemtap/tapset/libvirt_* 
-o|cut -f 2 -d :
+
+
+Tracing examples
+==
+
+Here is an example of the systemtap script to trace the QMP messages sent from 
libvirtd
+daemon to the qemu process.
+``qmp.stp``:
+
+::
+
+   probe begin
+   {
+ printf("Start tracing\n")
+   }
+   probe libvirt.qemu.monitor_send_msg
+   {
+ printf("QMPs: %s", msg);
+   }
+
+Then run the systemtap script attaching to the libvirtd process:
+
+::
+
+   stap qmp.stp -x `pidof libvirtd`
+
+
+To trace a libvirtd started from command line, use the option ``-c``
+
+::
+
+   stap qmp.stp -c "/usr/sbin/libvirtd"
+
+
+Then after seeing the welcome message "Start tracing" from systemtap, then 
execute a virsh
+command associated with QMP, for example ``virsh domstats``. Then get the QMP 
tracing logs
+from systemtap. For example, the result from ``virsh domstats``
+
+::
+
+   QMPs: {"execute":"query-balloon","id":"libvirt-393"}
+   QMPs: 
{"execute":"qom-get","arguments":{"path":"/machine/peripheral/balloon0","property":"guest-stats"},"id":"libvirt-394"}
+   QMPs: {"execute":"query-blockstats","id":"libvirt-395"}
+   QMPs: {"execute":"query-named-block-nodes","id":"libvirt-396"}
+   QMPs: {"execute":"query-iothreads","id":"libvirt-397"}
+
+For more examples of libvirt systemtap scripts, see the scripts in 
``/usr/share/doc/libvirt-docs/examples/systemtap``
+For more details of systemtap language, see `document of systemtap 
`__
-- 
2.27.0