Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier

[Jason Wang]

On 2020/7/8 上午3:54, Peter Xu wrote:

On Tue, Jul 07, 2020 at 04:03:10PM +0800, Jason Wang wrote:

On 2020/7/3 下午9:03, Peter Xu wrote:

On Fri, Jul 03, 2020 at 03:24:19PM +0800, Jason Wang wrote:

On 2020/7/2 下午11:45, Peter Xu wrote:

On Thu, Jul 02, 2020 at 11:01:54AM +0800, Jason Wang wrote:

So I think we agree that a new notifier is needed?

Good to me, or a new flag should be easier (IOMMU_NOTIFIER_DEV_IOTLB)?

That should work but I wonder something as following is better.

Instead of introducing new flags, how about carry the type of event in the
notifier then the device (vhost) can choose the message it want to process
like:

static vhost_iommu_event(IOMMUNotifier *n, IOMMUTLBEvent *event)

{

switch (event->type) {

case IOMMU_MAP:
case IOMMU_UNMAP:
case IOMMU_DEV_IOTLB_UNMAP:
...

}

Looks ok to me, though imo we should still keep the registration information,
so VT-d knows which notifiers is interested in which events.  E.g., we can
still do something like vtd_as_has_map_notifier().


Is this for a better performance?

I wonder whether it's worth since we can't not have both vhost and vtd to be
registered into the same as.

/me failed to follow this sentence.. :(



Sorry, I meant "vfio" instead "vtd".





So it should be functional equivalent to vtd_as_has_notifier().

For example: in vtd_iommu_replay() we'll skip the replay if vhost has
registered the iommu notifier because vtd_as_has_map_notifier() will return
false.



Two questions:

- Do we care the performance here? If not, vhost may just ignore the MAP 
event?
- If we care the performance, it's better to implement the MAP event for 
vhost, otherwise it could be a lot of IOTLB miss


Thanks



   It'll only return true if the device is a vfio-pci device.

Without vtd_as_has_map_notifier(), how could we do that?

[PATCH] docs/index.html.in: Add knowledge base link and description on index page

[Jianan Gao]

Add link and description of libvirt knowledge base to make it
easier for users and testers to understand libvirt.

Signed-off-by: Jianan Gao 
---
 docs/index.html.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/docs/index.html.in b/docs/index.html.in
index 26e8406917..586defff54 100644
--- a/docs/index.html.in
+++ b/docs/index.html.in
@@ -63,6 +63,9 @@
   backup jobs
 http://wiki.libvirt.org;>Wiki
 Read further community contributed content
+
+https://libvirt.org/kbase.html;>Knowledge base
+Learn more about libvirt through knowledge base
   
 
 
-- 
2.21.3

How to best handle NULL return from xmlNodeGetContent()

[Laine Stump]

libvirt has several uses of xmlNodeGetContent() (from libxml2) added at 
different times over the years. Some of those uses report an Out of 
Memory error when xmlNodeGetContent() returns NULL, and some of them 
ignore a NULL return (treating it as if it were ""), and some just 
assume that the return will never be NULL, but always at least a pointer 
to "".


I ran across this when I noticed a usage of the latter type - it wasn't 
checking for NULL at all. A lack of check seemed troubling, so I looked 
at other uses within libvirt and found the hodge-podge described above, 
so no help there in determining the right thing to do. I then looked at 
the libxml2 documentation for xmlNodeGetContent(), which says:


  Returns: a new #xmlChar * or NULL if no content is available.

To an uninformed outsider, this sounds like the function could return 
NULL simply if the node was empty (e.g. ""). But when we look at 
the return from xmlNodeGetContent() for this example, it says that the 
content is "", not NULL.


In the meantime, since libxml doesn't abort on OOM errors (as libvirt 
does), it could also be possible that it's returning NULL due to OOM. So 
using anecdotal evidence acquired so far, one *could* surmise that any 
time libvirt gets a NULL return from xmlNodeGetContent(), it is indeed 
an OOM error.


The purist in me thinks that isn't right, though - I took a quick look 
at the libxml code and saw cases where  it returns NULL that don't seem 
related to OOM, but rather to the type of node or something. But being 
an outsider and not wanting to learn any more than necessary about the 
internals of libxml, I'm not sure if any of those cases even apply to 
libvirt's simple use of xmlNodeGetContent().


So, in the end I just want to modify libvirt's dozen or so calls to 
xmlNodeGetContent() to consistently do the right thing, but first I want 
to learn the true answers to these questions:


1) Keeping in mind that we've already successfully parsed the XML, will 
calls to xmlNodeGetContent() in the simple cases as when libvirt calls 
it only return NULL for OOM, but not for any other reason?


2) If not, is the proper way to distinguish OOM in this case to call 
xmlGetLasterror(), and check if the domain is XML_FROM_MEMORY?


3) Aside from returning NULL in the case of errors, would it ever be 
possible for correct XML to return NULL as valid "node content", or is 
it always an error of some kind?


Since libvirt now aborts on OOM, an OOM error could be handled in one 
place by a wrapper function around xmlNodeGetContent() (we already have 
such a function, currently a one-liner passthrough, and not called by 
everyone). But if there is any chance that any other libxml error could 
be encountered, then I suppose we really should be reporting those 
without aborting, and then still checking for NULL on return from the 
wrapper function (presumably by just logging the contents of "message" 
from the xmlErrorPtr returned from xmlGetLastError().

[libvirt PATCH v2 07/15] network: use g_free() in place of remaining VIR_FREE()

[Laine Stump]

Signed-off-by: Laine Stump 
---
 src/network/bridge_driver.c | 45 +++--
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 79b2ca3330..7d81d4dd78 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -158,7 +158,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata)
 
 virStringListFreeCount(def->options, def->noptions);
 
-VIR_FREE(def);
+g_free(def);
 }
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(networkDnsmasqXmlNsDef, 
networkDnsmasqDefNamespaceFree);
 
@@ -707,7 +707,7 @@ networkStateInitialize(bool privileged,
 
 network_driver->lockFD = -1;
 if (virMutexInit(_driver->lock) < 0) {
-VIR_FREE(network_driver);
+g_clear_pointer(_driver, g_free);
 goto error;
 }
 
@@ -875,18 +875,18 @@ networkStateCleanup(void)
 virPidFileRelease(network_driver->stateDir, "driver",
   network_driver->lockFD);
 
-VIR_FREE(network_driver->networkConfigDir);
-VIR_FREE(network_driver->networkAutostartDir);
-VIR_FREE(network_driver->stateDir);
-VIR_FREE(network_driver->pidDir);
-VIR_FREE(network_driver->dnsmasqStateDir);
-VIR_FREE(network_driver->radvdStateDir);
+g_free(network_driver->networkConfigDir);
+g_free(network_driver->networkAutostartDir);
+g_free(network_driver->stateDir);
+g_free(network_driver->pidDir);
+g_free(network_driver->dnsmasqStateDir);
+g_free(network_driver->radvdStateDir);
 
 virObjectUnref(network_driver->dnsmasqCaps);
 
 virMutexDestroy(_driver->lock);
 
-VIR_FREE(network_driver);
+g_clear_pointer(_driver, g_free);
 
 return 0;
 }
@@ -2194,7 +2194,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
 /* Prevent guests from hijacking the host network by sending out
  * their own router advertisements.
  */
-VIR_FREE(field);
+g_free(field);
 field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/accept_ra",
 def->bridge);
 
@@ -2207,7 +2207,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
 /* All interfaces used as a gateway (which is what this is, by
  * definition), must always have autoconf=0.
  */
-VIR_FREE(field);
+g_free(field);
 field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/autoconf", 
def->bridge);
 
 if (virFileWriteStr(field, "0", 0) < 0) {
@@ -2714,19 +2714,19 @@ networkCreateInterfacePool(virNetworkDefPtr netdef)
 for (i = 0; i < netdef->forward.nifs; i++) {
 if (netdef->forward.ifs[i].type
 == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV)
-VIR_FREE(netdef->forward.ifs[i].device.dev);
+g_free(netdef->forward.ifs[i].device.dev);
 }
 netdef->forward.nifs = 0;
 }
 if (netdef->forward.nifs == 0)
-VIR_FREE(netdef->forward.ifs);
+g_clear_pointer(>forward.ifs, g_free);
 
 for (i = 0; i < numVirtFns; i++) {
-VIR_FREE(vfNames[i]);
-VIR_FREE(virtFns[i]);
+g_free(vfNames[i]);
+g_free(virtFns[i]);
 }
-VIR_FREE(vfNames);
-VIR_FREE(virtFns);
+g_free(vfNames);
+g_free(virtFns);
 return ret;
 }
 
@@ -3162,7 +3162,7 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
  */
 if (!(virNetworkObjBridgeInUse(nets, newname, def->name) ||
   virNetDevExists(newname) == 1)) {
-VIR_FREE(def->bridge); /*could contain template */
+g_free(def->bridge); /*could contain template */
 def->bridge = g_steal_pointer();
 return 0;
 }
@@ -4272,7 +4272,7 @@ networkGetDHCPLeases(virNetworkPtr net,
 if (leases_ret) {
 for (i = 0; i < nleases; i++)
 virNetworkDHCPLeaseFree(leases_ret[i]);
-VIR_FREE(leases_ret);
+g_free(leases_ret);
 }
 goto cleanup;
 }
@@ -4396,7 +4396,7 @@ networkAllocatePort(virNetworkObjPtr obj,
 return -1;
 }
 if (portprofile) {
-VIR_FREE(port->virtPortProfile);
+g_free(port->virtPortProfile);
 port->virtPortProfile = portprofile;
 }
 
@@ -5513,9 +5513,10 @@ networkPortSetParameters(virNetworkPortPtr port,
  * So if no average or floor is given, we free inbound/outbound
  * here which causes inbound/outbound to not be set. */
 if (!bandwidth->in->average && !bandwidth->in->floor)
-VIR_FREE(bandwidth->in);
+g_clear_pointer(>in, g_free);
+
 if (!bandwidth->out->average)
-VIR_FREE(bandwidth->out);
+g_clear_pointer(>out, g_free);
 
 if (networkUpdatePortBandwidth(obj,
>mac,
-- 
2.25.4

[libvirt PATCH v2 15/15] nwfilter: convert remaining VIR_FREE() to g_free()

[Laine Stump]

Signed-off-by: Laine Stump 
---
 src/nwfilter/nwfilter_dhcpsnoop.c | 16 
 src/nwfilter/nwfilter_driver.c| 10 +-
 src/nwfilter/nwfilter_ebiptables_driver.c |  2 +-
 src/nwfilter/nwfilter_gentech_driver.c|  6 +++---
 src/nwfilter/nwfilter_learnipaddr.c   |  8 
 5 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 64671af135..aafa6de322 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -314,7 +314,7 @@ virNWFilterSnoopCancel(char **threadKey)
 virNWFilterSnoopActiveLock();
 
 ignore_value(virHashRemoveEntry(virNWFilterSnoopState.active, *threadKey));
-VIR_FREE(*threadKey);
+g_free(*threadKey);
 
 virNWFilterSnoopActiveUnlock();
 }
@@ -600,7 +600,7 @@ virNWFilterSnoopReqFree(virNWFilterSnoopReqPtr req)
 virCondDestroy(>threadStatusCond);
 virFreeError(req->threadError);
 
-VIR_FREE(req);
+g_free(req);
 }
 
 /*
@@ -731,7 +731,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req,
 
 if (req->threadkey && virNWFilterSnoopIPLeaseInstallRule(pl, true) < 0) {
 virNWFilterSnoopReqUnlock(req);
-VIR_FREE(pl);
+g_free(pl);
 return -1;
 }
 
@@ -850,7 +850,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
 }
 
  skip_instantiate:
-VIR_FREE(ipl);
+g_free(ipl);
 
 ignore_value(!!g_atomic_int_dec_and_test());
 
@@ -1149,7 +1149,7 @@ virNWFilterSnoopDHCPDecodeJobSubmit(virThreadPoolPtr pool,
 if (ret == 0)
 g_atomic_int_add(qCtr, 1);
 else
-VIR_FREE(job);
+g_free(job);
 
 return ret;
 }
@@ -1502,7 +1502,7 @@ virNWFilterDHCPSnoopThread(void *req0)
 ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey,
 req->binding->portdevname));
 
-VIR_FREE(req->binding->portdevname);
+g_clear_pointer(>binding->portdevname, g_free);
 
 virNWFilterSnoopReqUnlock(req);
 virNWFilterSnoopUnlock();
@@ -1970,7 +1970,7 @@ virNWFilterSnoopRemAllReqIter(const void *payload,
  */
 virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname, NULL);
 
-VIR_FREE(req->binding->portdevname);
+g_clear_pointer(>binding->portdevname, g_free);
 }
 
 virNWFilterSnoopReqUnlock(req);
@@ -2079,7 +2079,7 @@ virNWFilterDHCPSnoopEnd(const char *ifname)
 /* keep valid lease req; drop interface association */
 virNWFilterSnoopCancel(>threadkey);
 
-VIR_FREE(req->binding->portdevname);
+g_clear_pointer(>binding->portdevname, g_free);
 
 virNWFilterSnoopReqUnlock(req);
 
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 39d0a2128e..7853ad59fa 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -303,7 +303,7 @@ nwfilterStateInitialize(bool privileged,
 
  err_free_driverstate:
 virNWFilterObjListFree(driver->nwfilters);
-VIR_FREE(driver);
+g_free(driver);
 
 return VIR_DRV_STATE_INIT_ERROR;
 }
@@ -367,9 +367,9 @@ nwfilterStateCleanup(void)
 if (driver->lockFD != -1)
 virPidFileRelease(driver->stateDir, "driver", driver->lockFD);
 
-VIR_FREE(driver->stateDir);
-VIR_FREE(driver->configDir);
-VIR_FREE(driver->bindingDir);
+g_free(driver->stateDir);
+g_free(driver->configDir);
+g_free(driver->bindingDir);
 nwfilterDriverUnlock();
 }
 
@@ -379,7 +379,7 @@ nwfilterStateCleanup(void)
 virNWFilterObjListFree(driver->nwfilters);
 
 virMutexDestroy(>lock);
-VIR_FREE(driver);
+g_free(driver);
 
 return 0;
 }
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c 
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 9c9d63f14b..6aefbe226b 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3517,7 +3517,7 @@ ebiptablesApplyNewRules(const char *ifname,
 
  cleanup:
 for (i = 0; i < nsubchains; i++)
-VIR_FREE(subchains[i]);
+g_free(subchains[i]);
 
 return ret;
 }
diff --git a/src/nwfilter/nwfilter_gentech_driver.c 
b/src/nwfilter/nwfilter_gentech_driver.c
index 071f15caea..c93f2ed18f 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -122,7 +122,7 @@ virNWFilterRuleInstFree(virNWFilterRuleInstPtr inst)
 return;
 
 virHashFree(inst->vars);
-VIR_FREE(inst);
+g_free(inst);
 }
 
 
@@ -234,12 +234,12 @@ virNWFilterInstReset(virNWFilterInstPtr inst)
 
 for (i = 0; i < inst->nfilters; i++)
 virNWFilterObjUnlock(inst->filters[i]);
-VIR_FREE(inst->filters);
+g_free(inst->filters);
 inst->nfilters = 0;
 
 for (i = 0; i < inst->nrules; i++)
 virNWFilterRuleInstFree(inst->rules[i]);
-VIR_FREE(inst->rules);
+g_free(inst->rules);
 inst->nrules = 0;
 }
 
diff 

[libvirt PATCH v2 01/15] replace g_new() with g_new0() for consistency

[Laine Stump]

g_new() is used in only 3 places. Switching them to g_new0() will do
no harm, reduces confusion, and helps me sleep better at night knowing
that all allocated memory is initialized to 0 :-) (Yes, I *know* that
in all three cases the associated memory is immediately assigned some
other value. Today.)

Signed-off-by: Laine Stump 
---
 src/qemu/qemu_backup.c  | 2 +-
 src/util/virutil.c  | 2 +-
 tests/qemuhotplugmock.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index 8dc9d2504d..dae9300567 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -64,7 +64,7 @@ qemuBackupPrepare(virDomainBackupDefPtr def)
 
 if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL) {
 if (!def->server) {
-def->server = g_new(virStorageNetHostDef, 1);
+def->server = g_new0(virStorageNetHostDef, 1);
 
 def->server->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
 def->server->name = g_strdup("localhost");
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 04f882fda7..ff664ea778 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -962,7 +962,7 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
 if (uid != (uid_t)-1 &&
 virGetUserEnt(uid, , , NULL, NULL, true) >= 0) {
 int nallocgrps = 10;
-gid_t *grps = g_new(gid_t, nallocgrps);
+gid_t *grps = g_new0(gid_t, nallocgrps);
 
 while (1) {
 int nprevallocgrps = nallocgrps;
diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c
index d2324913cf..29fac8a598 100644
--- a/tests/qemuhotplugmock.c
+++ b/tests/qemuhotplugmock.c
@@ -57,7 +57,7 @@ virDevMapperGetTargets(const char *path,
 *devPaths = NULL;
 
 if (STREQ(path, "/dev/mapper/virt")) {
-*devPaths = g_new(char *, 4);
+*devPaths = g_new0(char *, 4);
 (*devPaths)[0] = g_strdup("/dev/block/8:0");  /* /dev/sda */
 (*devPaths)[1] = g_strdup("/dev/block/8:16"); /* /dev/sdb */
 (*devPaths)[2] = g_strdup("/dev/block/8:32"); /* /dev/sdc */
-- 
2.25.4

[libvirt PATCH v2 05/15] network: use g_auto wherever appropriate

[Laine Stump]

This includes standard g_autofree() as well as other objects that have
a cleanup function defined to use via g_autoptr (virCommand,
virJSONValue)

Signed-off-by: Laine Stump 
---
 src/network/bridge_driver.c   | 206 ++
 src/network/bridge_driver_linux.c |   7 +-
 src/network/leaseshelper.c|  16 +--
 3 files changed, 76 insertions(+), 153 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ab359acdb5..31bd0dd92c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -160,6 +160,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata)
 
 VIR_FREE(def);
 }
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(networkDnsmasqXmlNsDef, 
networkDnsmasqDefNamespaceFree);
 
 
 static int
@@ -195,7 +196,7 @@ static int
 networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
 void **data)
 {
-networkDnsmasqXmlNsDefPtr nsdata = g_new0(networkDnsmasqXmlNsDef, 1);
+g_autoptr(networkDnsmasqXmlNsDef) nsdata = g_new0(networkDnsmasqXmlNsDef, 
1);
 int ret = -1;
 
 if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
@@ -207,7 +208,6 @@ networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
 ret = 0;
 
  cleanup:
-networkDnsmasqDefNamespaceFree(nsdata);
 return ret;
 }
 
@@ -329,7 +329,7 @@ networkRunHook(virNetworkObjPtr obj,
 {
 virNetworkDefPtr def;
 g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-char *xml = NULL;
+g_autofree char *xml = NULL;
 int hookret;
 int ret = -1;
 
@@ -366,7 +366,6 @@ networkRunHook(virNetworkObjPtr obj,
 
 ret = 0;
  cleanup:
-VIR_FREE(xml);
 return ret;
 }
 
@@ -431,14 +430,14 @@ static int
 networkRemoveInactive(virNetworkDriverStatePtr driver,
   virNetworkObjPtr obj)
 {
-char *leasefile = NULL;
-char *customleasefile = NULL;
-char *radvdconfigfile = NULL;
-char *configfile = NULL;
-char *radvdpidbase = NULL;
-char *statusfile = NULL;
-char *macMapFile = NULL;
-dnsmasqContext *dctx = NULL;
+g_autofree char *leasefile = NULL;
+g_autofree char *customleasefile = NULL;
+g_autofree char *radvdconfigfile = NULL;
+g_autofree char *configfile = NULL;
+g_autofree char *radvdpidbase = NULL;
+g_autofree char *statusfile = NULL;
+g_autofree char *macMapFile = NULL;
+g_autoptr(dnsmasqContext) dctx = NULL;
 virNetworkDefPtr def = virNetworkObjGetPersistentDef(obj);
 
 int ret = -1;
@@ -492,14 +491,6 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
 ret = 0;
 
  cleanup:
-VIR_FREE(leasefile);
-VIR_FREE(configfile);
-VIR_FREE(customleasefile);
-VIR_FREE(radvdconfigfile);
-VIR_FREE(radvdpidbase);
-VIR_FREE(statusfile);
-VIR_FREE(macMapFile);
-dnsmasqContextFree(dctx);
 return ret;
 }
 
@@ -550,9 +541,9 @@ networkUpdateState(virNetworkObjPtr obj,
 {
 virNetworkDefPtr def;
 virNetworkDriverStatePtr driver = opaque;
-dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver);
+g_autoptr(dnsmasqCaps) dnsmasq_caps = networkGetDnsmasqCaps(driver);
 virMacMapPtr macmap;
-char *macMapFile = NULL;
+g_autofree char *macMapFile = NULL;
 int ret = -1;
 
 virObjectLock(obj);
@@ -614,7 +605,7 @@ networkUpdateState(virNetworkObjPtr obj,
 if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) {
 pid_t radvdPid;
 pid_t dnsmasqPid;
-char *radvdpidbase;
+g_autofree char *radvdpidbase = NULL;
 
 ignore_value(virPidFileReadIfAlive(driver->pidDir,
def->name,
@@ -630,14 +621,11 @@ networkUpdateState(virNetworkObjPtr obj,
radvdpidbase,
, RADVD));
 virNetworkObjSetRadvdPid(obj, radvdPid);
-VIR_FREE(radvdpidbase);
 }
 
 ret = 0;
  cleanup:
 virObjectUnlock(obj);
-virObjectUnref(dnsmasq_caps);
-VIR_FREE(macMapFile);
 return ret;
 }
 
@@ -716,8 +704,8 @@ networkStateInitialize(bool privileged,
void *opaque G_GNUC_UNUSED)
 {
 int ret = VIR_DRV_STATE_INIT_ERROR;
-char *configdir = NULL;
-char *rundir = NULL;
+g_autofree char *configdir = NULL;
+g_autofree char *rundir = NULL;
 bool autostart = true;
 #ifdef WITH_FIREWALLD
 DBusConnection *sysbus = NULL;
@@ -845,8 +833,6 @@ networkStateInitialize(bool privileged,
 
 ret = VIR_DRV_STATE_INIT_COMPLETE;
  cleanup:
-VIR_FREE(configdir);
-VIR_FREE(rundir);
 return ret;
 
  error:
@@ -1047,10 +1033,11 @@ networkDnsmasqConfLocalPTRs(virBufferPtr buf,
 {
 virNetworkIPDefPtr ip;
 size_t i;
-char *ptr = NULL;
 int rc;
 
 for (i = 0; i < def->nips; i++) {
+g_autofree char *ptr = NULL;
+
 ip = def->ips + i;
 
 if (ip->localPTR != VIR_TRISTATE_BOOL_YES)
@@ -1071,7 +1058,6 @@ networkDnsmasqConfLocalPTRs(virBufferPtr 

[libvirt PATCH v2 12/15] nwfilter: use standard label names when reasonable

[Laine Stump]

Rather than having labels named exit, done, exit_snooprequnlock,
skip_rename, etc, use the standard "cleanup" label. And instead of
err_exit, malformed, tear_down_tmpebchains, use "error".

Signed-off-by: Laine Stump 
---
 src/nwfilter/nwfilter_dhcpsnoop.c | 36 +++
 src/nwfilter/nwfilter_ebiptables_driver.c | 12 
 src/nwfilter/nwfilter_gentech_driver.c| 32 ++--
 src/nwfilter/nwfilter_learnipaddr.c   | 24 +++
 4 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index f530341872..6de41ff209 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -456,11 +456,11 @@ 
virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
 virNWFilterSnoopReqLock(req);
 
 if (virNWFilterIPAddrMapAddIPAddr(req->binding->portdevname, ipaddr) < 0)
-goto exit_snooprequnlock;
+goto cleanup;
 
 if (!instantiate) {
 rc = 0;
-goto exit_snooprequnlock;
+goto cleanup;
 }
 
 /* instantiate the filters */
@@ -471,7 +471,7 @@ 
virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
   req->ifindex);
 }
 
- exit_snooprequnlock:
+ cleanup:
 virNWFilterSnoopReqUnlock(req);
 
 VIR_FREE(ipaddr);
@@ -732,7 +732,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req,
 
 virNWFilterSnoopReqUnlock(req);
 
-goto exit;
+goto cleanup;
 }
 
 virNWFilterSnoopReqUnlock(req);
@@ -757,7 +757,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req,
 
 g_atomic_int_add(, 1);
 
- exit:
+ cleanup:
 if (update_leasefile)
 virNWFilterSnoopLeaseFileSave(pl);
 
@@ -902,7 +902,7 @@ virNWFilterSnoopDHCPGetOpt(virNWFilterSnoopDHCPHdrPtr pd, 
int len,
 switch (pd->d_opts[oind]) {
 case DHCPO_LEASE:
 if (olen - oind < 6)
-goto malformed;
+goto error;
 if (*pleasetime)
 return -1;  /* duplicate lease time */
 memcpy(, (char *)pd->d_opts + oind + 2, sizeof(nwint));
@@ -910,7 +910,7 @@ virNWFilterSnoopDHCPGetOpt(virNWFilterSnoopDHCPHdrPtr pd, 
int len,
 break;
 case DHCPO_MTYPE:
 if (olen - oind < 3)
-goto malformed;
+goto error;
 if (*pmtype)
 return -1;  /* duplicate message type */
 *pmtype = pd->d_opts[oind + 2];
@@ -922,12 +922,12 @@ virNWFilterSnoopDHCPGetOpt(virNWFilterSnoopDHCPHdrPtr pd, 
int len,
 return 0;
 default:
 if (olen - oind < 2)
-goto malformed;
+goto error;
 }
 oind += pd->d_opts[oind + 1] + 2;
 }
 return 0;
- malformed:
+ error:
 VIR_WARN("got lost in the options!");
 return -1;
 }
@@ -1386,7 +1386,7 @@ virNWFilterDHCPSnoopThread(void *req0)
 virNWFilterSnoopReqUnlock(req);
 
 if (req->threadStatus != THREAD_STATUS_OK)
-goto exit;
+goto cleanup;
 
 while (!error) {
 if (virNWFilterSnoopAdjustPoll(pcapConf,
@@ -1414,7 +1414,7 @@ virNWFilterDHCPSnoopThread(void *req0)
  */
 if (!virNWFilterSnoopIsActive(threadkey) ||
 req->jobCompletionStatus != 0)
-goto exit;
+goto cleanup;
 
 for (i = 0; n > 0 && i < G_N_ELEMENTS(fds); i++) {
 if (!fds[i].revents)
@@ -1531,7 +1531,7 @@ virNWFilterDHCPSnoopThread(void *req0)
 virNWFilterSnoopReqUnlock(req);
 virNWFilterSnoopUnlock();
 
- exit:
+ cleanup:
 virThreadPoolFree(worker);
 
 virNWFilterSnoopReqPut(req);
@@ -1774,14 +1774,14 @@ 
virNWFilterSnoopLeaseFileSave(virNWFilterSnoopIPLeasePtr ipl)
 virNWFilterSnoopLeaseFileOpen();
 if (virNWFilterSnoopLeaseFileWrite(virNWFilterSnoopState.leaseFD,
req->ifkey, ipl) < 0)
-goto err_exit;
+goto error;
 
 /* keep dead leases at < ~95% of file size */
 if (g_atomic_int_add(, 1) >=
 g_atomic_int_get() * 20)
 virNWFilterSnoopLeaseFileLoad();   /* load & refresh lease file */
 
- err_exit:
+ error:
 virNWFilterSnoopUnlock();
 }
 
@@ -1876,7 +1876,7 @@ virNWFilterSnoopLeaseFileRefresh(void)
 if (VIR_CLOSE(tfd) < 0) {
 virReportSystemError(errno, _("unable to close %s"), TMPLEASEFILE);
 /* assuming the old lease file is still better, skip the renaming */
-goto skip_rename;
+goto cleanup;
 }
 
 if (rename(TMPLEASEFILE, LEASEFILE) < 0) {
@@ -1886,7 +1886,7 @@ virNWFilterSnoopLeaseFileRefresh(void)
 }
 g_atomic_int_set(, 0);
 
- skip_rename:
+ cleanup:
 virNWFilterSnoopLeaseFileOpen();
 }
 
@@ -2051,14 +2051,14 @@ virNWFilterDHCPSnoopInit(void)
 if (!virNWFilterSnoopState.ifnameToKey ||
 !virNWFilterSnoopState.snoopReqs ||
 

[libvirt PATCH v2 13/15] nwfilter: replace VIR_ALLOC with g_new0

[Laine Stump]

Signed-off-by: Laine Stump 
---
 src/nwfilter/nwfilter_dhcpsnoop.c | 9 +++--
 src/nwfilter/nwfilter_driver.c| 3 +--
 src/nwfilter/nwfilter_ebiptables_driver.c | 3 +--
 src/nwfilter/nwfilter_gentech_driver.c| 3 +--
 src/nwfilter/nwfilter_learnipaddr.c   | 6 ++
 5 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 6de41ff209..4bc1607694 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -562,8 +562,7 @@ virNWFilterSnoopReqNew(const char *ifkey)
 return NULL;
 }
 
-if (VIR_ALLOC(req) < 0)
-return NULL;
+req = g_new0(virNWFilterSnoopReq, 1);
 
 req->threadStatus = THREAD_STATUS_NONE;
 
@@ -737,8 +736,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req,
 
 virNWFilterSnoopReqUnlock(req);
 
-if (VIR_ALLOC(pl) < 0)
-return -1;
+pl = g_new0(virNWFilterSnoopIPLease, 1);
 *pl = *plnew;
 
 /* protect req->threadkey */
@@ -1160,8 +1158,7 @@ virNWFilterSnoopDHCPDecodeJobSubmit(virThreadPoolPtr pool,
 if (len <= MIN_VALID_DHCP_PKT_SIZE || len > sizeof(job->packet))
 return 0;
 
-if (VIR_ALLOC(job) < 0)
-return -1;
+job = g_new0(virNWFilterDHCPDecodeJob, 1);
 
 memcpy(job->packet, pep, len);
 job->caplen = len;
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 1c407727db..39d0a2128e 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -193,8 +193,7 @@ nwfilterStateInitialize(bool privileged,
 !(sysbus = virDBusGetSystemBus()))
 return VIR_DRV_STATE_INIT_ERROR;
 
-if (VIR_ALLOC(driver) < 0)
-return VIR_DRV_STATE_INIT_ERROR;
+driver = g_new0(virNWFilterDriverState, 1);
 
 driver->lockFD = -1;
 if (virMutexInit(>lock) < 0)
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c 
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 8ac3a7271e..177e7e62b9 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3312,8 +3312,7 @@ ebtablesGetSubChainInsts(virHashTablePtr chains,
 if ((int)idx < 0)
 continue;
 
-if (VIR_ALLOC(inst) < 0)
-goto cleanup;
+inst = g_new0(ebtablesSubChainInst, 1);
 inst->priority = *(const virNWFilterChainPriority 
*)filter_names[i].value;
 inst->incoming = incoming;
 inst->protoidx = idx;
diff --git a/src/nwfilter/nwfilter_gentech_driver.c 
b/src/nwfilter/nwfilter_gentech_driver.c
index 400d064724..acd5614987 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -262,8 +262,7 @@ virNWFilterRuleDefToRuleInst(virNWFilterDefPtr def,
 virNWFilterRuleInstPtr ruleinst;
 int ret = -1;
 
-if (VIR_ALLOC(ruleinst) < 0)
-goto cleanup;
+ruleinst = g_new0(virNWFilterRuleInst, 1);
 
 ruleinst->chainSuffix = def->chainsuffix;
 ruleinst->chainPriority = def->chainPriority;
diff --git a/src/nwfilter/nwfilter_learnipaddr.c 
b/src/nwfilter/nwfilter_learnipaddr.c
index 95e21050b4..63fac37132 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -151,8 +151,7 @@ virNWFilterLockIface(const char *ifname)
 
 ifaceLock = virHashLookup(ifaceLockMap, ifname);
 if (!ifaceLock) {
-if (VIR_ALLOC(ifaceLock) < 0)
-goto error;
+ifaceLock = g_new0(virNWFilterIfaceLock, 1);
 
 if (virMutexInitRecursive(>lock) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -718,8 +717,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr 
techdriver,
 return -1;
 }
 
-if (VIR_ALLOC(req) < 0)
-return -1;
+req = g_new0(virNWFilterIPAddrLearnReq, 1);
 
 if (!(req->binding = virNWFilterBindingDefCopy(binding)))
 goto err_free_req;
-- 
2.25.4

[libvirt PATCH v2 00/15] convert network and nwfilter directories to glib memory allocation.

[Laine Stump]

V1 was here:

https://www.redhat.com/archives/libvir-list/2020-June/msg01156.html

Some patches were ACKed and pushed. I re-ordered/re-organized most of
the rest, and removed some others to deal with separately (the
xmlNodeContent stuff)

What's left here is a few preliminary patches, then the standard set,
once for network and again for nwfilter:

1) convert from VIR_(RE)ALLOC(_N) to g_new0()/g_renew()
2) use g_auto*() where appropriate, removing unneeded free's
3) get rid of now-extraneous labels
4) (controversial) replace any remaining VIR_FREE() with g_free() (and
   possibly g_clear_pointer() when needed

NB: these patches require my virBuffer "convert to g_auto" series
as a prerequisite:

  https://www.redhat.com/archives/libvir-list/2020-July/msg00185.html

Changes from V1:

  * move conversion of virFirewall and virBuffer automatics to another
series (see above)
  
  * re-order to replace VIR_ALLOC first (without adding any g_auto*)
instead of doing it after g_auto conversion of automatics, then do
all g_auto additions at o

  * separate label elimination into separate patches per jtomko's
suggestion.


Laine Stump (15):
  replace g_new() with g_new0() for consistency
  util: define g_autoptr cleanups for a couple dnsmasq objects
  define g_autoptr cleanup function for virNetworkDHCPLease
  network: replace VIR_ALLOC/REALLOC with g_new0/g_renew
  network: use g_auto wherever appropriate
  network: eliminate unnecessary labels
  network: use g_free() in place of remaining VIR_FREE()
  nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
  nwfilter: clear nrules when resetting virNWFilterInst
  nwfilter: define a typedef for struct ebtablesSubChainInst
  nwfilter: transform logic in virNWFilterRuleInstSort to eliminate
label
  nwfilter: use standard label names when reasonable
  nwfilter: replace VIR_ALLOC with g_new0
  nwfilter: convert local pointers to use g_auto*
  nwfilter: convert remaining VIR_FREE() to g_free()

 src/datatypes.h   |   2 +
 src/network/bridge_driver.c   | 536 --
 src/network/bridge_driver_linux.c |  22 +-
 src/network/leaseshelper.c|  16 +-
 src/nwfilter/nwfilter_dhcpsnoop.c | 150 +++---
 src/nwfilter/nwfilter_driver.c|  13 +-
 src/nwfilter/nwfilter_ebiptables_driver.c | 119 ++---
 src/nwfilter/nwfilter_gentech_driver.c|  57 ++-
 src/nwfilter/nwfilter_learnipaddr.c   |  43 +-
 src/qemu/qemu_backup.c|   2 +-
 src/util/virdnsmasq.h |   4 +
 src/util/virutil.c|   2 +-
 tests/qemuhotplugmock.c   |   2 +-
 13 files changed, 379 insertions(+), 589 deletions(-)

-- 
2.25.4

[libvirt PATCH v2 14/15] nwfilter: convert local pointers to use g_auto*

[Laine Stump]

Signed-off-by: Laine Stump 
---
 src/nwfilter/nwfilter_dhcpsnoop.c | 91 +++
 src/nwfilter/nwfilter_ebiptables_driver.c | 75 +++
 src/nwfilter/nwfilter_gentech_driver.c| 15 ++--
 src/nwfilter/nwfilter_learnipaddr.c   |  7 +-
 4 files changed, 61 insertions(+), 127 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 4bc1607694..64671af135 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -292,18 +292,17 @@ static const unsigned char dhcp_magic[4] = { 99, 130, 83, 
99 };
 static char *
 virNWFilterSnoopActivate(virNWFilterSnoopReqPtr req)
 {
-char *key;
-
-key = g_strdup_printf("%p-%d", req, req->ifindex);
+g_autofree char *key = g_strdup_printf("%p-%d", req, req->ifindex);
+char *ret = NULL;
 
 virNWFilterSnoopActiveLock();
 
-if (virHashAddEntry(virNWFilterSnoopState.active, key, (void *)0x1) < 0)
-VIR_FREE(key);
+if (virHashAddEntry(virNWFilterSnoopState.active, key, (void *)0x1) == 0)
+ret = g_steal_pointer();
 
 virNWFilterSnoopActiveUnlock();
 
-return key;
+return ret;
 }
 
 static void
@@ -442,11 +441,10 @@ static int
 virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
bool instantiate)
 {
-char *ipaddr;
+g_autofree char *ipaddr = virSocketAddrFormat(>ipAddress);
 int rc = -1;
 virNWFilterSnoopReqPtr req;
 
-ipaddr = virSocketAddrFormat(>ipAddress);
 if (!ipaddr)
 return -1;
 
@@ -473,9 +471,6 @@ 
virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
 
  cleanup:
 virNWFilterSnoopReqUnlock(req);
-
-VIR_FREE(ipaddr);
-
 return rc;
 }
 
@@ -551,7 +546,7 @@ virNWFilterSnoopReqGet(virNWFilterSnoopReqPtr req)
 static virNWFilterSnoopReqPtr
 virNWFilterSnoopReqNew(const char *ifkey)
 {
-virNWFilterSnoopReqPtr req;
+g_autofree virNWFilterSnoopReqPtr req = g_new0(virNWFilterSnoopReq, 1);
 
 if (ifkey == NULL || strlen(ifkey) != VIR_IFKEY_LEN - 1) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -562,28 +557,20 @@ virNWFilterSnoopReqNew(const char *ifkey)
 return NULL;
 }
 
-req = g_new0(virNWFilterSnoopReq, 1);
-
 req->threadStatus = THREAD_STATUS_NONE;
 
-if (virStrcpyStatic(req->ifkey, ifkey) < 0||
-virMutexInitRecursive(>lock) < 0)
-goto err_free_req;
+if (virStrcpyStatic(req->ifkey, ifkey) < 0 ||
+virMutexInitRecursive(>lock) < 0) {
+return NULL;
+}
 
-if (virCondInit(>threadStatusCond) < 0)
-goto err_destroy_mutex;
+if (virCondInit(>threadStatusCond) < 0) {
+virMutexDestroy(>lock);
+return NULL;
+}
 
 virNWFilterSnoopReqGet(req);
-
-return req;
-
- err_destroy_mutex:
-virMutexDestroy(>lock);
-
- err_free_req:
-VIR_FREE(req);
-
-return NULL;
+return g_steal_pointer();
 }
 
 /*
@@ -815,7 +802,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
 {
 int ret = 0;
 virNWFilterSnoopIPLeasePtr ipl;
-char *ipstr = NULL;
+g_autofree char *ipstr = NULL;
 
 /* protect req->start, req->ifname and the lease */
 virNWFilterSnoopReqLock(req);
@@ -868,8 +855,6 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
 ignore_value(!!g_atomic_int_dec_and_test());
 
  lease_not_found:
-VIR_FREE(ipstr);
-
 virNWFilterSnoopReqUnlock(req);
 
 return ret;
@@ -1045,7 +1030,7 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr 
*mac,
 pcap_t *handle = NULL;
 struct bpf_program fp;
 char pcap_errbuf[PCAP_ERRBUF_SIZE];
-char *ext_filter = NULL;
+g_autofree char *ext_filter = NULL;
 char macaddr[VIR_MAC_STRING_BUFLEN];
 
 virMacAddrFormat(mac, macaddr);
@@ -1075,7 +1060,7 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr 
*mac,
 if (handle == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("pcap_create failed"));
-goto cleanup_nohandle;
+return NULL;
 }
 
 if (pcap_set_snaplen(handle, PCAP_PBUFSIZE) < 0 ||
@@ -1107,17 +1092,12 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr 
*mac,
 }
 
 pcap_freecode();
-VIR_FREE(ext_filter);
-
 return handle;
 
  cleanup_freecode:
 pcap_freecode();
  cleanup:
 pcap_close(handle);
- cleanup_nohandle:
-VIR_FREE(ext_filter);
-
 return NULL;
 }
 
@@ -1128,7 +1108,7 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr 
*mac,
 static void virNWFilterDHCPDecodeWorker(void *jobdata, void *opaque)
 {
 virNWFilterSnoopReqPtr req = opaque;
-virNWFilterDHCPDecodeJobPtr job = jobdata;
+g_autofree virNWFilterDHCPDecodeJobPtr job = jobdata;
 virNWFilterSnoopEthHdrPtr packet = (virNWFilterSnoopEthHdrPtr)job->packet;
 
 if (virNWFilterSnoopDHCPDecode(req, packet,
@@ -1140,7 +1120,6 @@ static void virNWFilterDHCPDecodeWorker(void *jobdata, 
void 

[libvirt PATCH v2 10/15] nwfilter: define a typedef for struct ebtablesSubChainInst

[Laine Stump]

Signed-off-by: Laine Stump 
---
 src/nwfilter/nwfilter_ebiptables_driver.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c 
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 426212e0dc..cc0f3f93d9 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3269,7 +3269,9 @@ ebtablesRuleInstCommand(virFirewallPtr fw,
 return ret;
 }
 
-struct ebtablesSubChainInst {
+typedef struct _ebtablesSubChainInst ebtablesSubChainInst;
+typedef ebtablesSubChainInst *ebtablesSubChainInstPtr;
+struct _ebtablesSubChainInst {
 virNWFilterChainPriority priority;
 bool incoming;
 enum l3_proto_idx protoidx;
@@ -3280,8 +3282,8 @@ struct ebtablesSubChainInst {
 static int
 ebtablesSubChainInstSort(const void *a, const void *b)
 {
-const struct ebtablesSubChainInst **insta = (const struct 
ebtablesSubChainInst **)a;
-const struct ebtablesSubChainInst **instb = (const struct 
ebtablesSubChainInst **)b;
+const ebtablesSubChainInst **insta = (const ebtablesSubChainInst **)a;
+const ebtablesSubChainInst **instb = (const ebtablesSubChainInst **)b;
 
 /* priorities are limited to range [-1000, 1000] */
 return (*insta)->priority - (*instb)->priority;
@@ -3291,7 +3293,7 @@ ebtablesSubChainInstSort(const void *a, const void *b)
 static int
 ebtablesGetSubChainInsts(virHashTablePtr chains,
  bool incoming,
- struct ebtablesSubChainInst ***insts,
+ ebtablesSubChainInstPtr **insts,
  size_t *ninsts)
 {
 virHashKeyValuePairPtr filter_names;
@@ -3304,7 +3306,7 @@ ebtablesGetSubChainInsts(virHashTablePtr chains,
 return -1;
 
 for (i = 0; filter_names[i].key; i++) {
-struct ebtablesSubChainInst *inst;
+ebtablesSubChainInstPtr inst;
 enum l3_proto_idx idx = ebtablesGetProtoIdxByFiltername(
   filter_names[i].key);
 
@@ -3344,7 +3346,7 @@ ebiptablesApplyNewRules(const char *ifname,
 bool haveEbtables = false;
 bool haveIptables = false;
 bool haveIp6tables = false;
-struct ebtablesSubChainInst **subchains = NULL;
+ebtablesSubChainInstPtr *subchains = NULL;
 size_t nsubchains = 0;
 int ret = -1;
 
-- 
2.25.4

[libvirt PATCH v2 09/15] nwfilter: clear nrules when resetting virNWFilterInst

[Laine Stump]

It's possible/probable the callers to virNWFilterInstReset() make it
unnecessary to set the object's nrules to 0 after freeing all its
rules, but that same function is setting nfilters to 0, so let's do
the same for the sake of consistency.

Signed-off-by: Laine Stump 
---
 src/nwfilter/nwfilter_gentech_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/nwfilter/nwfilter_gentech_driver.c 
b/src/nwfilter/nwfilter_gentech_driver.c
index b7633eb10a..aff42cbfb0 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -240,6 +240,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst)
 for (i = 0; i < inst->nrules; i++)
 virNWFilterRuleInstFree(inst->rules[i]);
 VIR_FREE(inst->rules);
+inst->nrules = 0;
 }
 
 
-- 
2.25.4

[libvirt PATCH v2 11/15] nwfilter: transform logic in virNWFilterRuleInstSort to eliminate label

[Laine Stump]

This rewrite of a nested conditional produces the same results, but
eliminate a goto and corresponding label.

Signed-off-by: Laine Stump 
---
 src/nwfilter/nwfilter_ebiptables_driver.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c 
b/src/nwfilter/nwfilter_ebiptables_driver.c
index cc0f3f93d9..94eaac927a 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3113,13 +3113,12 @@ virNWFilterRuleInstSort(const void *a, const void *b)
 /* ensure root chain commands appear before all others since
we will need them to create the child chains */
 if (root_a) {
-if (root_b)
-goto normal;
-return -1; /* a before b */
-}
-if (root_b)
+if (!root_b)
+return -1; /* a before b */
+} else if (root_b) {
 return 1; /* b before a */
- normal:
+}
+
 /* priorities are limited to range [-1000, 1000] */
 return insta->priority - instb->priority;
 }
-- 
2.25.4

[libvirt PATCH v2 08/15] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()

[Laine Stump]

On failure, this function would clear out and free the list of
subchains it had been called with. This is unnecessary, because the
*only* caller of this function will also clear out and free the list
of subchains if it gets a failure from ebtablesGetSubChainInsts().

(It also makes more logical sense for the function that is creating
the entire list to be the one freeing the entire list, rather than
having a function whose purpose is only to create *one item* on the
list freeing the entire list).

Signed-off-by: Laine Stump 
---
 src/nwfilter/nwfilter_ebiptables_driver.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c 
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 78a52408b2..426212e0dc 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3328,12 +3328,6 @@ ebtablesGetSubChainInsts(virHashTablePtr chains,
 
  cleanup:
 VIR_FREE(filter_names);
-if (ret < 0) {
-for (i = 0; i < *ninsts; i++)
-VIR_FREE(*insts[i]);
-VIR_FREE(*insts);
-*ninsts = 0;
-}
 return ret;
 
 }
-- 
2.25.4

[libvirt PATCH v2 04/15] network: replace VIR_ALLOC/REALLOC with g_new0/g_renew

[Laine Stump]

Signed-off-by: Laine Stump 
---
 src/network/bridge_driver.c | 29 ++---
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 713763130b..ab359acdb5 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -177,8 +177,7 @@ 
networkDnsmasqDefNamespaceParseOptions(networkDnsmasqXmlNsDefPtr nsdef,
 if (nnodes == 0)
 return 0;
 
-if (VIR_ALLOC_N(nsdef->options, nnodes) < 0)
-return -1;
+nsdef->options = g_new0(char *, nnodes);
 
 for (i = 0; i < nnodes; i++) {
 if (!(nsdef->options[nsdef->noptions++] = virXMLPropString(nodes[i], 
"value"))) {
@@ -196,12 +195,9 @@ static int
 networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
 void **data)
 {
-networkDnsmasqXmlNsDefPtr nsdata = NULL;
+networkDnsmasqXmlNsDefPtr nsdata = g_new0(networkDnsmasqXmlNsDef, 1);
 int ret = -1;
 
-if (VIR_ALLOC(nsdata) < 0)
-return -1;
-
 if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
 goto cleanup;
 
@@ -733,8 +729,7 @@ networkStateInitialize(bool privileged,
 return -1;
 }
 
-if (VIR_ALLOC(network_driver) < 0)
-goto error;
+network_driver = g_new0(virNetworkDriverState, 1);
 
 network_driver->lockFD = -1;
 if (virMutexInit(_driver->lock) < 0) {
@@ -2753,8 +2748,7 @@ networkCreateInterfacePool(virNetworkDefPtr netdef)
 goto cleanup;
 }
 
-if (VIR_ALLOC_N(netdef->forward.ifs, numVirtFns) < 0)
-goto cleanup;
+netdef->forward.ifs = g_new0(virNetworkForwardIfDef, numVirtFns);
 
 for (i = 0; i < numVirtFns; i++) {
 virPCIDeviceAddressPtr thisVirtFn = virtFns[i];
@@ -4323,8 +4317,7 @@ networkGetDHCPLeases(virNetworkPtr net,
 continue;
 
 if (need_results) {
-if (VIR_ALLOC(lease) < 0)
-goto error;
+lease = g_new0(virNetworkDHCPLease, 1);
 
 lease->expirytime = expirytime_tmp;
 
@@ -4378,9 +4371,8 @@ networkGetDHCPLeases(virNetworkPtr net,
 
 if (leases_ret) {
 /* NULL terminated array */
-ignore_value(VIR_REALLOC_N(leases_ret, nleases + 1));
-*leases = leases_ret;
-leases_ret = NULL;
+leases_ret = g_renew(virNetworkDHCPLeasePtr, leases_ret,  nleases + 1);
+*leases = g_steal_pointer(_ret);
 }
 
 rv = nleases;
@@ -5612,10 +5604,9 @@ networkPortSetParameters(virNetworkPortPtr port,
 if (!(dir = virNetworkObjGetPortStatusDir(obj, driver->stateDir)))
 goto cleanup;
 
-if ((VIR_ALLOC(bandwidth) < 0) ||
-(VIR_ALLOC(bandwidth->in) < 0) ||
-(VIR_ALLOC(bandwidth->out) < 0))
-goto cleanup;
+bandwidth = g_new0(virNetDevBandwidth, 1);
+bandwidth->in = g_new0(virNetDevBandwidthRate, 1);
+bandwidth->out = g_new0(virNetDevBandwidthRate, 1);
 
 for (i = 0; i < nparams; i++) {
 virTypedParameterPtr param = [i];
-- 
2.25.4

[libvirt PATCH v2 03/15] define g_autoptr cleanup function for virNetworkDHCPLease

[Laine Stump]

virNetworkDHCPLease and virNetworkDHCPLeaseFree() are declared in the
public API file libvirt-network.h, and we can't pollute that with glib
macro invocations, so put this in src/datatypes.h next to the other
virNetwork items.

Signed-off-by: Laine Stump 
---
 src/datatypes.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/datatypes.h b/src/datatypes.h
index d58429ad6c..ade3779e43 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -635,6 +635,8 @@ struct _virNetworkPort {
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetworkPort, virObjectUnref);
 
+/* virNetworkDHCPLease is defined in the public API - libvirt-network.h */
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetworkDHCPLease, virNetworkDHCPLeaseFree);
 
 /**
 * _virInterface:
-- 
2.25.4

[libvirt PATCH v2 02/15] util: define g_autoptr cleanups for a couple dnsmasq objects

[Laine Stump]

Signed-off-by: Laine Stump 
---
 src/util/virdnsmasq.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/util/virdnsmasq.h b/src/util/virdnsmasq.h
index 4c14bc6ca7..e3814c2eb1 100644
--- a/src/util/virdnsmasq.h
+++ b/src/util/virdnsmasq.h
@@ -78,10 +78,14 @@ typedef enum {
 typedef struct _dnsmasqCaps dnsmasqCaps;
 typedef dnsmasqCaps *dnsmasqCapsPtr;
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(dnsmasqCaps, virObjectUnref);
+
 
 dnsmasqContext * dnsmasqContextNew(const char *network_name,
const char *config_dir);
 void dnsmasqContextFree(dnsmasqContext *ctx);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(dnsmasqContext, dnsmasqContextFree);
+
 int  dnsmasqAddDhcpHost(dnsmasqContext *ctx,
 const char *mac,
 virSocketAddr *ip,
-- 
2.25.4

[libvirt PATCH v2 06/15] network: eliminate unnecessary labels

[Laine Stump]

All these cleanup/error labels were reduced to having just "return
ret" by a previous patch, so get rid of them and return directly.

Signed-off-by: Laine Stump 
---
 src/network/bridge_driver.c   | 264 --
 src/network/bridge_driver_linux.c |  15 +-
 2 files changed, 113 insertions(+), 166 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 31bd0dd92c..79b2ca3330 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -197,18 +197,14 @@ networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
 void **data)
 {
 g_autoptr(networkDnsmasqXmlNsDef) nsdata = g_new0(networkDnsmasqXmlNsDef, 
1);
-int ret = -1;
 
 if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
-goto cleanup;
+return -1;
 
 if (nsdata->noptions > 0)
 *data = g_steal_pointer();
 
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 
@@ -331,22 +327,20 @@ networkRunHook(virNetworkObjPtr obj,
 g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
 g_autofree char *xml = NULL;
 int hookret;
-int ret = -1;
 
 if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) {
 if (!obj) {
 VIR_DEBUG("Not running hook as @obj is NULL");
-ret = 0;
-goto cleanup;
+return 0;
 }
 def = virNetworkObjGetDef(obj);
 
 virBufferAddLit(, "\n");
 virBufferAdjustIndent(, 2);
 if (virNetworkDefFormatBuf(, def, network_driver->xmlopt, 0) < 0)
-goto cleanup;
+return -1;
 if (port && virNetworkPortDefFormatBuf(, port) < 0)
-goto cleanup;
+return -1;
 
 virBufferAdjustIndent(, -2);
 virBufferAddLit(, "");
@@ -359,14 +353,12 @@ networkRunHook(virNetworkObjPtr obj,
  * If the script raised an error, pass it to the callee.
  */
 if (hookret < 0)
-goto cleanup;
+return -1;
 
 networkNetworkObjTaint(obj, VIR_NETWORK_TAINT_HOOK);
 }
 
-ret = 0;
- cleanup:
-return ret;
+return 0;
 }
 
 
@@ -440,34 +432,32 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
 g_autoptr(dnsmasqContext) dctx = NULL;
 virNetworkDefPtr def = virNetworkObjGetPersistentDef(obj);
 
-int ret = -1;
-
 /* remove the (possibly) existing dnsmasq and radvd files */
 if (!(dctx = dnsmasqContextNew(def->name,
driver->dnsmasqStateDir))) {
-goto cleanup;
+return -1;
 }
 
 if (!(leasefile = networkDnsmasqLeaseFileNameDefault(driver, def->name)))
-goto cleanup;
+return -1;
 
 if (!(customleasefile = networkDnsmasqLeaseFileNameCustom(driver, 
def->bridge)))
-goto cleanup;
+return -1;
 
 if (!(radvdconfigfile = networkRadvdConfigFileName(driver, def->name)))
-goto cleanup;
+return -1;
 
 if (!(radvdpidbase = networkRadvdPidfileBasename(def->name)))
-goto cleanup;
+return -1;
 
 if (!(configfile = networkDnsmasqConfigFileName(driver, def->name)))
-goto cleanup;
+return -1;
 
 if (!(statusfile = virNetworkConfigFile(driver->stateDir, def->name)))
-goto cleanup;
+return -1;
 
 if (!(macMapFile = virMacMapFileName(driver->dnsmasqStateDir, 
def->bridge)))
-goto cleanup;
+return -1;
 
 /* dnsmasq */
 dnsmasqDelete(dctx);
@@ -488,10 +478,7 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
 /* remove the network definition */
 virNetworkObjRemoveInactive(driver->networks, obj);
 
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 
@@ -703,7 +690,6 @@ networkStateInitialize(bool privileged,
virStateInhibitCallback callback G_GNUC_UNUSED,
void *opaque G_GNUC_UNUSED)
 {
-int ret = VIR_DRV_STATE_INIT_ERROR;
 g_autofree char *configdir = NULL;
 g_autofree char *rundir = NULL;
 bool autostart = true;
@@ -831,13 +817,12 @@ networkStateInitialize(bool privileged,
 }
 #endif
 
-ret = VIR_DRV_STATE_INIT_COMPLETE;
- cleanup:
-return ret;
+return VIR_DRV_STATE_INIT_COMPLETE;
+
 
  error:
 networkStateCleanup();
-goto cleanup;
+return VIR_DRV_STATE_INIT_ERROR;
 }
 
 
@@ -1074,7 +1059,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
 {
 virNetworkDefPtr def = virNetworkObjGetDef(obj);
 g_auto(virBuffer) configbuf = VIR_BUFFER_INITIALIZER;
-int r, ret = -1;
+int r;
 int nbleases = 0;
 size_t i;
 virNetworkDNSDefPtr dns = >dns;
@@ -1138,7 +1123,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
 g_autofree char *addr = virSocketAddrFormat(>addr);
 
 if (!addr)
-goto cleanup;
+return -1;
 virBufferAsprintf(, "%s\n", addr);
 if (!fwd->domain)
 

Re: Release of libvirt-6.5.0

[Laine Stump]

On 7/3/20 3:56 AM, Daniel Veillard wrote:

It will also be my last release of libvirt after close to 15 years,


.

(I missed this sentence when I saw the mail the first time, and just now 
it randomly popped up when scrolling through messages.)



Thanks for your helpful and positive attitude, and in general being a 
good example for everyone to follow for all these years. It's a bit sad 
that you're not involved with the project as you once were, but 
comforting to know that you're still around and reachable.

Re: [PATCH v6 08/10] iotests: Specify explicit backing format where sensible

[Eric Blake]

On 7/7/20 11:07 AM, Kevin Wolf wrote:

Am 06.07.2020 um 22:39 hat Eric Blake geschrieben:

There are many existing qcow2 images that specify a backing file but
no format.  This has been the source of CVEs in the past, but has
become more prominent of a problem now that libvirt has switched to
-blockdev.  With older -drive, at least the probing was always done by
qemu (so the only risk of a changed format between successive boots of
a guest was if qemu was upgraded and probed differently).  But with
newer -blockdev, libvirt must specify a format; if libvirt guesses raw
where the image was formatted, this results in data corruption visible
to the guest; conversely, if libvirt guesses qcow2 where qemu was
using raw, this can result in potential security holes, so modern
libvirt instead refuses to use images without explicit backing format.

The change in libvirt to reject images without explicit backing format
has pointed out that a number of tools have been far too reliant on
probing in the past.  It's time to set a better example in our own
iotests of properly setting this parameter.

iotest calls to create, rebase, and convert are all impacted to some
degree.  It's a bit annoying that we are inconsistent on command line
- while all of those accept -o backing_file=...,backing_fmt=..., the
shortcuts are different: create and rebase have -b and -F, while
convert has -B but no -F.  (amend has no shortcuts, but the previous
patch just deprecated the use of amend to change backing chains).

Signed-off-by: Eric Blake 


This breaks at least 024 and 043 for qed because qemu-img info can't
print the backing file format there (qed only saves a flag whether it's
raw or non-raw).


Shoot.  I tend to avoid tests of qed and qcow (because they take so 
long), so while I may have tested them around v1 or v2 of the series, 
all the rebasing has changed the results by now.  Such is life.




We can fix the output filtering during the freeze, though.


Yes indeed, and I'll post such patches soon, now that you've pointed it out.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 4/4] backup: Allow configuring incremental backup per-disk individually

[Eric Blake]

On 7/7/20 10:23 AM, Peter Krempa wrote:

The semantics of the backup operation don't strictly require that all
disks being backed up are part of the same incremental part (when a disk
was checkpointed/backed up separately or in a different VM), or even
they may not have an previous checkpoint at all (e.g. when the disk


s/an/a/


was freshly hotplugged to the vm).

In such cases we can still create a common checkpoint for all of them
and backup differences according to configuration.

This patch adds a per-disk configuration of the checkpoint to do the
incremental backup from via the 'incremental' attribute and allows
perform full backups via the 'backupmode' attribute.

Note that no changes to the qemu driver are necessary to take advantage
of this as we already obey the per-disk 'incremental' field.

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

Signed-off-by: Peter Krempa 
---
v2:
 - backupmode=full incremental="..." is now rejected by the RNG
   schema
 - test output changes as we now validate virDomainBackupAlignDisks
 - typo fix


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier

[Peter Xu]

On Tue, Jul 07, 2020 at 04:03:10PM +0800, Jason Wang wrote:
> 
> On 2020/7/3 下午9:03, Peter Xu wrote:
> > On Fri, Jul 03, 2020 at 03:24:19PM +0800, Jason Wang wrote:
> > > On 2020/7/2 下午11:45, Peter Xu wrote:
> > > > On Thu, Jul 02, 2020 at 11:01:54AM +0800, Jason Wang wrote:
> > > > > So I think we agree that a new notifier is needed?
> > > > Good to me, or a new flag should be easier (IOMMU_NOTIFIER_DEV_IOTLB)?
> > > 
> > > That should work but I wonder something as following is better.
> > > 
> > > Instead of introducing new flags, how about carry the type of event in the
> > > notifier then the device (vhost) can choose the message it want to process
> > > like:
> > > 
> > > static vhost_iommu_event(IOMMUNotifier *n, IOMMUTLBEvent *event)
> > > 
> > > {
> > > 
> > > switch (event->type) {
> > > 
> > > case IOMMU_MAP:
> > > case IOMMU_UNMAP:
> > > case IOMMU_DEV_IOTLB_UNMAP:
> > > ...
> > > 
> > > }
> > Looks ok to me, though imo we should still keep the registration 
> > information,
> > so VT-d knows which notifiers is interested in which events.  E.g., we can
> > still do something like vtd_as_has_map_notifier().
> 
> 
> Is this for a better performance?
> 
> I wonder whether it's worth since we can't not have both vhost and vtd to be
> registered into the same as.

/me failed to follow this sentence.. :(

> 
> So it should be functional equivalent to vtd_as_has_notifier().

For example: in vtd_iommu_replay() we'll skip the replay if vhost has
registered the iommu notifier because vtd_as_has_map_notifier() will return
false.  It'll only return true if the device is a vfio-pci device.

Without vtd_as_has_map_notifier(), how could we do that?

-- 
Peter Xu

Re: [PATCH 3/4] backupxml2xmltest: Call 'virDomainBackupAlignDisks' before formatting output

[Eric Blake]

On 7/7/20 10:23 AM, Peter Krempa wrote:

Call the post-processing function so that we can validate that it does
the correct thing.

virDomainBackupAlignDisks requires disk definitions to be present so
let's fake them by copying disks from the backup definition and add one
extra disk 'vdextradisk'.

Signed-off-by: Peter Krempa 
---

More testing is always useful.

Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2 12/17] virshStreamSkip: Emulate skip for block devices

[Michal Privoznik]

This callback is called when the server sends us STREAM_HOLE
meaning there is no real data, only zeroes. For regular files
we would just seek() beyond EOF and ftruncate() to create the
hole. But for block devices this won't work. Not only we can't
seek() beyond EOF, and ftruncate() will fail, this approach won't
fill the device with zeroes. We have to do it manually.

Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1852528

Signed-off-by: Michal Privoznik 
---
 tools/virsh-util.c | 30 ++
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index 89f15efd08..884261eb49 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -189,11 +189,33 @@ virshStreamSkip(virStreamPtr st G_GNUC_UNUSED,
 virshStreamCallbackDataPtr cbData = opaque;
 off_t cur;
 
-if ((cur = lseek(cbData->fd, offset, SEEK_CUR)) == (off_t) -1)
-return -1;
+if (cbData->isBlock) {
+g_autofree char * buf = NULL;
+const size_t buflen = 1 * 1024 * 1024; /* 1MiB */
 
-if (ftruncate(cbData->fd, cur) < 0)
-return -1;
+/* While for files it's enough to lseek() and ftruncate() to create
+ * a hole which would emulate zeroes on read(), for block devices
+ * we have to write zeroes to read() zeroes. And we have to write
+ * @got bytes of zeroes. Do that in smaller chunks though.*/
+
+buf = g_new0(char, buflen);
+
+while (offset) {
+size_t count = MIN(offset, buflen);
+ssize_t r;
+
+if ((r = safewrite(cbData->fd, buf, count)) < 0)
+return -1;
+
+offset -= r;
+}
+} else {
+if ((cur = lseek(cbData->fd, offset, SEEK_CUR)) == (off_t) -1)
+return -1;
+
+if (ftruncate(cbData->fd, cur) < 0)
+return -1;
+}
 
 return 0;
 }
-- 
2.26.2

Re: [PATCH 2/4] virDomainBackupDiskDefFormat: Format internal disk state only when valid

[Eric Blake]

On 7/7/20 10:23 AM, Peter Krempa wrote:

Format the disk state only when it isn't _NONE.

Signed-off-by: Peter Krempa 
---
  src/conf/backup_conf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Eric Blake 



diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index 781dd53f6b..5e4144d371 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -370,7 +370,7 @@ virDomainBackupDiskDefFormat(virBufferPtr buf,

  virBufferEscapeString(, " name='%s'", disk->name);
  virBufferAsprintf(, " backup='%s'", 
virTristateBoolTypeToString(disk->backup));
-if (internal)
+if (internal && disk->state != VIR_DOMAIN_BACKUP_DISK_STATE_NONE)
  virBufferAsprintf(, " state='%s'", 
virDomainBackupDiskStateTypeToString(disk->state));


Pre-existing long line; do we care?



  if (disk->backup == VIR_TRISTATE_BOOL_YES) {



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/4] backupxml2xmltest: Remove output symlink of 'backup-pull-internal-invalid'

[Eric Blake]

On 7/7/20 10:23 AM, Peter Krempa wrote:

Replace the output by a copy of the input file for further changes once
we start testing virDomainBackupAlignDisks.

Signed-off-by: Peter Krempa 
---
  .../backup-pull-internal-invalid.xml  | 37 ++-
  1 file changed, 36 insertions(+), 1 deletion(-)
  mode change 12 => 100644 
tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2 14/17] virshStreamInData: Handle block devices

[Michal Privoznik]

This is very similar to previous commit.

The virshStreamInData() callback is used by virStreamSparseSendAll()
to detect whether the file the data is read from is in data or hole
section. The SendAll() will then send corresponding type of virStream
message to make server create a hole or write actual data. But the
callback uses virFileInData() even for block devices, which results in
an error. Switch to virFileInDataDetectZeroes() for block devices.

Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1852528

Signed-off-by: Michal Privoznik 
---
 tools/virsh-util.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index 884261eb49..867f69577f 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -230,12 +230,20 @@ virshStreamInData(virStreamPtr st G_GNUC_UNUSED,
 virshStreamCallbackDataPtr cbData = opaque;
 vshControl *ctl = cbData->ctl;
 int fd = cbData->fd;
-int ret;
 
-if ((ret = virFileInData(fd, inData, offset)) < 0)
-vshError(ctl, "%s", _("Unable to get current position in stream"));
+if (cbData->isBlock) {
+if (virFileInDataDetectZeroes(fd, inData, offset) < 0) {
+vshError(ctl, "%s", _("Unable to get current position in stream"));
+return -1;
+}
+} else {
+if (virFileInData(fd, inData, offset) < 0) {
+vshError(ctl, "%s", _("Unable to get current position in stream"));
+return -1;
+}
+}
 
-return ret;
+return 0;
 }
 
 
-- 
2.26.2

[PATCH v2 08/17] virstring: Introduce virStringIsNull()

[Michal Privoznik]

This function will be used to detect zero buffers (which are
going to be interpreted as hole in virStream later).

I shamelessly took inspiration from coreutils.

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  1 +
 src/util/virstring.c | 38 
 src/util/virstring.h |  2 ++
 tests/virstringtest.c| 47 
 4 files changed, 88 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ae0e253ab9..1d80aeb833 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3197,6 +3197,7 @@ virStringHasChars;
 virStringHasControlChars;
 virStringHasSuffix;
 virStringIsEmpty;
+virStringIsNull;
 virStringIsPrintable;
 virStringListAdd;
 virStringListAutoFree;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index e9e792f3bf..c26bc770d4 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -1404,3 +1404,41 @@ int virStringParseYesNo(const char *str, bool *result)
 
 return 0;
 }
+
+
+/**
+ * virStringIsNull:
+ * @buf: buffer to check
+ * @len: the length of the buffer
+ *
+ * For given buffer @buf and its size @len determine whether
+ * it contains only zero bytes (NUL) or not.
+ *
+ * Returns: true if buffer is full of zero bytes,
+ *  false otherwise.
+ */
+bool virStringIsNull(const char *buf, size_t len)
+{
+const char *p = buf;
+
+if (!len)
+return true;
+
+/* Check up to 16 first bytes. */
+for (;;) {
+if (*p)
+return false;
+
+p++;
+len--;
+
+if (!len)
+return true;
+
+if ((len & 0xf) == 0)
+break;
+}
+
+/* Now we know first 16 bytes are NUL, memcmp with self.  */
+return memcmp(buf, p, len) == 0;
+}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 360c68395c..d0e54358ac 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -185,6 +185,8 @@ int virStringParsePort(const char *str,
 int virStringParseYesNo(const char *str,
 bool *result)
 G_GNUC_WARN_UNUSED_RESULT;
+bool virStringIsNull(const char *buf, size_t len);
+
 /**
  * VIR_AUTOSTRINGLIST:
  *
diff --git a/tests/virstringtest.c b/tests/virstringtest.c
index bee49e6cb6..5838a57574 100644
--- a/tests/virstringtest.c
+++ b/tests/virstringtest.c
@@ -649,6 +649,27 @@ static int testFilterChars(const void *args)
 return ret;
 }
 
+struct testIsNulData {
+const char *buf;
+size_t len;
+bool nul;
+};
+
+static int
+testIsNul(const void *args)
+{
+const struct testIsNulData *data = args;
+int rc;
+
+rc = virStringIsNull(data->buf, data->len);
+if (rc != data->nul) {
+fprintf(stderr, "Returned %d, expected %d\n", rc, data->nul);
+return -1;
+}
+
+return 0;
+}
+
 static int
 mymain(void)
 {
@@ -977,6 +998,32 @@ mymain(void)
 TEST_FILTER_CHARS(NULL, NULL, NULL);
 TEST_FILTER_CHARS("hello 123 hello", "helo", "hellohello");
 
+#define TEST_IS_NUL(expect, ...) \
+do { \
+const char buf[] = {__VA_ARGS__ }; \
+struct testIsNulData isNulData = { \
+.buf = buf, \
+.len = G_N_ELEMENTS(buf), \
+.nul = expect \
+}; \
+if (virTestRun("isNul check", \
+   testIsNul, ) < 0) \
+ret = -1; \
+} while (0)
+
+TEST_IS_NUL(true);
+TEST_IS_NUL(true,
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
+TEST_IS_NUL(false, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01);
+TEST_IS_NUL(false,
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01);
+TEST_IS_NUL(false,
+0x00, 0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x01,
+0x00, 0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x01,
+0x00);
+
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
-- 
2.26.2

[PATCH v2 03/17] virfdstream: Use autoptr for virFDStreamMsg

[Michal Privoznik]

A cleanup function can be declared for virFDStreamMsg type so
that the structure doesn't have to be freed explicitly.

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 6efe6c17ad..25661736ca 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -208,6 +208,8 @@ virFDStreamMsgFree(virFDStreamMsgPtr msg)
 VIR_FREE(msg);
 }
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFDStreamMsg, virFDStreamMsgFree);
+
 
 static void
 virFDStreamMsgQueueFree(virFDStreamMsgPtr *queue)
@@ -428,7 +430,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 size_t *dataLen,
 size_t buflen)
 {
-virFDStreamMsgPtr msg = NULL;
+g_autoptr(virFDStreamMsg) msg = NULL;
 int inData = 0;
 long long sectionLen = 0;
 g_autofree char *buf = NULL;
@@ -494,7 +496,6 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 return got;
 
  error:
-virFDStreamMsgFree(msg);
 return -1;
 }
 
@@ -761,7 +762,7 @@ virFDStreamAbort(virStreamPtr st)
 static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
 {
 virFDStreamDataPtr fdst = st->privateData;
-virFDStreamMsgPtr msg = NULL;
+g_autoptr(virFDStreamMsg) msg = NULL;
 int ret = -1;
 
 if (nbytes > INT_MAX) {
@@ -838,7 +839,6 @@ static int virFDStreamWrite(virStreamPtr st, const char 
*bytes, size_t nbytes)
 
  cleanup:
 virObjectUnlock(fdst);
-virFDStreamMsgFree(msg);
 return ret;
 }
 
@@ -960,7 +960,7 @@ virFDStreamSendHole(virStreamPtr st,
 unsigned int flags)
 {
 virFDStreamDataPtr fdst = st->privateData;
-virFDStreamMsgPtr msg = NULL;
+g_autoptr(virFDStreamMsg) msg = NULL;
 off_t off;
 int ret = -1;
 
@@ -1028,7 +1028,6 @@ virFDStreamSendHole(virStreamPtr st,
 ret = 0;
  cleanup:
 virObjectUnlock(fdst);
-virFDStreamMsgFree(msg);
 return ret;
 }
 
-- 
2.26.2

[PATCH v2 16/17] stream: Don't read block device twice

[Michal Privoznik]

The aim of virFileInDataDetectZeroes() is to mimic virFileInData()
so that it can be used in sparse virStream with very little change
to logic. This was implemented in previous commits. These two
functions act alike - the passed FD is rewound back to the position
it was in when either of the functions was called. For regular
files (when virFileInData()) is used, this is not a problem -
rewinding an FD is usually cheap and data is read() only from data
section and it's read only once. But this is not the case for
virFileInDataDetectZeroes(), which doesn't use cheap lseek() to
learn data/hole sections, but uses read() and zero buffer
detection. Worst case scenario, virFileInDataDetectZeroes() reads
1MiB of bytes, finds it is non-zero data, rewinds FD that 1MiB back
only so that virFDStreamThreadDoRead() (in case of daemon) or
virshStreamSource() (in case of client) can read the same data
again.

Possibly, this is not that big problem because kernel is likely to
keep the data still in cache. But let's not rely on that and store
the buffer for later use if we learned it contains actual data.

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 27 +-
 src/util/virfile.c | 51 ++
 src/util/virfile.h |  3 ++-
 tools/virsh-util.c | 40 +
 tools/virsh-util.h |  3 +++
 tools/virsh-volume.c   |  3 ++-
 6 files changed, 76 insertions(+), 51 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 40825a8811..9d617c71da 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -440,7 +440,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 
 if (sparse && *dataLen == 0) {
 if (isBlock) {
-if (virFileInDataDetectZeroes(fdin, , ) < 0)
+if (virFileInDataDetectZeroes(fdin, , , ) < 
0)
 return -1;
 } else {
 if (virFileInData(fdin, , ) < 0)
@@ -468,7 +468,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 
 /* HACK: The message queue is one directional. So caller
  * cannot make us skip the hole. Do that for them instead. */
-if (sectionLen &&
+if (sectionLen && !isBlock &&
 lseek(fdin, sectionLen, SEEK_CUR) == (off_t) -1) {
 virReportSystemError(errno,
  _("unable to seek in %s"),
@@ -476,17 +476,22 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 return -1;
 }
 } else {
-if (sparse &&
-buflen > *dataLen)
-buflen = *dataLen;
+if (sparse && isBlock) {
+/* Data already read by virFileInDataDetectZeroes() */
+got = sectionLen;
+} else {
+if (sparse &&
+buflen > *dataLen)
+buflen = *dataLen;
 
-buf = g_new0(char, buflen);
+buf = g_new0(char, buflen);
 
-if ((got = saferead(fdin, buf, buflen)) < 0) {
-virReportSystemError(errno,
- _("Unable to read %s"),
- fdinname);
-return -1;
+if ((got = saferead(fdin, buf, buflen)) < 0) {
+virReportSystemError(errno,
+ _("Unable to read %s"),
+ fdinname);
+return -1;
+}
 }
 
 msg->type = VIR_FDSTREAM_MSG_TYPE_DATA;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index a35d9ccb7a..e33a1c280c 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -4071,12 +4071,18 @@ virFileInData(int fd G_GNUC_UNUSED,
  * @fd: file to check
  * @inData: true if current position in the @fd is in data section
  * @length: amount of bytes until the end of the current section
+ * @buf: data read if in data section
  *
- * This behaves exactly like virFileInData() except it doesn't use SEEK_DATA
- * and SEEK_HOLE rather than a buffer into which it reads data from @fd and
- * detects if the buffer is full of zeroes. Therefore, it is safe to use on
- * special files (e.g. block devices). On the other hand it sees only
- * DETECT_ZEORES_BLOCK_SIZE ahead.
+ * For given @fd it reads up to DETECT_ZEORES_BLOCK_SIZE bytes from it.
+ * If all bytes read are zero then @inData is set to 0, otherwise @inData
+ * is set to 1 and @buf is set to the read data. In either case, the
+ * number of bytes read is stored in @length.
+ *
+ * If @fd is at EOF, then this function sets @inData = 0 and @length = 0.
+ *
+ * Note, in contrast to virFileInData(), this function is safe to use on
+ * special files (e.g. block devices), does not rewind @fd back to its
+ * starting position, and sees only DETECT_ZEORES_BLOCK_SIZE bytes ahead.
  *
  * Returns: 0 on success,
  * -1 otherwise.
@@ -4084,50 +4090,27 @@ virFileInData(int fd G_GNUC_UNUSED,
 int
 virFileInDataDetectZeroes(int fd,
   int 

[PATCH v2 11/17] virsh: Track if vol-upload or vol-download work over a block device

[Michal Privoznik]

We can't use virFileInData() with block devices, but we could use
new virFileInDataDetectZeroes(). But to decide we need to know if
the FD we are reading data from / writing data to is a block
device. Store this information in _virshStreamCallbackData.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-util.h   |  1 +
 tools/virsh-volume.c | 14 ++
 2 files changed, 15 insertions(+)

diff --git a/tools/virsh-util.h b/tools/virsh-util.h
index 72653d9735..9ef28cfe0a 100644
--- a/tools/virsh-util.h
+++ b/tools/virsh-util.h
@@ -72,6 +72,7 @@ typedef virshStreamCallbackData *virshStreamCallbackDataPtr;
 struct _virshStreamCallbackData {
 vshControl *ctl;
 int fd;
+bool isBlock;
 };
 
 int
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 5cbc2efb7a..20823e2d10 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -679,6 +679,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
 virshControlPtr priv = ctl->privData;
 unsigned int flags = 0;
 virshStreamCallbackData cbData;
+struct stat sb;
 
 if (vshCommandOptULongLong(ctl, cmd, "offset", ) < 0)
 return false;
@@ -697,8 +698,14 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }
 
+if (fstat(fd, ) < 0) {
+vshError(ctl, _("unable to stat %s"), file);
+goto cleanup;
+}
+
 cbData.ctl = ctl;
 cbData.fd = fd;
+cbData.isBlock = !!S_ISBLK(sb.st_mode);
 
 if (vshCommandOptBool(cmd, "sparse"))
 flags |= VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM;
@@ -795,6 +802,7 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd)
 virshControlPtr priv = ctl->privData;
 virshStreamCallbackData cbData;
 unsigned int flags = 0;
+struct stat sb;
 
 if (vshCommandOptULongLong(ctl, cmd, "offset", ) < 0)
 return false;
@@ -821,8 +829,14 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd)
 created = true;
 }
 
+if (fstat(fd, ) < 0) {
+vshError(ctl, _("unable to stat %s"), file);
+goto cleanup;
+}
+
 cbData.ctl = ctl;
 cbData.fd = fd;
+cbData.isBlock = !!S_ISBLK(sb.st_mode);
 
 if (!(st = virStreamNew(priv->conn, 0))) {
 vshError(ctl, _("cannot create a new stream"));
-- 
2.26.2

[PATCH v2 07/17] libvirt-storage: Document volume upload/download stream format

[Michal Privoznik]

For libvirt, the volume is just a binary blob and it doesn't
interpret data on volume upload/download. But as it turns out,
this unspoken assumption is not clear to our users. Document it
explicitly.

Suggested in: https://bugzilla.redhat.com/show_bug.cgi?id=1851023#c17

Signed-off-by: Michal Privoznik 
---
 src/libvirt-storage.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
index a45c8b98c1..8738f6dd14 100644
--- a/src/libvirt-storage.c
+++ b/src/libvirt-storage.c
@@ -1590,7 +1590,9 @@ virStorageVolCreateXMLFrom(virStoragePoolPtr pool,
  *
  * Download the content of the volume as a stream. If @length
  * is zero, then the remaining contents of the volume after
- * @offset will be downloaded.
+ * @offset will be downloaded. Please note, that the data is
+ * not interpreted and therefore data received by stream
+ * client are in the very same format the volume is in.
  *
  * If VIR_STORAGE_VOL_DOWNLOAD_SPARSE_STREAM is set in @flags
  * effective transmission of holes is enabled. This assumes using
@@ -1663,7 +1665,9 @@ virStorageVolDownload(virStorageVolPtr vol,
  * will fail if @offset + @length exceeds the size of the
  * volume. Otherwise, if @length is non-zero, an error
  * will be raised if an attempt is made to upload greater
- * than @length bytes of data.
+ * than @length bytes of data. Please note, that the data is
+ * not interpreted and therefore data sent by stream client
+ * must be in the very same format the volume is in.
  *
  * If VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM is set in @flags
  * effective transmission of holes is enabled. This assumes using
-- 
2.26.2

[PATCH v2 17/17] news: Document sparse streams for block devcies

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 NEWS.rst | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 232387ebdc..e225cbbcd3 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -15,6 +15,13 @@ v6.6.0 (unreleased)
 
 * **Improvements**
 
+  * Allow sparse streams for block devices
+
+Sparse streams (e.g. ``virsh vol-download --sparse`` or ``virsh vol-upload
+--sparse``) now handle if one of the stream ends is a block device. A zero
+block detection is performed so that zero block are not transferred and
+thus bandwidth is saved.
+
 * **Bug fixes**
 
 
-- 
2.26.2

[PATCH v2 13/17] virfdstream: Allow sparse stream vol-download

[Michal Privoznik]

When handling sparse stream, a thread is executed. This thread
runs a read() or write() loop (depending what API is called; in
this case it's virStorageVolDownload() and  this the thread run
read() loop). The read() is handled in virFDStreamThreadDoRead()
which is then data/hole section aware, meaning it uses
virFileInData() to detect data and hole sections and sends
TYPE_DATA or TYPE_HOLE virStream messages accordingly.

However, virFileInData() does not work with block devices. Simply
because block devices don't have data and hole sections. But we
can use new virFileInDataDetectZeroes() which is block device
friendly for that.

Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1852528

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 50209a8bd4..44b4855549 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -398,6 +398,7 @@ struct _virFDStreamThreadData {
 size_t length;
 bool doRead;
 bool sparse;
+bool isBlock;
 int fdin;
 char *fdinname;
 int fdout;
@@ -421,6 +422,7 @@ virFDStreamThreadDataFree(virFDStreamThreadDataPtr data)
 static ssize_t
 virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 bool sparse,
+bool isBlock,
 const int fdin,
 const int fdout,
 const char *fdinname,
@@ -437,8 +439,13 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 ssize_t got;
 
 if (sparse && *dataLen == 0) {
-if (virFileInData(fdin, , ) < 0)
-return -1;
+if (isBlock) {
+if (virFileInDataDetectZeroes(fdin, , ) < 0)
+return -1;
+} else {
+if (virFileInData(fdin, , ) < 0)
+return -1;
+}
 
 if (length &&
 sectionLen > length - total)
@@ -568,6 +575,7 @@ virFDStreamThread(void *opaque)
 virStreamPtr st = data->st;
 size_t length = data->length;
 bool sparse = data->sparse;
+bool isBlock = data->isBlock;
 VIR_AUTOCLOSE fdin = data->fdin;
 char *fdinname = data->fdinname;
 VIR_AUTOCLOSE fdout = data->fdout;
@@ -604,7 +612,7 @@ virFDStreamThread(void *opaque)
 }
 
 if (doRead)
-got = virFDStreamThreadDoRead(fdst, sparse,
+got = virFDStreamThreadDoRead(fdst, sparse, isBlock,
   fdin, fdout,
   fdinname, fdoutname,
   length, total,
@@ -1271,6 +1279,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
 threadData->st = virObjectRef(st);
 threadData->length = length;
 threadData->sparse = sparse;
+threadData->isBlock = !!S_ISBLK(sb.st_mode);
 
 if ((oflags & O_ACCMODE) == O_RDONLY) {
 threadData->fdin = fd;
-- 
2.26.2

[PATCH v2 05/17] virfdstream: Use VIR_AUTOCLOSE()

[Michal Privoznik]

Again, instead of closing FDs explicitly, we can automatically
close them when they go out of their respective scopes.

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index c85dee05c3..bac1c95c0a 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -571,9 +571,9 @@ virFDStreamThread(void *opaque)
 virStreamPtr st = data->st;
 size_t length = data->length;
 bool sparse = data->sparse;
-int fdin = data->fdin;
+VIR_AUTOCLOSE fdin = data->fdin;
 char *fdinname = data->fdinname;
-int fdout = data->fdout;
+VIR_AUTOCLOSE fdout = data->fdout;
 char *fdoutname = data->fdoutname;
 virFDStreamDataPtr fdst = st->privateData;
 bool doRead = fdst->threadDoRead;
@@ -633,8 +633,6 @@ virFDStreamThread(void *opaque)
 virObjectUnref(fdst);
 if (virFDStreamDataDisposed)
 st->privateData = NULL;
-VIR_FORCE_CLOSE(fdin);
-VIR_FORCE_CLOSE(fdout);
 virFDStreamThreadDataFree(data);
 return;
 
@@ -1160,9 +1158,10 @@ int virFDStreamConnectUNIX(virStreamPtr st,
 {
 struct sockaddr_un sa;
 virTimeBackOffVar timeout;
+VIR_AUTOCLOSE fd = -1;
 int ret;
 
-int fd = socket(AF_UNIX, SOCK_STREAM, 0);
+fd = socket(AF_UNIX, SOCK_STREAM, 0);
 if (fd < 0) {
 virReportSystemError(errno, "%s", _("Unable to open UNIX socket"));
 goto error;
@@ -1197,10 +1196,11 @@ int virFDStreamConnectUNIX(virStreamPtr st,
 
 if (virFDStreamOpenInternal(st, fd, NULL, 0) < 0)
 goto error;
+
+fd = -1;
 return 0;
 
  error:
-VIR_FORCE_CLOSE(fd);
 return -1;
 }
 
-- 
2.26.2

[PATCH v2 15/17] virfdstream: Emulate skip for block devices

[Michal Privoznik]

This is similar to one of previous patches.

When receiving stream (on virStorageVolUpload() and subsequent
virStreamSparseSendAll()) we may receive a hole. If the volume we
are saving the incoming data into is a regular file we just
lseek() and ftruncate() to create the hole. But this won't work
if the file is a block device. If that is the case we must write
zeroes so that any subsequent reader reads nothing just zeroes
(just like they would from a hole in a regular file).

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

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 59 +++---
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 44b4855549..40825a8811 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -505,6 +505,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 static ssize_t
 virFDStreamThreadDoWrite(virFDStreamDataPtr fdst,
  bool sparse,
+ bool isBlock,
  const int fdin,
  const int fdout,
  const char *fdinname,
@@ -512,7 +513,6 @@ virFDStreamThreadDoWrite(virFDStreamDataPtr fdst,
 {
 ssize_t got = 0;
 virFDStreamMsgPtr msg = fdst->msg;
-off_t off;
 bool pop = false;
 
 switch (msg->type) {
@@ -540,19 +540,48 @@ virFDStreamThreadDoWrite(virFDStreamDataPtr fdst,
 }
 
 got = msg->stream.hole.len;
-off = lseek(fdout, got, SEEK_CUR);
-if (off == (off_t) -1) {
-virReportSystemError(errno,
- _("unable to seek in %s"),
- fdoutname);
-return -1;
-}
-
-if (ftruncate(fdout, off) < 0) {
-virReportSystemError(errno,
- _("unable to truncate %s"),
- fdoutname);
-return -1;
+if (isBlock) {
+g_autofree char * buf = NULL;
+const size_t buflen = 1 * 1024 * 1024; /* 1MiB */
+size_t toWrite = got;
+
+/* While for files it's enough to lseek() and ftruncate() to create
+ * a hole which would emulate zeroes on read(), for block devices
+ * we have to write zeroes to read() zeroes. And we have to write
+ * @got bytes of zeroes. Do that in smaller chunks though.*/
+
+buf = g_new0(char, buflen);
+
+while (toWrite) {
+size_t count = MIN(toWrite, buflen);
+ssize_t r;
+
+if ((r = safewrite(fdout, buf, count)) < 0) {
+virReportSystemError(errno,
+ _("Unable to write %s"),
+ fdoutname);
+return -1;
+}
+
+toWrite -= r;
+}
+} else {
+off_t off;
+
+off = lseek(fdout, got, SEEK_CUR);
+if (off == (off_t) -1) {
+virReportSystemError(errno,
+ _("unable to seek in %s"),
+ fdoutname);
+return -1;
+}
+
+if (ftruncate(fdout, off) < 0) {
+virReportSystemError(errno,
+ _("unable to truncate %s"),
+ fdoutname);
+return -1;
+}
 }
 
 pop = true;
@@ -618,7 +647,7 @@ virFDStreamThread(void *opaque)
   length, total,
   , buflen);
 else
-got = virFDStreamThreadDoWrite(fdst, sparse,
+got = virFDStreamThreadDoWrite(fdst, sparse, isBlock,
fdin, fdout,
fdinname, fdoutname);
 
-- 
2.26.2

[PATCH v2 04/17] virfdstream: Use g_new0() instead of VIR_ALLOC()

[Michal Privoznik]

This switch allow us to save a few lines of code.

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 20 +++-
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 25661736ca..c85dee05c3 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -452,8 +452,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 buflen > length - total)
 buflen = length - total;
 
-if (VIR_ALLOC(msg) < 0)
-goto error;
+msg = g_new0(virFDStreamMsg, 1);
 
 if (sparse && *dataLen == 0) {
 msg->type = VIR_FDSTREAM_MSG_TYPE_HOLE;
@@ -474,8 +473,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 buflen > *dataLen)
 buflen = *dataLen;
 
-if (VIR_ALLOC_N(buf, buflen) < 0)
-goto error;
+buf = g_new0(char, buflen);
 
 if ((got = saferead(fdin, buf, buflen)) < 0) {
 virReportSystemError(errno,
@@ -805,9 +803,8 @@ static int virFDStreamWrite(virStreamPtr st, const char 
*bytes, size_t nbytes)
 goto cleanup;
 }
 
-if (VIR_ALLOC(msg) < 0 ||
-VIR_ALLOC_N(buf, nbytes) < 0)
-goto cleanup;
+msg = g_new0(virFDStreamMsg, 1);
+buf = g_new0(char, nbytes);
 
 memcpy(buf, bytes, nbytes);
 msg->type = VIR_FDSTREAM_MSG_TYPE_DATA;
@@ -1003,8 +1000,7 @@ virFDStreamSendHole(virStreamPtr st,
 
 virFDStreamMsgQueuePop(fdst, fdst->fd, "pipe");
 } else {
-if (VIR_ALLOC(msg) < 0)
-goto cleanup;
+msg = g_new0(virFDStreamMsg, 1);
 
 msg->type = VIR_FDSTREAM_MSG_TYPE_HOLE;
 msg->stream.hole.len = length;
@@ -1123,8 +1119,7 @@ static int virFDStreamOpenInternal(virStreamPtr st,
 
 /* Create the thread after fdst and st were initialized.
  * The thread worker expects them to be that way. */
-if (VIR_ALLOC(fdst->thread) < 0)
-goto error;
+fdst->thread = g_new0(virThread, 1);
 
 if (virCondInit(>threadCond) < 0) {
 virReportSystemError(errno, "%s",
@@ -1277,8 +1272,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
 if (virPipe(pipefds) < 0)
 goto error;
 
-if (VIR_ALLOC(threadData) < 0)
-goto error;
+threadData = g_new0(virFDStreamThreadData, 1);
 
 threadData->st = virObjectRef(st);
 threadData->length = length;
-- 
2.26.2

[PATCH v2 09/17] virfile: Introduce virFileInDataDetectZeroes()

[Michal Privoznik]

For given file descriptor determine if the current position it is
in plus 1MiB (arbitrary chosen value) consists solely from zero
bytes or not. This is a block device friendly version of
virFileInData().

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  1 +
 src/util/virfile.c   | 67 
 src/util/virfile.h   |  4 +++
 3 files changed, 72 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1d80aeb833..6b5a751788 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2070,6 +2070,7 @@ virFileGetMountSubtree;
 virFileGetXAttr;
 virFileGetXAttrQuiet;
 virFileInData;
+virFileInDataDetectZeroes;
 virFileIsCDROM;
 virFileIsDir;
 virFileIsExecutable;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index c034df5931..a35d9ccb7a 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -4064,6 +4064,73 @@ virFileInData(int fd G_GNUC_UNUSED,
 #endif /* !HAVE_DECL_SEEK_HOLE */
 
 
+#define DETECT_ZEORES_BLOCK_SIZE (1 * 1024 * 1024)
+
+/*
+ * virFileInDataDetectZeroes:
+ * @fd: file to check
+ * @inData: true if current position in the @fd is in data section
+ * @length: amount of bytes until the end of the current section
+ *
+ * This behaves exactly like virFileInData() except it doesn't use SEEK_DATA
+ * and SEEK_HOLE rather than a buffer into which it reads data from @fd and
+ * detects if the buffer is full of zeroes. Therefore, it is safe to use on
+ * special files (e.g. block devices). On the other hand it sees only
+ * DETECT_ZEORES_BLOCK_SIZE ahead.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise.
+ */
+int
+virFileInDataDetectZeroes(int fd,
+  int *inData,
+  long long *length)
+{
+const size_t buflen = DETECT_ZEORES_BLOCK_SIZE;
+g_autofree char *bytes = NULL;
+off_t cur;
+ssize_t r;
+int ret = -1;
+
+/* Get current position */
+cur = lseek(fd, 0, SEEK_CUR);
+if (cur == (off_t) -1) {
+virReportSystemError(errno, "%s",
+ _("Unable to get current position in file"));
+goto cleanup;
+}
+
+bytes = g_new0(char, buflen);
+
+if ((r = saferead(fd, bytes, buflen)) < 0) {
+virReportSystemError(errno, "%s",
+ _("Unable to read from file"));
+goto cleanup;
+}
+
+*inData = !virStringIsNull(bytes, r);
+*length = r;
+ret = 0;
+
+ cleanup:
+/* At any rate, reposition back to where we started. */
+if (cur != (off_t) -1) {
+int theerrno = errno;
+
+if (lseek(fd, cur, SEEK_SET) == (off_t) -1) {
+virReportSystemError(errno, "%s",
+ _("unable to restore position in file"));
+ret = -1;
+if (theerrno == 0)
+theerrno = errno;
+}
+
+errno = theerrno;
+}
+return ret;
+}
+
+
 /**
  * virFileReadValueInt:
  * @value: pointer to int to be filled in with the value
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 7a92364a5c..9a5eade609 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -352,6 +352,10 @@ int virFileInData(int fd,
   int *inData,
   long long *length);
 
+int virFileInDataDetectZeroes(int fd,
+  int *inData,
+  long long *length);
+
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virFileWrapperFd, virFileWrapperFdFree);
 
 int virFileGetXAttr(const char *path,
-- 
2.26.2

[PATCH v2 10/17] virsh: Pass virshStreamCallbackDataPtr to virshStreamSink() and virshStreamSkip()

[Michal Privoznik]

These callback will need to know more that the FD they are
working on. Pass the structure that is passed to other stream
callbacks (e.g. virshStreamSource() or virshStreamSourceSkip())
instead of inventing a new one.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-util.c   | 10 +-
 tools/virsh-volume.c |  6 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index 932d6d0849..89f15efd08 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -146,9 +146,9 @@ virshStreamSink(virStreamPtr st G_GNUC_UNUSED,
 size_t nbytes,
 void *opaque)
 {
-int *fd = opaque;
+virshStreamCallbackDataPtr cbData = opaque;
 
-return safewrite(*fd, bytes, nbytes);
+return safewrite(cbData->fd, bytes, nbytes);
 }
 
 
@@ -186,13 +186,13 @@ virshStreamSkip(virStreamPtr st G_GNUC_UNUSED,
 long long offset,
 void *opaque)
 {
-int *fd = opaque;
+virshStreamCallbackDataPtr cbData = opaque;
 off_t cur;
 
-if ((cur = lseek(*fd, offset, SEEK_CUR)) == (off_t) -1)
+if ((cur = lseek(cbData->fd, offset, SEEK_CUR)) == (off_t) -1)
 return -1;
 
-if (ftruncate(*fd, cur) < 0)
+if (ftruncate(cbData->fd, cur) < 0)
 return -1;
 
 return 0;
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 72394915d8..5cbc2efb7a 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -793,6 +793,7 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd)
 unsigned long long offset = 0, length = 0;
 bool created = false;
 virshControlPtr priv = ctl->privData;
+virshStreamCallbackData cbData;
 unsigned int flags = 0;
 
 if (vshCommandOptULongLong(ctl, cmd, "offset", ) < 0)
@@ -820,6 +821,9 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd)
 created = true;
 }
 
+cbData.ctl = ctl;
+cbData.fd = fd;
+
 if (!(st = virStreamNew(priv->conn, 0))) {
 vshError(ctl, _("cannot create a new stream"));
 goto cleanup;
@@ -830,7 +834,7 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }
 
-if (virStreamSparseRecvAll(st, virshStreamSink, virshStreamSkip, ) < 0) 
{
+if (virStreamSparseRecvAll(st, virshStreamSink, virshStreamSkip, ) 
< 0) {
 vshError(ctl, _("cannot receive data from volume %s"), name);
 goto cleanup;
 }
-- 
2.26.2

[PATCH v2 01/17] virfdstream: Use g_autofree in virFDStreamThreadDoRead()

[Michal Privoznik]

The buffer that allocated in the virFDStreamThreadDoRead() can be
automatically freed, or if saved into the message structure it
can be stolen.

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 1c32be47a9..e29c95690b 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -431,7 +431,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 virFDStreamMsgPtr msg = NULL;
 int inData = 0;
 long long sectionLen = 0;
-char *buf = NULL;
+g_autofree char *buf = NULL;
 ssize_t got;
 
 if (sparse && *dataLen == 0) {
@@ -483,9 +483,8 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 }
 
 msg->type = VIR_FDSTREAM_MSG_TYPE_DATA;
-msg->stream.data.buf = buf;
+msg->stream.data.buf = g_steal_pointer();
 msg->stream.data.len = got;
-buf = NULL;
 if (sparse)
 *dataLen -= got;
 }
@@ -496,7 +495,6 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 return got;
 
  error:
-VIR_FREE(buf);
 virFDStreamMsgFree(msg);
 return -1;
 }
-- 
2.26.2

[PATCH v2 06/17] virfdstream: Drop some needless labels

[Michal Privoznik]

After previous cleanups, some labels in some functions have
nothing but 'return' statement in them. Drop the labels and
replace 'goto'-s with respective return statements.

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 24 +---
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index bac1c95c0a..50209a8bd4 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -438,7 +438,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 
 if (sparse && *dataLen == 0) {
 if (virFileInData(fdin, , ) < 0)
-goto error;
+return -1;
 
 if (length &&
 sectionLen > length - total)
@@ -466,7 +466,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 virReportSystemError(errno,
  _("unable to seek in %s"),
  fdinname);
-goto error;
+return -1;
 }
 } else {
 if (sparse &&
@@ -479,7 +479,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 virReportSystemError(errno,
  _("Unable to read %s"),
  fdinname);
-goto error;
+return -1;
 }
 
 msg->type = VIR_FDSTREAM_MSG_TYPE_DATA;
@@ -492,9 +492,6 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 virFDStreamMsgQueuePush(fdst, , fdout, fdoutname);
 
 return got;
-
- error:
-return -1;
 }
 
 
@@ -1164,22 +1161,22 @@ int virFDStreamConnectUNIX(virStreamPtr st,
 fd = socket(AF_UNIX, SOCK_STREAM, 0);
 if (fd < 0) {
 virReportSystemError(errno, "%s", _("Unable to open UNIX socket"));
-goto error;
+return -1;
 }
 
 memset(, 0, sizeof(sa));
 sa.sun_family = AF_UNIX;
 if (abstract) {
 if (virStrcpy(sa.sun_path+1, path, sizeof(sa.sun_path)-1) < 0)
-goto error;
+return -1;
 sa.sun_path[0] = '\0';
 } else {
 if (virStrcpyStatic(sa.sun_path, path) < 0)
-goto error;
+return -1;
 }
 
 if (virTimeBackOffStart(, 1, 3*1000 /* ms */) < 0)
-goto error;
+return -1;
 while (virTimeBackOffWait()) {
 ret = connect(fd, (struct sockaddr *), sizeof(sa));
 if (ret == 0)
@@ -1191,17 +1188,14 @@ int virFDStreamConnectUNIX(virStreamPtr st,
 continue;
 }
 
-goto error;
+return -1;
 }
 
 if (virFDStreamOpenInternal(st, fd, NULL, 0) < 0)
-goto error;
+return -1;
 
 fd = -1;
 return 0;
-
- error:
-return -1;
 }
 
 
-- 
2.26.2

[PATCH v2 00/17] Allow sparse streams for block devices

[Michal Privoznik]

v2 of:

https://www.redhat.com/archives/libvir-list/2020-July/msg00145.html

diff to v1:
- Switch virfdstream to glib (patches 1-6)
- Document the feature in NEWS.rst
- Introduced test cases for virStringIsNull()
- Included what was WIP patch in v1 => patch 16 which ensures block
  devices aren't read twice

Michal Prívozník (17):
  virfdstream: Use g_autofree in virFDStreamThreadDoRead()
  virFDStreamMsgQueuePush: Clear pointer to passed message
  virfdstream: Use autoptr for virFDStreamMsg
  virfdstream: Use g_new0() instead of VIR_ALLOC()
  virfdstream: Use VIR_AUTOCLOSE()
  virfdstream: Drop some needless labels
  libvirt-storage: Document volume upload/download stream format
  virstring: Introduce virStringIsNull()
  virfile: Introduce virFileInDataDetectZeroes()
  virsh: Pass virshStreamCallbackDataPtr to virshStreamSink() and
virshStreamSkip()
  virsh: Track if vol-upload or vol-download work over a block device
  virshStreamSkip: Emulate skip for block devices
  virfdstream: Allow sparse stream vol-download
  virshStreamInData: Handle block devices
  virfdstream: Emulate skip for block devices
  stream: Don't read block device twice
  news: Document sparse streams for block devcies

 NEWS.rst |   7 ++
 src/libvirt-storage.c|   8 +-
 src/libvirt_private.syms |   2 +
 src/util/virfdstream.c   | 179 ++-
 src/util/virfile.c   |  50 +++
 src/util/virfile.h   |   5 ++
 src/util/virstring.c |  38 +
 src/util/virstring.h |   2 +
 tests/virstringtest.c|  47 ++
 tools/virsh-util.c   |  90 +---
 tools/virsh-util.h   |   4 +
 tools/virsh-volume.c |  23 -
 12 files changed, 360 insertions(+), 95 deletions(-)

-- 
2.26.2

[PATCH v2 02/17] virFDStreamMsgQueuePush: Clear pointer to passed message

[Michal Privoznik]

All callers of virFDStreamMsgQueuePush() have the same pattern:
they explicitly set @msg passed to NULL to avoid freeing it later
on. Well, the function can take address of the pointer and clear
it for them.

Signed-off-by: Michal Privoznik 
---
 src/util/virfdstream.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index e29c95690b..6efe6c17ad 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -140,7 +140,7 @@ VIR_ONCE_GLOBAL_INIT(virFDStreamData);
 
 static int
 virFDStreamMsgQueuePush(virFDStreamDataPtr fdst,
-virFDStreamMsgPtr msg,
+virFDStreamMsgPtr *msg,
 int fd,
 const char *fdname)
 {
@@ -150,7 +150,7 @@ virFDStreamMsgQueuePush(virFDStreamDataPtr fdst,
 while (*tmp)
 tmp = &(*tmp)->next;
 
-*tmp = msg;
+*tmp = g_steal_pointer(msg);
 virCondSignal(>threadCond);
 
 if (safewrite(fd, , sizeof(c)) != sizeof(c)) {
@@ -489,8 +489,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst,
 *dataLen -= got;
 }
 
-virFDStreamMsgQueuePush(fdst, msg, fdout, fdoutname);
-msg = NULL;
+virFDStreamMsgQueuePush(fdst, , fdout, fdoutname);
 
 return got;
 
@@ -814,8 +813,7 @@ static int virFDStreamWrite(virStreamPtr st, const char 
*bytes, size_t nbytes)
 msg->stream.data.buf = buf;
 msg->stream.data.len = nbytes;
 
-virFDStreamMsgQueuePush(fdst, msg, fdst->fd, "pipe");
-msg = NULL;
+virFDStreamMsgQueuePush(fdst, , fdst->fd, "pipe");
 ret = nbytes;
 } else {
  retry:
@@ -1010,8 +1008,7 @@ virFDStreamSendHole(virStreamPtr st,
 
 msg->type = VIR_FDSTREAM_MSG_TYPE_HOLE;
 msg->stream.hole.len = length;
-virFDStreamMsgQueuePush(fdst, msg, fdst->fd, "pipe");
-msg = NULL;
+virFDStreamMsgQueuePush(fdst, , fdst->fd, "pipe");
 }
 } else {
 off = lseek(fdst->fd, length, SEEK_CUR);
-- 
2.26.2

Re: [PATCH v6 00/10] Tighten qemu-img rules on missing backing format

[Kevin Wolf]

Am 06.07.2020 um 22:39 hat Eric Blake geschrieben:
> v5 was here:
> https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg00679.html
> In v6:
> - add a few more patches
> - change qcow semantics based on several iterations of mailing list
> debates on what behavior is easiest to support
> - add iotesting that a probed raw file cannot be committed into
> - instead of recording an implicit probed raw file, instead we record
> only a non-raw file
> - rebase to a few more affected iotests, plus s/5.0/5.1/
> 
> Yes, I know this is really short notice to make it in before feature
> freeze for 5.1 (removal in 6.0), so it may end up slipping into 5.2
> (removal in 6.1); but we'll see how things go.

Thanks, applied to the block branch.

Kevin

Re: [PATCH v6 08/10] iotests: Specify explicit backing format where sensible

[Kevin Wolf]

Am 06.07.2020 um 22:39 hat Eric Blake geschrieben:
> There are many existing qcow2 images that specify a backing file but
> no format.  This has been the source of CVEs in the past, but has
> become more prominent of a problem now that libvirt has switched to
> -blockdev.  With older -drive, at least the probing was always done by
> qemu (so the only risk of a changed format between successive boots of
> a guest was if qemu was upgraded and probed differently).  But with
> newer -blockdev, libvirt must specify a format; if libvirt guesses raw
> where the image was formatted, this results in data corruption visible
> to the guest; conversely, if libvirt guesses qcow2 where qemu was
> using raw, this can result in potential security holes, so modern
> libvirt instead refuses to use images without explicit backing format.
> 
> The change in libvirt to reject images without explicit backing format
> has pointed out that a number of tools have been far too reliant on
> probing in the past.  It's time to set a better example in our own
> iotests of properly setting this parameter.
> 
> iotest calls to create, rebase, and convert are all impacted to some
> degree.  It's a bit annoying that we are inconsistent on command line
> - while all of those accept -o backing_file=...,backing_fmt=..., the
> shortcuts are different: create and rebase have -b and -F, while
> convert has -B but no -F.  (amend has no shortcuts, but the previous
> patch just deprecated the use of amend to change backing chains).
> 
> Signed-off-by: Eric Blake 

This breaks at least 024 and 043 for qed because qemu-img info can't
print the backing file format there (qed only saves a flag whether it's
raw or non-raw).

We can fix the output filtering during the freeze, though.

Kevin

[PATCH 3/4] backupxml2xmltest: Call 'virDomainBackupAlignDisks' before formatting output

[Peter Krempa]

Call the post-processing function so that we can validate that it does
the correct thing.

virDomainBackupAlignDisks requires disk definitions to be present so
let's fake them by copying disks from the backup definition and add one
extra disk 'vdextradisk'.

Signed-off-by: Peter Krempa 
---
 .../backup-pull-encrypted.xml |  1 +
 .../backup-pull-internal-invalid.xml  |  1 +
 .../backup-pull-seclabel.xml  |  1 +
 tests/domainbackupxml2xmlout/backup-pull.xml  |  1 +
 .../backup-push-encrypted.xml |  1 +
 .../backup-push-seclabel.xml  |  1 +
 tests/domainbackupxml2xmlout/backup-push.xml  |  1 +
 tests/domainbackupxml2xmlout/empty.xml|  8 -
 tests/genericxml2xmltest.c| 36 +++
 9 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/tests/domainbackupxml2xmlout/backup-pull-encrypted.xml 
b/tests/domainbackupxml2xmlout/backup-pull-encrypted.xml
index ea9dcf72b9..3c3042111d 100644
--- a/tests/domainbackupxml2xmlout/backup-pull-encrypted.xml
+++ b/tests/domainbackupxml2xmlout/backup-pull-encrypted.xml
@@ -26,5 +26,6 @@
 
   
 
+
   
 
diff --git a/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml 
b/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml
index ba8f7ca3ab..9702978ce0 100644
--- a/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml
+++ b/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml
@@ -26,6 +26,7 @@
 
   
 
+
   
   
 
diff --git a/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml 
b/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml
index 450f007d3a..38330394f7 100644
--- a/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml
+++ b/tests/domainbackupxml2xmlout/backup-pull-seclabel.xml
@@ -14,5 +14,6 @@
 
   
 
+
   
 
diff --git a/tests/domainbackupxml2xmlout/backup-pull.xml 
b/tests/domainbackupxml2xmlout/backup-pull.xml
index 24fce9c0e7..4952270a5a 100644
--- a/tests/domainbackupxml2xmlout/backup-pull.xml
+++ b/tests/domainbackupxml2xmlout/backup-pull.xml
@@ -6,5 +6,6 @@
   
 
 
+
   
 
diff --git a/tests/domainbackupxml2xmlout/backup-push-encrypted.xml 
b/tests/domainbackupxml2xmlout/backup-push-encrypted.xml
index a955340964..2a5aad93cd 100644
--- a/tests/domainbackupxml2xmlout/backup-push-encrypted.xml
+++ b/tests/domainbackupxml2xmlout/backup-push-encrypted.xml
@@ -25,5 +25,6 @@
 
   
 
+
   
 
diff --git a/tests/domainbackupxml2xmlout/backup-push-seclabel.xml 
b/tests/domainbackupxml2xmlout/backup-push-seclabel.xml
index 9986889ba3..59af3e6a6c 100644
--- a/tests/domainbackupxml2xmlout/backup-push-seclabel.xml
+++ b/tests/domainbackupxml2xmlout/backup-push-seclabel.xml
@@ -13,5 +13,6 @@
 
   
 
+
   
 
diff --git a/tests/domainbackupxml2xmlout/backup-push.xml 
b/tests/domainbackupxml2xmlout/backup-push.xml
index 1997c814ae..bc11a93d94 100644
--- a/tests/domainbackupxml2xmlout/backup-push.xml
+++ b/tests/domainbackupxml2xmlout/backup-push.xml
@@ -6,5 +6,6 @@
   
 
 
+
   
 
diff --git a/tests/domainbackupxml2xmlout/empty.xml 
b/tests/domainbackupxml2xmlout/empty.xml
index b1ba4953be..52d2b4f0af 100644
--- a/tests/domainbackupxml2xmlout/empty.xml
+++ b/tests/domainbackupxml2xmlout/empty.xml
@@ -1 +1,7 @@
-
+
+  
+
+  
+
+  
+
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index 2c1e8616dd..8b9b0bafb6 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -51,6 +51,23 @@ struct testCompareBackupXMLData {
 };


+static virDomainDiskDefPtr
+testCompareBackupXMLGetFakeDomdisk(const char *dst)
+{
+virDomainDiskDefPtr domdisk = NULL;
+
+if (!(domdisk = virDomainDiskDefNew(NULL)))
+abort();
+
+domdisk->dst = g_strdup(dst);
+domdisk->src->type = VIR_STORAGE_TYPE_FILE;
+domdisk->src->format = VIR_STORAGE_FILE_QCOW2;
+domdisk->src->path = g_strdup_printf("/fake/%s.qcow2", dst);
+
+return domdisk;
+}
+
+
 static int
 testCompareBackupXML(const void *opaque)
 {
@@ -63,6 +80,8 @@ testCompareBackupXML(const void *opaque)
 g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
 g_autofree char *actual = NULL;
 unsigned int parseFlags = 0;
+g_autoptr(virDomainDef) fakedef = NULL;
+size_t i;

 if (data->internal)
 parseFlags |= VIR_DOMAIN_BACKUP_PARSE_INTERNAL;
@@ -80,6 +99,23 @@ testCompareBackupXML(const void *opaque)
 return -1;
 }

+/* create a fake definition and fill it with disks */
+if (!(fakedef = virDomainDefNew()))
+return -1;
+
+fakedef->ndisks = backup->ndisks + 1;
+fakedef->disks = g_new0(virDomainDiskDefPtr, fakedef->ndisks);
+
+for (i = 0; i < backup->ndisks; i++)
+fakedef->disks[i] = 
testCompareBackupXMLGetFakeDomdisk(backup->disks[i].name);
+
+fakedef->disks[fakedef->ndisks -1 ] = 

[PATCH 2/4] virDomainBackupDiskDefFormat: Format internal disk state only when valid

[Peter Krempa]

Format the disk state only when it isn't _NONE.

Signed-off-by: Peter Krempa 
---
 src/conf/backup_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index 781dd53f6b..5e4144d371 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -370,7 +370,7 @@ virDomainBackupDiskDefFormat(virBufferPtr buf,

 virBufferEscapeString(, " name='%s'", disk->name);
 virBufferAsprintf(, " backup='%s'", 
virTristateBoolTypeToString(disk->backup));
-if (internal)
+if (internal && disk->state != VIR_DOMAIN_BACKUP_DISK_STATE_NONE)
 virBufferAsprintf(, " state='%s'", 
virDomainBackupDiskStateTypeToString(disk->state));

 if (disk->backup == VIR_TRISTATE_BOOL_YES) {
-- 
2.26.2

[PATCH 4/4] backup: Allow configuring incremental backup per-disk individually

[Peter Krempa]

The semantics of the backup operation don't strictly require that all
disks being backed up are part of the same incremental part (when a disk
was checkpointed/backed up separately or in a different VM), or even
they may not have an previous checkpoint at all (e.g. when the disk
was freshly hotplugged to the vm).

In such cases we can still create a common checkpoint for all of them
and backup differences according to configuration.

This patch adds a per-disk configuration of the checkpoint to do the
incremental backup from via the 'incremental' attribute and allows
perform full backups via the 'backupmode' attribute.

Note that no changes to the qemu driver are necessary to take advantage
of this as we already obey the per-disk 'incremental' field.

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

Signed-off-by: Peter Krempa 
---
v2:
- backupmode=full incremental="..." is now rejected by the RNG
  schema
- test output changes as we now validate virDomainBackupAlignDisks
- typo fix

 docs/formatbackup.rst | 11 +
 docs/schemas/domainbackup.rng | 23 +
 src/conf/backup_conf.c| 49 ++-
 src/conf/backup_conf.h| 11 +
 tests/domainbackupxml2xmlin/backup-pull.xml   | 12 +
 .../backup-pull-encrypted.xml |  6 +--
 .../backup-pull-internal-invalid.xml  |  6 +--
 .../backup-pull-seclabel.xml  |  4 +-
 tests/domainbackupxml2xmlout/backup-pull.xml  | 14 +-
 .../backup-push-encrypted.xml |  6 +--
 .../backup-push-seclabel.xml  |  4 +-
 tests/domainbackupxml2xmlout/backup-push.xml  |  2 +-
 tests/domainbackupxml2xmlout/empty.xml|  2 +-
 13 files changed, 133 insertions(+), 17 deletions(-)

diff --git a/docs/formatbackup.rst b/docs/formatbackup.rst
index 17431fe51a..1b9e6ebb22 100644
--- a/docs/formatbackup.rst
+++ b/docs/formatbackup.rst
@@ -69,6 +69,17 @@ were supplied). The following child elements and attributes 
are supported:
  should take part in the backup and using ``no`` excludes the disk from
  the backup.

+  ``backupmode``
+ This attribute overrides the implied backup mode inherited from the
+ definition of the backup itself. Value ``full`` forces a full backup
+ even if the backup calls for an incremental backup, and 
``incremental``
+ coupled with the attribute ``incremental='CHECKPOINTNAME`` for the 
disk
+ forces an incremental backup from ``CHECKPOINTNAME``.
+
+   ``incremental``
+ An optional attribute giving the name of an existing checkpoint of the
+ domain which overrides the one set by the  element.
+
   ``exportname``
  Allows modification of the NBD export name for the given disk. By
  default equal to disk target. Valid only for pull mode backups.
diff --git a/docs/schemas/domainbackup.rng b/docs/schemas/domainbackup.rng
index c0e17f512b..579b62a658 100644
--- a/docs/schemas/domainbackup.rng
+++ b/docs/schemas/domainbackup.rng
@@ -96,6 +96,27 @@
 
   

+
+  
+
+  
+
+  full
+
+
+  
+
+  incremental
+
+  
+  
+
+  
+
+  
+
+  
+
   
 
   
@@ -134,6 +155,7 @@
 
   
 
+
 
   
 
@@ -203,6 +225,7 @@
 
   
 
+
 
   
 
diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index 5e4144d371..02319f7245 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -56,6 +56,13 @@ VIR_ENUM_IMPL(virDomainBackupDiskState,
   "cancelling",
   "cancelled");

+VIR_ENUM_DECL(virDomainBackupDiskBackupMode);
+VIR_ENUM_IMPL(virDomainBackupDiskBackupMode,
+  VIR_DOMAIN_BACKUP_DISK_BACKUP_MODE_LAST,
+  "",
+  "full",
+  "incremental");
+
 void
 virDomainBackupDefFree(virDomainBackupDefPtr def)
 {
@@ -100,6 +107,7 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node,
 g_autofree char *driver = NULL;
 g_autofree char *backup = NULL;
 g_autofree char *state = NULL;
+g_autofree char *backupmode = NULL;
 int tmp;
 xmlNodePtr srcNode;
 unsigned int storageSourceParseFlags = 0;
@@ -137,6 +145,19 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node,
 def->exportbitmap = virXMLPropString(node, "exportbitmap");
 }

+if ((backupmode = virXMLPropString(node, "backupmode"))) {
+if ((tmp = virDomainBackupDiskBackupModeTypeFromString(backupmode)) < 
0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("invalid backupmode '%s' of disk '%s'"),
+   backupmode, def->name);
+return -1;
+}
+
+   

[PATCH 1/4] backupxml2xmltest: Remove output symlink of 'backup-pull-internal-invalid'

[Peter Krempa]

Replace the output by a copy of the input file for further changes once
we start testing virDomainBackupAlignDisks.

Signed-off-by: Peter Krempa 
---
 .../backup-pull-internal-invalid.xml  | 37 ++-
 1 file changed, 36 insertions(+), 1 deletion(-)
 mode change 12 => 100644 
tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml

diff --git a/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml 
b/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml
deleted file mode 12
index 055ca37a0b..00
--- a/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml
+++ /dev/null
@@ -1 +0,0 @@
-../domainbackupxml2xmlin/backup-pull-internal-invalid.xml
\ No newline at end of file
diff --git a/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml 
b/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml
new file mode 100644
index 00..ba8f7ca3ab
--- /dev/null
+++ b/tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml
@@ -0,0 +1,36 @@
+
+  1525889631
+  
+  
+
+  
+  
+
+  
+
+  
+
+
+  
+  
+
+  
+
+  
+
+
+  
+  
+
+  
+
+  
+
+  
+  
+
+  
+  
+
+  
+
-- 
2.26.2

[PATCH v2 0/4] backup: Allow configuring incremental backup per-disk individually

[Peter Krempa]

This series addresses some of the testing issues raised in the review of
4/4. Patches 1-3 are new adding testing of virDomainBackupAlignDisks.

Peter Krempa (4):
  backupxml2xmltest: Remove output symlink of
'backup-pull-internal-invalid'
  virDomainBackupDiskDefFormat: Format internal disk state only when
valid
  backupxml2xmltest: Call 'virDomainBackupAlignDisks' before formatting
output
  backup: Allow configuring incremental backup per-disk individually

 docs/formatbackup.rst | 11 
 docs/schemas/domainbackup.rng | 23 +
 src/conf/backup_conf.c| 51 ++-
 src/conf/backup_conf.h| 11 
 tests/domainbackupxml2xmlin/backup-pull.xml   | 12 +
 .../backup-pull-encrypted.xml |  7 +--
 .../backup-pull-internal-invalid.xml  | 38 +-
 .../backup-pull-seclabel.xml  |  5 +-
 tests/domainbackupxml2xmlout/backup-pull.xml  | 15 +-
 .../backup-push-encrypted.xml |  7 +--
 .../backup-push-seclabel.xml  |  5 +-
 tests/domainbackupxml2xmlout/backup-push.xml  |  3 +-
 tests/domainbackupxml2xmlout/empty.xml|  8 ++-
 tests/genericxml2xmltest.c| 36 +
 14 files changed, 216 insertions(+), 16 deletions(-)
 mode change 12 => 100644 
tests/domainbackupxml2xmlout/backup-pull-internal-invalid.xml

-- 
2.26.2

Re: [PATCH 14/24] backup: Allow configuring incremental backup per-disk individually

[Peter Krempa]

On Thu, Jul 02, 2020 at 14:31:19 -0500, Eric Blake wrote:
> On 7/2/20 9:40 AM, Peter Krempa wrote:
> > The semantics of the backup operation don't strictly require that all
> > disks being backed up are part of the same incremental part (when a disk
> > was checkpointed/backed up separately or in a different VM), or even
> > they may not have an previous checkpoint at all (e.g. when the disk
> > was freshly hotplugged to the vm).
> > 
> > In such cases we can still create a common checkpoint for all of them
> > and backup differences according to configuration.
> > 
> > This patch adds a per-disk configuration of the checkpoint to do the
> > incremental backup from via the 'incremental' attribute and allows
> > perform full backups via the 'backupmode' attribute.
> > 
> > Note that no changes to the qemu driver are necessary to take advantage
> > of this as we already obey the per-disk 'incremental' field.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1829829
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >   docs/formatbackup.rst| 11 
> >   docs/schemas/domainbackup.rng| 16 ++
> >   src/conf/backup_conf.c   | 57 +++-
> >   src/conf/backup_conf.h   | 11 
> >   tests/domainbackupxml2xmlin/backup-pull.xml  | 12 +
> >   tests/domainbackupxml2xmlout/backup-pull.xml | 12 +
> >   6 files changed, 118 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/formatbackup.rst b/docs/formatbackup.rst
> > index 66583f562b..e5b6fc6eb0 100644
> > --- a/docs/formatbackup.rst
> > +++ b/docs/formatbackup.rst
> > @@ -65,6 +65,17 @@ were supplied). The following child elements and 
> > attributes are supported:
> >should take part in the backup and using ``no`` excludes the 
> > disk from
> >the backup.
> > 
> > +  ``backupmode``
> > + This attribute overrides the implied backup mode inherited from 
> > the
> > + definition of the backup itself. Value ``full`` forces a full 
> > backup
> > + even if the backup calls for an incremental backup and 
> > ``incremental``
> 
> s/backup and/backup, and/
> 
> > + coupled with the attribute ``incremental='CHECKPOINTNAME`` for 
> > the disk
> > + forces an incremental backup from ``CHECKPOINTNAME``.
> > +
> > +   ``incremental``
> > + An optional attribute giving the name of an existing checkpoint 
> > of the
> > + domain which overrides the one set by the  
> > element.
> > +
> > ``exportname``
> >Allows modification of the NBD export name for the given disk. By
> >default equal to disk target. Valid only for pull mode backups.
> > diff --git a/docs/schemas/domainbackup.rng b/docs/schemas/domainbackup.rng
> > index 5165175152..650f5cd4c3 100644
> > --- a/docs/schemas/domainbackup.rng
> > +++ b/docs/schemas/domainbackup.rng
> > @@ -89,6 +89,20 @@
> >   
> > 
> > 
> > +  
> > +
> > +  
> > +
> > +  full
> > +  incremental
> > +
> > +  
> > +
> > +
> > +  
> > +
> > +  
> 
> As written, you validate:
> 
> backupmode="full" incremental="blah"
> 
> Better might be:
> 
> 
>   
> 
>   
> full
>   
>   
> 
>   
> incremental
>   
> 
> 
>   
> 
>   
> 
>   
> 
> 
> which also has the advantage of allowing the user to omit
> backupmode='incremental' when supplying incremental='name' (since then that
> mode is implied).

Nice, I'll use this one. My brain stopped working when doing the schema
and I couldn't figure this one out.

> 
> Do we need to restrict the set of values that can be supplied for a
> incremental name?  (That's a bigger issue than just this patch: for example,
> do we want to refuse a checkpoint named "../foo"?  As long as checkpoint
> names don't match directly to file names, we aren't at risk of a filesystem
> escape, but starting strict and relaxing later is better than starting
> relaxed and wishing we had limited certain patterns after all)

I'll think about this and possbily post a separate patch. The same also
applies to the  element which also doesn't do validation.

Luckily it's not officially supported yet so we can still make it more
strict.

> > @@ -465,6 +493,24 @@ virDomainBackupAlignDisks(virDomainBackupDefPtr def,
> >   return -1;
> >   }
> > 
> > +if (backupdisk->backupmode == 
> > VIR_DOMAIN_BACKUP_DISK_BACKUP_MODE_FULL &&
> > +backupdisk->incremental) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("'full' backup mode incompatible with 
> > 'incremental' for disk '%s'"),
> > +   backupdisk->name);
> > +return -1;
> > +}
> 
> You had to check this manually, instead of letting the .rng file enforce it
> for you by the 

[PATCH] Change the virtual NICs limit for the ESX driver

[Bastien Orivel]

Since the ESX virtual hardware version 4.0, virtual machines support up
to 10 virtual NICs instead of 4 previously. This changes the limit
accordingly based on the provided `virtualHW.version`.

Signed-off-by: Bastien Orivel 
---
 src/vmx/vmx.c | 20 ++--
 src/vmx/vmx.h |  2 +-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 67bbe27fde..afe6fe0a1a 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -290,7 +290,7 @@ def->fss[0]...<=>   
sharedFolder0.present = "true"
 

 ## nets 

 
-ethernet[0..3] -> 
+ethernet[0..9] -> 
 
 ethernet0.present = "true" 
 # defaults to "false"
 ethernet0.startConnected = "true"  
 # defaults to "true"
@@ -3376,7 +3376,7 @@ virVMXFormatConfig(virVMXContext *ctx, 
virDomainXMLOptionPtr xmlopt, virDomainDe
 
 /* def:nets */
 for (i = 0; i < def->nnets; ++i) {
-if (virVMXFormatEthernet(def->nets[i], i, ) < 0)
+if (virVMXFormatEthernet(def->nets[i], i, , virtualHW_version) 
< 0)
 goto cleanup;
 }
 
@@ -3732,15 +3732,23 @@ virVMXFormatFileSystem(virDomainFSDefPtr def, int 
number, virBufferPtr buffer)
 
 int
 virVMXFormatEthernet(virDomainNetDefPtr def, int controller,
- virBufferPtr buffer)
+ virBufferPtr buffer, int virtualHW_version)
 {
 char mac_string[VIR_MAC_STRING_BUFLEN];
 unsigned int prefix, suffix;
 
-if (controller < 0 || controller > 3) {
+/*
+ * Machines older than virtualHW.version = 7 (ESXi 4.0) only support up to 
4
+ * virtual NICs. New machines support up to 10.
+ */
+int controller_limit = 4;
+if (virtualHW_version >= 7)
+controller_limit = 10;
+
+if (controller < 0 || controller > controller_limit) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Ethernet controller index %d out of [0..3] range"),
-   controller);
+   _("Ethernet controller index %d out of [0..%d] range"),
+   controller, controller_limit - 1);
 return -1;
 }
 
diff --git a/src/vmx/vmx.h b/src/vmx/vmx.h
index 63f47822fb..7069a50b6e 100644
--- a/src/vmx/vmx.h
+++ b/src/vmx/vmx.h
@@ -131,7 +131,7 @@ int virVMXFormatFileSystem(virDomainFSDefPtr def, int 
number,
virBufferPtr buffer);
 
 int virVMXFormatEthernet(virDomainNetDefPtr def, int controller,
- virBufferPtr buffer);
+ virBufferPtr buffer, int virtualHW_version);
 
 int virVMXFormatSerial(virVMXContext *ctx, virDomainChrDefPtr def,
virBufferPtr buffer);
-- 
2.20.1

Re: [libvirt PATCH] util: fix accessibility check for hook directory

[Daniel Henrique Barboza]

On 7/7/20 9:14 AM, Ján Tomko wrote:

virFileIsAccessible does not return true on accessible
directories. Check whether it set EISDIR and only
then assume the directory is inaccessible.

Return 0 (not found) instead of 1 (found),
since the bridge driver taints the network based on
this return value, not whether the hook actually ran.

Remove the bogus check from virHookCall, since it already
checks the virHooksFound bitmap that was filled before
by virHookCheck.

Signed-off-by: Ján Tomko 
Fixes: 7fa7f7eeb6e969e002845928e155914da2fc8cd0
Closes: https://gitlab.com/libvirt/libvirt/-/issues/47
---


Reviewed-by: Daniel Henrique Barboza 


  src/util/virhook.c | 7 ++-
  1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/util/virhook.c b/src/util/virhook.c
index d1ac518c24..119ad1aae6 100644
--- a/src/util/virhook.c
+++ b/src/util/virhook.c
@@ -172,9 +172,9 @@ virHookCheck(int no, const char *driver)
  
  dir_path = g_strdup_printf("%s.d", path);
  
-if (!virFileIsExecutable(dir_path)) {

+if (!virFileIsExecutable(dir_path) && errno != EISDIR) {
  VIR_DEBUG("Hook dir %s is not accessible", dir_path);
-return 1;
+return 0;
  }
  
  if ((ret = virDirOpenIfExists(, dir_path)) < 0)

@@ -422,9 +422,6 @@ virHookCall(int driver,
  
  dir_path = g_strdup_printf("%s.d", path);
  
-if (!virFileIsExecutable(dir_path))

-return script_ret;
-
  if ((ret = virDirOpenIfExists(, dir_path)) < 0)
  return -1;
  

[PATCH] storage: fix vstorage backend build

[Nikolay Shirokovskiy]

Add headers with declarations of  geteuid/getegid
and virGetUserName/virGetGroupName.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/storage/storage_backend_vstorage.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/storage/storage_backend_vstorage.c 
b/src/storage/storage_backend_vstorage.c
index 85ab832..6cff9f1 100644
--- a/src/storage/storage_backend_vstorage.c
+++ b/src/storage/storage_backend_vstorage.c
@@ -6,10 +6,12 @@
 #include "storage_backend_vstorage.h"
 #include "virlog.h"
 #include "virstring.h"
+#include "virutil.h"
 #include 
 #include 
 #include 
 #include 
+#include 
 #include "storage_util.h"
 
 #define VIR_FROM_THIS VIR_FROM_STORAGE
-- 
1.8.3.1

[libvirt PATCH] util: fix accessibility check for hook directory

[Ján Tomko]

virFileIsAccessible does not return true on accessible
directories. Check whether it set EISDIR and only
then assume the directory is inaccessible.

Return 0 (not found) instead of 1 (found),
since the bridge driver taints the network based on
this return value, not whether the hook actually ran.

Remove the bogus check from virHookCall, since it already
checks the virHooksFound bitmap that was filled before
by virHookCheck.

Signed-off-by: Ján Tomko 
Fixes: 7fa7f7eeb6e969e002845928e155914da2fc8cd0
Closes: https://gitlab.com/libvirt/libvirt/-/issues/47
---
 src/util/virhook.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/util/virhook.c b/src/util/virhook.c
index d1ac518c24..119ad1aae6 100644
--- a/src/util/virhook.c
+++ b/src/util/virhook.c
@@ -172,9 +172,9 @@ virHookCheck(int no, const char *driver)
 
 dir_path = g_strdup_printf("%s.d", path);
 
-if (!virFileIsExecutable(dir_path)) {
+if (!virFileIsExecutable(dir_path) && errno != EISDIR) {
 VIR_DEBUG("Hook dir %s is not accessible", dir_path);
-return 1;
+return 0;
 }
 
 if ((ret = virDirOpenIfExists(, dir_path)) < 0)
@@ -422,9 +422,6 @@ virHookCall(int driver,
 
 dir_path = g_strdup_printf("%s.d", path);
 
-if (!virFileIsExecutable(dir_path))
-return script_ret;
-
 if ((ret = virDirOpenIfExists(, dir_path)) < 0)
 return -1;
 
-- 
2.25.4

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

[Peter Krempa]

On Tue, Jul 07, 2020 at 06:36:23 -0500, Eric Blake wrote:
> On 7/7/20 2:12 AM, Peter Krempa wrote:
> > 
> > 1) the virDomainBlockCopy operation flattens the backing chain into the
> > top level only. This means that  must be stripped or the
> > operation rejected, as otherwise shutting down the VM would end up
> > removing the disk image completely.
> 
> If you have marked a disk transient, does it still make sense to allow
> copying that disk?  Rejecting the operation is easiest, as permitting it
> implies that even though you already said you don't care about changes to
> your disk, you still want to be able to back up that disk.

Back up? This is more about moving to new storage.

> > 2) the same as above is used also for non-shared-storage migration where
> > we use block-copy internally to transport the disks, same as above
> > applies. Here again it requires careful consideration of the semantics,
> > e.g whether to reject it or e.g. copy it into the original filename and
> > strip  (we can't currently copy multiple layers).
> 
> The easiest solution is to make a transient disk a migration-blocker. Of
> course, this may annoy people, so migration properly creating a transient
> destination on top of the original base, to preserve the fact that when the
> migrated guest shuts down it reverts to original contents, is a nicer (but
> more complex) goal.

To be fair, our docs already point out that  prevents
migration. It still needs to be implemented.

Same appliest to snapshots.

> > 3) active-layer virDomainBlockCommit moves the data from the transient
> > overlay into the original (now backing image). The  flag is
> > stored in the disk struct though, so that would mean that the original
> > disk source would be removed after stopping the VM. block commit must
> > clear the  flag.
> 
> Why should commit be permitted, when you declared that disk contents
> shouldn't change?  For that matter, external snapshots should be blocked if
> there is a transient disk.

I might have read it in the qemu documentation, but commit is meant to
actually ... commit ... the changes user might want to keep.

I'm okay with forbidding it though in libvirt.

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

[Eric Blake]

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.




Additionally there are corner cases which this patch doesn't deal with:

1) the virDomainBlockCopy operation flattens the backing chain into the
top level only. This means that  must be stripped or the
operation rejected, as otherwise shutting down the VM would end up
removing the disk image completely.


If you have marked a disk transient, does it still make sense to allow 
copying that disk?  Rejecting the operation is easiest, as permitting it 
implies that even though you already said you don't care about changes 
to your disk, you still want to be able to back up that disk.




2) the same as above is used also for non-shared-storage migration where
we use block-copy internally to transport the disks, same as above
applies. Here again it requires careful consideration of the semantics,
e.g whether to reject it or e.g. copy it into the original filename and
strip  (we can't currently copy multiple layers).


The easiest solution is to make a transient disk a migration-blocker. 
Of course, this may annoy people, so migration properly creating a 
transient destination on top of the original base, to preserve the fact 
that when the migrated guest shuts down it reverts to original contents, 
is a nicer (but more complex) goal.




3) active-layer virDomainBlockCommit moves the data from the transient
overlay into the original (now backing image). The  flag is
stored in the disk struct though, so that would mean that the original
disk source would be removed after stopping the VM. block commit must
clear the  flag.


Why should commit be permitted, when you declared that disk contents 
shouldn't change?  For that matter, external snapshots should be blocked 
if there is a transient disk.




One nice-to-have but not required modification would be to allow
configuration of the transient disk's overlay path.




--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [libvirt PATCH v2 0/2] docs: Point to pkg.go.dev instead of godoc.org

[Daniel P . Berrangé]

On Tue, Jul 07, 2020 at 12:48:35PM +0200, Andrea Bolognani wrote:
> Changes from [v1]:
> 
>   * fix a few instances where we were still using github.com instead
> of libvirt.org for Go imports;
> 
>   * replace all uses of godoc.org, not just a subset.
> 
> [v1] https://www.redhat.com/archives/libvir-list/2020-July/msg00270.html
> 
> Andrea Bolognani (2):
>   docs: Use libvirt.org namespace for Go bindings
>   docs: Point to pkg.go.dev instead of godoc.org
> 
>  docs/bindings.html.in   | 2 +-
>  docs/docs.html.in   | 2 +-
>  docs/downloads.html.in  | 4 ++--
>  docs/libvirt-go-xml.rst | 2 +-
>  docs/libvirt-go.rst | 2 +-
>  5 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 23/24] conf: backup: Add 'tls' attribute for 'server' element

[Peter Krempa]

On Thu, Jul 02, 2020 at 14:53:28 -0500, Eric Blake wrote:
> On 7/2/20 9:40 AM, Peter Krempa wrote:
> > Allow enabling TLS for the NBD server used to do pull-mode backups. Note
> > that documentation already mentions 'tls', so this just implements the
> > schema and XML bits.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> 
> > +++ b/tests/domainbackupxml2xmlin/backup-pull-encrypted.xml
> > @@ -1,6 +1,6 @@
> >   
> > 1525889631
> > -  
> > +  
> 
> So this doesn't say what files are actually feeding the TLS configuration;
> the docs already mentioned 'tls', but do we need to add a cross-reference
> that states when tls='yes' is in effect then the server uses the files as
> configured in qemu.conf?  Knowing how the server is keyed is important for
> writing a client that can connect over TLS to the server.

Note that patch 22 actually adds the following paragraph to
formatbackup.rst into the NBD section:

+   Note that for the QEMU hypervisor the TLS environment in controlled using
+   ``backup_tls_x509_cert_dir``, ``backup_tls_x509_verify``, and
+   ``backup_tls_x509_secret_uuid`` properties in ``/etc/libvirt/qemu.conf``.


> But the overall idea makes sense.
> 
> Reviewed-by: Eric Blake 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 

Re: [libvirt PATCH] docs: Point to pkg.go.dev instead of godoc.org

[Daniel P . Berrangé]

On Tue, Jul 07, 2020 at 12:39:22PM +0200, Andrea Bolognani wrote:
> The former is the new recommended frontend for browsing Go API
> documentation online.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/libvirt-go-xml.rst | 2 +-
>  docs/libvirt-go.rst | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[libvirt PATCH v2 1/2] docs: Use libvirt.org namespace for Go bindings

[Andrea Bolognani]

Fixes: 193ad364062407c3fcd3267f0f135d8960b53020
Signed-off-by: Andrea Bolognani 
---
 docs/bindings.html.in | 2 +-
 docs/docs.html.in | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/bindings.html.in b/docs/bindings.html.in
index 8a482015b9..081af25ebb 100644
--- a/docs/bindings.html.in
+++ b/docs/bindings.html.in
@@ -16,7 +16,7 @@
   
  
 Go: Daniel Berrange develops
-https://godoc.org/github.com/libvirt/libvirt-go;>Go 
bindings.
+https://godoc.org/libvirt.org/libvirt-go;>Go bindings.
   
   
 Java: Daniel Veillard develops
diff --git a/docs/docs.html.in b/docs/docs.html.in
index f1d44fadc0..559910a2d0 100644
--- a/docs/docs.html.in
+++ b/docs/docs.html.in
@@ -82,7 +82,7 @@
 Language bindings and API modules
 Bindings of the libvirt API for
   c#,
-  https://godoc.org/github.com/libvirt/libvirt-go;>go,
+  https://godoc.org/libvirt.org/libvirt-go;>go,
   java,
   https://libvirt.org/ocaml/;>ocaml,
   http://search.cpan.org/dist/Sys-Virt/;>perl,
-- 
2.25.4

[libvirt PATCH v2 0/2] docs: Point to pkg.go.dev instead of godoc.org

[Andrea Bolognani]

Changes from [v1]:

  * fix a few instances where we were still using github.com instead
of libvirt.org for Go imports;

  * replace all uses of godoc.org, not just a subset.

[v1] https://www.redhat.com/archives/libvir-list/2020-July/msg00270.html

Andrea Bolognani (2):
  docs: Use libvirt.org namespace for Go bindings
  docs: Point to pkg.go.dev instead of godoc.org

 docs/bindings.html.in   | 2 +-
 docs/docs.html.in   | 2 +-
 docs/downloads.html.in  | 4 ++--
 docs/libvirt-go-xml.rst | 2 +-
 docs/libvirt-go.rst | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.25.4

[libvirt PATCH v2 2/2] docs: Point to pkg.go.dev instead of godoc.org

[Andrea Bolognani]

The former is the new recommended frontend for browsing Go API
documentation online.

Signed-off-by: Andrea Bolognani 
---
 docs/bindings.html.in   | 2 +-
 docs/docs.html.in   | 2 +-
 docs/downloads.html.in  | 4 ++--
 docs/libvirt-go-xml.rst | 2 +-
 docs/libvirt-go.rst | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/docs/bindings.html.in b/docs/bindings.html.in
index 081af25ebb..692390c843 100644
--- a/docs/bindings.html.in
+++ b/docs/bindings.html.in
@@ -16,7 +16,7 @@
   
  
 Go: Daniel Berrange develops
-https://godoc.org/libvirt.org/libvirt-go;>Go bindings.
+https://pkg.go.dev/libvirt.org/libvirt-go;>Go bindings.
   
   
 Java: Daniel Veillard develops
diff --git a/docs/docs.html.in b/docs/docs.html.in
index 559910a2d0..ceb95a4f17 100644
--- a/docs/docs.html.in
+++ b/docs/docs.html.in
@@ -82,7 +82,7 @@
 Language bindings and API modules
 Bindings of the libvirt API for
   c#,
-  https://godoc.org/libvirt.org/libvirt-go;>go,
+  https://pkg.go.dev/libvirt.org/libvirt-go;>go,
   java,
   https://libvirt.org/ocaml/;>ocaml,
   http://search.cpan.org/dist/Sys-Virt/;>perl,
diff --git a/docs/downloads.html.in b/docs/downloads.html.in
index 24ee27cc5c..33a49d9c1f 100644
--- a/docs/downloads.html.in
+++ b/docs/downloads.html.in
@@ -81,7 +81,7 @@
 https://github.com/libvirt/libvirt-go;>github
   
   
-https://godoc.org/libvirt.org/libvirt-go;>api ref
+https://pkg.go.dev/libvirt.org/libvirt-go;>api ref
   
 
 
@@ -245,7 +245,7 @@
 https://github.com/libvirt/libvirt-go-xml;>github
   
   
-https://godoc.org/libvirt.org/libvirt-go-xml;>api ref
+https://pkg.go.dev/libvirt.org/libvirt-go-xml;>api ref
   
 
 
diff --git a/docs/libvirt-go-xml.rst b/docs/libvirt-go-xml.rst
index ee3b878068..995fdd2b07 100644
--- a/docs/libvirt-go-xml.rst
+++ b/docs/libvirt-go-xml.rst
@@ -7,4 +7,4 @@ annotated Go struct definitions for parsing (and formatting) 
XML documents used
 with libvirt APIs.
 
 For details of Go specific behaviour consult the
-`Go package documentation `__
+`Go package documentation `__.
diff --git a/docs/libvirt-go.rst b/docs/libvirt-go.rst
index 6ec5c0c123..f2d3f80fb2 100644
--- a/docs/libvirt-go.rst
+++ b/docs/libvirt-go.rst
@@ -10,4 +10,4 @@ concepts to Go, so the native API documentation should serve 
as a reference
 for most behaviour.
 
 For details of Go specific behaviour consult the
-`Go package documentation `__
+`Go package documentation `__.
-- 
2.25.4

Re: [PATCH 18/24] qemu: checkpoint: Implement VIR_DOMAIN_CHECKPOINT_XML_SIZE

[Peter Krempa]

On Thu, Jul 02, 2020 at 14:42:50 -0500, Eric Blake wrote:
> On 7/2/20 9:40 AM, Peter Krempa wrote:
> > Introduce code which merges the appropriate bitmaps and queries the
> > final size of the backup, so that we can print the XML with size
> > information.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >   src/qemu/qemu_checkpoint.c | 143 -
> >   1 file changed, 142 insertions(+), 1 deletion(-)
> > 
> 
> > +/* we need to calculate the merged bitmap to obtain accurate data */
> > +for (i = 0; i < ndisks; i++) {
> > +virDomainDiskDefPtr domdisk = diskmap[i].domdisk;
> > +g_autoptr(virJSONValue) actions = NULL;
> > +
> > +/* possibly delete leftovers from previous cases */
> > +if (qemuBlockNamedNodeDataGetBitmapByName(blockNamedNodeData, 
> > domdisk->src,
> > +  "libvirt-tmp-size-xml")) 
> > {
> > +if (!recoveractions)
> > +recoveractions = virJSONValueNewArray();
> > +
> > +if (qemuMonitorTransactionBitmapRemove(recoveractions,
> > +   
> > domdisk->src->nodeformat,
> > +   "libvirt-tmp-size-xml") 
> > < 0)
> > +goto endjob;
> > +}
> 
> Odd that we may leave a temporary bitmap in qemu's memory if we fail partway
> through, but not the end of the world, and you handle it nicely here.

Well a SIGKILL or SIGSEGV between adding the bitmaps and querying and
removing them can always ruin your day. It's especially hard to recover
from such scenario, yet it doesn't feel to be worth adding status XML
for it.

[libvirt PATCH] docs: Point to pkg.go.dev instead of godoc.org

[Andrea Bolognani]

The former is the new recommended frontend for browsing Go API
documentation online.

Signed-off-by: Andrea Bolognani 
---
 docs/libvirt-go-xml.rst | 2 +-
 docs/libvirt-go.rst | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/libvirt-go-xml.rst b/docs/libvirt-go-xml.rst
index ee3b878068..995fdd2b07 100644
--- a/docs/libvirt-go-xml.rst
+++ b/docs/libvirt-go-xml.rst
@@ -7,4 +7,4 @@ annotated Go struct definitions for parsing (and formatting) 
XML documents used
 with libvirt APIs.
 
 For details of Go specific behaviour consult the
-`Go package documentation `__
+`Go package documentation `__.
diff --git a/docs/libvirt-go.rst b/docs/libvirt-go.rst
index 6ec5c0c123..f2d3f80fb2 100644
--- a/docs/libvirt-go.rst
+++ b/docs/libvirt-go.rst
@@ -10,4 +10,4 @@ concepts to Go, so the native API documentation should serve 
as a reference
 for most behaviour.
 
 For details of Go specific behaviour consult the
-`Go package documentation `__
+`Go package documentation `__.
-- 
2.25.4

Re: [PATCH 00/24] qemu: Incremental backup and TLS handling fixes

[Ján Tomko]

On a Tuesday in 2020, Andrea Bolognani wrote:

On Tue, 2020-07-07 at 09:47 +0200, Peter Krempa wrote:

On Thu, Jul 02, 2020 at 18:13:49 +0200, Andrea Bolognani wrote:
> Can you please include updates to the release notes in your series?
> Based on the summary above, it sounds like most of the changes will
> be user-visible. Thanks!

Incremental backup is still not enabled so I usually don't include
anything related to it in the news as users can't really use it without
hacking around.


Fair enough.


I'll definitely mention the encrypted keys support for NBD.

Also please note that I update news with my changes regularly when I
think it's worth mentioning so I'd prefer to be unsubscribed from these
notifications.


I can do that, but any reason not to update them at the same time as
you're introducing the corresponding code changes?


Conflict resolution. Though it should be possible to automate that.

Jano


If you're going to
do the work anyway, to me it makes sense to do all it one chunk and
not have to go back to it later. Makes it harder to accidentally lose
track and forget to do that, too.

--
Andrea Bolognani / Red Hat / Virtualization



signature.asc
Description: PGP signature

Re: [RFC PATCH 0/2] hw/sd: Deprecate the SPI mode and the SPI to SD adapter

[Alistair Francis]

On Sun, Jul 5, 2020 at 3:08 PM Philippe Mathieu-Daudé  wrote:
>
> I tried to maintain the SPI mode because it is useful in
> tiny embedded devices, and thought it would be helpful for
> the AVR MCUs.
> As AVR was blocked, I thought it was wise to deprecate the
> SPI mode as users are interested in the faster MMC mode.
> Today Thomas surprised me by posting an update of it!
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg720089.html
>
> I'm still posting this as RFC to discuss, but I'm reconsiderating
> keeping this mode a bit more.

I think it's worth keeping.

I'm pretty sure the HiFive Unleashed (sifive_u in QEMU) uses SD over
SPI. There isn't an upstream model but I think there are out of tree
patches that hopefully one day will make it upstream.

Alistair

>
> Philippe Mathieu-Daudé (2):
>   hw/sd/ssi-sd: Deprecate the SPI to SD card 'adapter'
>   hw/sd/sdcard: Deprecate the SPI mode
>
>  docs/system/deprecated.rst | 10 ++
>  1 file changed, 10 insertions(+)
>
> --
> 2.21.3
>
>

Re: [PATCH 00/24] qemu: Incremental backup and TLS handling fixes

[Andrea Bolognani]

On Tue, 2020-07-07 at 09:47 +0200, Peter Krempa wrote:
> On Thu, Jul 02, 2020 at 18:13:49 +0200, Andrea Bolognani wrote:
> > Can you please include updates to the release notes in your series?
> > Based on the summary above, it sounds like most of the changes will
> > be user-visible. Thanks!
> 
> Incremental backup is still not enabled so I usually don't include
> anything related to it in the news as users can't really use it without
> hacking around.

Fair enough.

> I'll definitely mention the encrypted keys support for NBD.
> 
> Also please note that I update news with my changes regularly when I
> think it's worth mentioning so I'd prefer to be unsubscribed from these
> notifications.

I can do that, but any reason not to update them at the same time as
you're introducing the corresponding code changes? If you're going to
do the work anyway, to me it makes sense to do all it one chunk and
not have to go back to it later. Makes it harder to accidentally lose
track and forget to do that, too.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier

[Jason Wang]

On 2020/7/3 下午9:03, Peter Xu wrote:

On Fri, Jul 03, 2020 at 03:24:19PM +0800, Jason Wang wrote:

On 2020/7/2 下午11:45, Peter Xu wrote:

On Thu, Jul 02, 2020 at 11:01:54AM +0800, Jason Wang wrote:

So I think we agree that a new notifier is needed?

Good to me, or a new flag should be easier (IOMMU_NOTIFIER_DEV_IOTLB)?


That should work but I wonder something as following is better.

Instead of introducing new flags, how about carry the type of event in the
notifier then the device (vhost) can choose the message it want to process
like:

static vhost_iommu_event(IOMMUNotifier *n, IOMMUTLBEvent *event)

{

switch (event->type) {

case IOMMU_MAP:
case IOMMU_UNMAP:
case IOMMU_DEV_IOTLB_UNMAP:
...

}

Looks ok to me, though imo we should still keep the registration information,
so VT-d knows which notifiers is interested in which events.  E.g., we can
still do something like vtd_as_has_map_notifier().



Is this for a better performance?

I wonder whether it's worth since we can't not have both vhost and vtd 
to be registered into the same as.


So it should be functional equivalent to vtd_as_has_notifier().

Thanks




So these are probably two different things: the new IOMMU_NOTIFIER_DEV_IOTLB
flag is one as discussed; the other one is whether we would like to introduce
IOMMUTLBEvent to include the type, so that we can avoid introduce two notifiers
for one device majorly to identify dev-iotlb from unmaps.

Thanks,

Re: [PATCH 00/24] qemu: Incremental backup and TLS handling fixes

[Peter Krempa]

On Thu, Jul 02, 2020 at 18:13:49 +0200, Andrea Bolognani wrote:
> On Thu, 2020-07-02 at 16:39 +0200, Peter Krempa wrote:
> > This series consists of multiple parts fixing the following bugs. Some
> > of them depend on previous so I'm sending it as one to prevent
> > conflicts.
> > 
> > - Patches 1 - 11:
> > 
> >   https://bugzilla.redhat.com/show_bug.cgi?id=1602328
> >   [RFE] Add support for encrypted TLS client keys for disks
> > 
> > - Patch 12:
> > 
> >   https://bugzilla.redhat.com/show_bug.cgi?id=1840053
> >   [incremental_backup] cannot do FULL backup for a READONLY disk
> > 
> > - Patches 13 - 14:
> > 
> >   https://bugzilla.redhat.com/show_bug.cgi?id=1829829
> >[incremental backup] Creating incremental backup that includes a new VM 
> > disk that requires full backup is impossible
> > 
> > - Patch 15:
> > 
> >   https://bugzilla.redhat.com/show_bug.cgi?id=1799010
> >   incremental-backup: RFE: Handle backup bitmaps during virDomainBlockPull
> > 
> > - Patches 16 - 24:
> > 
> >   https://bugzilla.redhat.com/show_bug.cgi?id=1822631
> >   [incremental backup] RFE: Support TLS for NBD connections for pull mode 
> > backup
> 
> Can you please include updates to the release notes in your series?
> Based on the summary above, it sounds like most of the changes will
> be user-visible. Thanks!

Incremental backup is still not enabled so I usually don't include
anything related to it in the news as users can't really use it without
hacking around.

I'll definitely mention the encrypted keys support for NBD.

Also please note that I update news with my changes regularly when I
think it's worth mentioning so I'd prefer to be unsubscribed from these
notifications.

Re: [PATCH 3/3] qemublocktest: add test of transient option for qcow2 disk

[Peter Krempa]

On Mon, Jul 06, 2020 at 14:20:25 -0400, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma 
> 
> Add a unit test for transient option for qcow2 file.
> 
> Signed-off-by: Masayoshi Mizuma 
> ---
>  tests/qemublocktest.c   | 10 ++
>  .../xml2json/qcow2-transient-srconly.json   |  9 +
>  .../qemublocktestdata/xml2json/qcow2-transient.json | 13 +
>  .../qemublocktestdata/xml2json/qcow2-transient.xml  | 13 +
>  4 files changed, 45 insertions(+)
>  create mode 100644 
> tests/qemublocktestdata/xml2json/qcow2-transient-srconly.json
>  create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient.json
>  create mode 100644 tests/qemublocktestdata/xml2json/qcow2-transient.xml
> 
> diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
> index 0cdedb9..1294c18 100644
> --- a/tests/qemublocktest.c
> +++ b/tests/qemublocktest.c
> @@ -266,6 +266,7 @@ testQemuDiskXMLToProps(const void *opaque)
>  g_autoptr(virJSONValue) formatProps = NULL;
>  g_autoptr(virJSONValue) storageProps = NULL;
>  g_autoptr(virJSONValue) storageSrcOnlyProps = NULL;
> +qemuDomainObjPrivate priv;
>  g_autofree char *xmlpath = NULL;
>  g_autofree char *xmlstr = NULL;
>  
> @@ -288,6 +289,13 @@ testQemuDiskXMLToProps(const void *opaque)
>  return -1;
>  }
>  
> +if (disk->transient) {
> +priv.driver = data->driver;
> +if (qemuBlockCreateTransientDisk(disk->src, ) < 0)

NACK, this would create files on the system running the test suite in
random paths according to the disk config. The test-suite must never do
that.

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

[Peter Krempa]

On Mon, Jul 06, 2020 at 14:20:23 -0400, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma 
> 
> Here is the implementation of transient option for qcow2 file.
> This gets available  directive in domain xml file
> like as:
> 
> 
>   
>   
>   
>   
> 
> 
> The internal procedure is as follows.
> When the qemu command line options are built, a new qcow2 image is created
> with backing qcow2 image by using qemu-img command. The backing image is the
> qcow2 file which is set as .
> The filename of the new qcow2 image is original-source-file.TRANSIENT.
> The qemu-img will be:
> 
> qemu-img create -f qcow2 -F qcow2 \
> -b /var/lib/libvirt/images/guest.qcow2 \
> /var/lib/libvirt/images/guest.qcow2.TRANSIENT
> 
> Then, it switches the disk path, virStorageSourcePtr src->path, to
> the new qcow2 image. The new image and the backing image is handled and
> the blockdev option of qemu will be built like as:
> 
> -blockdev 
> '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2",
> 
> "node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
> -blockdev 
> '{"node-name":"libvirt-2-format","read-only":true,"driver":"qcow2",
> "file":"libvirt-2-storage","backing":null}' \
> -blockdev 
> '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2.TRANSIENT",
> 
> "node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
> -blockdev 
> '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2",
> "file":"libvirt-1-storage","backing":"libvirt-2-format"}'
> 
> The new qcow2 image is removed when the Guest is shutdowned, 
> 
> Signed-off-by: Masayoshi Mizuma 
> ---
>  src/qemu/qemu_block.c| 71 
>  src/qemu/qemu_block.h|  7 
>  src/qemu/qemu_domain.c   |  4 +++
>  src/qemu/qemu_process.c  |  3 ++
>  src/qemu/qemu_validate.c |  2 +-
>  5 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 6f9c707..5eb0225 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -27,6 +27,7 @@
>  #include "viralloc.h"
>  #include "virstring.h"
>  #include "virlog.h"
> +#include "virqemu.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -3438,3 +3439,73 @@ qemuBlockUpdateRelativeBacking(virDomainObjPtr vm,
>  
>  return 0;
>  }
> +
> +int
> +qemuBlockCreateTransientDisk(virStorageSourcePtr src,
> + qemuDomainObjPrivatePtr priv)
> +{
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +virCommandPtr cmd = NULL;
> +g_autofree char *filename = NULL;
> +g_autofree char *dirname = NULL;
> +const char *qemuImgPath;
> +char *newpath;
> +int err = -1;
> +
> +if ((src->format != VIR_STORAGE_FILE_QCOW2)) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   "%s", _("transient option supports qcow2"));
> +return -1;
> +}
> +
> +if (!(qemuImgPath = qemuFindQemuImgBinary(priv->driver)))
> +return -1;
> +
> +newpath = g_strdup_printf("%s.TRANSIENT", src->path);

This assumes that 'src' is a local disk, but the code doesn't check it.
If e.g. an NBD disk which can have 'path' NULL is used it would crash.

Specifically since we can't even create a new NBD disk this won't even
wrok.

> +
> +if (virFileExists(newpath)) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _(" '%s' is already exists. Please remove it."), 
> newpath);
> +goto cleanup;
> +}
> +
> +if (!(cmd = virCommandNewArgList(qemuImgPath,
> + "create",
> + "-f",
> + "qcow2",
> + "-F",
> + "qcow2",
> + "-b",
> + NULL)))
> +goto cleanup;

I'd strongly suggest you implement this after qemu starts using the new
blockdev-create blockjob before starting the cpus and install the
overlays using the snapshot command.

The above will not work or will require many adjustments e.g. for
luks-encrypted qcow2 files. which would require substantial changes.


> +virQEMUBuildBufferEscapeComma(, src->path);
> +virCommandAddArgBuffer(cmd, );
> +
> +virQEMUBuildBufferEscapeComma(, newpath);
> +virCommandAddArgBuffer(cmd, );
> +
> +if (virCommandRun(cmd, NULL) < 0)
> +goto cleanup;
> +
> +VIR_DEBUG("Original disk: %s Transient disk: %s", src->path, newpath);
> +
> +g_free(src->path);
> +src->path = newpath;

This must add a new virStorageSource as 'src' and move the original to
src->backingStore in the live configuration object. we should not try to
detect the files which we know are there.


> +
> +err = 0;
> + cleanup:
> +

Re: [RFC PATCH 2/2] hw/sd/sdcard: Deprecate the SPI mode

[Peter Krempa]

On Mon, Jul 06, 2020 at 00:07:31 +0200, Philippe Mathieu-Daudé wrote:
> SD cards be used with SPI, SD or MMC protocol.
> 
> Unfortunately, maintaining the SPI protocol make improving the
> MMC mode very difficult. As of 2020 users are more interested
> in using cards with the MMC protocol.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  docs/system/deprecated.rst | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 5e67d7f3e0..01dca3d038 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -384,6 +384,11 @@ full SCSI support.  Use virtio-scsi instead when SCSI 
> passthrough is required.
>  Note this also applies to ``-device virtio-blk-pci,scsi=on|off``, which is an
>  alias.
>  
> +``-device sd-card,spi=on`` (since 5.1)
> +^^
> +
> +The SPI mode of the 'sd-card' device is deprecated.

libvirt didn't implement this knob so it's okay to remove it without
replacement.

ACKed-by: Peter Krempa 

Re: [RFC PATCH 1/2] hw/sd/ssi-sd: Deprecate the SPI to SD card 'adapter'

[Peter Krempa]

On Mon, Jul 06, 2020 at 00:07:30 +0200, Philippe Mathieu-Daudé wrote:
> This device duplicate the SPI mode of the sd-card device. The
> SPI protocol is better handler in the sd-card, however as the
> TYPE_SSI_SLAVE is not an interface, the sd-card can not implement
> it easily to be pluggable on a SPI bus. Meanwhile the ssi-sd
> device acts as a bridge, but is bitroting. Deprecate it.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  docs/system/deprecated.rst | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 47f84be8e0..5e67d7f3e0 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -343,6 +343,11 @@ The 'ide-drive' device is deprecated. Users should use 
> 'ide-hd' or
>  The 'scsi-disk' device is deprecated. Users should use 'scsi-hd' or
>  'scsi-cd' as appropriate to get a SCSI hard disk or CD-ROM as needed.
>  
> +``ssi-sd`` (since 5.1)
> +'
> +
> +The 'ssi-sd' (SSI to SD card adapter) device is deprecated.

libvirt didn't allow configuring this device yet, so from our view it's
okay to remove it.

ACKed-by: Peter Krempa 

RE: [question]qemu: any plan to support ivshmem-plain migration?

[Wangxin (Alexander, Cloud Infrastructure Service Product Dept.)]

> -Original Message-
> From: Daniel P. Berrangé [mailto:berra...@redhat.com]
> Sent: Monday, July 6, 2020 9:48 PM
> To: Wangxin (Alexander, Cloud Infrastructure Service Product Dept.)
> 
> Cc: libvir-list@redhat.com; mklet...@redhat.com; Huangweidong (C)
> 
> Subject: Re: [question]qemu: any plan to support ivshmem-plain migration?
> 
> On Mon, Jul 06, 2020 at 01:14:04PM +, Wangxin (Alexander, Cloud
> Infrastructure Service Product Dept.) wrote:
> > Hi, Martin
> >
> > Ivshmem-plain device support property role with 'master'(master=on) or
> > 'peer'(master=off, default mode), which controls to copy the shared memory
> on migration to the destination host or not.
> > https://git.qemu.org/?p=qemu.git;a=blob;f=docs/system/ivshmem.rst;h=b0
> > 3a48afa3ae7258bbe43490180fc5732b1f6c8c;hb=HEAD
> >
> > And libvirt does not support to configure 'role' yet, see msg
> > https://www.redhat.com/archives/libvir-list/2016-August/msg00591.html
> >
> > Qemu use 'master=off' as ivshmem-plain default property, the guest create
> by libvirt api can't be migrated now.
> >
> > Do you have an plan to support the property 'role' and ivshmem-palin
> migration?
> 
> I would assume no one is actively working on it since it is 4 years since that
> quoted message. If you wish to try to implement it, we'll review any patches.

Sure, I would like to do it.

> 
> Regards,
> Daniel
> --
> |: https://berrange.com  -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-
> https://www.instagram.com/dberrange :|