Re: [PATCH 16/25] squash into 'network: convert local pointers to g_auto*'
OOPS!!
I meant to squash this into patch 10 before posting. If you want to just
review it separately I can squash it in before push. Or if you want to
be pedantic I can squash it in and resend :-)
On 6/24/20 11:34 PM, Laine Stump wrote:
Signed-off-by: Laine Stump
---
src/network/bridge_driver_linux.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/network/bridge_driver_linux.c
b/src/network/bridge_driver_linux.c
index 0d0ac730f2..7f765bcf99 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -834,7 +834,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
{
size_t i;
virNetworkIPDefPtr ipdef;
-g_autoptr(virFirewall) fw = NULL;
+g_autoptr(virFirewall) fw = virFirewallNew();
if (virOnce(, networkSetupPrivateChains) < 0)
return -1;
@@ -920,8 +920,6 @@ int networkAddFirewallRules(virNetworkDefPtr def)
}
}
-fw = virFirewallNew();
-
virFirewallStartTransaction(fw, 0);
networkAddGeneralFirewallRules(fw, def);
@@ -946,10 +944,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
networkAddChecksumFirewallRules(fw, def);
-if (virFirewallApply(fw) < 0)
-return -1;
-
-return 0;
+return virFirewallApply(fw);
}
/* Remove all rules for all ip addresses (and general rules) on a network */
[PATCH 14/25] network: replace VIR_ALLOC/REALLOC with g_new0/g_renew
most of these are long-lived or attached to some other object, but a
couple are automatics, and can take advantage of g_autoptr.
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 44 +++--
1 file changed, 13 insertions(+), 31 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 275502b778..1dee2fac6e 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -160,6 +160,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata)
g_free(def);
}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(networkDnsmasqXmlNsDefPtr,
networkDnsmasqDefNamespaceFree);
static int
@@ -177,8 +178,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,23 +196,15 @@ static int
networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
void **data)
{
-networkDnsmasqXmlNsDefPtr nsdata = NULL;
-int ret = -1;
-
-if (VIR_ALLOC(nsdata) < 0)
-return -1;
+networkDnsmasqXmlNsDefPtr nsdata = g_new0(networkDnsmasqXmlNsDef, 1);
if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
-goto cleanup;
+return -1;
if (nsdata->noptions > 0)
*data = g_steal_pointer();
-ret = 0;
-
- cleanup:
-networkDnsmasqDefNamespaceFree(nsdata);
-return ret;
+return 0;
}
@@ -711,8 +703,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) {
@@ -2658,8 +2649,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];
@@ -4129,7 +4119,6 @@ networkGetDHCPLeases(virNetworkPtr net,
virJSONValuePtr lease_tmp = NULL;
g_autoptr(virJSONValue) leases_array = NULL;
virNetworkIPDefPtr ipdef_tmp = NULL;
-virNetworkDHCPLeasePtr lease = NULL;
virNetworkDHCPLeasePtr *leases_ret = NULL;
virNetworkObjPtr obj;
virNetworkDefPtr def;
@@ -4218,8 +4207,7 @@ networkGetDHCPLeases(virNetworkPtr net,
continue;
if (need_results) {
-if (VIR_ALLOC(lease) < 0)
-goto error;
+g_autoptr(virNetworkDHCPLease) lease = g_new0(virNetworkDHCPLease,
1);
lease->expirytime = expirytime_tmp;
@@ -4267,22 +4255,17 @@ networkGetDHCPLeases(virNetworkPtr net,
} else {
nleases++;
}
-
-g_free(lease);
-lease = NULL;
}
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;
cleanup:
-g_free(lease);
virNetworkObjEndAPI();
return rv;
@@ -5504,10 +5487,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
[PATCH 18/25] nwfilter: define a typedef for struct ebtablesSubChainInst
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 8b77578117..cc814235aa 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3274,7 +3274,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;
@@ -3285,8 +3287,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;
@@ -3296,7 +3298,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;
@@ -3309,7 +3311,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);
@@ -3355,7 +3357,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
[PATCH 24/25] nwfilter: use standard label names when reasonable
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 | 22 +++---
4 files changed, 51 insertions(+), 51 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c
b/src/nwfilter/nwfilter_dhcpsnoop.c
index e41062feca..efb3257e92 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -454,11 +454,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 */
@@ -469,7 +469,7 @@
virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
req->ifindex);
}
- exit_snooprequnlock:
+ cleanup:
virNWFilterSnoopReqUnlock(req);
return rc;
}
@@ -718,7 +718,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req,
virNWFilterSnoopReqUnlock(req);
-goto exit;
+goto cleanup;
}
virNWFilterSnoopReqUnlock(req);
@@ -742,7 +742,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req,
g_atomic_int_add(, 1);
- exit:
+ cleanup:
if (update_leasefile)
virNWFilterSnoopLeaseFileSave(pl);
@@ -885,7 +885,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));
@@ -893,7 +893,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];
@@ -905,12 +905,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;
}
@@ -1362,7 +1362,7 @@ virNWFilterDHCPSnoopThread(void *req0)
virNWFilterSnoopReqUnlock(req);
if (req->threadStatus != THREAD_STATUS_OK)
-goto exit;
+goto cleanup;
while (!error) {
if (virNWFilterSnoopAdjustPoll(pcapConf,
@@ -1390,7 +1390,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)
@@ -1507,7 +1507,7 @@ virNWFilterDHCPSnoopThread(void *req0)
virNWFilterSnoopReqUnlock(req);
virNWFilterSnoopUnlock();
- exit:
+ cleanup:
virThreadPoolFree(worker);
virNWFilterSnoopReqPut(req);
@@ -1736,14 +1736,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();
}
@@ -1838,7 +1838,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) {
@@ -1848,7 +1848,7 @@ virNWFilterSnoopLeaseFileRefresh(void)
}
g_atomic_int_set(, 0);
- skip_rename:
+ cleanup:
virNWFilterSnoopLeaseFileOpen();
}
@@ -2013,14 +2013,14 @@ virNWFilterDHCPSnoopInit(void)
if (!virNWFilterSnoopState.ifnameToKey ||
!virNWFilterSnoopState.snoopReqs ||
[PATCH 08/25] network: fix memory leak in networkBuildDhcpDaemonCommandLine()
hostsfilestr was not being freed. This will be turned into g_autofree in an upcoming patch converting a lot more of the same file to using g_auto*, but I wanted to make a separate patch for this first so the other patch is simpler to review. Signed-off-by: Laine Stump --- src/network/bridge_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 47d5d95678..aff1b7b1bc 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1628,6 +1628,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver, virObjectUnref(dnsmasq_caps); VIR_FREE(configfile); VIR_FREE(configstr); +VIR_FREE(hostsfilestr); VIR_FREE(leaseshelper_path); return ret; } -- 2.25.4
[PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 59 +
1 file changed, 33 insertions(+), 26 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 668aa9ca88..a1b2f5b6c7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -148,7 +148,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata)
virStringListFreeCount(def->options, def->noptions);
-VIR_FREE(def);
+g_free(def);
}
@@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
network_driver->lockFD = -1;
if (virMutexInit(_driver->lock) < 0) {
-VIR_FREE(network_driver);
+g_free(network_driver);
+network_driver = NULL;
goto error;
}
@@ -874,18 +875,19 @@ 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_free(network_driver);
+network_driver = NULL;
return 0;
}
@@ -2192,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);
@@ -2205,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) {
@@ -2713,19 +2715,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_free(netdef->forward.ifs);
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;
}
@@ -3161,7 +3163,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;
}
@@ -4256,7 +4258,8 @@ networkGetDHCPLeases(virNetworkPtr net,
nleases++;
}
-VIR_FREE(lease);
+g_free(lease);
+lease = NULL;
}
if (leases_ret) {
@@ -4269,7 +4272,7 @@ networkGetDHCPLeases(virNetworkPtr net,
rv = nleases;
cleanup:
-VIR_FREE(lease);
+g_free(lease);
virNetworkObjEndAPI();
return rv;
@@ -4278,7 +4281,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;
}
@@ -4402,7 +4405,7 @@ networkAllocatePort(virNetworkObjPtr obj,
return -1;
}
if (portprofile) {
-VIR_FREE(port->virtPortProfile);
+g_free(port->virtPortProfile);
port->virtPortProfile = portprofile;
}
@@ -5519,10 +5522,14 @@ networkPortSetParameters(virNetworkPortPtr port,
/* average or floor are mandatory, peak and burst are optional.
* 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);
-if (!bandwidth->out->average)
-VIR_FREE(bandwidth->out);
+if (!bandwidth->in->average &&
[PATCH 21/25] nwfilter: convert local pointers to use g_auto*
Signed-off-by: Laine Stump
---
src/nwfilter/nwfilter_dhcpsnoop.c | 91
src/nwfilter/nwfilter_ebiptables_driver.c | 170 +-
src/nwfilter/nwfilter_gentech_driver.c| 19 +--
src/nwfilter/nwfilter_learnipaddr.c | 9 +-
4 files changed, 108 insertions(+), 181 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c
b/src/nwfilter/nwfilter_dhcpsnoop.c
index f54e1a88e0..32cd6492ad 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,
exit_snooprequnlock:
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,
[PATCH 20/25] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
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 89c131e17f..8fdc8e8897 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3334,12 +3334,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
[PATCH 10/25] network: convert local pointers to g_auto*
This includes those that use plain VIR_FREE() as well as those that
have a cleanup function defined for use via g_auto/g_autoptr
(virCommand, virFirewall, virBuffer, virJSONValue etc).
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 477 +++---
src/network/bridge_driver_linux.c | 55 ++--
src/network/leaseshelper.c| 16 +-
src/util/virdnsmasq.h | 4 +
4 files changed, 209 insertions(+), 343 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index aff1b7b1bc..668aa9ca88 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -322,25 +322,23 @@ networkRunHook(virNetworkObjPtr obj,
int sub_op)
{
virNetworkDefPtr def;
-virBuffer buf = VIR_BUFFER_INITIALIZER;
-char *xml = NULL;
+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(, "");
@@ -353,16 +351,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:
-virBufferFreeAndReset();
-VIR_FREE(xml);
-return ret;
+return 0;
}
@@ -426,44 +420,42 @@ 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;
-
/* 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);
@@ -484,18 +476,7 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
/* remove the network definition */
virNetworkObjRemoveInactive(driver->networks, obj);
-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;
+return 0;
}
@@ -545,9 +526,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);
@@ -609,7 +590,7 @@ networkUpdateState(virNetworkObjPtr obj,
if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) {
[PATCH 12/25] network: make networkDnsmasqXmlNsDef private to bridge_driver.c
This struct isn't used anywhere else.
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 10 ++
src/network/bridge_driver.h | 9 -
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a1b2f5b6c7..275502b778 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -139,6 +139,16 @@ networkDnsmasqCapsRefresh(virNetworkDriverStatePtr driver)
}
+extern virXMLNamespace networkDnsmasqXMLNamespace;
+
+typedef struct _networkDnsmasqXmlNsDef networkDnsmasqXmlNsDef;
+typedef networkDnsmasqXmlNsDef *networkDnsmasqXmlNsDefPtr;
+struct _networkDnsmasqXmlNsDef {
+size_t noptions;
+char **options;
+};
+
+
static void
networkDnsmasqDefNamespaceFree(void *nsdata)
{
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index fb0ccad4b1..2613fc629d 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -27,15 +27,6 @@
#include "virdnsmasq.h"
#include "virnetworkobj.h"
-extern virXMLNamespace networkDnsmasqXMLNamespace;
-
-typedef struct _networkDnsmasqXmlNsDef networkDnsmasqXmlNsDef;
-typedef networkDnsmasqXmlNsDef *networkDnsmasqXmlNsDefPtr;
-struct _networkDnsmasqXmlNsDef {
-size_t noptions;
-char **options;
-};
-
virNetworkXMLOptionPtr
networkDnsmasqCreateXMLConf(void);
--
2.25.4
[PATCH 16/25] squash into 'network: convert local pointers to g_auto*'
Signed-off-by: Laine Stump
---
src/network/bridge_driver_linux.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/network/bridge_driver_linux.c
b/src/network/bridge_driver_linux.c
index 0d0ac730f2..7f765bcf99 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -834,7 +834,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
{
size_t i;
virNetworkIPDefPtr ipdef;
-g_autoptr(virFirewall) fw = NULL;
+g_autoptr(virFirewall) fw = virFirewallNew();
if (virOnce(, networkSetupPrivateChains) < 0)
return -1;
@@ -920,8 +920,6 @@ int networkAddFirewallRules(virNetworkDefPtr def)
}
}
-fw = virFirewallNew();
-
virFirewallStartTransaction(fw, 0);
networkAddGeneralFirewallRules(fw, def);
@@ -946,10 +944,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
networkAddChecksumFirewallRules(fw, def);
-if (virFirewallApply(fw) < 0)
-return -1;
-
-return 0;
+return virFirewallApply(fw);
}
/* Remove all rules for all ip addresses (and general rules) on a network */
--
2.25.4
[PATCH 04/25] util: validate return from xmlNodeGetContent before use
There were a few uses of xmlNodeGetContent() that didn't check for
NULL before using the result.
A NULL return from xmlNodeGetContent() *could* (probably does) mean
that there was an Out of Memory condition, but it is unclear from the
documentation if that is always the case, or if it could just indicate
a missing value in the document, so we don't report an OOM error, but
just don't try to use it for, e.g., conversion to an integer.
Signed-off-by: Laine Stump
---
src/conf/domain_conf.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8cde1cd0e8..4d27c9caa8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10556,22 +10556,22 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
virXMLNodeNameEqual(cur, "wwn")) {
wwn = (char *)xmlNodeGetContent(cur);
-if (!virValidateWWN(wwn))
+if (wwn && !virValidateWWN(wwn))
goto error;
} else if (!vendor &&
virXMLNodeNameEqual(cur, "vendor")) {
-vendor = (char *)xmlNodeGetContent(cur);
-
-if (strlen(vendor) > VENDOR_LEN) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk vendor is more than 8 characters"));
-goto error;
-}
+if ((vendor = (char *)xmlNodeGetContent(cur))) {
+if (strlen(vendor) > VENDOR_LEN) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("disk vendor is more than 8 characters"));
+goto error;
+}
-if (!virStringIsPrintable(vendor)) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk vendor is not printable string"));
-goto error;
+if (!virStringIsPrintable(vendor)) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("disk vendor is not printable string"));
+goto error;
+}
}
} else if (!product &&
virXMLNodeNameEqual(cur, "product")) {
@@ -20374,8 +20374,8 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
if (STREQ_NULLABLE(tmp, "slic")) {
VIR_FREE(tmp);
-tmp = virXMLNodeContentString(nodes[0]);
-def->os.slic_table = virFileSanitizePath(tmp);
+if ((tmp = virXMLNodeContentString(nodes[0])))
+def->os.slic_table = virFileSanitizePath(tmp);
} else {
virReportError(VIR_ERR_XML_ERROR,
_("Unknown acpi table type: %s"),
--
2.25.4
[PATCH 22/25] nwfilter: convert remaining VIR_FREE() to g_free()
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 32cd6492ad..e41062feca 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_free(req->binding->portdevname);
virNWFilterSnoopReqUnlock(req);
virNWFilterSnoopUnlock();
@@ -1970,7 +1970,7 @@ virNWFilterSnoopRemAllReqIter(const void *payload,
*/
virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname, NULL);
-VIR_FREE(req->binding->portdevname);
+g_free(req->binding->portdevname);
}
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_free(req->binding->portdevname);
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 b382b9405d..6e05e638aa 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3518,7 +3518,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 f586c7e938..183e2f0a91 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);
}
diff --git a/src/nwfilter/nwfilter_learnipaddr.c
[PATCH 00/25] Several g_auto* conversion and related small bugfixes
This started out with me noticing a memory leak in a patch that led to the realization that domain_conf.c hadn't been converted to use g_autofree yet, and each step of the way uncovered some other annoyance that I wanted to get rid of. Most of the changes are related to converting code to use g_auto*, g_new0, and g_free, but there is also a some of elimination of labels, fixing actual or theoretical memory leaks, modifications for style, etc. None of it should have any functional effect (except the fixing one or two memory leaks). Laine Stump (25): conf, vmx: check for OOM after calling xmlBufferCreate() use g_autoptr for all xmlBuffers conf: refactor virDomainBlkioDeviceParseXML to remove possible NULL dereference util: validate return from xmlNodeGetContent before use util: remove OOM error log from virGetHostnameImpl() conf: eliminate useless error label in virDomainFeaturesDefParse() util: eliminate error label in virDomainDefFormatInternalSetRootName() network: fix memory leak in networkBuildDhcpDaemonCommandLine() util: add g_autoptr cleanup function for virFirewall objects network: convert local pointers to g_auto* network: use g_free() in place of remaining VIR_FREE() network: make networkDnsmasqXmlNsDef private to bridge_driver.c define g_autoptr cleanup function for virNetworkDHCPLease network: replace VIR_ALLOC/REALLOC with g_new0/g_renew network: use proper arg type when calling virNetDevSetOnline() squash into 'network: convert local pointers to g_auto*' use g_autoptr() for all usages of virFirewallNew/Free nwfilter: define a typedef for struct ebtablesSubChainInst nwfilter replace VIR_ALLOC with g_new0 nwfilter: remove unnecessary code from ebtablesGetSubChainInsts() nwfilter: convert local pointers to use g_auto* nwfilter: convert remaining VIR_FREE() to g_free() nwfilter: transform logic in virNWFilterRuleInstSort to eliminate label nwfilter: use standard label names when reasonable replace g_new() with g_new0() for consistency src/conf/domain_conf.c| 254 +- src/conf/network_conf.c | 10 +- src/datatypes.h | 2 + src/network/bridge_driver.c | 585 +- src/network/bridge_driver.h | 9 - src/network/bridge_driver_linux.c | 58 +-- src/network/leaseshelper.c| 16 +- src/nwfilter/nwfilter_dhcpsnoop.c | 150 +++--- src/nwfilter/nwfilter_driver.c| 13 +- src/nwfilter/nwfilter_ebiptables_driver.c | 277 -- src/nwfilter/nwfilter_gentech_driver.c| 60 +-- src/nwfilter/nwfilter_learnipaddr.c | 45 +- src/qemu/qemu_backup.c| 2 +- src/util/virdnsmasq.h | 4 + src/util/virebtables.c| 24 +- src/util/virfirewall.h| 1 + src/util/viriptables.c| 14 +- src/util/virutil.c| 12 +- src/util/virxml.c | 12 +- src/vmx/vmx.c | 17 +- tests/qemuhotplugmock.c | 2 +- tests/virfirewalltest.c | 50 +- 22 files changed, 640 insertions(+), 977 deletions(-) -- 2.25.4
[PATCH 13/25] define g_autoptr cleanup function for virNetworkDHCPLease
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
[PATCH 02/25] use g_autoptr for all xmlBuffers
AUTOPTR_CLEANUP_FUNC is set to xmlBufferFree() in util/virxml.h (This
is actually new - added accidentally (but fortunately harmlessly!) in
commit 257aba2dafe. I had added it along with the hunks in this patch,
then decided to remove it and submit separately, but missed taking out
the hunk in virxml.h)
Signed-off-by: Laine Stump
---
src/conf/domain_conf.c | 4 +---
src/conf/network_conf.c | 4 +---
src/util/virxml.c | 12 +++-
src/vmx/vmx.c | 10 +++---
4 files changed, 8 insertions(+), 22 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9d057f8c78..1916b51d38 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29579,7 +29579,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
def->description);
if (def->metadata) {
-xmlBufferPtr xmlbuf;
+g_autoptr(xmlBuffer) xmlbuf = NULL;
int oldIndentTreeOutput = xmlIndentTreeOutput;
/* Indentation on output requires that we previously set
@@ -29596,12 +29596,10 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata,
virBufferGetIndent(buf) / 2, 1) < 0) {
-xmlBufferFree(xmlbuf);
xmlIndentTreeOutput = oldIndentTreeOutput;
goto error;
}
virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
-xmlBufferFree(xmlbuf);
xmlIndentTreeOutput = oldIndentTreeOutput;
}
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 5b578f894c..4ebad1483c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2508,7 +2508,7 @@ virNetworkDefFormatBuf(virBufferPtr buf,
virBufferAsprintf(buf, "%s\n", uuidstr);
if (def->metadata) {
-xmlBufferPtr xmlbuf;
+g_autoptr(xmlBuffer) xmlbuf = NULL;
int oldIndentTreeOutput = xmlIndentTreeOutput;
/* Indentation on output requires that we previously set
@@ -2525,12 +2525,10 @@ virNetworkDefFormatBuf(virBufferPtr buf,
if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata,
virBufferGetIndent(buf) / 2, 1) < 0) {
-xmlBufferFree(xmlbuf);
xmlIndentTreeOutput = oldIndentTreeOutput;
return -1;
}
virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
-xmlBufferFree(xmlbuf);
xmlIndentTreeOutput = oldIndentTreeOutput;
}
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 02b59ea2f8..848d549a8b 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -953,8 +953,7 @@ char *
virXMLNodeToString(xmlDocPtr doc,
xmlNodePtr node)
{
-xmlBufferPtr xmlbuf = NULL;
-char *ret = NULL;
+g_autoptr(xmlBuffer) xmlbuf = NULL;
if (!(xmlbuf = xmlBufferCreate())) {
virReportOOMError();
@@ -964,15 +963,10 @@ virXMLNodeToString(xmlDocPtr doc,
if (xmlNodeDump(xmlbuf, doc, node, 0, 1) == 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to convert the XML node tree"));
-goto cleanup;
+return NULL;
}
-ret = g_strdup((const char *)xmlBufferContent(xmlbuf));
-
- cleanup:
-xmlBufferFree(xmlbuf);
-
-return ret;
+return g_strdup((const char *)xmlBufferContent(xmlbuf));
}
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index fa9766995c..67bbe27fde 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -697,8 +697,8 @@ virVMXConvertToUTF8(const char *encoding, const char
*string)
{
char *result = NULL;
xmlCharEncodingHandlerPtr handler;
-xmlBufferPtr input;
-xmlBufferPtr utf8;
+g_autoptr(xmlBuffer) input = NULL;
+g_autoptr(xmlBuffer) utf8 = NULL;
handler = xmlFindCharEncodingHandler(encoding);
@@ -720,14 +720,10 @@ virVMXConvertToUTF8(const char *encoding, const char
*string)
goto cleanup;
}
-result = (char *)utf8->content;
-utf8->content = NULL;
+result = (char *)g_steal_pointer(>content);
cleanup:
xmlCharEncCloseFunc(handler);
-xmlBufferFree(input);
-xmlBufferFree(utf8);
-
return result;
}
--
2.25.4
[PATCH 09/25] util: add g_autoptr cleanup function for virFirewall objects
Put in a separate patch so that two future patches can be re-ordered / selectively backported independent of each other. Signed-off-by: Laine Stump --- src/util/virfirewall.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index 6148f46827..ff690b36f0 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -40,6 +40,7 @@ virFirewallPtr virFirewallNew(void); void virFirewallFree(virFirewallPtr firewall); + /** * virFirewallAddRule: * @firewall: firewall ruleset to add to -- 2.25.4
[PATCH 01/25] conf, vmx: check for OOM after calling xmlBufferCreate()
Although libvirt itself uses g_malloc0() and friends, which exit when
there isn't enouogh memory, libxml2 uses standard malloc(), which just
returns NULL on OOM - this means we must check for NULL on return from
any libxml2 functions that allocate memory.
xmlBufferCreate(), for example, might return NULL, and we don't always
check for it. This patch adds checks where it isn't already done.
(NB: Although libxml2 has a provision for changing behavior on OOM (by
calling xmlMemSetup() to change what functions are used to
allocating/freeing memory), we can't use that, since parts of libvirt
code end up in libvirt.so, which is linked and called directly by
applications that may themselves use libxml2 (and may have already set
their own alternate malloc()), e.g. drivers like esx which live totally
in the library rather than a separate process.)
Signed-off-by: Laine Stump
---
src/conf/domain_conf.c | 6 +-
src/conf/network_conf.c | 6 +-
src/vmx/vmx.c | 7 +--
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fc7fcfb0c6..9d057f8c78 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29589,7 +29589,11 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
* Thankfully, libxml maps what looks like globals into
* thread-local uses, so we are thread-safe. */
xmlIndentTreeOutput = 1;
-xmlbuf = xmlBufferCreate();
+if (!(xmlbuf = xmlBufferCreate())) {
+virReportOOMError();
+goto error;
+}
+
if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata,
virBufferGetIndent(buf) / 2, 1) < 0) {
xmlBufferFree(xmlbuf);
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 87d43de1e3..5b578f894c 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2518,7 +2518,11 @@ virNetworkDefFormatBuf(virBufferPtr buf,
* Thankfully, libxml maps what looks like globals into
* thread-local uses, so we are thread-safe. */
xmlIndentTreeOutput = 1;
-xmlbuf = xmlBufferCreate();
+if (!(xmlbuf = xmlBufferCreate())) {
+virReportOOMError();
+return -1;
+}
+
if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata,
virBufferGetIndent(buf) / 2, 1) < 0) {
xmlBufferFree(xmlbuf);
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index f2248cef53..fa9766995c 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -708,8 +708,11 @@ virVMXConvertToUTF8(const char *encoding, const char
*string)
return NULL;
}
-input = xmlBufferCreateStatic((char *)string, strlen(string));
-utf8 = xmlBufferCreate();
+if (!(input = xmlBufferCreateStatic((char *)string, strlen(string))) ||
+!(utf8 = xmlBufferCreate())) {
+virReportOOMError();
+goto cleanup;
+}
if (xmlCharEncInFunc(handler, utf8, input) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
2.25.4
[PATCH 06/25] conf: eliminate useless error label in virDomainFeaturesDefParse()
The error: label in this function just does "return -1", so replace
all the "goto error" in the function with "return -1".
Signed-off-by: Laine Stump
---
src/conf/domain_conf.c | 91 --
1 file changed, 44 insertions(+), 47 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4d27c9caa8..243590854f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19312,14 +19312,14 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
int n;
if ((n = virXPathNodeSet("./features/*", ctxt, )) < 0)
-goto error;
+return -1;
for (i = 0; i < n; i++) {
int val = virDomainFeatureTypeFromString((const char *)nodes[i]->name);
if (val < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unexpected feature '%s'"), nodes[i]->name);
-goto error;
+return -1;
}
switch ((virDomainFeature) val) {
@@ -19330,7 +19330,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown value for attribute eoi: '%s'"),
tmp);
-goto error;
+return -1;
}
def->apic_eoi = eoi;
VIR_FREE(tmp);
@@ -19353,7 +19353,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown policy attribute '%s' of feature
'%s'"),
tmp, virDomainFeatureTypeToString(val));
-goto error;
+return -1;
}
VIR_FREE(tmp);
} else {
@@ -19372,7 +19372,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown state attribute '%s' of feature
'%s'"),
tmp, virDomainFeatureTypeToString(val));
-goto error;
+return -1;
}
VIR_FREE(tmp);
} else {
@@ -19386,7 +19386,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
if (gic_version < 0 || gic_version == VIR_GIC_VERSION_NONE) {
virReportError(VIR_ERR_XML_ERROR,
_("malformed gic version: %s"), tmp);
-goto error;
+return -1;
}
def->gic_version = gic_version;
VIR_FREE(tmp);
@@ -19402,7 +19402,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown driver mode: %s"),
tmp);
-goto error;
+return -1;
}
def->features[val] = value;
VIR_FREE(tmp);
@@ -19417,7 +19417,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown HPT resizing setting: %s"),
tmp);
-goto error;
+return -1;
}
def->hpt_resizing = (virDomainHPTResizing) value;
VIR_FREE(tmp);
@@ -19433,7 +19433,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s",
_("Unable to parse HPT maxpagesize setting"));
-goto error;
+return -1;
}
def->hpt_maxpagesize = VIR_DIV_UP(def->hpt_maxpagesize, 1024);
@@ -19451,7 +19451,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown value: %s"),
tmp);
-goto error;
+return -1;
}
def->features[val] = value;
VIR_FREE(tmp);
@@ -19466,7 +19466,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown value: %s"),
tmp);
-goto error;
+return -1;
}
def->features[val] = value;
VIR_FREE(tmp);
@@ -19481,7 +19481,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown value: %s"),
tmp);
[PATCH 25/25] replace g_new() with g_new0() for consistency
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
[PATCH 05/25] util: remove OOM error log from virGetHostnameImpl()
The strings allocated in virGetHostnameImpl() are all allocated via
g_strdup(), which will exit on OOM anyway, so the call to
virReportOOMError() is redundant, and removing it allows slight
modification to the code, in particular the cleanup label can be
eliminated.
Signed-off-by: Laine Stump
---
src/util/virutil.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index f39122e8dd..04f882fda7 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -503,8 +503,7 @@ virGetHostnameImpl(bool quiet)
* string as-is; it's up to callers to check whether "localhost"
* is allowed.
*/
-result = g_strdup(hostname);
-goto cleanup;
+return g_strdup(hostname);
}
/* otherwise, it's a shortened, non-localhost, hostname. Attempt to
@@ -519,8 +518,7 @@ virGetHostnameImpl(bool quiet)
if (!quiet)
VIR_WARN("getaddrinfo failed for '%s': %s",
hostname, gai_strerror(r));
-result = g_strdup(hostname);
-goto cleanup;
+return g_strdup(hostname);
}
/* Tell static analyzers about getaddrinfo semantics. */
@@ -538,10 +536,6 @@ virGetHostnameImpl(bool quiet)
result = g_strdup(info->ai_canonname);
freeaddrinfo(info);
-
- cleanup:
-if (!result)
-virReportOOMError();
return result;
}
--
2.25.4
[PATCH 19/25] nwfilter replace VIR_ALLOC with g_new0
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 f530341872..f54e1a88e0 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 cc814235aa..89c131e17f 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3318,8 +3318,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 6789a4a3fa..8ba555358d 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -261,8 +261,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 4ce8d5ba03..3bb8c27167 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 err_exit;
+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
[PATCH 17/25] use g_autoptr() for all usages of virFirewallNew/Free
Signed-off-by: Laine Stump
---
src/nwfilter/nwfilter_ebiptables_driver.c | 63 +++
src/util/virebtables.c| 24 ++---
src/util/viriptables.c| 14 ++---
tests/virfirewalltest.c | 50 --
4 files changed, 35 insertions(+), 116 deletions(-)
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 6fc8044c8d..8b77578117 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -2863,7 +2863,7 @@ static int
ebtablesApplyBasicRules(const char *ifname,
const virMacAddr *macaddr)
{
-virFirewallPtr fw = virFirewallNew();
+g_autoptr(virFirewall) fw = virFirewallNew();
char chain[MAX_CHAINNAME_LENGTH];
char chainPrefix = CHAINPREFIX_HOST_IN_TEMP;
char macaddr_str[VIR_MAC_STRING_BUFLEN];
@@ -2871,7 +2871,7 @@ ebtablesApplyBasicRules(const char *ifname,
virMacAddrFormat(macaddr, macaddr_str);
if (ebiptablesAllTeardown(ifname) < 0)
-goto error;
+return -1;
virFirewallStartTransaction(fw, 0);
@@ -2900,13 +2900,10 @@ ebtablesApplyBasicRules(const char *ifname,
if (virFirewallApply(fw) < 0)
goto tear_down_tmpebchains;
-virFirewallFree(fw);
return 0;
tear_down_tmpebchains:
ebtablesCleanAll(ifname);
- error:
-virFirewallFree(fw);
return -1;
}
@@ -2939,12 +2936,12 @@ ebtablesApplyDHCPOnlyRules(const char *ifname,
char macaddr_str[VIR_MAC_STRING_BUFLEN];
unsigned int idx = 0;
unsigned int num_dhcpsrvrs;
-virFirewallPtr fw = virFirewallNew();
+g_autoptr(virFirewall) fw = virFirewallNew();
virMacAddrFormat(macaddr, macaddr_str);
if (ebiptablesAllTeardown(ifname) < 0)
-goto error;
+return -1;
virFirewallStartTransaction(fw, 0);
@@ -3019,14 +3016,10 @@ ebtablesApplyDHCPOnlyRules(const char *ifname,
if (virFirewallApply(fw) < 0)
goto tear_down_tmpebchains;
-virFirewallFree(fw);
-
return 0;
tear_down_tmpebchains:
ebtablesCleanAll(ifname);
- error:
-virFirewallFree(fw);
return -1;
}
@@ -3045,10 +3038,10 @@ ebtablesApplyDropAllRules(const char *ifname)
{
char chain_in [MAX_CHAINNAME_LENGTH],
chain_out[MAX_CHAINNAME_LENGTH];
-virFirewallPtr fw = virFirewallNew();
+g_autoptr(virFirewall) fw = virFirewallNew();
if (ebiptablesAllTeardown(ifname) < 0)
-goto error;
+return -1;
virFirewallStartTransaction(fw, 0);
@@ -3074,13 +3067,10 @@ ebtablesApplyDropAllRules(const char *ifname)
if (virFirewallApply(fw) < 0)
goto tear_down_tmpebchains;
-virFirewallFree(fw);
return 0;
tear_down_tmpebchains:
ebtablesCleanAll(ifname);
- error:
-virFirewallFree(fw);
return -1;
}
@@ -3095,8 +3085,7 @@ ebtablesRemoveBasicRules(const char *ifname)
static int
ebtablesCleanAll(const char *ifname)
{
-virFirewallPtr fw = virFirewallNew();
-int ret = -1;
+g_autoptr(virFirewall) fw = virFirewallNew();
virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
@@ -3112,9 +3101,7 @@ ebtablesCleanAll(const char *ifname)
ebtablesRemoveTmpRootChainFW(fw, true, ifname);
ebtablesRemoveTmpRootChainFW(fw, false, ifname);
-ret = virFirewallApply(fw);
-virFirewallFree(fw);
-return ret;
+return virFirewallApply(fw);
}
@@ -3362,7 +3349,7 @@ ebiptablesApplyNewRules(const char *ifname,
size_t nrules)
{
size_t i, j;
-virFirewallPtr fw = virFirewallNew();
+g_autoptr(virFirewall) fw = virFirewallNew();
virHashTablePtr chains_in_set = virHashCreate(10, NULL);
virHashTablePtr chains_out_set = virHashCreate(10, NULL);
bool haveEbtables = false;
@@ -3563,7 +3550,6 @@ ebiptablesApplyNewRules(const char *ifname,
for (i = 0; i < nsubchains; i++)
VIR_FREE(subchains[i]);
VIR_FREE(subchains);
-virFirewallFree(fw);
virHashFree(chains_in_set);
virHashFree(chains_out_set);
@@ -3591,23 +3577,19 @@ ebiptablesTearNewRulesFW(virFirewallPtr fw, const char
*ifname)
static int
ebiptablesTearNewRules(const char *ifname)
{
-virFirewallPtr fw = virFirewallNew();
-int ret = -1;
+g_autoptr(virFirewall) fw = virFirewallNew();
virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
ebiptablesTearNewRulesFW(fw, ifname);
-ret = virFirewallApply(fw);
-virFirewallFree(fw);
-return ret;
+return virFirewallApply(fw);
}
static int
ebiptablesTearOldRules(const char *ifname)
{
-virFirewallPtr fw = virFirewallNew();
-int ret = -1;
+g_autoptr(virFirewall) fw = virFirewallNew();
virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
@@ -3626,9 +3608,7 @@ ebiptablesTearOldRules(const char *ifname)
[PATCH 07/25] util: eliminate error label in virDomainDefFormatInternalSetRootName()
The only reason for the error label in this function is to call
virBufferFreeAndReset(). It's actually more common for a failed format
function to just leave the virBuffer alone and let the caller free it
when there is a failure, and in fact the only caller of this function
that *wasn't* already calling virBufferFreeAndReset() on failure was
virDomainDefFormat() (via virDomainDefFormatInternal()).
That is easily solved by modifying virDomainDefFormat() to declare its
virBuffer buf with g_auto(), so that virBufferFreeAndReset() is
automatically called.
Signed-off-by: Laine Stump
---
src/conf/domain_conf.c | 88 --
1 file changed, 42 insertions(+), 46 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 243590854f..0307ffcbd6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29534,7 +29534,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
if (!(type = virDomainVirtTypeToString(def->virtType))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected domain type %d"), def->virtType);
-goto error;
+return -1;
}
if (def->id == -1)
@@ -29579,13 +29579,13 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
xmlIndentTreeOutput = 1;
if (!(xmlbuf = xmlBufferCreate())) {
virReportOOMError();
-goto error;
+return -1;
}
if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata,
virBufferGetIndent(buf) / 2, 1) < 0) {
xmlIndentTreeOutput = oldIndentTreeOutput;
-goto error;
+return -1;
}
virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
xmlIndentTreeOutput = oldIndentTreeOutput;
@@ -29608,13 +29608,13 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
def->mem.cur_balloon);
if (virDomainDefFormatBlkiotune(buf, def) < 0)
-goto error;
+return -1;
virDomainMemtuneFormat(buf, >mem);
virDomainMemorybackingFormat(buf, >mem);
if (virDomainCpuDefFormat(buf, def) < 0)
-goto error;
+return -1;
if (def->niothreadids > 0) {
virBufferAsprintf(buf, "%zu\n",
@@ -29632,10 +29632,10 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
}
if (virDomainCputuneDefFormat(buf, def, flags) < 0)
-goto error;
+return -1;
if (virDomainNumatuneFormatXML(buf, def->numa) < 0)
-goto error;
+return -1;
if (def->resource)
virDomainResourceDefFormat(buf, def->resource);
@@ -29720,7 +29720,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected boot device type %d"),
def->os.bootDevs[n]);
-goto error;
+return -1;
}
virBufferAsprintf(buf, "\n", boottype);
}
@@ -29752,7 +29752,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
if (mode == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected smbios mode %d"),
def->os.smbios_mode);
-goto error;
+return -1;
}
virBufferAsprintf(buf, "\n", mode);
}
@@ -29783,10 +29783,10 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
}
if (virDomainDefFormatFeatures(buf, def) < 0)
-goto error;
+return -1;
if (virCPUDefFormatBufFull(buf, def->cpu, def->numa) < 0)
-goto error;
+return -1;
virBufferAsprintf(buf, "clock.offset));
@@ -29817,7 +29817,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
virBufferAdjustIndent(buf, 2);
for (n = 0; n < def->clock.ntimers; n++) {
if (virDomainTimerDefFormat(buf, def->clock.timers[n]) < 0)
-goto error;
+return -1;
}
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "\n");
@@ -29826,20 +29826,20 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr
def,
if (virDomainEventActionDefFormat(buf, def->onPoweroff,
"on_poweroff",
virDomainLifecycleActionTypeToString) <
0)
-goto error;
+return -1;
if (virDomainEventActionDefFormat(buf, def->onReboot,
"on_reboot",
virDomainLifecycleActionTypeToString) <
0)
-goto error;
+return -1;
if (virDomainEventActionDefFormat(buf, def->onCrash,
"on_crash",
virDomainLifecycleActionTypeToString) <
0)
-goto error;
+return -1;
[PATCH 03/25] conf: refactor virDomainBlkioDeviceParseXML to remove possible NULL dereference
virDomainBlkioDeviceParseXML() has multiple cases of sending the
return from xmlNodeGetContent() directly to virStrToLong_xx() without
checking for NULL. Although it is *very* rare for xmlNodeGetContent()
to return NULL (possibly it only happens in an OOM condition? The
documentation is unclear), it could happen, and the refactor in this
patch manages to eliminate several lines of repeated code while adding
in a (single) check for NULL.
Signed-off-by: Laine Stump
---
src/conf/domain_conf.c | 39 +++
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1916b51d38..8cde1cd0e8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
virBlkioDevicePtr dev)
{
xmlNodePtr node;
-g_autofree char *c = NULL;
+g_autofree char *path = NULL;
node = root->children;
while (node) {
-if (node->type == XML_ELEMENT_NODE) {
-if (virXMLNodeNameEqual(node, "path") && !dev->path) {
-dev->path = (char *)xmlNodeGetContent(node);
+g_autofree char *c = NULL;
+
+if (node->type == XML_ELEMENT_NODE &&
+(c = (char *)xmlNodeGetContent(node))) {
+if (virXMLNodeNameEqual(node, "path") && !path) {
+path = g_steal_pointer();
} else if (virXMLNodeNameEqual(node, "weight")) {
-c = (char *)xmlNodeGetContent(node);
if (virStrToLong_ui(c, NULL, 10, >weight) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("could not parse weight %s"),
c);
-goto error;
+return -1;
}
-VIR_FREE(c);
} else if (virXMLNodeNameEqual(node, "read_bytes_sec")) {
-c = (char *)xmlNodeGetContent(node);
if (virStrToLong_ull(c, NULL, 10, >rbps) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("could not parse read bytes sec %s"),
c);
-goto error;
+return -1;
}
-VIR_FREE(c);
} else if (virXMLNodeNameEqual(node, "write_bytes_sec")) {
-c = (char *)xmlNodeGetContent(node);
if (virStrToLong_ull(c, NULL, 10, >wbps) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("could not parse write bytes sec %s"),
c);
-goto error;
+return -1;
}
-VIR_FREE(c);
} else if (virXMLNodeNameEqual(node, "read_iops_sec")) {
-c = (char *)xmlNodeGetContent(node);
if (virStrToLong_ui(c, NULL, 10, >riops) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("could not parse read iops sec %s"),
c);
-goto error;
+return -1;
}
-VIR_FREE(c);
} else if (virXMLNodeNameEqual(node, "write_iops_sec")) {
-c = (char *)xmlNodeGetContent(node);
if (virStrToLong_ui(c, NULL, 10, >wiops) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("could not parse write iops sec %s"),
c);
-goto error;
+return -1;
}
-VIR_FREE(c);
}
}
node = node->next;
}
-if (!dev->path) {
+
+if (!path) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("missing per-device path"));
return -1;
}
+dev->path = g_steal_pointer();
return 0;
-
- error:
-VIR_FREE(dev->path);
-return -1;
}
--
2.25.4
[PATCH 15/25] network: use proper arg type when calling virNetDevSetOnline()
The 2nd arg to this function is a bool, not an int. Signed-off-by: Laine Stump --- src/network/bridge_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1dee2fac6e..cbf5f05f30 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2427,7 +2427,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, goto error; /* Bring up the bridge interface */ -if (virNetDevSetOnline(def->bridge, 1) < 0) +if (virNetDevSetOnline(def->bridge, true) < 0) goto error; devOnline = true; @@ -2505,7 +2505,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, } if (devOnline) -ignore_value(virNetDevSetOnline(def->bridge, 0)); +ignore_value(virNetDevSetOnline(def->bridge, false)); if (firewalRulesAdded && def->forward.type != VIR_NETWORK_FORWARD_OPEN) @@ -2558,7 +2558,7 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, ignore_value(virNetDevTapDelete(macTapIfName, NULL)); } -ignore_value(virNetDevSetOnline(def->bridge, 0)); +ignore_value(virNetDevSetOnline(def->bridge, false)); if (def->forward.type != VIR_NETWORK_FORWARD_OPEN) networkRemoveFirewallRules(def); -- 2.25.4
[PATCH 23/25] nwfilter: transform logic in virNWFilterRuleInstSort to eliminate label
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 6e05e638aa..90afbe21f1 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3101,13 +3101,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
Re: [libvirt PATCH] qemu: ramfb video device doesn't support PCI address
On 6/24/20 6:06 PM, Jonathon Jongsma wrote:
Although a ramfb video device is not a PCI device, we don't currently
report an error for ramfb device definitions containing a PCI address.
However, a guest configured with such a device will fail to start:
# virsh start test1
error: Failed to start domain test1
error: internal error: qemu unexpectedly closed the monitor:
2020-06-16T05:23:02.759221Z qemu-kvm: -device
ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus
A better approach is to reject any device definitions that contain PCI
addresses. While this is a change in behavior, any existing
configurations were non-functional.
https://bugzilla.redhat.com/show_bug.cgi?id=1847259
Signed-off-by: Jonathon Jongsma
---
src/conf/domain_conf.c | 7 +++
tests/qemuxml2argvtest.c | 1 +
2 files changed, 8 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fc7fcfb0c6..1a06cb3f4b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6608,6 +6608,13 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
return -1;
}
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
+video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'address' is not supported for 'ramfb' video
devices"));
+return -1;
+}
+
There may be disagreement about this, and in practical terms it makes no
difference (because no domain type other than QEMU is ever going to have
one of these devices anyway), but since ramfb is a qemu-specific device
(isn't it?), I think this check would be better put in
qemu_validate.c:qemuValidateDomainDeviceDevVideo(), which is the
qemu-specific validation function for video devices.
(I see there is already validation in virDomainVideoDefValidate() for a
qemu-specific (afaik) video type - i is even checking for one backend
that is named VIR_DOMAIN_VIDEO_BACKEND_TYPE_QEMU)
I don't really have a strong opinion though, since the other hypervisor
drivers don't seem all that concerned about fleshing out their
validation functions, and what you have works properly for qemu :-P
/* it doesn't make sense to pair video device type 'none' with any other
* types, there can be only a single video device in such case
*/
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1195f9c982..f2522fa530 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2276,6 +2276,7 @@ mymain(void)
QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
DO_TEST_CAPS_LATEST("video-bochs-display-device");
DO_TEST_CAPS_LATEST("video-ramfb-display-device");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-pci-address");
DO_TEST("video-none-device",
QEMU_CAPS_VNC);
DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE);
Re: [PATCH] network: Fix a race condition when shutdown & start vm at the same time
On 6/19/20 1:16 PM, Daniel P. Berrangé wrote: On Fri, Jun 19, 2020 at 01:06:33PM -0400, Laine Stump wrote: On 6/19/20 12:26 PM, Daniel P. Berrangé wrote: Maintain a global "int nextTapID" counter, and just iterate on this. NIC names can be upto 16 bytes, so we'll create billions of devices before we have any chance of wrapping around and risking a collision with a concurrently shutting down guest. I remember I tried doing a monotonically increasing "next" counter for macvtap (never actually pushed, but shown to [some users, I forget who, maybe ovirt devs?], and they didn't like it because the devices ended up with long difficult-for-a-human-to-remember/pick-out/recite names like macvtap42301 and macvtap43021. So instead we keep track of the names that are in use, and always look for the lowest available number when creating a new one. (of course doing that would greatly increase the likelyhood of exposing any race conditions, so...) Definitely if we change the behavior in this way we're going to hear about it, though :-) People might complain, but I'm not convinced it really matters. Pid numbers used to be nice & short until someone raised pid max and now my processes have 7-digit long pids. It is surprising at first, but it didn't really cause me any functional problems. This is a good point. But even more convincing than that is the revelation that this isn't an issue just with OVS switch ports - the same race is also causing problems with libvirt nwfilter driver bindings, as reported here: https://bugzilla.redhat.com/show_bug.cgi?id=1837395 After seeing that report and realizing that this same race is the cause, I've decided to change libvirt's tap device creation to provide names based on a monotonically increasing counter as you suggest, rather than relying on the kernel. I'm hoping to have something to send to the list in a day or two. And there's still the option of just providing a fixed name if you really need something predictable for a guest. Another issue I remember from macvtap is the possibility of a different process (not libvirt) having already created a macvtap device with the name that we are wanting to use. That is easily solved in the case of macvtap by just iterating through until we find one that we can create without failure. So basically you have the list/bitmap/whatever of devicenames currently in use, use that as a starting point to pick a name for the new device, but then also allow for that name to already be used, and retry. I wasn't thinking we need a bitmap, literally just a single counter. We start where we left off, trying until we succeeed, which is what the kernel does anyway IIRC. Most other competing processes will rely on the kernel auto-allocation so won't clash with us very frequently if at all. Also, in the name of backward compatibility, we will need to support tap device names from the config that have %d in the name, e.g. the default name we send is "vnet%d", but we allow for someone to specify "mytap%d". We could keep a counter per prefix, or just have a single global counter. Regards, Daniel
Re: [libvirt PATCH] qemu: ramfb video device doesn't support PCI address
On 6/24/20 7:06 PM, Jonathon Jongsma wrote: Although a ramfb video device is not a PCI device, we don't currently report an error for ramfb device definitions containing a PCI address. However, a guest configured with such a device will fail to start: # virsh start test1 error: Failed to start domain test1 error: internal error: qemu unexpectedly closed the monitor: 2020-06-16T05:23:02.759221Z qemu-kvm: -device ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus A better approach is to reject any device definitions that contain PCI addresses. While this is a change in behavior, any existing configurations were non-functional. https://bugzilla.redhat.com/show_bug.cgi?id=1847259 Signed-off-by: Jonathon Jongsma --- Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 13/13] news: Document HMAT addition
On 6/24/20 10:49 AM, Michal Privoznik wrote: Signed-off-by: Michal Privoznik --- NEWS.rst | 5 + 1 file changed, 5 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 2fc43c31b9..8990cdaf61 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -47,6 +47,11 @@ v6.5.0 (unreleased) alphabetical order. Hook script in old place will be executed as first for backward compatibility. + * Allow configuring of NUMA HMAT + +Libvirt allows configuring Heterogeneous Memory Attribute Table to hint +software running inside the guest on optimization. + * **Improvements** Please add an ACPI mention here like: "Libvirt allows configuring ACPI Heterogeneous " to avoid PPC64 devs like myself having to deal with "HMAT does not work in Power9" bugs. In fact, please mention that this is an ACPI exclusive feature in formatdomain.html.in (patch 08/13) as well :) Reviewed-by: Daniel Henrique Barboza * **Bug fixes**
Re: [PATCH 12/13] qemu: Build HMAT command line
On 6/24/20 10:49 AM, Michal Privoznik wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1786303
Signed-off-by: Michal Privoznik
---
src/conf/numa_conf.c | 7 +
src/qemu/qemu_command.c | 183 ++
.../numatune-hmat.x86_64-latest.args | 53 +
tests/qemuxml2argvtest.c | 1 +
4 files changed, 244 insertions(+)
create mode 100644 tests/qemuxml2argvdata/numatune-hmat.x86_64-latest.args
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index b35e5040fc..7fbbe0f52e 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1904,6 +1904,13 @@ virDomainNumaGetNodeInitiator(const virDomainNuma *numa,
if (!numa || node >= numa->nmem_nodes)
return -1;
+/* A NUMA node which has at least one vCPU is initiator to itself by
+ * definition. */
+if (numa->mem_nodes[node].cpumask)
+return node;
+
+/* For the rest, "NUMA node that has best performance (the lowest
+ * latency or largest bandwidth) to this NUMA node." */
for (i = 0; i < numa->nlatencies; i++) {
const virDomainNumaLatency *l = >latencies[i];
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 628ff970bd..b28c43f110 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6932,6 +6932,16 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
virBufferAsprintf(, ",pflash1=%s", priv->pflash1->nodeformat);
}
+if (virDomainNumaHasHMAT(def->numa)) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_HMAT)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("HMAT is not supported with this QEMU"));
+return -1;
+}
This qemuCaps validation must be moved to qemu_validation.c,
qemuValidateDomainDefNuma(), to allow post-parse checking instead of runtime.
This is the original intent of qemu_validate.c. Yeah, I know that there are
still
some validations being done here in qemu_command.c. I'll come back to this
work of moving this stuff to qemu_validate.c soon (TM).
As a bonus:
+
+virBufferAddLit(, ",hmat=on");
+}
+
virCommandAddArgBuffer(cmd, );
return 0;
@@ -7114,6 +7124,134 @@ qemuBuildIOThreadCommandLine(virCommandPtr cmd,
}
[...]
+}
+
+
static int
qemuBuildNumaCommandLine(virQEMUDriverConfigPtr cfg,
virDomainDefPtr def,
@@ -7126,9 +7264,11 @@ qemuBuildNumaCommandLine(virQEMUDriverConfigPtr cfg,
char *next = NULL;
virBufferPtr nodeBackends = NULL;
bool needBackend = false;
+bool hmat = false;
int rc;
int ret = -1;
size_t ncells = virDomainNumaGetNodeCount(def->numa);
+ssize_t masterInitiator = -1;
if (!virDomainNumatuneNodesetIsAvailable(def->numa, priv->autoNodeset))
goto cleanup;
@@ -7138,6 +7278,16 @@ qemuBuildNumaCommandLine(virQEMUDriverConfigPtr cfg,
def->os.machine))
needBackend = true;
+if (virDomainNumaHasHMAT(def->numa)) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_HMAT)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("HMAT is not supported with this QEMU"));
+goto cleanup;
+}
+needBackend = true;
+hmat = true;
+}
+
You can get rid of this second duplicated validation block since this would be
already be checked once in qemuValidateDomainDefNuma().
With the validation code moved:
Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 11/13] qemu: Introduce QEMU_CAPS_NUMA_HMAT capability
On 6/24/20 10:49 AM, Michal Privoznik wrote: This capability tracks whether QEMU is capable of defining HMAT ACPI table for the guest. Signed-off-by: Michal Privoznik --- Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 10/13] numa: expose HMAT APIs
On 6/24/20 10:49 AM, Michal Privoznik wrote: These APIs will be used by QEMU driver when building the command line. Signed-off-by: Michal Privoznik --- Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 09/13] conf: Validate NUMA HMAT configuration
On 6/24/20 10:49 AM, Michal Privoznik wrote: There are several restrictions, for instance @initiator and @target have to refer to existing NUMA nodes (daa), @cache has to refer to a defined cache level and so on. Signed-off-by: Michal Privoznik --- Reviewed-by: Daniel Henrique Barboza
[libvirt PATCH] qemu: ramfb video device doesn't support PCI address
Although a ramfb video device is not a PCI device, we don't currently
report an error for ramfb device definitions containing a PCI address.
However, a guest configured with such a device will fail to start:
# virsh start test1
error: Failed to start domain test1
error: internal error: qemu unexpectedly closed the monitor:
2020-06-16T05:23:02.759221Z qemu-kvm: -device
ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus
A better approach is to reject any device definitions that contain PCI
addresses. While this is a change in behavior, any existing
configurations were non-functional.
https://bugzilla.redhat.com/show_bug.cgi?id=1847259
Signed-off-by: Jonathon Jongsma
---
src/conf/domain_conf.c | 7 +++
tests/qemuxml2argvtest.c | 1 +
2 files changed, 8 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fc7fcfb0c6..1a06cb3f4b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6608,6 +6608,13 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
return -1;
}
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
+video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'address' is not supported for 'ramfb' video
devices"));
+return -1;
+}
+
/* it doesn't make sense to pair video device type 'none' with any other
* types, there can be only a single video device in such case
*/
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1195f9c982..f2522fa530 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2276,6 +2276,7 @@ mymain(void)
QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
DO_TEST_CAPS_LATEST("video-bochs-display-device");
DO_TEST_CAPS_LATEST("video-ramfb-display-device");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-pci-address");
DO_TEST("video-none-device",
QEMU_CAPS_VNC);
DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE);
--
2.21.3
Re: [PATCH 08/13] conf: Parse and format HMAT
On 6/24/20 10:49 AM, Michal Privoznik wrote: To cite ACPI specification: Heterogeneous Memory Attribute Table describes the memory attributes, such as memory side cache attributes and bandwidth and latency details, related to the System Physical Address (SPA) Memory Ranges. The software is expected to use this information as hint for optimization. According to our upstream discussion [1] this is exposed under as under NUMA and or under numa/latencies. 1: https://www.redhat.com/archives/libvir-list/2020-January/msg00422.html Signed-off-by: Michal Privoznik --- docs/formatdomain.html.in | 88 ++ docs/schemas/cputypes.rng | 110 ++- src/conf/numa_conf.c | 350 - src/conf/numa_conf.h | 33 ++ src/libvirt_private.syms | 6 + tests/qemuxml2argvdata/numatune-hmat.xml | 52 +++ tests/qemuxml2xmloutdata/numatune-hmat.xml | 1 + tests/qemuxml2xmltest.c| 1 + 8 files changed, 625 insertions(+), 16 deletions(-) create mode 100644 tests/qemuxml2argvdata/numatune-hmat.xml create mode 12 tests/qemuxml2xmloutdata/numatune-hmat.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 07dcca57f5..78b2d2828d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1931,6 +1931,94 @@ using 10 for local and 20 for remote distances. +Heterogeneous Memory Attribute Table + + +... +cpu + ... + numa +cell id='0' cpus='0-3' memory='512000' unit='KiB' discard='yes'/ +cell id='1' cpus='4-7' memory='512000' unit='KiB' memAccess='shared'/ +cell id='3' cpus='0-3' memory='2097152' unit='KiB' + cache level='1' associativity='direct' policy='writeback' +size value='10' unit='KiB'/ +line value='8' unit='B'/ + /cache +/cell +latencies + latency initiator='0' target='0' type='access' value='5'/ + latency initiator='0' target='0' cache='1' type='access' value='10'/ + bandwidth initiator='0' target='0' type='access' value='204800' unit='KiB'/ +/latencies + /numa + ... +/cpu +... + + + Since 6.5.0 the cell element can + have cache child element which describes memory side cache "have a cache child " + for memory proximity domains. The cache element has "cache element has a" + level attribute describing the cache level and thus the + element can be repeated multiple times to describe different levels of + the cache. + + + + The cache element then has associativity + attribute (accepted values are none, direct and + full) describing the cache associativity, and + policy attribute (accepted values are none, + writeback and writethrough) describing cache + write associativity. The element has two mandatory child elements then: + size and line which describe cache size and + cache line size. Both elements accept two attributes: value + and unit which set the value of corresponding cache + attribute. + This second paragraph is about the same element that you started to describe in the previous paragraph. Might as well make a big paragraph talking just about the cache element. And there's a lot of different elements and attributes at different levels to unpack at once here. I think you can use a description list () here, like you did below with the element, or any other hierarchical HTML struct for that matter, to make it easier to display all the info you're providing here. + + + The NUMA description has optional latencies element that "The NUMA description has an optional ..." Everything else LGTM and these HTML changes are somewhat cosmetic, so: Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 07/13] Allow NUMA nodes without vCPUs
On 6/24/20 10:49 AM, Michal Privoznik wrote: QEMU allows creating NUMA nodes that have memory only. These are somehow important for HMAT. With check done in qemuValidateDomainDef() for QEMU 2.7 or newer, You're mentioning "QEMU 2.7 or newer" but the code in qemu_validate is checking for QEMU_CAPS_NUMA. I'm assuming that QEMU 2.7 is where QEMU_CAPS_NUMA first appeared. In this case, I think that an adendum like "QEMU 2.7 or newer (checked via QEMU_CAPS_NUMA)" would be nice to clarify. we can be sure that the vCPUs are fully assigned to NUMA nodes in domain XML. Signed-off-by: Michal Privoznik --- docs/formatdomain.html.in | 2 + docs/schemas/cputypes.rng | 8 ++- src/conf/numa_conf.c | 59 ++- src/libxl/xen_xl.c| 10 ++-- src/qemu/qemu_command.c | 26 src/qemu/qemu_validate.c | 22 +++ tests/qemuxml2argvdata/numatune-no-vcpu.args | 33 +++ tests/qemuxml2argvdata/numatune-no-vcpu.xml | 42 + tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/numatune-no-vcpu.xml | 1 + tests/qemuxml2xmltest.c | 1 + 11 files changed, 149 insertions(+), 56 deletions(-) create mode 100644 tests/qemuxml2argvdata/numatune-no-vcpu.args create mode 100644 tests/qemuxml2argvdata/numatune-no-vcpu.xml create mode 12 tests/qemuxml2xmloutdata/numatune-no-vcpu.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bd662727d3..07dcca57f5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1840,6 +1840,8 @@ consistent across qemu and libvirt versions. memory specifies the node memory in kibibytes (i.e. blocks of 1024 bytes). + Since 6.6.0 the cpus attribute s/Since 6.6.0/Since 6.5.0 ? Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 06/13] numa_conf: Make virDomainNumaSetNodeCpumask() return void
On 6/24/20 10:49 AM, Michal Privoznik wrote: There is only one caller of virDomainNumaSetNodeCpumask() which checks for the return value but because the function will return NULL iff the @cpumask was NULL in the first place. But in that place @cpumask can't be NULL because it was just allocated by virBitmapParse(). Signed-off-by: Michal Privoznik --- Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 01/13] qemuxml2xmltest: Add "numatune-distance" test case
On 6/24/20 5:43 PM, Daniel Henrique Barboza wrote: On 6/24/20 10:48 AM, Michal Privoznik wrote: This test case tests that expanding of NUMA distances work. On This first sentence seems odd. Perhaps change it to "This test case checks that expanding NUMA distance works" Forgot to add my r-b: Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 05/13] qemuBuildMachineCommandLine: Drop needless check
On 6/24/20 10:49 AM, Michal Privoznik wrote: The machine can not be NULL at this point - qemuDomainDefPostParse() makes sure it isn't. Signed-off-by: Michal Privoznik --- Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 04/13] qemu_command: Rename qemuBuildNumaArgStr()
On 6/24/20 10:49 AM, Michal Privoznik wrote: The function doesn't just build the argument for -numa. Since the -numa can be repeated multiple times, it also puts -numa onto the cmd line. Also, the rest of the functions has 'Command' infix. Signed-off-by: Michal Privoznik --- Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 03/13] numa_conf: Drop CPU from name of two functions
On 6/24/20 10:49 AM, Michal Privoznik wrote: There are two functions virDomainNumaDefCPUFormatXML() and virDomainNumaDefCPUParseXML() which format and parse domain's . There is nothing CPU specific about them. Drop the infix. Signed-off-by: Michal Privoznik --- Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 02/13] conf: Move and rename virDomainParseScaledValue()
On 6/24/20 10:49 AM, Michal Privoznik wrote: There is nothing domain specific about the function, thus it should not have virDomain prefix. Also, the fact that it is a static function makes it impossible to use from other files. Move the function to virxml.c and drop the 'Domain' infix. Signed-off-by: Michal Privoznik --- Reviewed-by: Daniel Henrique Barboza
Re: [PATCH 01/13] qemuxml2xmltest: Add "numatune-distance" test case
On 6/24/20 10:48 AM, Michal Privoznik wrote:
This test case tests that expanding of NUMA distances work. On
This first sentence seems odd. Perhaps change it to
"This test case checks that expanding NUMA distance works"
input we accept if only distance from A to B is specified. On the
output we format the B to A distance too.
Signed-off-by: Michal Privoznik
---
.../qemuxml2xmloutdata/numatune-distances.xml | 96 +++
tests/qemuxml2xmltest.c | 1 +
2 files changed, 97 insertions(+)
create mode 100644 tests/qemuxml2xmloutdata/numatune-distances.xml
diff --git a/tests/qemuxml2xmloutdata/numatune-distances.xml
b/tests/qemuxml2xmloutdata/numatune-distances.xml
new file mode 100644
index 00..48f89cb015
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/numatune-distances.xml
@@ -0,0 +1,96 @@
+
+ QEMUGuest
+ c7a5fdb2-cdaf-9455-926a-d65c16db1809
+ 8388608
+ 8388608
+ 12
+
+hvm
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+ destroy
+ restart
+ restart
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 5a124853b4..d203c97e36 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1104,6 +1104,7 @@ mymain(void)
DO_TEST("numatune-auto-prefer", NONE);
DO_TEST("numatune-memnode", QEMU_CAPS_NUMA, QEMU_CAPS_OBJECT_MEMORY_FILE);
DO_TEST("numatune-memnode-no-memory", QEMU_CAPS_OBJECT_MEMORY_FILE);
+DO_TEST("numatune-distances", QEMU_CAPS_NUMA, QEMU_CAPS_NUMA_DIST);
DO_TEST("bios-nvram", NONE);
DO_TEST("bios-nvram-os-interleave", NONE);
Re: [libvirt-publican PATCH 3/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
On Wed, 2020-06-24 at 18:11 +0100, Daniel P. Berrangé wrote: > With the introduction of automated CI pipelines, we are now ready to switch > to using merge requests for the project. With this switch we longer wish > to have patches sent to the mailing list. > > Signed-off-by: Daniel P. Berrangé > --- > .gitpublish | 4 > CONTRIBUTING.rst | 28 > 2 files changed, 28 insertions(+), 4 deletions(-) > delete mode 100644 .gitpublish > create mode 100644 CONTRIBUTING.rst Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-publican PATCH 2/3] gitlab: introduce CI jobs for building content
On Wed, 2020-06-24 at 18:11 +0100, Daniel P. Berrangé wrote: > +++ b/ci/containers/refresh > @@ -0,0 +1,22 @@ > +#!/bin/sh > + > +if test -z "$1" > +then > +echo "syntax: $0 PATH-TO-LCITOOL" > +exit 1 > +fi > + > +LCITOOL=$1 > + > +if ! test -x "$LCITOOL" > +then > +echo "$LCITOOL is not executable" > +exit 1 > +fi > + > +HOSTS=$($LCITOOL hosts | grep -v -E '(freebsd|centos-8|opensuse)') You need to exclude CentOS Stream as well, and drop the corresponding Dockerfile. With that fixed, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt-publican PATCH 1/3] cfg: remove obsolete "release" config entry
On Wed, 2020-06-24 at 18:11 +0100, Daniel P. Berrangé wrote: > Signed-off-by: Daniel P. Berrangé > --- > publican.cfg | 1 - > 1 file changed, 1 deletion(-) Under the assumption that this preemptively fixes a build failure that would show up once GitLab CI is enabled Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH libvirt-cim 1/2] gitlab: introduce CI jobs testing git master & distro libvirt
On Wed, 2020-06-24 at 17:20 +0100, Daniel P. Berrangé wrote: > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > new file mode 100644 > index 000..75d34c3 > --- /dev/null > +++ b/.gitlab-ci.yml Oh and this doesn't apply cleanly on top of master, because you're trying to create a new file while .gitlab-ci.yml already exists. I think you made this commit on top of master^ instead of master. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH libvirt v2 0/5] Fix zPCI address auto-generation on s390
On Wed, 2020-06-24 at 18:59 +0200, Shalini Chellathurai Saroja wrote: > Ping, in case you missed it. Please wait at least a week or so before pinging a series, and refrain from CCing individual developers when you do - we're all subscribed to libvir-list. Anyway I've seen the series and I'm planning to review it, I just haven't gotten around to it yet :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH libvirt-cim 1/2] gitlab: introduce CI jobs testing git master & distro libvirt
On Wed, 2020-06-24 at 17:20 +0100, Daniel P. Berrangé wrote: > The sandbox build needs to validate two axis > > - A variety of distro versions > - A variety of libvirt versions > > We test a variety of libvirt versions by running a build against the > distro provided libvirt packages. All that is then missing is a build > against the latest libvirt git master, which only needs to be run on > a single distro, for which CentOS 7 is picked as a stable long life > base. This... > +x64-fedora-rawhide-git-build: > + <<: *git_native_build_job_definition > + variables: > +NAME: fedora-rawhide ... disagrees with this... > +++ b/ci/containers/refresh > +for host in $HOSTS > +do > +if test "$host" = "libvirt-fedora-rawhide" > +then > +$LCITOOL dockerfile $host libvirt+minimal,libvirt+dist,libvirt-cim > > $host.Dockerfile ... and this. I think the commit message is correct here: we want git builds to happen on long-term supported distro, which Rawhide couldn't be further from, but perhaps I'm missing something? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH libvirt-cim 2/2] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
On Wed, 2020-06-24 at 17:20 +0100, Daniel P. Berrangé wrote: > With the introduction of automated CI pipelines, we are now ready to switch > to using merge requests for the project. With this switch we longer wish > to have patches sent to the mailing list, and thus the git-publish > config is removed. > > Signed-off-by: Daniel P. Berrangé > --- > .gitpublish | 4 > CONTRIBUTING.rst | 28 > 2 files changed, 28 insertions(+), 4 deletions(-) > delete mode 100644 .gitpublish > create mode 100644 CONTRIBUTING.rst Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH libvirt-dbus v2 2/3] gitlab: introduce CI jobs testing git master & distro libvirt
On Wed, 2020-06-24 at 17:26 +0100, Daniel P. Berrangé wrote: > +++ b/ci/containers/libvirt-centos-7.Dockerfile > +dnf install -y \ > +python3-docutils \ Sorry, one more thing: if I refresh Dockerfiles by pointing the included script to lcitool coming from libvirt-ci master, this line disappears from each Dockerfile except those for CentOS 8 and CentOS Stream. Since it's actually supposed to be there, should I assume that you have local libvirt-ci patches that add a python3-docutils dependency to the libvirt-dbus project? -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH libvirt-dbus v2 2/3] gitlab: introduce CI jobs testing git master & distro libvirt
On Wed, 2020-06-24 at 17:26 +0100, Daniel P. Berrangé wrote: > The dbus build needs to validate one axis > > - A variety of libvirt versions > > We test a variety of libvirt versions by running a build against the > distro provided libvirt packages. All that is then missing is a build > against the latest libvirt git master, which only needs to be run on > a single distro, for which CentOS 8 is picked as a stable long life > base. This... > +++ b/.gitlab-ci.yml > +x64-centos-8-git-build: > + <<: *git_native_build_job_definition > + variables: > +NAME: centos-8 > + > +x64-centos-stream-git-build: > + <<: *git_native_build_job_definition > + variables: > +NAME: centos-stream ... contradicts this... > +++ b/ci/containers/refresh > +for host in $HOSTS > +do > +if test "$host" = "libvirt-centos-8" || test "$host" = > "libvirt-centos-stream" > +then > +$LCITOOL dockerfile $host libvirt+minimal,libvirt-glib,libvirt-dbus > > $host.Dockerfile ... and this. What's the rationale for building libvirt and libvirt-glib from git on CentOS Stream in addition to CentOS 8? > +if test "$host" = "libvirt-debian-9" || test "$host" = > "libvirt-ubuntu-1804" > +then > + sed -i -e 's/libvirt-dev/libvirt-dev libvirt-daemon/' $host.Dockerfile This line is not indented correctly. Additionally, please add a comment explaining why this hack is needed in the first place, something along the lines of Before Debian version 4.10.0-2, some of the runtime files needed by libvirt were mistakenly shipped in the libvirt-daemon package One more nitpick: the conditional would look nicer and be easier to tweak later as if test "$host" = "libvirt-debian-9" || test "$host" = "libvirt-ubuntu-1804" then Same applies above. -- Andrea Bolognani / Red Hat / Virtualization
[libvirt-publican PATCH 1/3] cfg: remove obsolete "release" config entry
Signed-off-by: Daniel P. Berrangé --- publican.cfg | 1 - 1 file changed, 1 deletion(-) diff --git a/publican.cfg b/publican.cfg index bd5bb97..5832d17 100644 --- a/publican.cfg +++ b/publican.cfg @@ -3,7 +3,6 @@ version: 0.1 xml_lang: en-US -release: 0 type: brand brand: libvirt -- 2.26.2
[libvirt-publican PATCH 3/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
With the introduction of automated CI pipelines, we are now ready to switch to using merge requests for the project. With this switch we longer wish to have patches sent to the mailing list. Signed-off-by: Daniel P. Berrangé --- .gitpublish | 4 CONTRIBUTING.rst | 28 2 files changed, 28 insertions(+), 4 deletions(-) delete mode 100644 .gitpublish create mode 100644 CONTRIBUTING.rst diff --git a/.gitpublish b/.gitpublish deleted file mode 100644 index 2c098c2..000 --- a/.gitpublish +++ /dev/null @@ -1,4 +0,0 @@ -[gitpublishprofile "default"] -base = master -to = libvir-list@redhat.com -prefix = libvirt-publican PATCH diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst new file mode 100644 index 000..1405596 --- /dev/null +++ b/CONTRIBUTING.rst @@ -0,0 +1,28 @@ + +Contributing to libvirt-publican + + +The libvirt publican theme accepts code contributions +via merge requests on the GitLab project: + +https://gitlab.com/libvirt/libvirt-publican/-/merge_requests + +It is required that automated CI pipelines succeed before a merge request +will be accepted. The global pipeline status for the ``master`` branch is +visible at: + +https://gitlab.com/libvirt/libvirt-publican/pipelines + +CI pipeline results for merge requests will be visible via the contributors' +own private repository fork: + +https://gitlab.com/yourusername/libvirt-publican/pipelines + +Contributions submitted to the project must be in compliance with the +Developer Certificate of Origin Version 1.1. This is documented at: + +https://developercertificate.org/ + +To indicate compliance, each commit in a series must have a "Signed-off-by" +tag with the submitter's name and email address. This can be added by passing +the ``-s`` flag to ``git commit`` when creating the patches. -- 2.26.2
[libvirt-publican PATCH 2/3] gitlab: introduce CI jobs for building content
The docs build needs to validate one axis - A variety of publican versions We get coverage for this by running builds across various distros. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml| 118 ++ ci/containers/README.rst | 14 +++ ci/containers/libvirt-centos-7.Dockerfile | 83 .../libvirt-centos-stream.Dockerfile | 52 ci/containers/libvirt-debian-10.Dockerfile| 53 ci/containers/libvirt-debian-9.Dockerfile | 56 + ci/containers/libvirt-debian-sid.Dockerfile | 53 ci/containers/libvirt-fedora-31.Dockerfile| 50 ci/containers/libvirt-fedora-32.Dockerfile| 50 .../libvirt-fedora-rawhide.Dockerfile | 51 ci/containers/libvirt-ubuntu-1804.Dockerfile | 56 + ci/containers/libvirt-ubuntu-2004.Dockerfile | 53 ci/containers/refresh | 22 13 files changed, 711 insertions(+) create mode 100644 ci/containers/README.rst create mode 100644 ci/containers/libvirt-centos-7.Dockerfile create mode 100644 ci/containers/libvirt-centos-stream.Dockerfile create mode 100644 ci/containers/libvirt-debian-10.Dockerfile create mode 100644 ci/containers/libvirt-debian-9.Dockerfile create mode 100644 ci/containers/libvirt-debian-sid.Dockerfile create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-1804.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-2004.Dockerfile create mode 100755 ci/containers/refresh diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 50dae92..d564261 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,6 +1,33 @@ stages: - prebuild + - containers + - docs + +.container_job_template: _job_definition + image: docker:stable + stage: containers + services: +- docker:dind + before_script: +- export TAG="$CI_REGISTRY_IMAGE/ci-$NAME:latest" +- export COMMON_TAG="$CI_REGISTRY/libvirt/libvirt-publican/ci-$NAME:latest" +- docker info +- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p "$CI_REGISTRY_PASSWORD" + script: +- docker pull "$TAG" || docker pull "$COMMON_TAG" || true +- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" -f "ci/containers/libvirt-$NAME.Dockerfile" ci +- docker push "$TAG" + after_script: +- docker logout + +.docs_job_template: _job_definition + image: $CI_REGISTRY_IMAGE/ci-$NAME:latest + stage: docs + before_script: +- export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)" + script: +- $MAKE # Check that all commits are signed-off for the DCO. # Skip on "libvirt" namespace, since we only need to run @@ -14,3 +41,94 @@ check-dco: except: variables: - $CI_PROJECT_NAMESPACE == 'libvirt' + +centos-7-container: + <<: *container_job_definition + variables: +NAME: centos-7 + +debian-9-container: + <<: *container_job_definition + variables: +NAME: debian-9 + +debian-10-container: + <<: *container_job_definition + variables: +NAME: debian-10 + +debian-sid-container: + <<: *container_job_definition + variables: +NAME: debian-sid + +fedora-31-container: + <<: *container_job_definition + variables: +NAME: fedora-31 + +fedora-32-container: + <<: *container_job_definition + variables: +NAME: fedora-32 + +fedora-rawhide-container: + <<: *container_job_definition + variables: +NAME: fedora-rawhide + +ubuntu-1804-container: + <<: *container_job_definition + variables: +NAME: ubuntu-1804 + +ubuntu-2004-container: + <<: *container_job_definition + variables: +NAME: ubuntu-2004 + + +centos-7-docs: + <<: *docs_job_definition + variables: +NAME: centos-7 + +debian-9-docs: + <<: *docs_job_definition + variables: +NAME: debian-9 + +debian-10-docs: + <<: *docs_job_definition + variables: +NAME: debian-10 + +debian-sid-docs: + <<: *docs_job_definition + variables: +NAME: debian-sid + +fedora-31-docs: + <<: *docs_job_definition + variables: +NAME: fedora-31 + +fedora-32-docs: + <<: *docs_job_definition + variables: +NAME: fedora-32 + +fedora-rawhide-docs: + <<: *docs_job_definition + variables: +NAME: fedora-rawhide + +ubuntu-1804-docs: + <<: *docs_job_definition + variables: +NAME: ubuntu-1804 + +ubuntu-2004-docs: + <<: *docs_job_definition + variables: +NAME: ubuntu-2004 diff --git a/ci/containers/README.rst b/ci/containers/README.rst new file mode 100644 index 000..530897e --- /dev/null +++ b/ci/containers/README.rst @@ -0,0 +1,14 @@ +CI job assets += + +This directory contains assets used in the automated CI jobs, most +notably the Dockerfiles used to build container images in which the +CI jobs then run. + +The ``refresh`` script
[libvirt-publican PATCH 0/3] introduce GitLab CI and merge requests
The publican theme is used by the libvirt-appdev-guide-python, so we should test it. Daniel P. Berrangé (3): cfg: remove obsolete "release" config entry gitlab: introduce CI jobs for building content gitlab: add CONTRIBUTING.rst file to indicate use of merge requests .gitlab-ci.yml| 118 ++ .gitpublish | 4 - CONTRIBUTING.rst | 28 + ci/containers/README.rst | 14 +++ ci/containers/libvirt-centos-7.Dockerfile | 83 .../libvirt-centos-stream.Dockerfile | 52 ci/containers/libvirt-debian-10.Dockerfile| 53 ci/containers/libvirt-debian-9.Dockerfile | 56 + ci/containers/libvirt-debian-sid.Dockerfile | 53 ci/containers/libvirt-fedora-31.Dockerfile| 50 ci/containers/libvirt-fedora-32.Dockerfile| 50 .../libvirt-fedora-rawhide.Dockerfile | 51 ci/containers/libvirt-ubuntu-1804.Dockerfile | 56 + ci/containers/libvirt-ubuntu-2004.Dockerfile | 53 ci/containers/refresh | 22 publican.cfg | 1 - 16 files changed, 739 insertions(+), 5 deletions(-) delete mode 100644 .gitpublish create mode 100644 CONTRIBUTING.rst create mode 100644 ci/containers/README.rst create mode 100644 ci/containers/libvirt-centos-7.Dockerfile create mode 100644 ci/containers/libvirt-centos-stream.Dockerfile create mode 100644 ci/containers/libvirt-debian-10.Dockerfile create mode 100644 ci/containers/libvirt-debian-9.Dockerfile create mode 100644 ci/containers/libvirt-debian-sid.Dockerfile create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-1804.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-2004.Dockerfile create mode 100755 ci/containers/refresh -- 2.26.2
Re: [PATCH libvirt v2 0/5] Fix zPCI address auto-generation on s390
Ping, in case you missed it. On 6/18/20 10:25 AM, Shalini Chellathurai Saroja wrote: The zPCI address validation or autogeneration does not work as expected in the following scenarios 1. uid = 0 and fid = 0 2. uid = 0 and fid > 0 3. uid = 0 and fid not specified 4. uid not specified and fid > 0 5. 2 zPCI devices with uid > 0 and fid not specified. This is because of the following reasons 1. If uid = 0 or fid = 0 the code assumes that user has not specified the corresponding address 2. If either uid or fid is provided, the code assumes that both uid and fid addresses are specified by the user. This patch fixes these issues. --- v2: - Call same function to reserve zPCI address when zPCI address is fully/partially/not specified by the user. - Redefine structure of zPCI address. - Add tests to verify the auto-generated xml(qemuxml2xmltest.c). - Separate/merge patches. - Minor changes based on review feedback. v1: https://www.redhat.com/archives/libvir-list/2020-April/msg00479.html Shalini Chellathurai Saroja (5): conf: use g_autofree to ensure automatic cleanup conf: fix zPCI address auto-generation on s390 qemu: move ZPCI uid validation into device validation tests: qemu: add more tests for ZPCI on S390 tests: add test with PCI and CCW device src/conf/device_conf.c| 45 +-- src/conf/domain_addr.c| 77 ++- src/conf/domain_conf.c| 10 ++- src/libvirt_private.syms | 4 +- src/qemu/qemu_command.c | 6 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_validate.c | 18 - src/util/virpci.c | 23 ++ src/util/virpci.h | 15 +++- .../hostdev-vfio-zpci-autogenerate-fids.args | 31 .../hostdev-vfio-zpci-autogenerate-fids.xml | 29 +++ .../hostdev-vfio-zpci-autogenerate-uids.args | 31 .../hostdev-vfio-zpci-autogenerate-uids.xml | 29 +++ .../hostdev-vfio-zpci-ccw-memballoon.args | 28 +++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 17 .../hostdev-vfio-zpci-duplicate.xml | 30 ...ostdev-vfio-zpci-invalid-uid-valid-fid.xml | 21 + .../hostdev-vfio-zpci-set-zero.xml| 21 + .../hostdev-vfio-zpci-uid-set-zero.xml| 20 + tests/qemuxml2argvtest.c | 25 ++ .../hostdev-vfio-zpci-autogenerate-fids.xml | 43 +++ .../hostdev-vfio-zpci-autogenerate-uids.xml | 43 +++ .../hostdev-vfio-zpci-ccw-memballoon.xml | 32 tests/qemuxml2xmltest.c | 10 +++ 24 files changed, 498 insertions(+), 112 deletions(-) create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-fids.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate-uids.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.args create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-ccw-memballoon.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-duplicate.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-invalid-uid-valid-fid.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-set-zero.xml create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-zpci-uid-set-zero.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-fids.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-autogenerate-uids.xml create mode 100644 tests/qemuxml2xmloutdata/hostdev-vfio-zpci-ccw-memballoon.xml
[PATCH libvirt-dbus v2 2/3] gitlab: introduce CI jobs testing git master & distro libvirt
The dbus build needs to validate one axis - A variety of libvirt versions We test a variety of libvirt versions by running a build against the distro provided libvirt packages. All that is then missing is a build against the latest libvirt git master, which only needs to be run on a single distro, for which CentOS 8 is picked as a stable long life base. RPM build and tests are skipped on CentOS 7 since it lacks the required min version of some packages. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml| 218 ++ ci/containers/README.rst | 14 ++ ci/containers/libvirt-centos-7.Dockerfile | 90 ci/containers/libvirt-centos-8.Dockerfile | 68 ++ .../libvirt-centos-stream.Dockerfile | 69 ++ ci/containers/libvirt-debian-10.Dockerfile| 61 + ci/containers/libvirt-debian-9.Dockerfile | 64 + ci/containers/libvirt-debian-sid.Dockerfile | 61 + ci/containers/libvirt-fedora-31.Dockerfile| 58 + ci/containers/libvirt-fedora-32.Dockerfile| 58 + .../libvirt-fedora-rawhide.Dockerfile | 59 + ci/containers/libvirt-opensuse-151.Dockerfile | 60 + ci/containers/libvirt-ubuntu-1804.Dockerfile | 64 + ci/containers/libvirt-ubuntu-2004.Dockerfile | 61 + ci/containers/refresh | 32 +++ 15 files changed, 1037 insertions(+) create mode 100644 ci/containers/README.rst create mode 100644 ci/containers/libvirt-centos-7.Dockerfile create mode 100644 ci/containers/libvirt-centos-8.Dockerfile create mode 100644 ci/containers/libvirt-centos-stream.Dockerfile create mode 100644 ci/containers/libvirt-debian-10.Dockerfile create mode 100644 ci/containers/libvirt-debian-9.Dockerfile create mode 100644 ci/containers/libvirt-debian-sid.Dockerfile create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile create mode 100644 ci/containers/libvirt-opensuse-151.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-1804.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-2004.Dockerfile create mode 100755 ci/containers/refresh diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 50dae92..9b61c70 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,6 +1,99 @@ stages: - prebuild + - containers + - builds + +.container_job_template: _job_definition + image: docker:stable + stage: containers + services: +- docker:dind + before_script: +- export TAG="$CI_REGISTRY_IMAGE/ci-$NAME:latest" +- export COMMON_TAG="$CI_REGISTRY/libvirt/libvirt-dbus/ci-$NAME:latest" +- docker info +- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p "$CI_REGISTRY_PASSWORD" + script: +- docker pull "$TAG" || docker pull "$COMMON_TAG" || true +- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" -f "ci/containers/libvirt-$NAME.Dockerfile" ci +- docker push "$TAG" + after_script: +- docker logout + +.script_variables: _variables | + export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)" + export SCRATCH_DIR="/tmp/scratch" + export VROOT="$SCRATCH_DIR/vroot" + export CCACHE_DIR="$PWD/ccache" + export CCACHE_MAXSIZE="500M" + export PATH="$CCACHE_WRAPPERSDIR:$VROOT/bin:$PATH" + export PKG_CONFIG_PATH="$VROOT/lib/pkgconfig" + export XDG_DATA_DIRS="$VROOT/share:/usr/share" + export GI_TYPELIB_PATH="$VROOT/lib/girepository-1.0" + export LD_LIBRARY_PATH="$VROOT/lib" + +.git_native_build_job_template: _native_build_job_definition + image: $CI_REGISTRY_IMAGE/ci-$NAME:latest + stage: builds + cache: +paths: + - ccache/ +key: "$CI_JOB_NAME" + before_script: +- *script_variables + script: +- pushd "$PWD" +- mkdir -p "$SCRATCH_DIR" +- cd "$SCRATCH_DIR" +- git clone --depth 1 https://gitlab.com/libvirt/libvirt.git +- git clone --depth 1 https://gitlab.com/libvirt/libvirt-glib.git +- mkdir libvirt/build +- cd libvirt/build +- ../autogen.sh --prefix="$VROOT" --without-libvirtd +- $MAKE install +- cd ../.. +- mkdir libvirt-glib/build +- cd libvirt-glib/build +- ../autogen.sh --prefix="$VROOT" +- $MAKE install +- popd +- meson build --prefix="$VROOT" +- $NINJA -C build +- if test "$TESTS" != "skip"; + then +$NINJA -C build test; +$NINJA -C build install; +$NINJA -C build dist; + fi +- if test -x /usr/bin/rpmbuild && test "$RPM" != "skip"; + then +rpmbuild --nodeps -ta build/meson-dist/libvirt-dbus*.tar.xz; + fi + +.dist_native_build_job_template: _native_build_job_definition + image: $CI_REGISTRY_IMAGE/ci-$NAME:latest + stage: builds + cache: +paths: + - ccache/ +key: "$CI_JOB_NAME" + before_script: +- *script_variables + script: +- meson build --prefix="$VROOT" +
[PATCH libvirt-dbus v2 0/3] Introduce GitLab CI and merge requests
This introduces CI build coverage and then documents the switch to use merge requests. Daniel P. Berrangé (3): src: fix double free of virDomain object gitlab: introduce CI jobs testing git master & distro libvirt gitlab: add CONTRIBUTING.rst file to indicate use of merge requests .gitlab-ci.yml| 218 ++ .gitpublish | 4 - CONTRIBUTING.rst | 28 +++ ci/containers/README.rst | 14 ++ ci/containers/libvirt-centos-7.Dockerfile | 90 ci/containers/libvirt-centos-8.Dockerfile | 68 ++ .../libvirt-centos-stream.Dockerfile | 69 ++ ci/containers/libvirt-debian-10.Dockerfile| 61 + ci/containers/libvirt-debian-9.Dockerfile | 64 + ci/containers/libvirt-debian-sid.Dockerfile | 61 + ci/containers/libvirt-fedora-31.Dockerfile| 58 + ci/containers/libvirt-fedora-32.Dockerfile| 58 + .../libvirt-fedora-rawhide.Dockerfile | 59 + ci/containers/libvirt-opensuse-151.Dockerfile | 60 + ci/containers/libvirt-ubuntu-1804.Dockerfile | 64 + ci/containers/libvirt-ubuntu-2004.Dockerfile | 61 + ci/containers/refresh | 32 +++ src/domainsnapshot.c | 4 +- 18 files changed, 1067 insertions(+), 6 deletions(-) delete mode 100644 .gitpublish create mode 100644 CONTRIBUTING.rst create mode 100644 ci/containers/README.rst create mode 100644 ci/containers/libvirt-centos-7.Dockerfile create mode 100644 ci/containers/libvirt-centos-8.Dockerfile create mode 100644 ci/containers/libvirt-centos-stream.Dockerfile create mode 100644 ci/containers/libvirt-debian-10.Dockerfile create mode 100644 ci/containers/libvirt-debian-9.Dockerfile create mode 100644 ci/containers/libvirt-debian-sid.Dockerfile create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile create mode 100644 ci/containers/libvirt-opensuse-151.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-1804.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-2004.Dockerfile create mode 100755 ci/containers/refresh -- 2.26.2
[PATCH libvirt-dbus v2 3/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
With the introduction of automated CI pipelines, we are now ready to switch to using merge requests for the project. With this switch we longer wish to have patches sent to the mailing list, and thus the git-publish config is removed. Reviewed-by: Andrea Bolognani Signed-off-by: Daniel P. Berrangé --- .gitpublish | 4 CONTRIBUTING.rst | 28 2 files changed, 28 insertions(+), 4 deletions(-) delete mode 100644 .gitpublish create mode 100644 CONTRIBUTING.rst diff --git a/.gitpublish b/.gitpublish deleted file mode 100644 index d21df70..000 --- a/.gitpublish +++ /dev/null @@ -1,4 +0,0 @@ -[gitpublishprofile "default"] -base = master -to = libvir-list@redhat.com -prefix = libvirt-dbus PATCH diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst new file mode 100644 index 000..8391896 --- /dev/null +++ b/CONTRIBUTING.rst @@ -0,0 +1,28 @@ + +Contributing to libvirt-dbus + + +The libvirt DBus API binding accepts code contributions via merge requests +on the GitLab project: + +https://gitlab.com/libvirt/libvirt-dbus/-/merge_requests + +It is required that automated CI pipelines succeed before a merge request +will be accepted. The global pipeline status for the ``master`` branch is +visible at: + +https://gitlab.com/libvirt/libvirt-dbus/pipelines + +CI pipeline results for merge requests will be visible via the contributors' +own private repository fork: + +https://gitlab.com/yourusername/libvirt-dbus/pipelines + +Contributions submitted to the project must be in compliance with the +Developer Certificate of Origin Version 1.1. This is documented at: + +https://developercertificate.org/ + +To indicate compliance, each commit in a series must have a "Signed-off-by" +tag with the submitter's name and email address. This can be added by passing +the ``-s`` flag to ``git commit`` when creating the patches. -- 2.26.2
[PATCH libvirt-dbus v2 1/3] src: fix double free of virDomain object
The virDomainSnapshotGetDomain() method does NOT increment the refcount on the returned virDomain, so it must not be unrefed. This double free is responsible for random failures of the test_snapshot.py tests. Reviewed-by: Andrea Bolognani Signed-off-by: Daniel P. Berrangé --- src/domainsnapshot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/domainsnapshot.c b/src/domainsnapshot.c index f3571a1..78a9f9a 100644 --- a/src/domainsnapshot.c +++ b/src/domainsnapshot.c @@ -63,7 +63,7 @@ virtDBusDomainSnapshotGetParent(GVariant *inArgs, g_autoptr(virDomainSnapshot) domainSnapshot = NULL; g_autoptr(virDomainSnapshot) parent = NULL; guint flags; -g_autoptr(virDomain) domain = NULL; +virDomainPtr domain = NULL; g_autofree gchar *parentPath = NULL; g_variant_get(inArgs, "(u)", ); @@ -159,7 +159,7 @@ virtDBusDomainSnapshotListAllChildren(GVariant *inArgs, guint flags; GVariantBuilder builder; GVariant *gdomainSnapshots; -g_autoptr(virDomain) domain = NULL; +virDomainPtr domain = NULL; g_variant_get(inArgs, "(u)", ); -- 2.26.2
Re: [PATCH libvirt-dbus 3/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
On Wed, 2020-06-24 at 16:17 +0100, Daniel P. Berrangé wrote: > With the introduction of automated CI pipelines, we are now ready to switch > to using merge requests for the project. With this switch we longer wish > to have patches sent to the mailing list, and thus the git-publish > config is removed. > > Signed-off-by: Daniel P. Berrangé > --- > .gitpublish | 4 > CONTRIBUTING.rst | 28 > 2 files changed, 28 insertions(+), 4 deletions(-) > delete mode 100644 .gitpublish > create mode 100644 CONTRIBUTING.rst Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
[PATCH libvirt-cim 1/2] gitlab: introduce CI jobs testing git master & distro libvirt
The sandbox build needs to validate two axis - A variety of distro versions - A variety of libvirt versions We test a variety of libvirt versions by running a build against the distro provided libvirt packages. All that is then missing is a build against the latest libvirt git master, which only needs to be run on a single distro, for which CentOS 7 is picked as a stable long life base. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml| 130 ++ ci/containers/README.rst | 14 ++ ci/containers/libvirt-centos-7.Dockerfile | 89 ci/containers/libvirt-fedora-31.Dockerfile| 56 ci/containers/libvirt-fedora-32.Dockerfile| 56 .../libvirt-fedora-rawhide.Dockerfile | 65 + ci/containers/refresh | 27 7 files changed, 437 insertions(+) create mode 100644 .gitlab-ci.yml create mode 100644 ci/containers/README.rst create mode 100644 ci/containers/libvirt-centos-7.Dockerfile create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile create mode 100755 ci/containers/refresh diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml new file mode 100644 index 000..75d34c3 --- /dev/null +++ b/.gitlab-ci.yml @@ -0,0 +1,130 @@ + +stages: + - prebuild + - containers + - builds + +.container_job_template: _job_definition + image: docker:stable + stage: containers + services: +- docker:dind + before_script: +- export TAG="$CI_REGISTRY_IMAGE/ci-$NAME:latest" +- export COMMON_TAG="$CI_REGISTRY/libvirt/libvirt-cim/ci-$NAME:latest" +- docker info +- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p "$CI_REGISTRY_PASSWORD" + script: +- docker pull "$TAG" || docker pull "$COMMON_TAG" || true +- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" -f "ci/containers/libvirt-$NAME.Dockerfile" ci +- docker push "$TAG" + after_script: +- docker logout + +.script_variables: _variables | + export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)" + export SCRATCH_DIR="/tmp/scratch" + export VROOT="$SCRATCH_DIR/vroot" + export CCACHE_DIR="$PWD/ccache" + export CCACHE_MAXSIZE="500M" + export PATH="$CCACHE_WRAPPERSDIR:$VROOT/bin:$PATH" + export PKG_CONFIG_PATH="$VROOT/lib/pkgconfig" + export LD_LIBRARY_PATH="$VROOT/lib" + +.git_native_build_job_template: _native_build_job_definition + image: $CI_REGISTRY_IMAGE/ci-$NAME:latest + stage: builds + cache: +paths: + - ccache/ +key: "$CI_JOB_NAME" + before_script: +- *script_variables + script: +- pushd "$PWD" +- mkdir -p "$SCRATCH_DIR" +- cd "$SCRATCH_DIR" +- git clone --depth 1 https://gitlab.com/libvirt/libvirt.git +- mkdir libvirt/build +- cd libvirt/build +- ../autogen.sh --prefix="$VROOT" --without-libvirtd +- $MAKE install +- popd +- mkdir build +- cd build +- ../autogen.sh --prefix="$VROOT" +- $MAKE install +- $MAKE dist +- if test -x /usr/bin/rpmbuild && test "$RPM" != "skip" ; then rpmbuild --nodeps -ta libvirt-cim*.tar.gz ; fi + +.dist_native_build_job_template: _native_build_job_definition + image: $CI_REGISTRY_IMAGE/ci-$NAME:latest + stage: builds + cache: +paths: + - ccache/ +key: "$CI_JOB_NAME" + before_script: +- *script_variables + script: +- mkdir build +- cd build +- ../autogen.sh --prefix="$VROOT" +- $MAKE install +- $MAKE dist +- if test -x /usr/bin/rpmbuild && test "$RPM" != "skip" ; then rpmbuild -ta libvirt-cim*.tar.gz ; fi + +# Check that all commits are signed-off for the DCO. +# Skip on "libvirt" namespace, since we only need to run +# this test on developer's personal forks from which +# merge requests are submitted +check-dco: + stage: prebuild + image: registry.gitlab.com/libvirt/libvirt-ci/check-dco:master + script: +- /check-dco + except: +variables: + - $CI_PROJECT_NAMESPACE == 'libvirt' + +x64-centos-7-container: + <<: *container_job_definition + variables: +NAME: centos-7 + +x64-fedora-31-container: + <<: *container_job_definition + variables: +NAME: fedora-31 + +x64-fedora-32-container: + <<: *container_job_definition + variables: +NAME: fedora-32 + +x64-fedora-rawhide-container: + <<: *container_job_definition + variables: +NAME: fedora-rawhide + + + +x64-centos-7-dist-build: + <<: *dist_native_build_job_definition + variables: +NAME: centos-7 + +x64-fedora-31-dist-build: + <<: *dist_native_build_job_definition + variables: +NAME: fedora-31 + +x64-fedora-32-dist-build: + <<: *dist_native_build_job_definition + variables: +NAME: fedora-32 + +x64-fedora-rawhide-git-build: + <<: *git_native_build_job_definition + variables: +NAME: fedora-rawhide diff --git a/ci/containers/README.rst
[PATCH libvirt-cim 2/2] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
With the introduction of automated CI pipelines, we are now ready to switch to using merge requests for the project. With this switch we longer wish to have patches sent to the mailing list, and thus the git-publish config is removed. Signed-off-by: Daniel P. Berrangé --- .gitpublish | 4 CONTRIBUTING.rst | 28 2 files changed, 28 insertions(+), 4 deletions(-) delete mode 100644 .gitpublish create mode 100644 CONTRIBUTING.rst diff --git a/.gitpublish b/.gitpublish deleted file mode 100644 index 2a466ac..000 --- a/.gitpublish +++ /dev/null @@ -1,4 +0,0 @@ -[gitpublishprofile "default"] -base = master -to = libvir-list@redhat.com -prefix = libvirt-cim PATCH diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst new file mode 100644 index 000..57a7951 --- /dev/null +++ b/CONTRIBUTING.rst @@ -0,0 +1,28 @@ +=== +Contributing to libvirt-cim +=== + +The libvirt CIM API binding accepts code contributions via merge requests +on the GitLab project: + +https://gitlab.com/libvirt/libvirt-cim/-/merge_requests + +It is required that automated CI pipelines succeed before a merge request +will be accepted. The global pipeline status for the ``master`` branch is +visible at: + +https://gitlab.com/libvirt/libvirt-cim/pipelines + +CI pipeline results for merge requests will be visible via the contributors' +own private repository fork: + +https://gitlab.com/yourusername/libvirt-cim/pipelines + +Contributions submitted to the project must be in compliance with the +Developer Certificate of Origin Version 1.1. This is documented at: + +https://developercertificate.org/ + +To indicate compliance, each commit in a series must have a "Signed-off-by" +tag with the submitter's name and email address. This can be added by passing +the ``-s`` flag to ``git commit`` when creating the patches. -- 2.26.2
Re: [PATCH libvirt-dbus 2/3] gitlab: introduce CI jobs testing git master & distro libvirt
On Wed, 2020-06-24 at 16:17 +0100, Daniel P. Berrangé wrote: > The dbus build needs to validate one axis > > - A variety of libvirt versions > > We test a variety of libvirt versions by running a build against the > distro provided libvirt packages. All that is then missing is a build > against the latest libvirt git master, which only needs to be run on > a single distro, for which CentOS 8 is picked as a stable long life > base. > > RPM build and tests are skipped on CentOS 7 since it lacks the required > min version of some packages. > > Signed-off-by: Daniel P. Berrangé > --- > .gitlab-ci.yml| 218 ++ > ci/containers/libvirt-centos-7.Dockerfile | 90 > ci/containers/libvirt-centos-8.Dockerfile | 68 ++ > .../libvirt-centos-stream.Dockerfile | 69 ++ > ci/containers/libvirt-debian-10.Dockerfile| 61 + > ci/containers/libvirt-debian-9.Dockerfile | 64 + > ci/containers/libvirt-debian-sid.Dockerfile | 61 + > ci/containers/libvirt-fedora-31.Dockerfile| 58 + > ci/containers/libvirt-fedora-32.Dockerfile| 58 + > .../libvirt-fedora-rawhide.Dockerfile | 59 + > ci/containers/libvirt-opensuse-151.Dockerfile | 60 + > ci/containers/libvirt-ubuntu-1804.Dockerfile | 64 + > ci/containers/libvirt-ubuntu-2004.Dockerfile | 61 + > 13 files changed, 991 insertions(+) Everything in this commit looks good, but you're missing ci/containers/refresh.sh ci/containers/README.rst -- Andrea Bolognani / Red Hat / Virtualization
[PATCH libvirt-cim 0/2] Introduce GitLab CI and merge requests
Daniel P. Berrangé (2): gitlab: introduce CI jobs testing git master & distro libvirt gitlab: add CONTRIBUTING.rst file to indicate use of merge requests .gitlab-ci.yml| 130 ++ .gitpublish | 4 - CONTRIBUTING.rst | 28 ci/containers/README.rst | 14 ++ ci/containers/libvirt-centos-7.Dockerfile | 89 ci/containers/libvirt-fedora-31.Dockerfile| 56 ci/containers/libvirt-fedora-32.Dockerfile| 56 .../libvirt-fedora-rawhide.Dockerfile | 65 + ci/containers/refresh | 27 9 files changed, 465 insertions(+), 4 deletions(-) create mode 100644 .gitlab-ci.yml delete mode 100644 .gitpublish create mode 100644 CONTRIBUTING.rst create mode 100644 ci/containers/README.rst create mode 100644 ci/containers/libvirt-centos-7.Dockerfile create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile create mode 100755 ci/containers/refresh -- 2.26.2
Re: [PATCH libvirt-dbus 1/3] src: fix double free of virDomain object
On Wed, 2020-06-24 at 16:17 +0100, Daniel P. Berrangé wrote: > The virDomainSnapshotGetDomain() method does NOT increment the > refcount on the returned virDomain, so it must not be unrefed. > > This double free is responsible for random failures of the > test_snapshot.py tests. > > Signed-off-by: Daniel P. Berrangé > --- > src/domainsnapshot.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Nice! Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [GSoC][PATCH 0/2] Extracting qemu domain jobs into a separate file
On 6/24/20 12:45 PM, Prathamesh Chavan wrote: These patches aim towards extracting out the domain jobs into a separate file. Curretnly, we have named it as `qemu_domainjob`, keeping it similar to the already exisiting `qemu_blockjob` files. Prathamesh Chavan (2): qemu_domain: Avoid using qemuDomainObjPrivatePtr as parameter qemu_domainjob: moved domain job APIs to a separate file po/POTFILES.in|1 + src/qemu/Makefile.inc.am |2 + src/qemu/qemu_domain.c| 1172 +--- src/qemu/qemu_domain.h| 247 +--- src/qemu/qemu_domainjob.c | 1192 + src/qemu/qemu_domainjob.h | 269 + 6 files changed, 1472 insertions(+), 1411 deletions(-) create mode 100644 src/qemu/qemu_domainjob.c create mode 100644 src/qemu/qemu_domainjob.h Reviewed-by: Michal Privoznik and pushed. Michal
[PATCH libvirt-dbus 1/3] src: fix double free of virDomain object
The virDomainSnapshotGetDomain() method does NOT increment the refcount on the returned virDomain, so it must not be unrefed. This double free is responsible for random failures of the test_snapshot.py tests. Signed-off-by: Daniel P. Berrangé --- src/domainsnapshot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/domainsnapshot.c b/src/domainsnapshot.c index f3571a1..78a9f9a 100644 --- a/src/domainsnapshot.c +++ b/src/domainsnapshot.c @@ -63,7 +63,7 @@ virtDBusDomainSnapshotGetParent(GVariant *inArgs, g_autoptr(virDomainSnapshot) domainSnapshot = NULL; g_autoptr(virDomainSnapshot) parent = NULL; guint flags; -g_autoptr(virDomain) domain = NULL; +virDomainPtr domain = NULL; g_autofree gchar *parentPath = NULL; g_variant_get(inArgs, "(u)", ); @@ -159,7 +159,7 @@ virtDBusDomainSnapshotListAllChildren(GVariant *inArgs, guint flags; GVariantBuilder builder; GVariant *gdomainSnapshots; -g_autoptr(virDomain) domain = NULL; +virDomainPtr domain = NULL; g_variant_get(inArgs, "(u)", ); -- 2.26.2
[PATCH libvirt-dbus 2/3] gitlab: introduce CI jobs testing git master & distro libvirt
The dbus build needs to validate one axis - A variety of libvirt versions We test a variety of libvirt versions by running a build against the distro provided libvirt packages. All that is then missing is a build against the latest libvirt git master, which only needs to be run on a single distro, for which CentOS 8 is picked as a stable long life base. RPM build and tests are skipped on CentOS 7 since it lacks the required min version of some packages. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml| 218 ++ ci/containers/libvirt-centos-7.Dockerfile | 90 ci/containers/libvirt-centos-8.Dockerfile | 68 ++ .../libvirt-centos-stream.Dockerfile | 69 ++ ci/containers/libvirt-debian-10.Dockerfile| 61 + ci/containers/libvirt-debian-9.Dockerfile | 64 + ci/containers/libvirt-debian-sid.Dockerfile | 61 + ci/containers/libvirt-fedora-31.Dockerfile| 58 + ci/containers/libvirt-fedora-32.Dockerfile| 58 + .../libvirt-fedora-rawhide.Dockerfile | 59 + ci/containers/libvirt-opensuse-151.Dockerfile | 60 + ci/containers/libvirt-ubuntu-1804.Dockerfile | 64 + ci/containers/libvirt-ubuntu-2004.Dockerfile | 61 + 13 files changed, 991 insertions(+) create mode 100644 ci/containers/libvirt-centos-7.Dockerfile create mode 100644 ci/containers/libvirt-centos-8.Dockerfile create mode 100644 ci/containers/libvirt-centos-stream.Dockerfile create mode 100644 ci/containers/libvirt-debian-10.Dockerfile create mode 100644 ci/containers/libvirt-debian-9.Dockerfile create mode 100644 ci/containers/libvirt-debian-sid.Dockerfile create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile create mode 100644 ci/containers/libvirt-opensuse-151.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-1804.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-2004.Dockerfile diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 50dae92..9b61c70 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,6 +1,99 @@ stages: - prebuild + - containers + - builds + +.container_job_template: _job_definition + image: docker:stable + stage: containers + services: +- docker:dind + before_script: +- export TAG="$CI_REGISTRY_IMAGE/ci-$NAME:latest" +- export COMMON_TAG="$CI_REGISTRY/libvirt/libvirt-dbus/ci-$NAME:latest" +- docker info +- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p "$CI_REGISTRY_PASSWORD" + script: +- docker pull "$TAG" || docker pull "$COMMON_TAG" || true +- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" -f "ci/containers/libvirt-$NAME.Dockerfile" ci +- docker push "$TAG" + after_script: +- docker logout + +.script_variables: _variables | + export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)" + export SCRATCH_DIR="/tmp/scratch" + export VROOT="$SCRATCH_DIR/vroot" + export CCACHE_DIR="$PWD/ccache" + export CCACHE_MAXSIZE="500M" + export PATH="$CCACHE_WRAPPERSDIR:$VROOT/bin:$PATH" + export PKG_CONFIG_PATH="$VROOT/lib/pkgconfig" + export XDG_DATA_DIRS="$VROOT/share:/usr/share" + export GI_TYPELIB_PATH="$VROOT/lib/girepository-1.0" + export LD_LIBRARY_PATH="$VROOT/lib" + +.git_native_build_job_template: _native_build_job_definition + image: $CI_REGISTRY_IMAGE/ci-$NAME:latest + stage: builds + cache: +paths: + - ccache/ +key: "$CI_JOB_NAME" + before_script: +- *script_variables + script: +- pushd "$PWD" +- mkdir -p "$SCRATCH_DIR" +- cd "$SCRATCH_DIR" +- git clone --depth 1 https://gitlab.com/libvirt/libvirt.git +- git clone --depth 1 https://gitlab.com/libvirt/libvirt-glib.git +- mkdir libvirt/build +- cd libvirt/build +- ../autogen.sh --prefix="$VROOT" --without-libvirtd +- $MAKE install +- cd ../.. +- mkdir libvirt-glib/build +- cd libvirt-glib/build +- ../autogen.sh --prefix="$VROOT" +- $MAKE install +- popd +- meson build --prefix="$VROOT" +- $NINJA -C build +- if test "$TESTS" != "skip"; + then +$NINJA -C build test; +$NINJA -C build install; +$NINJA -C build dist; + fi +- if test -x /usr/bin/rpmbuild && test "$RPM" != "skip"; + then +rpmbuild --nodeps -ta build/meson-dist/libvirt-dbus*.tar.xz; + fi + +.dist_native_build_job_template: _native_build_job_definition + image: $CI_REGISTRY_IMAGE/ci-$NAME:latest + stage: builds + cache: +paths: + - ccache/ +key: "$CI_JOB_NAME" + before_script: +- *script_variables + script: +- meson build --prefix="$VROOT" +- $NINJA -C build +- if test "$TESTS" != "skip"; + then +$NINJA -C build test; +$NINJA -C build install; +$NINJA -C build dist; + fi +- if test -x
[PATCH libvirt-dbus 0/3] Introduce GitLab CI and merge requests
This introduces CI build coverage and then documents the switch to use merge requests. Daniel P. Berrangé (3): src: fix double free of virDomain object gitlab: introduce CI jobs testing git master & distro libvirt gitlab: add CONTRIBUTING.rst file to indicate use of merge requests .gitlab-ci.yml| 218 ++ .gitpublish | 4 - CONTRIBUTING.rst | 28 +++ ci/containers/libvirt-centos-7.Dockerfile | 90 ci/containers/libvirt-centos-8.Dockerfile | 68 ++ .../libvirt-centos-stream.Dockerfile | 69 ++ ci/containers/libvirt-debian-10.Dockerfile| 61 + ci/containers/libvirt-debian-9.Dockerfile | 64 + ci/containers/libvirt-debian-sid.Dockerfile | 61 + ci/containers/libvirt-fedora-31.Dockerfile| 58 + ci/containers/libvirt-fedora-32.Dockerfile| 58 + .../libvirt-fedora-rawhide.Dockerfile | 59 + ci/containers/libvirt-opensuse-151.Dockerfile | 60 + ci/containers/libvirt-ubuntu-1804.Dockerfile | 64 + ci/containers/libvirt-ubuntu-2004.Dockerfile | 61 + src/domainsnapshot.c | 4 +- 16 files changed, 1021 insertions(+), 6 deletions(-) delete mode 100644 .gitpublish create mode 100644 CONTRIBUTING.rst create mode 100644 ci/containers/libvirt-centos-7.Dockerfile create mode 100644 ci/containers/libvirt-centos-8.Dockerfile create mode 100644 ci/containers/libvirt-centos-stream.Dockerfile create mode 100644 ci/containers/libvirt-debian-10.Dockerfile create mode 100644 ci/containers/libvirt-debian-9.Dockerfile create mode 100644 ci/containers/libvirt-debian-sid.Dockerfile create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile create mode 100644 ci/containers/libvirt-opensuse-151.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-1804.Dockerfile create mode 100644 ci/containers/libvirt-ubuntu-2004.Dockerfile -- 2.26.2
[PATCH libvirt-dbus 3/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
With the introduction of automated CI pipelines, we are now ready to switch to using merge requests for the project. With this switch we longer wish to have patches sent to the mailing list, and thus the git-publish config is removed. Signed-off-by: Daniel P. Berrangé --- .gitpublish | 4 CONTRIBUTING.rst | 28 2 files changed, 28 insertions(+), 4 deletions(-) delete mode 100644 .gitpublish create mode 100644 CONTRIBUTING.rst diff --git a/.gitpublish b/.gitpublish deleted file mode 100644 index d21df70..000 --- a/.gitpublish +++ /dev/null @@ -1,4 +0,0 @@ -[gitpublishprofile "default"] -base = master -to = libvir-list@redhat.com -prefix = libvirt-dbus PATCH diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst new file mode 100644 index 000..8391896 --- /dev/null +++ b/CONTRIBUTING.rst @@ -0,0 +1,28 @@ + +Contributing to libvirt-dbus + + +The libvirt DBus API binding accepts code contributions via merge requests +on the GitLab project: + +https://gitlab.com/libvirt/libvirt-dbus/-/merge_requests + +It is required that automated CI pipelines succeed before a merge request +will be accepted. The global pipeline status for the ``master`` branch is +visible at: + +https://gitlab.com/libvirt/libvirt-dbus/pipelines + +CI pipeline results for merge requests will be visible via the contributors' +own private repository fork: + +https://gitlab.com/yourusername/libvirt-dbus/pipelines + +Contributions submitted to the project must be in compliance with the +Developer Certificate of Origin Version 1.1. This is documented at: + +https://developercertificate.org/ + +To indicate compliance, each commit in a series must have a "Signed-off-by" +tag with the submitter's name and email address. This can be added by passing +the ``-s`` flag to ``git commit`` when creating the patches. -- 2.26.2
Re: [PATCH] spec: Add the build dependency of make
On 6/24/20 11:07 AM, Han Han wrote:
For some minimal OS like fedora cloud image, the make is not installed
by default.
Signed-off-by: Han Han
---
libvirt.spec.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index cd7c33ab1a..9f24e06aa4 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -269,6 +269,7 @@ BuildRequires: python36-docutils
BuildRequires: python3-docutils
%endif
BuildRequires: gcc
+BuildRequires: make
BuildRequires: git
%if 0%{?fedora} || 0%{?rhel} > 7
BuildRequires: perl-interpreter
Reviewed-by: Michal Privoznik
and pushed.
Michal
Re: [PATCH 6/6] kbase: incrementalbackupinternals: Describe 'block commit'
On 6/24/20 9:07 AM, Peter Krempa wrote: oVirt does merge images out of libvirt in some cases. Add docs outlining how it's done from a high level. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 37 +++ 1 file changed, 37 insertions(+) diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index 9a96ef6df3..e50bf52e89 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -245,3 +245,40 @@ the snapshot. continue create RECORDING bitmap named BITMAP in OVERLAY with GRANULARITY + +Commiting external snapshots manually Committing +- + +``block commit`` refers to an operation where data from a subchain of the +backing chain is merged down into the backing image of the subchain removing all +images in the subchain . Drop space before . Does oVirt care about block pull, or only block commit? But block pull can be a separate patch if we need more information; this one is useful as-is (well, with typos fixed). Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 5/6] kbase: incrementalbackupinternals: Replace bash with pseudocode
On 6/24/20 9:07 AM, Peter Krempa wrote: Simplify the docs and reduce maintenance burden by just describing the algorithm by a pseudo-language. Users are encouraged to use libvirt anyways and projects such as oVirt which do some management of storage themselves are unlikely to use bash anyways. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 44 ++- 1 file changed, 11 insertions(+), 33 deletions(-) Nice! Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 4/6] kbase: incrementalbackupinternals: Add section on 'qemu-img bitmap' use
On 6/24/20 9:07 AM, Peter Krempa wrote: Define what users should look for when wanting to manipulate bitmaps themselves. Later on a patch will turn the bash algorithms into pseudocode for simplicity. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 14 ++ 1 file changed, 14 insertions(+) diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index adc415e282..c8a0a66baa 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -190,6 +190,20 @@ It's reccomended to use ``--output=json`` parameter to work with a machine readable output rather than trying to process the human readable output by scripts. For processing JSON in shell the ``jq`` tool can be used. +``qemu-img bitmap`` command allows modification of block-dirty-bitmaps of an s/``qemu-img/The ``qemu-img/ s/an/an offline/ +image. It supports the following operations relevant to this document (see man +page for full list of operations): + +``--add NAME`` +Creates a new bitmap named ``NAME``. Optionally ``-g`` can be used to +specify granularity. + +``--remove NAME`` +Deletes bitmap ``NAME``. + +``--merge SRCBITMAP -b SRCFILE -F SRCFILEFMT DSTBITMAP`` +Merges bitmap ``SRCBITMAP`` from ``SRCFILE`` into ``DSTBITMAP``. + Checking bitmap health -- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 3/6] kbase: incrementalbackupinternals: Add secion on bitmap handling in shell
On 6/24/20 9:07 AM, Peter Krempa wrote: Add a section that outlines usage of tools to handle bitmaps and introduce terms corresponding to the output of qemu-img to be used in further sections. With this we can simplify the section about checking bitmap health as we don't have to explain the qemu-img output but can refer to the newly defined terms. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 58 --- 1 file changed, 40 insertions(+), 18 deletions(-) @@ -186,6 +174,40 @@ following rules: refcount bits: 16 corrupt: false +The ``flags`` have following meanings: + +``auto`` - **recording** + +The bitmap is automatically activated when the image is opened for wrinting writing +and thus it's actively recording writes. + +``in-use`` - **inconsistent** + +The bitmap was not properly saved when the qemu process was shut down last +time thus didn't conistently record all the changed sectors. consistently + +It's reccomended to use ``--output=json`` parameter to work with a machine recommended +readable output rather than trying to process the human readable output by +scripts. For processing JSON in shell the ``jq`` tool can be used. + +Checking bitmap health +-- + +QEMU optimizes disk writes by only updating the bitmaps in certain cases. This +also can cause problems in cases when e.g. QEMU crashes. + +For a chain of corresponding bitmaps in a backing chain images to be considered +valid and eligible for use for an incremental backup with +``virDomainBackupBegin`` the bitmaps intended to be used must conform to the +following rules: + +1) active/topmost image must contain the bitmap +2) if bitmap with the same name is contained in one of the backing images it s/if/if a/ + must be a contiguougs subchain starting from the topmost image which contains contiguous + the bitmaps (no gaps) +3) all of the above bitmaps must be marked as **recording** +4) all of the above bitmaps must not be **inconsistent** + (See also the ``qemuBlockBitmapChainIsValid`` helper method in ``src/qemu/qemu_block.c``) With fixes, Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/6] kbase: incrementalbackupinternals: Clarify language in snapshots section
On 6/24/20 9:07 AM, Peter Krempa wrote: Emphaisze what needs to happen and also that creating a snapshot doesn't Emphasize create the appropriate bitmaps. Also mention that granularity is kept. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 23 --- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index eada0d2935..dbef1e96f3 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -109,17 +109,18 @@ as ``base image``. The topmost overlay is the image which is being written to by the VM and is also described as the ``active`` layer or image. -Handling of bitmaps - -Creating an external snapshot involves adding a new layer to the backing chain -on top of the previous chain. In this step there are no new bitmaps created by -default, which would mean that backups become impossible after this step. - -To prevent this from happening we need to re-create the active bitmaps in the -new top/active layer of the backing chain which allows us to continue tracking -the changes with same granularity as before and also allows libvirt to stitch -together all the corresponding bitmaps to do a backup across snapshots. +Handling of bitmaps during snapshots + + +Creating an external snapshot involves adding a overlay on top of the previously +active image. Libvirt requires that all ``block-dirty-bitmaps`` which correspond +to the checkpoint must be created in the new overlay before any write from the +guest reaches the overlay to continue tracking which blocks are dirtied. + +Since there are no new bitmaps created by ``qemu`` or ``qemu-img`` by default +when creating an overlay, we need to re-create the appropriate (see below.) +bitmaps in the new overlay based on the previously active bitmaps in the active s/(see below.) bitmaps/bitmaps (see below)/ +image. The new bitmaps are created with the same granularity. After taking a snapshot of the ``vda`` disk from the example above placed into ``vda-2.qcow2`` the following topology will be created: With the fixes, Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 1/6] kbase: incrementalbackupinternals: Add snapshot terminology
On 6/24/20 9:07 AM, Peter Krempa wrote: Make it obvious what's meant by 'overlay' and 'backing image' for sake of extension of the document. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 15 +++ 1 file changed, 15 insertions(+) Reviewed-by: Eric Blake diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index 0c4b4f7486..eada0d2935 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -94,6 +94,21 @@ so, even if we obviously can't guarantee that. Integration with external snapshots === +External snapshot terminology +- + +External snapshots on a disk level consist of layered chains of disk images. An +image in the chain can have a ``backing image`` placed below. Any chunk in the +current image which was not written explicitly is transparent and if read the +data from the backing image is passed through. An image placed on top of the +current image is called ``overlay``. + +The bottommost backing image at the end of the chain is also usually described +as ``base image``. + +The topmost overlay is the image which is being written to by the VM and is also +described as the ``active`` layer or image. Maybe it's also worth a paragraph mentioning how diagramming the chains we typically use <- for 'Backed by', as in: Base <- Top -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH 6/6] kbase: incrementalbackupinternals: Describe 'block commit'
oVirt does merge images out of libvirt in some cases. Add docs outlining how it's done from a high level. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 37 +++ 1 file changed, 37 insertions(+) diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index 9a96ef6df3..e50bf52e89 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -245,3 +245,40 @@ the snapshot. continue create RECORDING bitmap named BITMAP in OVERLAY with GRANULARITY + +Commiting external snapshots manually +- + +``block commit`` refers to an operation where data from a subchain of the +backing chain is merged down into the backing image of the subchain removing all +images in the subchain . + +``COMMIT_TOP`` refers to the top of the subchain to merge into ``COMMIT_BASE`` +(which stays in the new chain). + +It's strongly advised to use ``virDomainBlockCommit`` API in libvirt directly if +possible. Inactive VMs can be started with ``VIR_DOMAIN_START_PAUSED`` flag +(``virsh start --paused``) to prevent OS from running. + +Otherwise the following pseudo-algorithm can be used: + +Note: A ``valid`` bitmap chain is a set of images containing bitmaps which +conform to the rules about valid bitmaps mentioned above. + +:: + +commit data from COMMIT_TOP to COMMIT_BASE + +let BITMAPS = valid bitmap chains in COMMIT_TOP + +for each BITMAP in BITMAPS +let GRANULARITY = granularity of BITMAP in ACTIVE + +if BITMAP is not present in COMMIT_BASE: +create RECORDING bitmap named BITMAP in COMMIT_BASE with GRANULARITY + +for each IMAGE between COMMIT_TOP(inclusive) and COMMIT_BASE(exclusive): +if BITMAP is not present in IMAGE: +break + +merge BITMAP in IMAGE into BITMAP in COMMIT_BASE -- 2.26.2
[PATCH 5/6] kbase: incrementalbackupinternals: Replace bash with pseudocode
Simplify the docs and reduce maintenance burden by just describing the algorithm by a pseudo-language. Users are encouraged to use libvirt anyways and projects such as oVirt which do some management of storage themselves are unlikely to use bash anyways. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 44 ++- 1 file changed, 11 insertions(+), 33 deletions(-) diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index c8a0a66baa..9a96ef6df3 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -228,42 +228,20 @@ following rules: Creating external snapshots manually -- -To create the same topology outside of libvirt (e.g when doing snapshots offline) -a new ``qemu-img`` which supports the ``bitmap`` subcommand is recommended. The -following algorithm then ensures that the new image after snapshot will work -with backups (note that ``jq`` is a JSON processor): +To create the same topology outside of libvirt (e.g when doing snapshots +offline) the following pseudo-algorithm ensures that the new image after +snapshot will work with backups. ``OVERLAY`` corresponds to the new overlay +image, ``ACTIVE`` corresponds to the topmost image of the active chain prior to +the snapshot. :: - #!/bin/bash +create image OVERLAY on top of ACTIVE - # arguments - SNAP_IMG="vda-2.qcow2" - BACKING_IMG="vda-1.qcow2" +for each BITMAP in ACTIVE: +let GRANULARITY = granularity of BITMAP in ACTIVE - # constants - snapshots and bitmaps work only with qcow2 - SNAP_FMT="qcow2" - BACKING_IMG_FMT="qcow2" +if BITMAP isn't RECORDING or is INCONSISTENT: +continue - # create snapshot overlay - qemu-img create -f "$SNAP_FMT" -F "$BACKING_IMG_FMT" -b "$BACKING_IMG" "$SNAP_IMG" - - BACKING_IMG_INFO=$(qemu-img info --output=json -f "$BACKING_IMG_FMT" "$BACKING_IMG") - BACKING_BITMAPS=$(jq '."format-specific".data.bitmaps' <<< "$BACKING_IMG_INFO") - - if [ "x$BACKING_BITMAPS" = "xnull" ]; then - exit 0 - fi - - for BACKING_BITMAP_ in $(jq -c '.[]' <<< "$BACKING_BITMAPS"); do - BITMAP_FLAGS=$(jq -c -r '.flags[]' <<< "$BACKING_BITMAP_") - BITMAP_NAME=$(jq -r '.name' <<< "$BACKING_BITMAP_") - - if grep 'in-use' <<< "$BITMAP_FLAGS" || - grep -v 'auto' <<< "$BITMAP_FLAGS"; then - continue - fi - - qemu-img bitmap -f "$SNAP_FMT" "$SNAP_IMG" --add "$BITMAP_NAME" - - done +create RECORDING bitmap named BITMAP in OVERLAY with GRANULARITY -- 2.26.2
[PATCH 0/6] kbase: Improve incremental backup docs
Refactor some sections, get rid of bash in favor of pseudo-code and describe block-commit. Peter Krempa (6): kbase: incrementalbackupinternals: Add snapshot terminology kbase: incrementalbackupinternals: Clarify language in snapshots section kbase: incrementalbackupinternals: Add secion on bitmap handling in shell kbase: incrementalbackupinternals: Add section on 'qemu-img bitmap' use kbase: incrementalbackupinternals: Replace bash with pseudocode kbase: incrementalbackupinternals: Describe 'block commit' docs/kbase/incrementalbackupinternals.rst | 175 +++--- 1 file changed, 121 insertions(+), 54 deletions(-) -- 2.26.2
[PATCH 4/6] kbase: incrementalbackupinternals: Add section on 'qemu-img bitmap' use
Define what users should look for when wanting to manipulate bitmaps themselves. Later on a patch will turn the bash algorithms into pseudocode for simplicity. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 14 ++ 1 file changed, 14 insertions(+) diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index adc415e282..c8a0a66baa 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -190,6 +190,20 @@ It's reccomended to use ``--output=json`` parameter to work with a machine readable output rather than trying to process the human readable output by scripts. For processing JSON in shell the ``jq`` tool can be used. +``qemu-img bitmap`` command allows modification of block-dirty-bitmaps of an +image. It supports the following operations relevant to this document (see man +page for full list of operations): + +``--add NAME`` +Creates a new bitmap named ``NAME``. Optionally ``-g`` can be used to +specify granularity. + +``--remove NAME`` +Deletes bitmap ``NAME``. + +``--merge SRCBITMAP -b SRCFILE -F SRCFILEFMT DSTBITMAP`` +Merges bitmap ``SRCBITMAP`` from ``SRCFILE`` into ``DSTBITMAP``. + Checking bitmap health -- -- 2.26.2
[PATCH 3/6] kbase: incrementalbackupinternals: Add secion on bitmap handling in shell
Add a section that outlines usage of tools to handle bitmaps and introduce terms corresponding to the output of qemu-img to be used in further sections. With this we can simplify the section about checking bitmap health as we don't have to explain the qemu-img output but can refer to the newly defined terms. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 58 --- 1 file changed, 40 insertions(+), 18 deletions(-) diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index dbef1e96f3..adc415e282 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -139,29 +139,17 @@ After taking a snapshot of the ``vda`` disk from the example above placed into |||| ++++ -Checking bitmap health --- +Manipulating bitmaps in shell +- -QEMU optimizes disk writes by only updating the bitmaps in certain cases. This -also can cause problems in cases when e.g. QEMU crashes. +**NOTE:** Any of the examples expect that the full image chain isn't used by any +running VM at the time. -For a chain of corresponding bitmaps in a backing chain to be considered valid -and eligible for use with ``virDomainBackupBegin`` it must conform to the -following rules: - -1) Top image must contain the bitmap -2) If any of the backing images in the chain contain the bitmap too, all - contiguous images must have the bitmap (no gaps) -3) all of the above bitmaps must be marked as active - (``auto`` flag in ``qemu-img`` output, ``recording`` in qemu) -4) none of the above bitmaps can be inconsistent - (``in-use`` flag in ``qemu-img`` provided that it's not used on image which - is currently in use by a qemu instance, or ``inconsistent`` in qemu) +``qemu-img info`` command reports information about dirty bitmaps in an image: :: - # check that image has bitmaps - $ qemu-img info vda-1.qcow2 + $ qemu-img info -f qcow2 vda-1.qcow2 image: vda-1.qcow2 file format: qcow2 virtual size: 100 MiB (104857600 bytes) @@ -186,6 +174,40 @@ following rules: refcount bits: 16 corrupt: false +The ``flags`` have following meanings: + +``auto`` - **recording** + +The bitmap is automatically activated when the image is opened for wrinting +and thus it's actively recording writes. + +``in-use`` - **inconsistent** + +The bitmap was not properly saved when the qemu process was shut down last +time thus didn't conistently record all the changed sectors. + +It's reccomended to use ``--output=json`` parameter to work with a machine +readable output rather than trying to process the human readable output by +scripts. For processing JSON in shell the ``jq`` tool can be used. + +Checking bitmap health +-- + +QEMU optimizes disk writes by only updating the bitmaps in certain cases. This +also can cause problems in cases when e.g. QEMU crashes. + +For a chain of corresponding bitmaps in a backing chain images to be considered +valid and eligible for use for an incremental backup with +``virDomainBackupBegin`` the bitmaps intended to be used must conform to the +following rules: + +1) active/topmost image must contain the bitmap +2) if bitmap with the same name is contained in one of the backing images it + must be a contiguougs subchain starting from the topmost image which contains + the bitmaps (no gaps) +3) all of the above bitmaps must be marked as **recording** +4) all of the above bitmaps must not be **inconsistent** + (See also the ``qemuBlockBitmapChainIsValid`` helper method in ``src/qemu/qemu_block.c``) -- 2.26.2
[PATCH 2/6] kbase: incrementalbackupinternals: Clarify language in snapshots section
Emphaisze what needs to happen and also that creating a snapshot doesn't create the appropriate bitmaps. Also mention that granularity is kept. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 23 --- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index eada0d2935..dbef1e96f3 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -109,17 +109,18 @@ as ``base image``. The topmost overlay is the image which is being written to by the VM and is also described as the ``active`` layer or image. -Handling of bitmaps - -Creating an external snapshot involves adding a new layer to the backing chain -on top of the previous chain. In this step there are no new bitmaps created by -default, which would mean that backups become impossible after this step. - -To prevent this from happening we need to re-create the active bitmaps in the -new top/active layer of the backing chain which allows us to continue tracking -the changes with same granularity as before and also allows libvirt to stitch -together all the corresponding bitmaps to do a backup across snapshots. +Handling of bitmaps during snapshots + + +Creating an external snapshot involves adding a overlay on top of the previously +active image. Libvirt requires that all ``block-dirty-bitmaps`` which correspond +to the checkpoint must be created in the new overlay before any write from the +guest reaches the overlay to continue tracking which blocks are dirtied. + +Since there are no new bitmaps created by ``qemu`` or ``qemu-img`` by default +when creating an overlay, we need to re-create the appropriate (see below.) +bitmaps in the new overlay based on the previously active bitmaps in the active +image. The new bitmaps are created with the same granularity. After taking a snapshot of the ``vda`` disk from the example above placed into ``vda-2.qcow2`` the following topology will be created: -- 2.26.2
[PATCH 1/6] kbase: incrementalbackupinternals: Add snapshot terminology
Make it obvious what's meant by 'overlay' and 'backing image' for sake of extension of the document. Signed-off-by: Peter Krempa --- docs/kbase/incrementalbackupinternals.rst | 15 +++ 1 file changed, 15 insertions(+) diff --git a/docs/kbase/incrementalbackupinternals.rst b/docs/kbase/incrementalbackupinternals.rst index 0c4b4f7486..eada0d2935 100644 --- a/docs/kbase/incrementalbackupinternals.rst +++ b/docs/kbase/incrementalbackupinternals.rst @@ -94,6 +94,21 @@ so, even if we obviously can't guarantee that. Integration with external snapshots === +External snapshot terminology +- + +External snapshots on a disk level consist of layered chains of disk images. An +image in the chain can have a ``backing image`` placed below. Any chunk in the +current image which was not written explicitly is transparent and if read the +data from the backing image is passed through. An image placed on top of the +current image is called ``overlay``. + +The bottommost backing image at the end of the chain is also usually described +as ``base image``. + +The topmost overlay is the image which is being written to by the VM and is also +described as the ``active`` layer or image. + Handling of bitmaps --- -- 2.26.2
Re: [PATCH 3/3] leaseshelper: Report more errors
On Mon, Jun 15, 2020 at 01:32:36PM +0200, Michal Privoznik wrote: > Some functions or code paths that may fail don't report error > (e.g. when acquiring PID file fails) leading to a silent quit > of the leaseshelper. This makes it super hard for us and users > to debug what is happening. Fortunately, dnsmasq captures both > stdout and stderr so we can write an error message there. > > Signed-off-by: Michal Privoznik > --- > src/network/leaseshelper.c | 18 ++ > 1 file changed, 14 insertions(+), 4 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 2/3] leaseshelper: Wait to acquire PID file
On Mon, Jun 15, 2020 at 01:32:35PM +0200, Michal Privoznik wrote: > On a DHCP transaction, dnsmasq runs our leases helper which > updates corresponding JSON files. While one dnsmasq won't run the > leaseshelper in parallel, two dnsmasqs (from two distinct > networks) might. To avoid corrupting JSON file, the leaseshelper > acquires PID file first. Well, the way it's acquiring it is not > ideal - it calls virPidFileAcquirePath(wait = false); which > means, that either it acquires the PID file instantly or returns > an error and does not touch the JSON at all. This in turn means > that there might be a leases record missing. With wait = true, > this won't happen. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1840307 > > Signed-off-by: Michal Privoznik > --- > src/network/leaseshelper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c > index 02759f2314..c4258cae4e 100644 > --- a/src/network/leaseshelper.c > +++ b/src/network/leaseshelper.c > @@ -160,7 +160,7 @@ main(int argc, char **argv) > pid_file = g_strdup(RUNSTATEDIR "/leaseshelper.pid"); > > /* Try to claim the pidfile, exiting if we can't */ > -if ((pid_file_fd = virPidFileAcquirePath(pid_file, false, getpid())) < 0) > +if ((pid_file_fd = virPidFileAcquirePath(pid_file, true, getpid())) < 0) > goto cleanup; > > /* Since interfaces can be hot plugged, we need to make sure that the 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 0/3] leaseshelper: Wait to acquire PID file
On 6/15/20 1:32 PM, Michal Privoznik wrote: Patch 1/3 is for demonstration purposes only. The rest patches actual problem.I was also thinking of making the PID file per bridge, i.e. include bridge name in the name (e.g. /var/run/virbr0_leaseshelper.pid) to decrease pressure on the single PID file. However, DHCP transactions are not that frequent IMO so it's not really a bottle neck nor performance issue. Michal Prívozník (3): *** DO NOT APPLY UPSTREAM *** leaseshelper: Wait to acquire PID file leaseshelper: Report more errors src/network/leaseshelper.c | 20 1 file changed, 16 insertions(+), 4 deletions(-) Ping? We are getting close to the freeze and while this is a bugfix and could go in, we are getting close to the release too. Michal
[PATCH 05/13] qemuBuildMachineCommandLine: Drop needless check
The machine can not be NULL at this point - qemuDomainDefPostParse() makes sure it isn't. Signed-off-by: Michal Privoznik --- src/qemu/qemu_command.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 05e5c19118..c5b0ee231e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6722,13 +6722,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; size_t i; -/* This should *never* be NULL, since we always provide - * a machine in the capabilities data for QEMU. So this - * check is just here as a safety in case the unexpected - * happens */ -if (!def->os.machine) -return 0; - virCommandAddArg(cmd, "-machine"); virBufferAdd(, def->os.machine, -1); -- 2.26.2
[PATCH 13/13] news: Document HMAT addition
Signed-off-by: Michal Privoznik --- NEWS.rst | 5 + 1 file changed, 5 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 2fc43c31b9..8990cdaf61 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -47,6 +47,11 @@ v6.5.0 (unreleased) alphabetical order. Hook script in old place will be executed as first for backward compatibility. + * Allow configuring of NUMA HMAT + +Libvirt allows configuring Heterogeneous Memory Attribute Table to hint +software running inside the guest on optimization. + * **Improvements** * **Bug fixes** -- 2.26.2
[PATCH 06/13] numa_conf: Make virDomainNumaSetNodeCpumask() return void
There is only one caller of virDomainNumaSetNodeCpumask() which
checks for the return value but because the function will return
NULL iff the @cpumask was NULL in the first place. But in that
place @cpumask can't be NULL because it was just allocated by
virBitmapParse().
Signed-off-by: Michal Privoznik
---
src/conf/numa_conf.c | 4 +---
src/conf/numa_conf.h | 6 +++---
src/libxl/xen_xl.c | 4 ++--
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index 2685a3e828..7b9084456e 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1317,14 +1317,12 @@ virDomainNumaGetNodeCpumask(virDomainNumaPtr numa,
}
-virBitmapPtr
+void
virDomainNumaSetNodeCpumask(virDomainNumaPtr numa,
size_t node,
virBitmapPtr cpumask)
{
numa->mem_nodes[node].cpumask = cpumask;
-
-return numa->mem_nodes[node].cpumask;
}
diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
index 206dffe304..25e896826b 100644
--- a/src/conf/numa_conf.h
+++ b/src/conf/numa_conf.h
@@ -155,9 +155,9 @@ size_t virDomainNumaSetNodeDistanceCount(virDomainNumaPtr
numa,
size_t ndistances)
ATTRIBUTE_NONNULL(1);
-virBitmapPtr virDomainNumaSetNodeCpumask(virDomainNumaPtr numa,
- size_t node,
- virBitmapPtr cpumask)
+void virDomainNumaSetNodeCpumask(virDomainNumaPtr numa,
+ size_t node,
+ virBitmapPtr cpumask)
ATTRIBUTE_NONNULL(1);
/*
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index d40c2e1d8e..1c1279e1cb 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -508,10 +508,10 @@ xenParseXLVnuma(virConfPtr conf,
goto cleanup;
}
-if ((virBitmapParse(vtoken, ,
VIR_DOMAIN_CPUMASK_LEN) < 0) ||
-(virDomainNumaSetNodeCpumask(numa, vnodeCnt,
cpumask) == NULL))
+if (virBitmapParse(vtoken, ,
VIR_DOMAIN_CPUMASK_LEN) < 0)
goto cleanup;
+virDomainNumaSetNodeCpumask(numa, vnodeCnt, cpumask);
vcpus += virBitmapCountBits(cpumask);
} else if (STRPREFIX(str, "vdistances")) {
--
2.26.2
[PATCH 04/13] qemu_command: Rename qemuBuildNumaArgStr()
The function doesn't just build the argument for -numa. Since the
-numa can be repeated multiple times, it also puts -numa onto the
cmd line. Also, the rest of the functions has 'Command' infix.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_command.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f355ddbfd5..05e5c19118 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7122,10 +7122,10 @@ qemuBuildIOThreadCommandLine(virCommandPtr cmd,
static int
-qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg,
-virDomainDefPtr def,
-virCommandPtr cmd,
-qemuDomainObjPrivatePtr priv)
+qemuBuildNumaCommandLine(virQEMUDriverConfigPtr cfg,
+ virDomainDefPtr def,
+ virCommandPtr cmd,
+ qemuDomainObjPrivatePtr priv)
{
size_t i, j;
virQEMUCapsPtr qemuCaps = priv->qemuCaps;
@@ -9725,7 +9725,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
return NULL;
if (virDomainNumaGetNodeCount(def->numa) &&
-qemuBuildNumaArgStr(cfg, def, cmd, priv) < 0)
+qemuBuildNumaCommandLine(cfg, def, cmd, priv) < 0)
return NULL;
if (qemuBuildMemoryDeviceCommandLine(cmd, cfg, def, priv) < 0)
--
2.26.2
[PATCH 09/13] conf: Validate NUMA HMAT configuration
There are several restrictions, for instance @initiator and
@target have to refer to existing NUMA nodes (daa), @cache has to
refer to a defined cache level and so on.
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c | 3 ++
src/conf/numa_conf.c | 99 ++
src/conf/numa_conf.h | 1 +
3 files changed, 103 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ce75831037..0d73968221 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7321,6 +7321,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
if (virDomainDefCputuneValidate(def) < 0)
return -1;
+if (virDomainNumaDefValidate(def->numa) < 0)
+return -1;
+
return 0;
}
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index f510dfebce..bf8442eada 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1363,6 +1363,105 @@ virDomainNumaDefFormatXML(virBufferPtr buf,
}
+int
+virDomainNumaDefValidate(const virDomainNuma *def)
+{
+size_t i;
+size_t j;
+
+if (!def)
+return 0;
+
+for (i = 0; i < def->nmem_nodes; i++) {
+const virDomainNumaNode *node = >mem_nodes[i];
+g_autoptr(virBitmap) levelsSeen = virBitmapNewEmpty();
+
+for (j = 0; j < node->ncaches; j++) {
+const virDomainNumaCache *cache = >caches[j];
+
+/* Relax this if there's ever fourth layer of cache */
+if (cache->level > 3) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Ain't nobody heard of that much cache
level"));
+return -1;
+}
+
+if (virBitmapIsBitSet(levelsSeen, cache->level)) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("Cache level '%u' already defined"),
+ cache->level);
+return -1;
+}
+
+if (virBitmapSetBitExpand(levelsSeen, cache->level))
+return -1;
+}
+}
+
+for (i = 0; i < def->nlatencies; i++) {
+const virDomainNumaLatency *l = >latencies[i];
+
+if (l->initiator >= def->nmem_nodes) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("'initiator' refers to a non-existent NUMA
node"));
+return -1;
+}
+
+if (l->target >= def->nmem_nodes) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("'target' refers to a non-existent NUMA node"));
+return -1;
+}
+
+if (!def->mem_nodes[l->initiator].cpumask) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("NUMA nodes without CPUs can't be initiator"));
+return -1;
+}
+
+if (l->cache > 0) {
+for (j = 0; j < def->mem_nodes[l->target].ncaches; j++) {
+const virDomainNumaCache *cache =
def->mem_nodes[l->target].caches;
+
+if (l->cache == cache->level)
+break;
+}
+
+if (j == def->mem_nodes[l->target].ncaches) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("'cache' refers to a non-existent NUMA node
cache"));
+return -1;
+}
+}
+
+for (j = 0; j < i; j++) {
+virDomainNumaLatencyPtr ll = >latencies[j];
+
+if (l->type == ll->type &&
+l->initiator == ll->initiator &&
+l->target == ll->target &&
+l->cache == ll->cache &&
+l->accessType == ll->accessType) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Duplicate info for NUMA latencies"));
+return -1;
+}
+
+
+if (l->initiator != l->target &&
+l->initiator == ll->target &&
+l->target == ll->initiator) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Link already defined"));
+return -1;
+}
+}
+}
+
+return 0;
+}
+
+
unsigned int
virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa)
{
diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
index a7dc3190d3..6d6e9de551 100644
--- a/src/conf/numa_conf.h
+++ b/src/conf/numa_conf.h
@@ -216,6 +216,7 @@ bool virDomainNumatuneNodeSpecified(virDomainNumaPtr
numatune,
int virDomainNumaDefParseXML(virDomainNumaPtr def, xmlXPathContextPtr ctxt);
int virDomainNumaDefFormatXML(virBufferPtr buf, virDomainNumaPtr def);
+int virDomainNumaDefValidate(const virDomainNuma *def);
unsigned int virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa);
--
2.26.2
[PATCH 10/13] numa: expose HMAT APIs
These APIs will be used by QEMU driver when building the command
line.
Signed-off-by: Michal Privoznik
---
src/conf/numa_conf.c | 139 +++
src/conf/numa_conf.h | 28
src/libvirt_private.syms | 6 ++
3 files changed, 173 insertions(+)
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index bf8442eada..b35e5040fc 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1829,3 +1829,142 @@ virDomainNumaFillCPUsInNode(virDomainNumaPtr numa,
return 0;
}
+
+
+bool
+virDomainNumaHasHMAT(const virDomainNuma *numa)
+{
+size_t i;
+
+if (!numa)
+return false;
+
+if (numa->nlatencies)
+return true;
+
+for (i = 0; i < numa->nmem_nodes; i++) {
+if (numa->mem_nodes[i].ncaches)
+return true;
+}
+
+return false;
+}
+
+
+size_t
+virDomainNumaGetNodeCacheCount(const virDomainNuma *numa,
+ size_t node)
+{
+if (!numa || node >= numa->nmem_nodes)
+return 0;
+
+return numa->mem_nodes[node].ncaches;
+}
+
+
+int
+virDomainNumaGetNodeCache(const virDomainNuma *numa,
+ size_t node,
+ size_t cache,
+ unsigned int *level,
+ unsigned int *size,
+ unsigned int *line,
+ virDomainCacheAssociativity *associativity,
+ virDomainCachePolicy *policy)
+{
+const virDomainNumaNode *cell;
+
+if (!numa || node >= numa->nmem_nodes)
+return -1;
+
+cell = >mem_nodes[node];
+
+if (cache >= cell->ncaches)
+return -1;
+
+*level = cell->caches[cache].level;
+*size = cell->caches[cache].size;
+*line = cell->caches[cache].line;
+*associativity = cell->caches[cache].associativity;
+*policy = cell->caches[cache].policy;
+return 0;
+}
+
+
+ssize_t
+virDomainNumaGetNodeInitiator(const virDomainNuma *numa,
+ size_t node)
+{
+size_t i;
+unsigned int maxBandwidth = 0;
+ssize_t candidateBandwidth = -1;
+unsigned int minLatency = UINT_MAX;
+ssize_t candidateLatency = -1;
+
+if (!numa || node >= numa->nmem_nodes)
+return -1;
+
+for (i = 0; i < numa->nlatencies; i++) {
+const virDomainNumaLatency *l = >latencies[i];
+
+if (l->target != node)
+continue;
+
+switch (l->type) {
+case VIR_DOMAIN_NUMA_LATENCY_TYPE_LATENCY:
+if (l->value < minLatency) {
+minLatency = l->value;
+candidateLatency = l->initiator;
+}
+break;
+
+case VIR_DOMAIN_NUMA_LATENCY_TYPE_BANDWIDTH:
+if (l->value > maxBandwidth) {
+maxBandwidth = l->value;
+candidateBandwidth = l->initiator;
+}
+break;
+}
+}
+
+if (candidateLatency >= 0)
+return candidateLatency;
+
+return candidateBandwidth;
+}
+
+
+size_t
+virDomainNumaGetLatenciesCount(const virDomainNuma *numa)
+{
+if (!numa)
+return 0;
+
+return numa->nlatencies;
+}
+
+
+int
+virDomainNumaGetLatency(const virDomainNuma *numa,
+size_t i,
+virDomainNumaLatencyType *type,
+unsigned int *initiator,
+unsigned int *target,
+unsigned int *cache,
+virDomainMemoryLatency *accessType,
+unsigned long *value)
+{
+const virDomainNumaLatency *l;
+
+if (!numa || i >= numa->nlatencies)
+return -1;
+
+l = >latencies[i];
+*type = l->type;
+*initiator = l->initiator;
+*target = l->target;
+*cache = l->cache;
+*accessType = l->accessType;
+*value = l->value;
+return 0;
+}
diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h
index 6d6e9de551..b150fe4037 100644
--- a/src/conf/numa_conf.h
+++ b/src/conf/numa_conf.h
@@ -222,3 +222,31 @@ unsigned int
virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa);
int virDomainNumaFillCPUsInNode(virDomainNumaPtr numa, size_t node,
unsigned int maxCpus);
+
+bool virDomainNumaHasHMAT(const virDomainNuma *numa);
+
+size_t virDomainNumaGetNodeCacheCount(const virDomainNuma *numa,
+ size_t node);
+
+int virDomainNumaGetNodeCache(const virDomainNuma *numa,
+ size_t node,
+ size_t cache,
+ unsigned int *level,
+ unsigned int *size,
+ unsigned int *line,
+ virDomainCacheAssociativity *associativity,
+ virDomainCachePolicy *policy);
+
+ssize_t virDomainNumaGetNodeInitiator(const virDomainNuma *numa,
+ size_t node);
+
[PATCH 12/13] qemu: Build HMAT command line
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1786303
Signed-off-by: Michal Privoznik
---
src/conf/numa_conf.c | 7 +
src/qemu/qemu_command.c | 183 ++
.../numatune-hmat.x86_64-latest.args | 53 +
tests/qemuxml2argvtest.c | 1 +
4 files changed, 244 insertions(+)
create mode 100644 tests/qemuxml2argvdata/numatune-hmat.x86_64-latest.args
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index b35e5040fc..7fbbe0f52e 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1904,6 +1904,13 @@ virDomainNumaGetNodeInitiator(const virDomainNuma *numa,
if (!numa || node >= numa->nmem_nodes)
return -1;
+/* A NUMA node which has at least one vCPU is initiator to itself by
+ * definition. */
+if (numa->mem_nodes[node].cpumask)
+return node;
+
+/* For the rest, "NUMA node that has best performance (the lowest
+ * latency or largest bandwidth) to this NUMA node." */
for (i = 0; i < numa->nlatencies; i++) {
const virDomainNumaLatency *l = >latencies[i];
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 628ff970bd..b28c43f110 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6932,6 +6932,16 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
virBufferAsprintf(, ",pflash1=%s", priv->pflash1->nodeformat);
}
+if (virDomainNumaHasHMAT(def->numa)) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NUMA_HMAT)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("HMAT is not supported with this QEMU"));
+return -1;
+}
+
+virBufferAddLit(, ",hmat=on");
+}
+
virCommandAddArgBuffer(cmd, );
return 0;
@@ -7114,6 +7124,134 @@ qemuBuildIOThreadCommandLine(virCommandPtr cmd,
}
+static int
+qemuBuilNumaCellCache(virCommandPtr cmd,
+ const virDomainDef *def,
+ size_t cell)
+{
+size_t ncaches = virDomainNumaGetNodeCacheCount(def->numa, cell);
+size_t i;
+
+if (ncaches == 0)
+return 0;
+
+for (i = 0; i < ncaches; i++) {
+g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+unsigned int level;
+unsigned int size;
+unsigned int line;
+virDomainCacheAssociativity associativity;
+virDomainCachePolicy policy;
+
+if (virDomainNumaGetNodeCache(def->numa, cell, i,
+ , , ,
+ , ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to format NUMA node cache"));
+return -1;
+}
+
+virBufferAsprintf(,
+ "hmat-cache,node-id=%zu,size=%uK,level=%u",
+ cell, size, level);
+
+switch (associativity) {
+case VIR_DOMAIN_CACHE_ASSOCIATIVITY_NONE:
+virBufferAddLit(, ",associativity=none");
+break;
+case VIR_DOMAIN_CACHE_ASSOCIATIVITY_DIRECT:
+virBufferAddLit(, ",associativity=direct");
+break;
+case VIR_DOMAIN_CACHE_ASSOCIATIVITY_FULL:
+virBufferAddLit(, ",associativity=complex");
+break;
+case VIR_DOMAIN_CACHE_ASSOCIATIVITY_LAST:
+break;
+}
+
+switch (policy) {
+case VIR_DOMAIN_CACHE_POLICY_NONE:
+virBufferAddLit(, ",policy=none");
+break;
+case VIR_DOMAIN_CACHE_POLICY_WRITEBACK:
+virBufferAddLit(, ",policy=write-back");
+break;
+case VIR_DOMAIN_CACHE_POLICY_WRITETHROUGH:
+virBufferAddLit(, ",policy=write-through");
+break;
+case VIR_DOMAIN_CACHE_POLICY_LAST:
+break;
+}
+
+if (line > 0)
+virBufferAsprintf(, ",line=%u", line);
+
+virCommandAddArg(cmd, "-numa");
+virCommandAddArgBuffer(cmd, );
+}
+
+return 0;
+}
+
+
+VIR_ENUM_DECL(qemuDomainMemoryHierarchy);
+VIR_ENUM_IMPL(qemuDomainMemoryHierarchy,
+ 4, /* Maximum level of cache */
+ "memory", /* Special case, whole memory not specific cache */
+ "first-level",
+ "second-level",
+ "third-level");
+
+static int
+qemuBuildNumaHMATCommandLine(virCommandPtr cmd,
+ const virDomainDef *def)
+{
+size_t nlatencies;
+size_t i;
+
+if (!def->numa)
+return 0;
+
+nlatencies = virDomainNumaGetLatenciesCount(def->numa);
+for (i = 0; i < nlatencies; i++) {
+g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+virDomainNumaLatencyType type;
+unsigned int initiator;
+unsigned int target;
+unsigned int cache;
+virDomainMemoryLatency accessType;
+unsigned long value;
+const char *hierarchyStr;
+
[PATCH 08/13] conf: Parse and format HMAT
To cite ACPI specification: Heterogeneous Memory Attribute Table describes the memory attributes, such as memory side cache attributes and bandwidth and latency details, related to the System Physical Address (SPA) Memory Ranges. The software is expected to use this information as hint for optimization. According to our upstream discussion [1] this is exposed under as under NUMA and or under numa/latencies. 1: https://www.redhat.com/archives/libvir-list/2020-January/msg00422.html Signed-off-by: Michal Privoznik --- docs/formatdomain.html.in | 88 ++ docs/schemas/cputypes.rng | 110 ++- src/conf/numa_conf.c | 350 - src/conf/numa_conf.h | 33 ++ src/libvirt_private.syms | 6 + tests/qemuxml2argvdata/numatune-hmat.xml | 52 +++ tests/qemuxml2xmloutdata/numatune-hmat.xml | 1 + tests/qemuxml2xmltest.c| 1 + 8 files changed, 625 insertions(+), 16 deletions(-) create mode 100644 tests/qemuxml2argvdata/numatune-hmat.xml create mode 12 tests/qemuxml2xmloutdata/numatune-hmat.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 07dcca57f5..78b2d2828d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1931,6 +1931,94 @@ using 10 for local and 20 for remote distances. +Heterogeneous Memory Attribute Table + + +... +cpu + ... + numa +cell id='0' cpus='0-3' memory='512000' unit='KiB' discard='yes'/ +cell id='1' cpus='4-7' memory='512000' unit='KiB' memAccess='shared'/ +cell id='3' cpus='0-3' memory='2097152' unit='KiB' + cache level='1' associativity='direct' policy='writeback' +size value='10' unit='KiB'/ +line value='8' unit='B'/ + /cache +/cell +latencies + latency initiator='0' target='0' type='access' value='5'/ + latency initiator='0' target='0' cache='1' type='access' value='10'/ + bandwidth initiator='0' target='0' type='access' value='204800' unit='KiB'/ +/latencies + /numa + ... +/cpu +... + + + Since 6.5.0 the cell element can + have cache child element which describes memory side cache + for memory proximity domains. The cache element has + level attribute describing the cache level and thus the + element can be repeated multiple times to describe different levels of + the cache. + + + + The cache element then has associativity + attribute (accepted values are none, direct and + full) describing the cache associativity, and + policy attribute (accepted values are none, + writeback and writethrough) describing cache + write associativity. The element has two mandatory child elements then: + size and line which describe cache size and + cache line size. Both elements accept two attributes: value + and unit which set the value of corresponding cache + attribute. + + + + The NUMA description has optional latencies element that + describes the normalized memory read/write latency, read/write bandwidth + between Initiator Proximity Domains (Processor or I/O) and Target + Proximity Domains (Memory). + + + + The latencies element can have zero or more + latency child elements to describe latency between two + memory nodes and zero or more bandwidth child elements to + describe bandwidth between two memory nodes. Both these have the + following mandatory attributes: + + + + initiator + Refers to the source NUMA node + + target + Refers to the target NUMA node + + type + The type of the access. Accepted values: access, + read, write + + value + The actual value. For latency this is delay in nanoseconds, for + bandwidth this value is in kibibytes per second. Use additional + unit attribute to change the units. + + + + To describe latency from one NUMA node to a cache of another NUMA node + the latency element has optional cache + attribute which in combination with target attribute creates + full reference to distant NUMA node's cache level. For instance, + target='0' cache='1' refers to the first level cache of NUMA + node 0. + + Events configuration diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng index a1682a1003..70c455cfa9 100644 --- a/docs/schemas/cputypes.rng +++ b/docs/schemas/cputypes.rng @@ -102,9 +102,14 @@ - - - + + + + + + + + @@ -148,6 +153,9 @@ + + + @@ -162,6 +170,102 @@ + + + + + + + + none + direct + full + + + + + none
[PATCH 07/13] Allow NUMA nodes without vCPUs
QEMU allows creating NUMA nodes that have memory only.
These are somehow important for HMAT.
With check done in qemuValidateDomainDef() for QEMU 2.7 or newer,
we can be sure that the vCPUs are fully assigned to NUMA nodes in
domain XML.
Signed-off-by: Michal Privoznik
---
docs/formatdomain.html.in | 2 +
docs/schemas/cputypes.rng | 8 ++-
src/conf/numa_conf.c | 59 ++-
src/libxl/xen_xl.c| 10 ++--
src/qemu/qemu_command.c | 26
src/qemu/qemu_validate.c | 22 +++
tests/qemuxml2argvdata/numatune-no-vcpu.args | 33 +++
tests/qemuxml2argvdata/numatune-no-vcpu.xml | 42 +
tests/qemuxml2argvtest.c | 1 +
tests/qemuxml2xmloutdata/numatune-no-vcpu.xml | 1 +
tests/qemuxml2xmltest.c | 1 +
11 files changed, 149 insertions(+), 56 deletions(-)
create mode 100644 tests/qemuxml2argvdata/numatune-no-vcpu.args
create mode 100644 tests/qemuxml2argvdata/numatune-no-vcpu.xml
create mode 12 tests/qemuxml2xmloutdata/numatune-no-vcpu.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index bd662727d3..07dcca57f5 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1840,6 +1840,8 @@
consistent across qemu and libvirt versions.
memory specifies the node memory
in kibibytes (i.e. blocks of 1024 bytes).
+ Since 6.6.0 the cpus attribute
+ is optional and if omitted a CPU-less NUMA node is created.
Since 1.2.11 one can use an additional unit attribute to
define units in which memory is specified.
diff --git a/docs/schemas/cputypes.rng b/docs/schemas/cputypes.rng
index e2744acad3..a1682a1003 100644
--- a/docs/schemas/cputypes.rng
+++ b/docs/schemas/cputypes.rng
@@ -115,9 +115,11 @@
-
-
-
+
+
+
+
+
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index 7b9084456e..7cf62ce7da 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -889,32 +889,28 @@ virDomainNumaDefParseXML(virDomainNumaPtr def,
}
VIR_FREE(tmp);
-if (def->mem_nodes[cur_cell].cpumask) {
+if (def->mem_nodes[cur_cell].mem) {
virReportError(VIR_ERR_XML_ERROR,
_("Duplicate NUMA cell info for cell id '%u'"),
cur_cell);
goto cleanup;
}
-if (!(tmp = virXMLPropString(nodes[i], "cpus"))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Missing 'cpus' attribute in NUMA cell"));
-goto cleanup;
-}
+if ((tmp = virXMLPropString(nodes[i], "cpus"))) {
+g_autoptr(virBitmap) cpumask = NULL;
-if (virBitmapParse(tmp, >mem_nodes[cur_cell].cpumask,
- VIR_DOMAIN_CPUMASK_LEN) < 0)
-goto cleanup;
+if (virBitmapParse(tmp, , VIR_DOMAIN_CPUMASK_LEN) < 0)
+goto cleanup;
-if (virBitmapIsAllClear(def->mem_nodes[cur_cell].cpumask)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("NUMA cell %d has no vCPUs assigned"), cur_cell);
-goto cleanup;
+if (!virBitmapIsAllClear(cpumask))
+def->mem_nodes[cur_cell].cpumask = g_steal_pointer();
+VIR_FREE(tmp);
}
-VIR_FREE(tmp);
for (j = 0; j < n; j++) {
-if (j == cur_cell || !def->mem_nodes[j].cpumask)
+if (j == cur_cell ||
+!def->mem_nodes[j].cpumask ||
+!def->mem_nodes[cur_cell].cpumask)
continue;
if (virBitmapOverlaps(def->mem_nodes[j].cpumask,
@@ -975,7 +971,6 @@ virDomainNumaDefFormatXML(virBufferPtr buf,
{
virDomainMemoryAccess memAccess;
virTristateBool discard;
-char *cpustr;
size_t ncells = virDomainNumaGetNodeCount(def);
size_t i;
@@ -985,17 +980,22 @@ virDomainNumaDefFormatXML(virBufferPtr buf,
virBufferAddLit(buf, "\n");
virBufferAdjustIndent(buf, 2);
for (i = 0; i < ncells; i++) {
+virBitmapPtr cpumask = virDomainNumaGetNodeCpumask(def, i);
int ndistances;
memAccess = virDomainNumaGetNodeMemoryAccessMode(def, i);
discard = virDomainNumaGetNodeDiscard(def, i);
-if (!(cpustr = virBitmapFormat(virDomainNumaGetNodeCpumask(def, i
-return -1;
-
virBufferAddLit(buf, "\n");
}
-
-VIR_FREE(cpustr);
}
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "\n");
@@ -1047,8 +1045,12 @@ virDomainNumaGetCPUCountTotal(virDomainNumaPtr numa)
size_t i;
unsigned int ret = 0;
-for (i = 0; i < numa->nmem_nodes; i++)
-ret +=
