[libvirt PATCH 6/9] esx: reorder code to avoid need to VIR_FREE mimeType
mimeType is initialized to NULL, and then only set in one place, just
before a check (not involving mimeType) that then VIR_FREEs mimeType
if it fails. If we just reorder the code to do the check prior to
setting mimeType, then there won't be any need to VIR_FREE(mimeType)
on failure (because it will already be empty/NULL).
Signed-off-by: Laine Stump
---
src/esx/esx_driver.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 47873c0d54..2d010096a5 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -2374,12 +2374,10 @@ esxDomainScreenshot(virDomainPtr domain, virStreamPtr
stream,
url = virBufferContentAndReset(&buffer);
-mimeType = g_strdup("image/png");
-
-if (esxStreamOpenDownload(stream, priv, url, 0, 0) < 0) {
-VIR_FREE(mimeType);
+if (esxStreamOpenDownload(stream, priv, url, 0, 0) < 0)
goto cleanup;
-}
+
+mimeType = g_strdup("image/png");
cleanup:
--
2.29.2
[libvirt PATCH 8/9] esx: eliminate unnecessary cleanup: labels and result variables
switching to g_autofree left many cleanup: sections empty.
Signed-off-by: Laine Stump
---
src/esx/esx_driver.c | 22 ++-
src/esx/esx_storage_backend_vmfs.c | 22 +--
src/esx/esx_util.c | 8 ++
src/esx/esx_vi.c | 45 +-
src/esx/esx_vi_types.c | 8 ++
5 files changed, 38 insertions(+), 67 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index fe98f90ea9..e49975de86 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -690,7 +690,6 @@ esxConnectToVCenter(esxPrivate *priv,
const char *hostname,
const char *hostSystemIPAddress)
{
-int result = -1;
g_autofree char *ipAddress = NULL;
g_autofree char *username = NULL;
g_autofree char *password = NULL;
@@ -711,11 +710,11 @@ esxConnectToVCenter(esxPrivate *priv,
} else {
if (!(username = virAuthGetUsername(conn, auth, "esx", "administrator",
hostname)))
-goto cleanup;
+return -1;
}
if (!(password = virAuthGetPassword(conn, auth, "esx", username,
hostname)))
-goto cleanup;
+return -1;
url = g_strdup_printf("%s://%s:%d/sdk", priv->parsedUri->transport,
hostname,
conn->uri->port);
@@ -723,7 +722,7 @@ esxConnectToVCenter(esxPrivate *priv,
if (esxVI_Context_Alloc(&priv->vCenter) < 0 ||
esxVI_Context_Connect(priv->vCenter, url, ipAddress, username,
password, priv->parsedUri) < 0) {
-goto cleanup;
+return -1;
}
if (priv->vCenter->productLine != esxVI_ProductLine_VPX) {
@@ -732,25 +731,20 @@ esxConnectToVCenter(esxPrivate *priv,
hostname,
esxVI_ProductLineToDisplayName(esxVI_ProductLine_VPX),
esxVI_ProductLineToDisplayName(priv->vCenter->productLine));
-goto cleanup;
+return -1;
}
if (hostSystemIPAddress) {
-if (esxVI_Context_LookupManagedObjectsByHostSystemIp
- (priv->vCenter, hostSystemIPAddress) < 0) {
-goto cleanup;
-}
+if (esxVI_Context_LookupManagedObjectsByHostSystemIp(priv->vCenter,
hostSystemIPAddress) < 0)
+return -1;
} else {
if (esxVI_Context_LookupManagedObjectsByPath(priv->vCenter,
priv->parsedUri->path) <
0) {
-goto cleanup;
+return -1;
}
}
-result = 0;
-
- cleanup:
-return result;
+return 0;
}
diff --git a/src/esx/esx_storage_backend_vmfs.c
b/src/esx/esx_storage_backend_vmfs.c
index 95505d6796..cb2be59a33 100644
--- a/src/esx/esx_storage_backend_vmfs.c
+++ b/src/esx/esx_storage_backend_vmfs.c
@@ -667,7 +667,6 @@ static virStorageVolPtr
esxStorageVolLookupByName(virStoragePoolPtr pool,
const char *name)
{
-virStorageVolPtr volume = NULL;
esxPrivate *priv = pool->conn->privateData;
g_autofree char *datastorePath = NULL;
g_autofree char *key = NULL;
@@ -676,14 +675,11 @@ esxStorageVolLookupByName(virStoragePoolPtr pool,
if (esxVI_LookupStorageVolumeKeyByDatastorePath(priv->primary,
datastorePath, &key) < 0) {
-goto cleanup;
+return NULL;
}
-volume = virGetStorageVol(pool->conn, pool->name, name, key,
- &esxStorageBackendVMFS, NULL);
-
- cleanup:
-return volume;
+return virGetStorageVol(pool->conn, pool->name, name, key,
+&esxStorageBackendVMFS, NULL);
}
@@ -691,7 +687,6 @@ esxStorageVolLookupByName(virStoragePoolPtr pool,
static virStorageVolPtr
esxStorageVolLookupByPath(virConnectPtr conn, const char *path)
{
-virStorageVolPtr volume = NULL;
esxPrivate *priv = conn->privateData;
g_autofree char *datastoreName = NULL;
g_autofree char *directoryAndFileName = NULL;
@@ -699,19 +694,16 @@ esxStorageVolLookupByPath(virConnectPtr conn, const char
*path)
if (esxUtil_ParseDatastorePath(path, &datastoreName, NULL,
&directoryAndFileName) < 0) {
-goto cleanup;
+return NULL;
}
if (esxVI_LookupStorageVolumeKeyByDatastorePath(priv->primary, path,
&key) < 0) {
-goto cleanup;
+return NULL;
}
-volume = virGetStorageVol(conn, datastoreName, directoryAndFileName, key,
- &esxStorageBackendVMFS, NULL);
-
- cleanup:
-return volume;
+return virGetStorageVol(conn, datastoreName, directoryAndFileName, key,
+&esxStorageBackendVMFS, NULL);
}
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
index f1e8339fe0..ef070a4f04 100644
--
[libvirt PATCH 9/9] esx: replace some VIR_FREE with g_clear_pointer(x, g_free)
These are all cases when 1) the pointer is passed by reference from
the caller (ie.e. **) and expects it to be NULL on return if there is
an error, or 2) the variable holding the pointer is being checked or
re-used in the same function, but not right away.
Signed-off-by: Laine Stump
---
src/esx/esx_network_driver.c | 2 +-
src/esx/esx_util.c | 8
src/esx/esx_vi.c | 4 ++--
src/esx/esx_vi_types.c | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/esx/esx_network_driver.c b/src/esx/esx_network_driver.c
index b489f4de8a..4d0fba8c9f 100644
--- a/src/esx/esx_network_driver.c
+++ b/src/esx/esx_network_driver.c
@@ -914,7 +914,7 @@ esxConnectListAllNetworks(virConnectPtr conn,
if (nets && *nets) {
for (i = 0; i < count; ++i)
g_free((*nets)[i]);
-VIR_FREE(*nets);
+g_clear_pointer(nets, g_free);
}
}
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
index ef070a4f04..24e1c73ec4 100644
--- a/src/esx/esx_util.c
+++ b/src/esx/esx_util.c
@@ -95,7 +95,7 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri)
/* Expected format: [://][:] */
(*parsedUri)->proxy = true;
(*parsedUri)->proxy_type = CURLPROXY_HTTP;
-VIR_FREE((*parsedUri)->proxy_hostname);
+g_clear_pointer(&(*parsedUri)->proxy_hostname, g_free);
(*parsedUri)->proxy_port = 1080;
if ((tmp = STRSKIP(queryParam->value, "http://";))) {
@@ -261,13 +261,13 @@ esxUtil_ParseDatastorePath(const char *datastorePath,
char **datastoreName,
cleanup:
if (result < 0) {
if (datastoreName)
-VIR_FREE(*datastoreName);
+g_clear_pointer(datastoreName, g_free);
if (directoryName)
-VIR_FREE(*directoryName);
+g_clear_pointer(directoryName, g_free);
if (directoryAndFileName)
-VIR_FREE(*directoryAndFileName);
+g_clear_pointer(directoryAndFileName, g_free);
}
return result;
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index db5035c035..e1c1a15ab6 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -73,7 +73,7 @@ VIR_LOG_INIT("esx.esx_vi");
\
_body \
\
-VIR_FREE(*ptrptr); \
+g_clear_pointer(ptrptr, g_free); \
}
@@ -2516,7 +2516,7 @@ esxVI_GetVirtualMachineIdentity(esxVI_ObjectContent
*virtualMachine,
failure:
if (name)
-VIR_FREE(*name);
+g_clear_pointer(name, g_free);
return -1;
}
diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c
index 4dc7c30680..59735194ae 100644
--- a/src/esx/esx_vi_types.c
+++ b/src/esx/esx_vi_types.c
@@ -67,7 +67,7 @@ VIR_LOG_INIT("esx.esx_vi_types");
\
_body \
\
-VIR_FREE(*ptrptr); \
+g_clear_pointer(ptrptr, g_free); \
}
--
2.29.2
[libvirt PATCH 1/9] esx: use g_autofree for char* where it is trivially possible
All of these strings are allocated once, freed once, and are never
returned out of the function where they are created, used, and are
freed.
Signed-off-by: Laine Stump
---
src/esx/esx_driver.c | 128 +
src/esx/esx_storage_backend_vmfs.c | 102 ---
src/esx/esx_stream.c | 7 +-
src/esx/esx_util.c | 11 +--
src/esx/esx_vi.c | 53
src/esx/esx_vi_methods.c | 3 +-
src/esx/esx_vi_types.c | 8 +-
7 files changed, 93 insertions(+), 219 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 0271f81a56..df257341b8 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -139,8 +139,8 @@ esxParseVMXFileName(const char *fileName,
char *datastoreName;
char *tmp;
char *saveptr;
-char *strippedFileName = NULL;
-char *copyOfFileName = NULL;
+g_autofree char *strippedFileName = NULL;
+g_autofree char *copyOfFileName = NULL;
char *directoryAndFileName;
int ret = -1;
@@ -253,8 +253,6 @@ esxParseVMXFileName(const char *fileName,
esxVI_String_Free(&propertyNameList);
esxVI_ObjectContent_Free(&datastoreList);
esxVI_DatastoreHostMount_Free(&hostMount);
-VIR_FREE(strippedFileName);
-VIR_FREE(copyOfFileName);
return ret;
}
@@ -280,8 +278,8 @@ esxFormatVMXFileName(const char *fileName, void *opaque)
bool success = false;
char *result = NULL;
esxVMX_Data *data = opaque;
-char *datastoreName = NULL;
-char *directoryAndFileName = NULL;
+g_autofree char *datastoreName = NULL;
+g_autofree char *directoryAndFileName = NULL;
esxVI_ObjectContent *datastore = NULL;
esxVI_DatastoreHostMount *hostMount = NULL;
char separator = '/';
@@ -349,8 +347,6 @@ esxFormatVMXFileName(const char *fileName, void *opaque)
if (! success)
VIR_FREE(result);
-VIR_FREE(datastoreName);
-VIR_FREE(directoryAndFileName);
esxVI_ObjectContent_Free(&datastore);
esxVI_DatastoreHostMount_Free(&hostMount);
@@ -613,9 +609,9 @@ esxConnectToHost(esxPrivate *priv,
{
int result = -1;
g_autofree char *ipAddress = NULL;
-char *username = NULL;
-char *password = NULL;
-char *url = NULL;
+g_autofree char *username = NULL;
+g_autofree char *password = NULL;
+g_autofree char *url = NULL;
esxVI_String *propertyNameList = NULL;
esxVI_ObjectContent *hostSystem = NULL;
esxVI_Boolean inMaintenanceMode = esxVI_Boolean_Undefined;
@@ -683,9 +679,6 @@ esxConnectToHost(esxPrivate *priv,
result = 0;
cleanup:
-VIR_FREE(username);
-VIR_FREE(password);
-VIR_FREE(url);
esxVI_String_Free(&propertyNameList);
esxVI_ObjectContent_Free(&hostSystem);
@@ -703,9 +696,9 @@ esxConnectToVCenter(esxPrivate *priv,
{
int result = -1;
g_autofree char *ipAddress = NULL;
-char *username = NULL;
-char *password = NULL;
-char *url = NULL;
+g_autofree char *username = NULL;
+g_autofree char *password = NULL;
+g_autofree char *url = NULL;
if (!hostSystemIPAddress &&
(!priv->parsedUri->path || STREQ(priv->parsedUri->path, "/"))) {
@@ -761,10 +754,6 @@ esxConnectToVCenter(esxPrivate *priv,
result = 0;
cleanup:
-VIR_FREE(username);
-VIR_FREE(password);
-VIR_FREE(url);
-
return result;
}
@@ -822,7 +811,7 @@ esxConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
{
virDrvOpenStatus result = VIR_DRV_OPEN_ERROR;
esxPrivate *priv = NULL;
-char *potentialVCenterIPAddress = NULL;
+g_autofree char *potentialVCenterIPAddress = NULL;
g_autofree char *vCenterIPAddress = NULL;
virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
@@ -938,8 +927,6 @@ esxConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
cleanup:
esxFreePrivate(&priv);
-VIR_FREE(potentialVCenterIPAddress);
-
return result;
}
@@ -1472,7 +1459,7 @@ esxDomainLookupByUUID(virConnectPtr conn, const unsigned
char *uuid)
esxVI_ObjectContent *virtualMachine = NULL;
esxVI_VirtualMachinePowerState powerState;
int id = -1;
-char *name = NULL;
+g_autofree char *name = NULL;
virDomainPtr domain = NULL;
if (esxVI_EnsureSession(priv->primary) < 0)
@@ -1498,8 +1485,6 @@ esxDomainLookupByUUID(virConnectPtr conn, const unsigned
char *uuid)
cleanup:
esxVI_String_Free(&propertyNameList);
esxVI_ObjectContent_Free(&virtualMachine);
-VIR_FREE(name);
-
return domain;
}
@@ -1559,7 +1544,7 @@ esxDomainSuspend(virDomainPtr domain)
esxVI_VirtualMachinePowerState powerState;
esxVI_ManagedObjectReference *task = NULL;
esxVI_TaskInfoState taskInfoState;
-char *taskInfoErrorMessage = NULL;
+g_autofree char *taskInfoErrorMessage = NULL;
if (esxVI_EnsureSession(priv->primary) < 0)
return -1;
@@ -1599,8 +1584,6 @@ esxDomainSuspend(virDomainPtr domain)
[libvirt PATCH 4/9] esx: switch VIR_FREE->g_free in esx*Free*()
Although the three functions esxFreePrivate(), esxFreeStreamPrivate(), and esxUtil_FreeParsedUri() are calling VIR_FREE on *object, and so in theory the caller of the function might rely on "object" (the free function's arg) being set to NULL, in practice these functions are only called from a couple places each, and in all cases the pointer that is passed is a local variable, and goes out of scope almost immediately after calling the Free function, so it is safe to change VIR_FREE() into g_free(). Signed-off-by: Laine Stump --- src/esx/esx_driver.c | 2 +- src/esx/esx_stream.c | 4 ++-- src/esx/esx_util.c | 10 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 8afec464dd..952e769376 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -71,7 +71,7 @@ esxFreePrivate(esxPrivate **priv) esxUtil_FreeParsedUri(&(*priv)->parsedUri); virObjectUnref((*priv)->caps); virObjectUnref((*priv)->xmlopt); -VIR_FREE(*priv); +g_free(*priv); } diff --git a/src/esx/esx_stream.c b/src/esx/esx_stream.c index cc48c182d9..131fbc100b 100644 --- a/src/esx/esx_stream.c +++ b/src/esx/esx_stream.c @@ -336,8 +336,8 @@ esxFreeStreamPrivate(esxStreamPrivate **priv) return; esxVI_CURL_Free(&(*priv)->curl); -VIR_FREE((*priv)->backlog); -VIR_FREE(*priv); +g_free((*priv)->backlog); +g_free(*priv); } static int diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 64a2c968f0..e9b74f386f 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -171,12 +171,12 @@ esxUtil_FreeParsedUri(esxUtil_ParsedUri **parsedUri) if (!parsedUri || !(*parsedUri)) return; -VIR_FREE((*parsedUri)->transport); -VIR_FREE((*parsedUri)->vCenter); -VIR_FREE((*parsedUri)->proxy_hostname); -VIR_FREE((*parsedUri)->path); +g_free((*parsedUri)->transport); +g_free((*parsedUri)->vCenter); +g_free((*parsedUri)->proxy_hostname); +g_free((*parsedUri)->path); -VIR_FREE(*parsedUri); +g_free(*parsedUri); } -- 2.29.2
[libvirt PATCH 7/9] esx: switch VIR_FREE->g_free when the pointer will immediately go out of scope
Or when it will be immediately have a new value assigned to it.
Signed-off-by: Laine Stump
---
src/esx/esx_driver.c| 4 ++--
src/esx/esx_network_driver.c| 2 +-
src/esx/esx_storage_backend_iscsi.c | 4 ++--
src/esx/esx_storage_backend_vmfs.c | 4 ++--
src/esx/esx_util.c | 4 ++--
src/esx/esx_vi.c| 4 ++--
6 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 2d010096a5..fe98f90ea9 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -2619,7 +2619,7 @@ esxDomainGetXMLDesc(virDomainPtr domain, unsigned int
flags)
cleanup:
esxVI_String_Free(&propertyNameList);
esxVI_ObjectContent_Free(&virtualMachine);
-VIR_FREE(data.datastorePathWithoutFileName);
+g_free(data.datastorePathWithoutFileName);
virDomainDefFree(def);
return xml;
@@ -4946,7 +4946,7 @@ esxConnectListAllDomains(virConnectPtr conn,
for (id = 0; id < count; id++)
virObjectUnref(doms[id]);
-VIR_FREE(doms);
+g_free(doms);
}
esxVI_AutoStartDefaults_Free(&autoStartDefaults);
diff --git a/src/esx/esx_network_driver.c b/src/esx/esx_network_driver.c
index 15fc7931c0..b489f4de8a 100644
--- a/src/esx/esx_network_driver.c
+++ b/src/esx/esx_network_driver.c
@@ -913,7 +913,7 @@ esxConnectListAllNetworks(virConnectPtr conn,
if (ret < 0) {
if (nets && *nets) {
for (i = 0; i < count; ++i)
-VIR_FREE((*nets)[i]);
+g_free((*nets)[i]);
VIR_FREE(*nets);
}
}
diff --git a/src/esx/esx_storage_backend_iscsi.c
b/src/esx/esx_storage_backend_iscsi.c
index 41bb9f6094..d89b5a4ba8 100644
--- a/src/esx/esx_storage_backend_iscsi.c
+++ b/src/esx/esx_storage_backend_iscsi.c
@@ -348,7 +348,7 @@ esxStoragePoolGetXMLDesc(virStoragePoolPtr pool, unsigned
int flags)
xml = virStoragePoolDefFormat(&def);
cleanup:
-VIR_FREE(def.source.hosts);
+g_free(def.source.hosts);
esxVI_HostInternetScsiHba_Free(&hostInternetScsiHba);
return xml;
@@ -727,7 +727,7 @@ esxStorageVolGetXMLDesc(virStorageVolPtr volume,
cleanup:
esxVI_ScsiLun_Free(&scsiLunList);
-VIR_FREE(def.key);
+g_free(def.key);
return xml;
}
diff --git a/src/esx/esx_storage_backend_vmfs.c
b/src/esx/esx_storage_backend_vmfs.c
index 225b2a4751..95505d6796 100644
--- a/src/esx/esx_storage_backend_vmfs.c
+++ b/src/esx/esx_storage_backend_vmfs.c
@@ -544,7 +544,7 @@ esxStoragePoolGetXMLDesc(virStoragePoolPtr pool, unsigned
int flags)
xml = virStoragePoolDefFormat(&def);
cleanup:
-VIR_FREE(def.source.hosts);
+g_free(def.source.hosts);
esxVI_String_Free(&propertyNameList);
esxVI_ObjectContent_Free(&datastore);
esxVI_DatastoreHostMount_Free(&hostMount);
@@ -1390,7 +1390,7 @@ esxStorageVolGetXMLDesc(virStorageVolPtr volume,
cleanup:
esxVI_FileInfo_Free(&fileInfo);
-VIR_FREE(def.key);
+g_free(def.key);
return xml;
}
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
index e9b74f386f..f1e8339fe0 100644
--- a/src/esx/esx_util.c
+++ b/src/esx/esx_util.c
@@ -55,7 +55,7 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri)
virURIParamPtr queryParam = &uri->params[i];
if (STRCASEEQ(queryParam->name, "transport")) {
-VIR_FREE((*parsedUri)->transport);
+g_free((*parsedUri)->transport);
(*parsedUri)->transport = g_strdup(queryParam->value);
@@ -68,7 +68,7 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri)
goto cleanup;
}
} else if (STRCASEEQ(queryParam->name, "vcenter")) {
-VIR_FREE((*parsedUri)->vCenter);
+g_free((*parsedUri)->vCenter);
(*parsedUri)->vCenter = g_strdup(queryParam->value);
} else if (STRCASEEQ(queryParam->name, "no_verify")) {
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index 987259be4b..2a999f1cc1 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -4280,7 +4280,7 @@ esxVI_WaitForTaskCompletion(esxVI_Context *ctx,
if (esxVI_WaitForUpdates(ctx, version, &updateSet) < 0)
goto cleanup;
-VIR_FREE(version);
+g_free(version);
version = g_strdup(updateSet->version);
if (!updateSet->filterSet)
@@ -4355,7 +4355,7 @@ esxVI_WaitForTaskCompletion(esxVI_Context *ctx,
esxVI_PropertyFilterSpec_Free(&propertyFilterSpec);
esxVI_ManagedObjectReference_Free(&propertyFilter);
-VIR_FREE(version);
+g_free(version);
esxVI_UpdateSet_Free(&updateSet);
esxVI_TaskInfo_Free(&taskInfo);
--
2.29.2
[libvirt PATCH 5/9] esx: use g_steal_pointer+g_autofree on return value
If we put the potential return string into the g_autofreed tmpResult,
and the move it to the returned "result" only as a final step ater, we
can avoid the need to explicitly VIR_FREE (or g_free) on failure.
Signed-off-by: Laine Stump
---
src/esx/esx_driver.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 952e769376..47873c0d54 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -275,7 +275,7 @@ esxParseVMXFileName(const char *fileName,
static char *
esxFormatVMXFileName(const char *fileName, void *opaque)
{
-bool success = false;
+g_autofree char *tmpResult = NULL;
char *result = NULL;
esxVMX_Data *data = opaque;
g_autofree char *datastoreName = NULL;
@@ -329,10 +329,10 @@ esxFormatVMXFileName(const char *fileName, void *opaque)
virBufferAddChar(&buffer, separator);
virBufferAdd(&buffer, directoryAndFileName, -1);
-result = virBufferContentAndReset(&buffer);
+tmpResult = virBufferContentAndReset(&buffer);
} else if (*fileName == '/') {
/* FIXME: need to deal with Windows paths here too */
-result = g_strdup(fileName);
+tmpResult = g_strdup(fileName);
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not handle file name '%s'"), fileName);
@@ -341,15 +341,11 @@ esxFormatVMXFileName(const char *fileName, void *opaque)
/* FIXME: Check if referenced path/file really exists */
-success = true;
+result = g_steal_pointer(&tmpResult);
cleanup:
-if (! success)
-VIR_FREE(result);
-
esxVI_ObjectContent_Free(&datastore);
esxVI_DatastoreHostMount_Free(&hostMount);
-
return result;
}
--
2.29.2
[libvirt PATCH 0/9] eliminate (almost) all VIR_FREE in esx directory
142 more down, 3084 to go :-) This goes through all the standard methods of eliminating VIR_FREE - first converting as many pointers as possible to g_autofree, then converting a few more *Free* functions (that were more questionable than previous Frees) to use g_free, then some trivial refactoring. And finally at the end a few stragglers that really do need the pointers cleared were changed to g_clear_pointer(x, g_free). A couple of issues: 1) There are two definitions of a macro called ESX_VI__TEMPLATE__FREE that generate functions with names in the form of "esxVI_.*_Free". These generated functions were doing "VIR_FREE(*ptrptr)" at the end; because of the "*", the caller's pointer would be cleared out (not just the local copy). There are something over 400 calls to these functions, and all but 10 or so that I audited will never reference the pointer after return from esxVI_Blah_Free(). But there are several that do. Since I'm unfamiliar with this code and don't want to risk breaking it, I opted to just use g_clear_pointer(ptrptr, g_free), thus preserving current behavior exactly. 2) There are still 7 instances of VIR_FREE in the esx directory. All 7 of them are in loops that are clearing out an array of names prior to returning failure, and from a quick glance it looks like there are at least a few places where it is important to clear the array entries. But rather than brute force convert to using g_clear_pointer in the loop, I figured someone may come up with an elegant translation to use GArray or GPtrArray instead, so I'm leaving them for now. Laine Stump (9): esx: use g_autofree for char* where it is trivially possible esx: use g_autofree when made possible by reducing scope esx: fix memory leak by switching to g_autofree esx: switch VIR_FREE->g_free in esx*Free*() esx: use g_steal_pointer+g_autofree on return value esx: reorder code to avoid need to VIR_FREE mimeType esx: switch VIR_FREE->g_free when the pointer will immediately go out of scope esx: eliminate unnecessary cleanup: labels and result variables esx: replace some VIR_FREE with g_clear_pointer(x, g_free) src/esx/esx_driver.c| 189 +--- src/esx/esx_network_driver.c| 4 +- src/esx/esx_storage_backend_iscsi.c | 16 +-- src/esx/esx_storage_backend_vmfs.c | 150 +++--- src/esx/esx_stream.c| 11 +- src/esx/esx_util.c | 41 +++--- src/esx/esx_vi.c| 111 ++-- src/esx/esx_vi_methods.c| 3 +- src/esx/esx_vi_types.c | 18 +-- 9 files changed, 179 insertions(+), 364 deletions(-) -- 2.29.2
[libvirt PATCH 2/9] esx: use g_autofree when made possible by reducing scope
These strings were being VIR_FREEd multiple times because they were
defined at the top of a function, but then set each time through a
loop. But they are only used inside that loop, so they can be
converted to use g_autofree if their definition is also placed inside
that loop.
Signed-off-by: Laine Stump
---
src/esx/esx_driver.c| 13 -
src/esx/esx_storage_backend_iscsi.c | 12
src/esx/esx_storage_backend_vmfs.c | 18 --
src/esx/esx_vi.c| 5 +
4 files changed, 13 insertions(+), 35 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index df257341b8..8afec464dd 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -1391,7 +1391,6 @@ esxDomainLookupByID(virConnectPtr conn, int id)
esxVI_ObjectContent *virtualMachine = NULL;
esxVI_VirtualMachinePowerState powerState;
int id_candidate = -1;
-char *name_candidate = NULL;
unsigned char uuid_candidate[VIR_UUID_BUFLEN];
virDomainPtr domain = NULL;
@@ -1410,6 +1409,8 @@ esxDomainLookupByID(virConnectPtr conn, int id)
for (virtualMachine = virtualMachineList; virtualMachine;
virtualMachine = virtualMachine->_next) {
+g_autofree char *name_candidate = NULL;
+
if (esxVI_GetVirtualMachinePowerState(virtualMachine,
&powerState) < 0) {
goto cleanup;
@@ -1419,8 +1420,6 @@ esxDomainLookupByID(virConnectPtr conn, int id)
if (powerState == esxVI_VirtualMachinePowerState_PoweredOff)
continue;
-VIR_FREE(name_candidate);
-
if (esxVI_GetVirtualMachineIdentity(virtualMachine,
&id_candidate, &name_candidate,
uuid_candidate) < 0) {
@@ -1444,8 +1443,6 @@ esxDomainLookupByID(virConnectPtr conn, int id)
cleanup:
esxVI_String_Free(&propertyNameList);
esxVI_ObjectContent_Free(&virtualMachineList);
-VIR_FREE(name_candidate);
-
return domain;
}
@@ -4754,7 +4751,6 @@ esxConnectListAllDomains(virConnectPtr conn,
esxVI_AutoStartPowerInfo *powerInfoList = NULL;
esxVI_AutoStartPowerInfo *powerInfo = NULL;
esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL;
-char *name = NULL;
int id;
unsigned char uuid[VIR_UUID_BUFLEN];
int count = 0;
@@ -4839,9 +4835,9 @@ esxConnectListAllDomains(virConnectPtr conn,
for (virtualMachine = virtualMachineList; virtualMachine;
virtualMachine = virtualMachine->_next) {
-if (needIdentity) {
-VIR_FREE(name);
+g_autofree char *name = NULL;
+if (needIdentity) {
if (esxVI_GetVirtualMachineIdentity(virtualMachine, &id,
&name, uuid) < 0) {
goto cleanup;
@@ -4959,7 +4955,6 @@ esxConnectListAllDomains(virConnectPtr conn,
VIR_FREE(doms);
}
-VIR_FREE(name);
esxVI_AutoStartDefaults_Free(&autoStartDefaults);
esxVI_AutoStartPowerInfo_Free(&powerInfoList);
esxVI_String_Free(&propertyNameList);
diff --git a/src/esx/esx_storage_backend_iscsi.c
b/src/esx/esx_storage_backend_iscsi.c
index 4d16ba2520..41bb9f6094 100644
--- a/src/esx/esx_storage_backend_iscsi.c
+++ b/src/esx/esx_storage_backend_iscsi.c
@@ -494,7 +494,6 @@ esxStorageVolLookupByPath(virConnectPtr conn, const char
*path)
esxVI_ScsiLun *scsiLunList = NULL;
esxVI_ScsiLun *scsiLun;
esxVI_HostScsiDisk *hostScsiDisk = NULL;
-char *poolName = NULL;
/* VIR_CRYPTO_HASH_SIZE_MD5 = VIR_UUID_BUFLEN = 16 */
unsigned char md5[VIR_CRYPTO_HASH_SIZE_MD5];
char uuid_string[VIR_UUID_STRING_BUFLEN] = "";
@@ -503,11 +502,12 @@ esxStorageVolLookupByPath(virConnectPtr conn, const char
*path)
goto cleanup;
for (scsiLun = scsiLunList; scsiLun; scsiLun = scsiLun->_next) {
+g_autofree char *poolName = NULL;
+
hostScsiDisk = esxVI_HostScsiDisk_DynamicCast(scsiLun);
if (hostScsiDisk && STREQ(hostScsiDisk->devicePath, path)) {
/* Found matching device */
-VIR_FREE(poolName);
if (esxVI_LookupStoragePoolNameByScsiLunKey(priv->primary,
hostScsiDisk->key,
@@ -527,8 +527,6 @@ esxStorageVolLookupByPath(virConnectPtr conn, const char
*path)
cleanup:
esxVI_ScsiLun_Free(&scsiLunList);
-VIR_FREE(poolName);
-
return volume;
}
@@ -539,7 +537,6 @@ esxStorageVolLookupByKey(virConnectPtr conn, const char
*key)
{
virStorageVolPtr volume = NULL;
esxPrivate *priv = conn->privateData;
-char *poolName = NULL;
esxVI_ScsiLun *scsiLunList = NULL;
esxVI_ScsiLun *scsiLun;
/* VIR_CRYPTO_HASH_SIZE_MD5 = VIR_UUID_BUFLEN = 16 */
@@ -555,6 +552,8 @@ esxStorageVolLookupByKey(virConnectPtr conn, const char
*key)
for (scsiLun = scsiLunList; scsiLun;
[libvirt PATCH 3/9] esx: fix memory leak by switching to g_autofree
volumeName was defined at the top of the function, then a new string
was assigned to it each time through a loop, but after the first
iteration of the loop, the previous string wasn't freed before
allocating a new string the next time. By reducing the scope of
volumeName to be just the loop, and making it g_autofree, we eliminate
the leak.
Signed-off-by: Laine Stump
---
src/esx/esx_storage_backend_vmfs.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/esx/esx_storage_backend_vmfs.c
b/src/esx/esx_storage_backend_vmfs.c
index 63959ec237..225b2a4751 100644
--- a/src/esx/esx_storage_backend_vmfs.c
+++ b/src/esx/esx_storage_backend_vmfs.c
@@ -728,7 +728,6 @@ esxStorageVolLookupByKey(virConnectPtr conn, const char
*key)
esxVI_HostDatastoreBrowserSearchResults *searchResultsList = NULL;
esxVI_HostDatastoreBrowserSearchResults *searchResults = NULL;
size_t length;
-char *volumeName = NULL;
esxVI_FileInfo *fileInfo = NULL;
char key_candidate[VIR_UUID_STRING_BUFLEN] = "";
@@ -789,6 +788,7 @@ esxStorageVolLookupByKey(virConnectPtr conn, const char
*key)
/* Build datastore path and query the UUID */
for (fileInfo = searchResults->file; fileInfo;
fileInfo = fileInfo->_next) {
+g_autofree char *volumeName = NULL;
g_autofree char *datastorePath = NULL;
g_autofree char *uuid_string = NULL;
@@ -831,8 +831,6 @@ esxStorageVolLookupByKey(virConnectPtr conn, const char
*key)
esxVI_String_Free(&propertyNameList);
esxVI_ObjectContent_Free(&datastoreList);
esxVI_HostDatastoreBrowserSearchResults_Free(&searchResultsList);
-VIR_FREE(volumeName);
-
return volume;
}
--
2.29.2
Re: [libvirt PATCH v2 5/6] qemu: implement virDomainGetMessages API
On 2/9/21 11:01 AM, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé
> ---
> src/conf/domain_conf.c | 17
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_domain.h | 3 +++
> src/qemu/qemu_driver.c | 58
> 5 files changed, 81 insertions(+)
>
[...]
> +static int
> +qemuDomainGetMessages(virDomainPtr dom,
> + char ***msgs,
> + unsigned int flags)
> +{
> +virDomainObjPtr vm = NULL;
> +int rv = -1;
> +size_t i, n;
> +int nmsgs;
> +
> +virCheckFlags(VIR_DOMAIN_MESSAGE_DEPRECATION |
> + VIR_DOMAIN_MESSAGE_TAINTING, -1);
> +
> +if (!(vm = qemuDomainObjFromDomain(dom)))
> +return -1;
> +
> +if (virDomainGetMessagesEnsureACL(dom->conn, vm->def) < 0)
> +goto cleanup;
> +
> +*msgs = NULL;
> +nmsgs = 0;
> +n = 0;
> +
> +if (!flags || (flags & VIR_DOMAIN_MESSAGE_TAINTING)) {
> +nmsgs += __builtin_popcount(vm->taint);
> +*msgs = g_renew(char *, *msgs, nmsgs+1);
> +
> +for (i = 0; i < VIR_DOMAIN_TAINT_LAST; i++) {
> +if (vm->taint & (1 << i)) {
> +(*msgs)[n++] = g_strdup_printf(
> +_("tainted: %s"),
> +_(virDomainTaintMessageTypeToString(i)));
> +}
> +}
> +}
> +
> +if (!flags || (flags & VIR_DOMAIN_MESSAGE_DEPRECATION)) {
> +nmsgs += vm->ndeprecations;
> +*msgs = g_renew(char *, *msgs, nmsgs+1);
> +
> +for (i = 0; i < vm->ndeprecations; i++) {
> +(*msgs)[n++] = g_strdup_printf(
> +_("deprecated configuration: %s"),
> +vm->deprecations[i]);
> +}
> +}
> +
> +(*msgs)[nmsgs] = NULL;
FYI: Coverity got grumpy right here as it believes *msgs could be NULL
because of the !flags || condition and not realizing that flags could
only be one of the two or 0.
Wasn't sure whether being safe and adding a if (*msgs) is desired, but
figured I'd at least note it.
John
> +
> +rv = nmsgs;
> +
> + cleanup:
> +virDomainObjEndAPI(&vm);
> +return rv;
> +}
> +
[...]
Re: [libvirt RFC PATCH 0/5] eliminating VIR_FREE in the *Clear() functions
On 2/12/21 5:25 AM, Daniel P. Berrangé wrote: On Fri, Feb 12, 2021 at 11:07:21AM +0100, Erik Skultety wrote: On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote: On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote: I've looked at a few of these, and one thing I've found is that very often we have a function called somethingSomethingClear(), and: 1) The only places it is ever called will immediately free the memory of the object as soon as they clear it. and very possibly 2) It doesn't actually *clear* everything. Some items are cleared via VIR_FREE(), but then some of the other pointers call bobLoblawFree(def->bobloblaw) and then don't actually set def->bobloblaw to NULL - so the functions aren't actually "Clearing", they're "Freeing and then clearing a few things, but not everything". One thing I am wondering is whether this is really only used where it makes sense. As far as I understand, and please correct me if I am way off, the purpose of the Clear functions is to: a) provide a way to remove everything from a structure that the current function cannot recreate (there is a pointer to it somewhere else which would not be updated) and b) provide a way to reset a structure so that it can be filled again without needless reallocation. I think (b) is obviously pointless, especially lately, so the only reasonable usage would be for the scenario (a). However, I think I remember this also being used in places where it would be perfectly fine to free the variable and recreate it. Maybe it could ease up the decision, at least by eliminating some of the code, if my hunch is correct. In my quick search I only found virDomainVideoDefClear to be used in this manner and I am not convinced that it is worth having this extra function with extra You could always memset it explicitly, someone might find the code more readable then. IMO I'd vote for an explicit memset just for the sake of better security practice (since we'll have to wait a little while for something like SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how many cycles exactly would be wasted, but IIRC a recent discussion memset can be optimized away (correct me if I don't remember it well!), so Dan P.B. suggested to gradually convert to some platform-specific ways on how to sanitize the memory safely - with that in mind, I'd say we use an explicit memset in all the functions in question and convert them later? I only suggest that for places where security is required. ie to scrub passwords. Yeah, I'm not planning to touch anything that is clearing out passwords and such. Only the *Clear() functions that currently have the dual purposes of: 1) freeing memory pointed to by the object in question (and any sub-objects) 2) clearing out the object so that it can be re-used with no side effects (e.g., pointers NULLed so that subsequent uses believe (correctly) that nothing is being pointed at, setting counters to 0, types to ..._NONE, etc. If the compiler wants to cull memsets in places unrelated to security that's fine by me, or at least, not our problem to worry about. I would hope that the compiler would be smart enough to not optimize it out if it can't determine 100% that it will never make a difference. This would mean that, for example, unless a *Clear() function is defined static, it couldn't optimize out a memset() at the end (because it can't know what would be done with the object after return). But if it's going to optimize out a memset, it would likely also optimize out the "loblaw = NULL;" in the VIR_FREE invocation, so... (My mind keeps going back to 1994, when I turned on the 80386 "invalid address faults" bit (forget the exact name) on our router product that was running 8086 realmode *BSD, and suddenly so many stupid pointer bugs were immediately revealed )by a segfault) instead of the code just silently going off into the weeds. And when we started NULLing out pointers as things were freed we found so many more; the sources of mysterious problems that customers had been reporting for months were suddenly obvious. So my subconscious tells me that NULLing out freed pointers (and the memory they point to) is just "safer", and we're spending all this time removing that safety; kind of like going through all the cars in the world to remove their seatbelts because they make driving less convenient, and airbags offer a similar type of protection...)
[PATCH 11/25] virJSONValueObjectInsert: Clear @value on successful insertion
The function takes ownership of @value on success so the proper
semantics will be to clear out the @value pointer. Convert @value to a
double pointer to do this.
Signed-off-by: Peter Krempa
---
src/util/virjson.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index a509943fde..80ebcb587c 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -577,10 +577,10 @@ virJSONValueNewObject(void)
static int
virJSONValueObjectInsert(virJSONValuePtr object,
const char *key,
- virJSONValuePtr value,
+ virJSONValuePtr *value,
bool prepend)
{
-virJSONObjectPair pair = { NULL, value };
+virJSONObjectPair pair = { NULL, *value };
int ret = -1;
if (object->type != VIR_JSON_TYPE_OBJECT) {
@@ -604,6 +604,9 @@ virJSONValueObjectInsert(virJSONValuePtr object,
object->data.object.npairs, pair);
}
+if (ret == 0)
+*value = NULL;
+
VIR_FREE(pair.key);
return ret;
}
@@ -614,7 +617,7 @@ virJSONValueObjectAppend(virJSONValuePtr object,
const char *key,
virJSONValuePtr value)
{
-return virJSONValueObjectInsert(object, key, value, false);
+return virJSONValueObjectInsert(object, key, &value, false);
}
@@ -627,10 +630,8 @@ virJSONValueObjectInsertString(virJSONValuePtr object,
virJSONValuePtr jvalue = virJSONValueNewString(value);
if (!jvalue)
return -1;
-if (virJSONValueObjectInsert(object, key, jvalue, prepend) < 0) {
-virJSONValueFree(jvalue);
+if (virJSONValueObjectInsert(object, key, &jvalue, prepend) < 0)
return -1;
-}
return 0;
}
--
2.29.2
[PATCH 23/25] virJSONParserHandle*: Refactor memory cleanup and drop NULL checks
virJSONValueNew* won't return error nowadays so NULL checks are not
necessary. The memory can be cleared via g_autoptr.
Signed-off-by: Peter Krempa
---
src/util/virjson.c | 68 --
1 file changed, 24 insertions(+), 44 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 65215f03c5..de83441e27 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1604,17 +1604,13 @@ static int
virJSONParserHandleNull(void *ctx)
{
virJSONParserPtr parser = ctx;
-virJSONValuePtr value = virJSONValueNewNull();
+g_autoptr(virJSONValue) value = virJSONValueNewNull();
VIR_DEBUG("parser=%p", parser);
-if (!value)
+if (virJSONParserInsertValue(parser, value) < 0)
return 0;
-
-if (virJSONParserInsertValue(parser, value) < 0) {
-virJSONValueFree(value);
-return 0;
-}
+value = NULL;
return 1;
}
@@ -1625,17 +1621,13 @@ virJSONParserHandleBoolean(void *ctx,
int boolean_)
{
virJSONParserPtr parser = ctx;
-virJSONValuePtr value = virJSONValueNewBoolean(boolean_);
+g_autoptr(virJSONValue) value = virJSONValueNewBoolean(boolean_);
VIR_DEBUG("parser=%p boolean=%d", parser, boolean_);
-if (!value)
-return 0;
-
-if (virJSONParserInsertValue(parser, value) < 0) {
-virJSONValueFree(value);
+if (virJSONParserInsertValue(parser, value) < 0)
return 0;
-}
+value = NULL;
return 1;
}
@@ -1647,22 +1639,14 @@ virJSONParserHandleNumber(void *ctx,
size_t l)
{
virJSONParserPtr parser = ctx;
-char *str;
-virJSONValuePtr value;
-
-str = g_strndup(s, l);
-value = virJSONValueNewNumber(str);
-VIR_FREE(str);
+g_autofree char *str = g_strndup(s, l);
+g_autoptr(virJSONValue) value = virJSONValueNewNumber(str);
VIR_DEBUG("parser=%p str=%s", parser, str);
-if (!value)
+if (virJSONParserInsertValue(parser, value) < 0)
return 0;
-
-if (virJSONParserInsertValue(parser, value) < 0) {
-virJSONValueFree(value);
-return 0;
-}
+value = NULL;
return 1;
}
@@ -1674,18 +1658,14 @@ virJSONParserHandleString(void *ctx,
size_t stringLen)
{
virJSONParserPtr parser = ctx;
-virJSONValuePtr value = virJSONValueNewStringLen((const char *)stringVal,
- stringLen);
+g_autoptr(virJSONValue) value = virJSONValueNewStringLen((const char
*)stringVal,
+ stringLen);
VIR_DEBUG("parser=%p str=%p", parser, (const char *)stringVal);
-if (!value)
-return 0;
-
-if (virJSONParserInsertValue(parser, value) < 0) {
-virJSONValueFree(value);
+if (virJSONParserInsertValue(parser, value) < 0)
return 0;
-}
+value = NULL;
return 1;
}
@@ -1716,21 +1696,21 @@ static int
virJSONParserHandleStartMap(void *ctx)
{
virJSONParserPtr parser = ctx;
-virJSONValuePtr value = virJSONValueNewObject();
+g_autoptr(virJSONValue) value = virJSONValueNewObject();
+virJSONValuePtr tmp = value;
VIR_DEBUG("parser=%p", parser);
-if (virJSONParserInsertValue(parser, value) < 0) {
-virJSONValueFree(value);
+if (virJSONParserInsertValue(parser, value) < 0)
return 0;
-}
+value = NULL;
if (VIR_REALLOC_N(parser->state,
parser->nstate + 1) < 0) {
return 0;
}
-parser->state[parser->nstate].value = value;
+parser->state[parser->nstate].value = tmp;
parser->state[parser->nstate].key = NULL;
parser->nstate++;
@@ -1765,20 +1745,20 @@ static int
virJSONParserHandleStartArray(void *ctx)
{
virJSONParserPtr parser = ctx;
-virJSONValuePtr value = virJSONValueNewArray();
+g_autoptr(virJSONValue) value = virJSONValueNewArray();
+virJSONValuePtr tmp = value;
VIR_DEBUG("parser=%p", parser);
-if (virJSONParserInsertValue(parser, value) < 0) {
-virJSONValueFree(value);
+if (virJSONParserInsertValue(parser, value) < 0)
return 0;
-}
+value = NULL;
if (VIR_REALLOC_N(parser->state,
parser->nstate + 1) < 0)
return 0;
-parser->state[parser->nstate].value = value;
+parser->state[parser->nstate].value = tmp;
parser->state[parser->nstate].key = NULL;
parser->nstate++;
--
2.29.2
[PATCH 18/25] virMACMapHashDumper: Refactor array addition
Use automatic memory freeing and don't check return value of
virJSONValueNewString as it can't fail.
Signed-off-by: Peter Krempa
---
src/util/virmacmap.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
index 521ab89b5b..12df325933 100644
--- a/src/util/virmacmap.c
+++ b/src/util/virmacmap.c
@@ -214,13 +214,11 @@ virMACMapHashDumper(void *payload,
GSList *next;
for (next = macs; next; next = next->next) {
-virJSONValuePtr m = virJSONValueNewString((const char *) next->data);
+g_autoptr(virJSONValue) m = virJSONValueNewString((const char *)
next->data);
-if (!m ||
-virJSONValueArrayAppend(arr, m) < 0) {
-virJSONValueFree(m);
+if (virJSONValueArrayAppend(arr, m) < 0)
return -1;
-}
+m = NULL;
}
if (virJSONValueObjectAppendString(obj, "domain", name) < 0 ||
--
2.29.2
[PATCH 13/25] virJSONValue(Array|Object)Append*: Simplify handling of appended object
Use g_autofree for the pointer of the added object and remove the NULL
checks for values returned by virJSONValueNew* (except
virJSONValueNewNumberDouble) since they can't fail nowadays.
Signed-off-by: Peter Krempa
---
src/util/virjson.c | 88 ++
1 file changed, 42 insertions(+), 46 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index d794eed17f..adf1cfbcbc 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -676,13 +676,12 @@ virJSONValueObjectAppendNumberInt(virJSONValuePtr object,
const char *key,
int number)
{
-virJSONValuePtr jvalue = virJSONValueNewNumberInt(number);
-if (!jvalue)
-return -1;
-if (virJSONValueObjectAppend(object, key, jvalue) < 0) {
-virJSONValueFree(jvalue);
+g_autoptr(virJSONValue) jvalue = virJSONValueNewNumberInt(number);
+
+if (virJSONValueObjectAppend(object, key, jvalue) < 0)
return -1;
-}
+jvalue = NULL;
+
return 0;
}
@@ -692,13 +691,12 @@ virJSONValueObjectAppendNumberUint(virJSONValuePtr object,
const char *key,
unsigned int number)
{
-virJSONValuePtr jvalue = virJSONValueNewNumberUint(number);
-if (!jvalue)
-return -1;
-if (virJSONValueObjectAppend(object, key, jvalue) < 0) {
-virJSONValueFree(jvalue);
+g_autoptr(virJSONValue) jvalue = virJSONValueNewNumberUint(number);
+
+if (virJSONValueObjectAppend(object, key, jvalue) < 0)
return -1;
-}
+jvalue = NULL;
+
return 0;
}
@@ -708,13 +706,12 @@ virJSONValueObjectAppendNumberLong(virJSONValuePtr object,
const char *key,
long long number)
{
-virJSONValuePtr jvalue = virJSONValueNewNumberLong(number);
-if (!jvalue)
-return -1;
-if (virJSONValueObjectAppend(object, key, jvalue) < 0) {
-virJSONValueFree(jvalue);
+g_autoptr(virJSONValue) jvalue = virJSONValueNewNumberLong(number);
+
+if (virJSONValueObjectAppend(object, key, jvalue) < 0)
return -1;
-}
+jvalue = NULL;
+
return 0;
}
@@ -724,13 +721,12 @@ virJSONValueObjectAppendNumberUlong(virJSONValuePtr
object,
const char *key,
unsigned long long number)
{
-virJSONValuePtr jvalue = virJSONValueNewNumberUlong(number);
-if (!jvalue)
-return -1;
-if (virJSONValueObjectAppend(object, key, jvalue) < 0) {
-virJSONValueFree(jvalue);
+g_autoptr(virJSONValue) jvalue = virJSONValueNewNumberUlong(number);
+
+if (virJSONValueObjectAppend(object, key, jvalue) < 0)
return -1;
-}
+jvalue = NULL;
+
return 0;
}
@@ -740,13 +736,16 @@ virJSONValueObjectAppendNumberDouble(virJSONValuePtr
object,
const char *key,
double number)
{
-virJSONValuePtr jvalue = virJSONValueNewNumberDouble(number);
+g_autoptr(virJSONValue) jvalue = virJSONValueNewNumberDouble(number);
+
+/* virJSONValueNewNumberDouble may return NULL if locale setting fails */
if (!jvalue)
return -1;
-if (virJSONValueObjectAppend(object, key, jvalue) < 0) {
-virJSONValueFree(jvalue);
+
+if (virJSONValueObjectAppend(object, key, jvalue) < 0)
return -1;
-}
+jvalue = NULL;
+
return 0;
}
@@ -756,13 +755,12 @@ virJSONValueObjectAppendBoolean(virJSONValuePtr object,
const char *key,
int boolean_)
{
-virJSONValuePtr jvalue = virJSONValueNewBoolean(boolean_);
-if (!jvalue)
-return -1;
-if (virJSONValueObjectAppend(object, key, jvalue) < 0) {
-virJSONValueFree(jvalue);
+g_autoptr(virJSONValue) jvalue = virJSONValueNewBoolean(boolean_);
+
+if (virJSONValueObjectAppend(object, key, jvalue) < 0)
return -1;
-}
+jvalue = NULL;
+
return 0;
}
@@ -771,13 +769,12 @@ int
virJSONValueObjectAppendNull(virJSONValuePtr object,
const char *key)
{
-virJSONValuePtr jvalue = virJSONValueNewNull();
-if (!jvalue)
-return -1;
-if (virJSONValueObjectAppend(object, key, jvalue) < 0) {
-virJSONValueFree(jvalue);
+g_autoptr(virJSONValue) jvalue = virJSONValueNewNull();
+
+if (virJSONValueObjectAppend(object, key, jvalue) < 0)
return -1;
-}
+jvalue = NULL;
+
return 0;
}
@@ -806,13 +803,12 @@ int
virJSONValueArrayAppendString(virJSONValuePtr object,
const char *value)
{
-virJSONValuePtr jvalue = virJSONValueNewString(value);
-if (!jvalue)
-return -1;
-if (virJSONValueArrayAppend(object, jvalue) < 0) {
-virJSONValueFree(jvalue);
+g_autoptr(virJSONValue) jvalue =
[PATCH 22/25] qemuAgentSetVCPUsCommand: Refactor cleanup
Signed-off-by: Peter Krempa
---
src/qemu/qemu_agent.c | 39 +--
1 file changed, 13 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 9aec0fdb4b..9a74b802b8 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1483,20 +1483,17 @@ qemuAgentSetVCPUsCommand(qemuAgentPtr agent,
size_t ninfo,
int *nmodified)
{
-int ret = -1;
-virJSONValuePtr cmd = NULL;
-virJSONValuePtr reply = NULL;
-virJSONValuePtr cpus = NULL;
-virJSONValuePtr cpu = NULL;
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+g_autoptr(virJSONValue) cpus = virJSONValueNewArray();
size_t i;
+int ret;
*nmodified = 0;
-/* create the key data array */
-cpus = virJSONValueNewArray();
-
for (i = 0; i < ninfo; i++) {
qemuAgentCPUInfoPtr in = &info[i];
+g_autoptr(virJSONValue) cpu = virJSONValueNewObject();
/* don't set state for cpus that were not touched */
if (!in->modified)
@@ -1504,31 +1501,26 @@ qemuAgentSetVCPUsCommand(qemuAgentPtr agent,
(*nmodified)++;
-/* create single cpu object */
-cpu = virJSONValueNewObject();
-
if (virJSONValueObjectAppendNumberInt(cpu, "logical-id", in->id) < 0)
-goto cleanup;
+return -1;
if (virJSONValueObjectAppendBoolean(cpu, "online", in->online) < 0)
-goto cleanup;
+return -1;
if (virJSONValueArrayAppend(cpus, &cpu) < 0)
-goto cleanup;
+return -1;
}
-if (*nmodified == 0) {
-ret = 0;
-goto cleanup;
-}
+if (*nmodified == 0)
+return 0;
if (!(cmd = qemuAgentMakeCommand("guest-set-vcpus",
"a:vcpus", &cpus,
NULL)))
-goto cleanup;
+return -1;
if (qemuAgentCommand(agent, cmd, &reply, agent->timeout) < 0)
-goto cleanup;
+return -1;
/* All negative values are invalid. Return of 0 is bogus since we wouldn't
* call the guest agent so that 0 cpus would be set successfully. Reporting
@@ -1537,14 +1529,9 @@ qemuAgentSetVCPUsCommand(qemuAgentPtr agent,
ret <= 0 || ret > *nmodified) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest agent returned malformed or invalid return
value"));
-ret = -1;
+return -1;
}
- cleanup:
-virJSONValueFree(cmd);
-virJSONValueFree(reply);
-virJSONValueFree(cpu);
-virJSONValueFree(cpus);
return ret;
}
--
2.29.2
[PATCH 21/25] qemuMonitorJSONTransactionAdd: Refactor cleanup
Signed-off-by: Peter Krempa
---
src/qemu/qemu_monitor_json.c | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 94365c775b..76401c4d3c 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -510,31 +510,28 @@ qemuMonitorJSONTransactionAdd(virJSONValuePtr actions,
const char *cmdname,
...)
{
-virJSONValuePtr entry = NULL;
-virJSONValuePtr data = NULL;
+g_autoptr(virJSONValue) entry = NULL;
+g_autoptr(virJSONValue) data = NULL;
va_list args;
-int ret = -1;
va_start(args, cmdname);
-if (virJSONValueObjectCreateVArgs(&data, args) < 0)
-goto cleanup;
+if (virJSONValueObjectCreateVArgs(&data, args) < 0) {
+va_end(args);
+return -1;
+}
+
+va_end(args);
if (virJSONValueObjectCreate(&entry,
"s:type", cmdname,
"A:data", &data, NULL) < 0)
-goto cleanup;
+return -1;
if (virJSONValueArrayAppend(actions, &entry) < 0)
-goto cleanup;
-
-ret = 0;
+return -1;
- cleanup:
-virJSONValueFree(entry);
-virJSONValueFree(data);
-va_end(args);
-return ret;
+return 0;
}
--
2.29.2
[PATCH 19/25] testQEMUSchemaValidateObjectMergeVariantMember: Fix theoretical leak
Signed-off-by: Peter Krempa
---
tests/testutilsqemuschema.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c
index 101687e657..4bb303a427 100644
--- a/tests/testutilsqemuschema.c
+++ b/tests/testutilsqemuschema.c
@@ -163,13 +163,14 @@ testQEMUSchemaValidateObjectMergeVariantMember(size_t pos
G_GNUC_UNUSED,
void *opaque)
{
virJSONValuePtr array = opaque;
-virJSONValuePtr copy;
+g_autoptr(virJSONValue) copy = NULL;
if (!(copy = virJSONValueCopy(item)))
return -1;
if (virJSONValueArrayAppend(array, copy) < 0)
return -1;
+copy = NULL;
return 1;
}
--
2.29.2
[PATCH 24/25] virJSONValueNewNumber: Take ownership of passed string
Avoid pointless copies of temporary strings when constructing number
JSON objects.
Signed-off-by: Peter Krempa
---
src/util/virjson.c | 36 +---
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index de83441e27..b21b1fc63f 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -465,15 +465,22 @@ virJSONValueNewStringLen(const char *data,
}
+/**
+ * virJSONValueNewNumber:
+ * @data: string representing the number
+ *
+ * Creates a new virJSONValue of VIR_JSON_TYPE_NUMBER type. Note that this
+ * function takes ownership of @data.
+ */
static virJSONValuePtr
-virJSONValueNewNumber(const char *data)
+virJSONValueNewNumber(char *data)
{
virJSONValuePtr val;
val = g_new0(virJSONValue, 1);
val->type = VIR_JSON_TYPE_NUMBER;
-val->data.number = g_strdup(data);
+val->data.number = data;
return val;
}
@@ -482,43 +489,35 @@ virJSONValueNewNumber(const char *data)
virJSONValuePtr
virJSONValueNewNumberInt(int data)
{
-g_autofree char *str = NULL;
-str = g_strdup_printf("%i", data);
-return virJSONValueNewNumber(str);
+return virJSONValueNewNumber(g_strdup_printf("%i", data));
}
virJSONValuePtr
virJSONValueNewNumberUint(unsigned int data)
{
-g_autofree char *str = NULL;
-str = g_strdup_printf("%u", data);
-return virJSONValueNewNumber(str);
+return virJSONValueNewNumber(g_strdup_printf("%u", data));
}
virJSONValuePtr
virJSONValueNewNumberLong(long long data)
{
-g_autofree char *str = NULL;
-str = g_strdup_printf("%lld", data);
-return virJSONValueNewNumber(str);
+return virJSONValueNewNumber(g_strdup_printf("%lld", data));
}
virJSONValuePtr
virJSONValueNewNumberUlong(unsigned long long data)
{
-g_autofree char *str = NULL;
-str = g_strdup_printf("%llu", data);
-return virJSONValueNewNumber(str);
+return virJSONValueNewNumber(g_strdup_printf("%llu", data));
}
virJSONValuePtr
virJSONValueNewNumberDouble(double data)
{
-g_autofree char *str = NULL;
+char *str = NULL;
if (virDoubleToStr(&str, data) < 0)
return NULL;
return virJSONValueNewNumber(str);
@@ -1534,7 +1533,7 @@ virJSONValueCopy(const virJSONValue *in)
out = virJSONValueNewString(in->data.string);
break;
case VIR_JSON_TYPE_NUMBER:
-out = virJSONValueNewNumber(in->data.number);
+out = virJSONValueNewNumber(g_strdup(in->data.number));
break;
case VIR_JSON_TYPE_BOOLEAN:
out = virJSONValueNewBoolean(in->data.boolean);
@@ -1639,10 +1638,9 @@ virJSONParserHandleNumber(void *ctx,
size_t l)
{
virJSONParserPtr parser = ctx;
-g_autofree char *str = g_strndup(s, l);
-g_autoptr(virJSONValue) value = virJSONValueNewNumber(str);
+g_autoptr(virJSONValue) value = virJSONValueNewNumber(g_strndup(s, l));
-VIR_DEBUG("parser=%p str=%s", parser, str);
+VIR_DEBUG("parser=%p str=%s", parser, value->data.number);
if (virJSONParserInsertValue(parser, value) < 0)
return 0;
--
2.29.2
[PATCH 25/25] virJSONParserInsertValue: Take double pointer for @value
The function calls virJSONValueObjectAppend/virJSONValueArrayAppend, so
by taking a double pointer we can drop the pointer clearing from
callers.
Signed-off-by: Peter Krempa
---
src/util/virjson.c | 26 ++
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index b21b1fc63f..b952ad500d 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1550,10 +1550,10 @@ virJSONValueCopy(const virJSONValue *in)
#if WITH_YAJL
static int
virJSONParserInsertValue(virJSONParserPtr parser,
- virJSONValuePtr value)
+ virJSONValuePtr *value)
{
if (!parser->head) {
-parser->head = value;
+parser->head = g_steal_pointer(value);
} else {
virJSONParserStatePtr state;
if (!parser->nstate) {
@@ -1572,7 +1572,7 @@ virJSONParserInsertValue(virJSONParserPtr parser,
if (virJSONValueObjectAppend(state->value,
state->key,
- &value) < 0)
+ value) < 0)
return -1;
VIR_FREE(state->key);
@@ -1585,7 +1585,7 @@ virJSONParserInsertValue(virJSONParserPtr parser,
}
if (virJSONValueArrayAppend(state->value,
-&value) < 0)
+value) < 0)
return -1;
} break;
@@ -1607,9 +1607,8 @@ virJSONParserHandleNull(void *ctx)
VIR_DEBUG("parser=%p", parser);
-if (virJSONParserInsertValue(parser, value) < 0)
+if (virJSONParserInsertValue(parser, &value) < 0)
return 0;
-value = NULL;
return 1;
}
@@ -1624,9 +1623,8 @@ virJSONParserHandleBoolean(void *ctx,
VIR_DEBUG("parser=%p boolean=%d", parser, boolean_);
-if (virJSONParserInsertValue(parser, value) < 0)
+if (virJSONParserInsertValue(parser, &value) < 0)
return 0;
-value = NULL;
return 1;
}
@@ -1642,9 +1640,8 @@ virJSONParserHandleNumber(void *ctx,
VIR_DEBUG("parser=%p str=%s", parser, value->data.number);
-if (virJSONParserInsertValue(parser, value) < 0)
+if (virJSONParserInsertValue(parser, &value) < 0)
return 0;
-value = NULL;
return 1;
}
@@ -1661,9 +1658,8 @@ virJSONParserHandleString(void *ctx,
VIR_DEBUG("parser=%p str=%p", parser, (const char *)stringVal);
-if (virJSONParserInsertValue(parser, value) < 0)
+if (virJSONParserInsertValue(parser, &value) < 0)
return 0;
-value = NULL;
return 1;
}
@@ -1699,9 +1695,8 @@ virJSONParserHandleStartMap(void *ctx)
VIR_DEBUG("parser=%p", parser);
-if (virJSONParserInsertValue(parser, value) < 0)
+if (virJSONParserInsertValue(parser, &value) < 0)
return 0;
-value = NULL;
if (VIR_REALLOC_N(parser->state,
parser->nstate + 1) < 0) {
@@ -1748,9 +1743,8 @@ virJSONParserHandleStartArray(void *ctx)
VIR_DEBUG("parser=%p", parser);
-if (virJSONParserInsertValue(parser, value) < 0)
+if (virJSONParserInsertValue(parser, &value) < 0)
return 0;
-value = NULL;
if (VIR_REALLOC_N(parser->state,
parser->nstate + 1) < 0)
--
2.29.2
[PATCH 14/25] virJSONValueNewArrayFromBitmap: Refactor cleanup
Use g_autoptr for the JSON value objects and remove the cleanup label
and inline freeing of objects.
Signed-off-by: Peter Krempa
---
src/util/virjson.c | 22 +++---
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index adf1cfbcbc..e4d71d3e09 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1241,29 +1241,21 @@ virJSONValueGetArrayAsBitmap(const virJSONValue *val,
virJSONValuePtr
virJSONValueNewArrayFromBitmap(virBitmapPtr bitmap)
{
-virJSONValuePtr ret;
+g_autoptr(virJSONValue) ret = virJSONValueNewArray();
ssize_t pos = -1;
-ret = virJSONValueNewArray();
-
if (!bitmap)
-return ret;
+return g_steal_pointer(&ret);
while ((pos = virBitmapNextSetBit(bitmap, pos)) > -1) {
-virJSONValuePtr newelem;
+g_autoptr(virJSONValue) newelem = virJSONValueNewNumberLong(pos);
-if (!(newelem = virJSONValueNewNumberLong(pos)) ||
-virJSONValueArrayAppend(ret, newelem) < 0) {
-virJSONValueFree(newelem);
-goto error;
-}
+if (virJSONValueArrayAppend(ret, newelem) < 0)
+return NULL;
+newelem = NULL;
}
-return ret;
-
- error:
-virJSONValueFree(ret);
-return NULL;
+return g_steal_pointer(&ret);
}
--
2.29.2
[PATCH 20/25] virJSONValueArrayAppend: Clear pointer when taking ownership of passed value
The parent array takes ownership of the inserted value once all checks
pass. Don't make the callers second-guess when that happens and modify
the function to take a double pointer so that it can be cleared once the
ownership is taken.
Signed-off-by: Peter Krempa
---
src/locking/lock_daemon.c| 3 +--
src/logging/log_handler.c| 3 +--
src/network/leaseshelper.c | 3 +--
src/node_device/node_device_driver.c | 2 +-
src/qemu/qemu_agent.c| 7 ++-
src/qemu/qemu_block.c| 16
src/qemu/qemu_command.c | 3 +--
src/qemu/qemu_firmware.c | 4 +---
src/qemu/qemu_migration_params.c | 4 +---
src/qemu/qemu_monitor_json.c | 11 +++
src/rpc/virnetserver.c | 6 ++
src/rpc/virnetserverservice.c| 3 +--
src/util/virjson.c | 12 +---
src/util/virjson.h | 3 ++-
src/util/virlease.c | 2 +-
src/util/virlockspace.c | 6 ++
src/util/virmacmap.c | 6 ++
tests/testutilsqemuschema.c | 3 +--
18 files changed, 32 insertions(+), 65 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 26905a462b..d68c61b099 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -728,9 +728,8 @@ virLockDaemonPreExecRestart(const char *state_file,
if (!(child = virLockSpacePreExecRestart(lockspace)))
return -1;
-if (virJSONValueArrayAppend(lockspaces, child) < 0)
+if (virJSONValueArrayAppend(lockspaces, &child) < 0)
return -1;
-child = NULL;
tmp++;
}
diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
index d649c4d132..9e027c7579 100644
--- a/src/logging/log_handler.c
+++ b/src/logging/log_handler.c
@@ -642,9 +642,8 @@ virLogHandlerPreExecRestart(virLogHandlerPtr handler)
return NULL;
}
-if (virJSONValueArrayAppend(files, file) < 0)
+if (virJSONValueArrayAppend(files, &file) < 0)
return NULL;
-file = NULL;
}
if (virJSONValueObjectAppend(ret, "files", &files) < 0)
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index c20e63efa9..ca2c640672 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -226,12 +226,11 @@ main(int argc, char **argv)
case VIR_LEASE_ACTION_OLD:
case VIR_LEASE_ACTION_ADD:
-if (lease_new && virJSONValueArrayAppend(leases_array_new, lease_new)
< 0) {
+if (lease_new && virJSONValueArrayAppend(leases_array_new, &lease_new)
< 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to create json"));
goto cleanup;
}
-lease_new = NULL;
G_GNUC_FALLTHROUGH;
case VIR_LEASE_ACTION_DEL:
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index d946e64ad6..543e5bb90a 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -606,7 +606,7 @@ nodeDeviceDefToMdevctlConfig(virNodeDeviceDefPtr def, char
**buf)
if (virJSONValueObjectAppendString(jsonattr, attr->name,
attr->value) < 0)
return -1;
-if (virJSONValueArrayAppend(attributes,
g_steal_pointer(&jsonattr)) < 0)
+if (virJSONValueArrayAppend(attributes, &jsonattr) < 0)
return -1;
}
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index d6816ef9de..9aec0fdb4b 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1180,9 +1180,8 @@ qemuAgentMakeStringsArray(const char **strings, unsigned
int len)
for (i = 0; i < len; i++) {
g_autoptr(virJSONValue) str = virJSONValueNewString(strings[i]);
-if (virJSONValueArrayAppend(ret, str) < 0)
+if (virJSONValueArrayAppend(ret, &str) < 0)
return NULL;
-str = NULL;
}
return g_steal_pointer(&ret);
@@ -1514,10 +1513,8 @@ qemuAgentSetVCPUsCommand(qemuAgentPtr agent,
if (virJSONValueObjectAppendBoolean(cpu, "online", in->online) < 0)
goto cleanup;
-if (virJSONValueArrayAppend(cpus, cpu) < 0)
+if (virJSONValueArrayAppend(cpus, &cpu) < 0)
goto cleanup;
-
-cpu = NULL;
}
if (*nmodified == 0) {
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 1df51ba97b..94cbb1d12e 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -534,10 +534,8 @@
qemuBlockStorageSourceBuildHostsJSONSocketAddress(virStorageSourcePtr src,
if (!(server = qemuBlockStorageSourceBuildJSONSocketAddress(host,
legacy)))
return NULL;
-if (virJSONValueArrayAppend(servers, server) < 0)
+if (virJSONValueArrayAppend(servers, &server) < 0)
return NULL;
-
-server = NULL;
[PATCH 15/25] virJSONValueObjectAddVArgs: Use autofree for the temporary bitmap
Signed-off-by: Peter Krempa
---
src/util/virjson.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index e4d71d3e09..7b52525797 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -305,7 +305,7 @@ virJSONValueObjectAddVArgs(virJSONValuePtr obj,
case 'M':
case 'm': {
virBitmapPtr map = va_arg(args, virBitmapPtr);
-virJSONValuePtr jsonMap;
+g_autoptr(virJSONValue) jsonMap = NULL;
if (!map) {
if (type == 'M')
@@ -321,7 +321,8 @@ virJSONValueObjectAddVArgs(virJSONValuePtr obj,
return -1;
if ((rc = virJSONValueObjectAppend(obj, key, jsonMap)) < 0)
-virJSONValueFree(jsonMap);
+return -1;
+jsonMap = NULL;
} break;
default:
--
2.29.2
[PATCH 17/25] qemuAgentMakeStringsArray: Refactor cleanup
Signed-off-by: Peter Krempa
---
src/qemu/qemu_agent.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 4712aeb529..d6816ef9de 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1175,23 +1175,17 @@ static virJSONValuePtr
qemuAgentMakeStringsArray(const char **strings, unsigned int len)
{
size_t i;
-virJSONValuePtr ret = virJSONValueNewArray(), str;
+g_autoptr(virJSONValue) ret = virJSONValueNewArray();
for (i = 0; i < len; i++) {
-str = virJSONValueNewString(strings[i]);
-if (!str)
-goto error;
+g_autoptr(virJSONValue) str = virJSONValueNewString(strings[i]);
-if (virJSONValueArrayAppend(ret, str) < 0) {
-virJSONValueFree(str);
-goto error;
-}
+if (virJSONValueArrayAppend(ret, str) < 0)
+return NULL;
+str = NULL;
}
-return ret;
- error:
-virJSONValueFree(ret);
-return NULL;
+return g_steal_pointer(&ret);
}
void qemuAgentNotifyEvent(qemuAgentPtr agent,
--
2.29.2
[PATCH 12/25] virJSONValueCopy: Don't use virJSONValue(Object|Array)Append
We know the exact number of keys or array members for the copied objects
so we can pre-allocate the arrays rather than inserting into them in a
loop incurring realloc copy penalty.
Also virJSONValueCopy now can't fail since all of the functions
allocating the different cases use just g_new/g_strdup internally so we
can remove the NULL checks from the recursive calls.
Signed-off-by: Peter Krempa
---
src/util/virjson.c | 30 +++---
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 80ebcb587c..d794eed17f 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1530,27 +1530,23 @@ virJSONValueCopy(const virJSONValue *in)
switch ((virJSONType) in->type) {
case VIR_JSON_TYPE_OBJECT:
out = virJSONValueNewObject();
+
+out->data.object.pairs = g_new0(virJSONObjectPair,
in->data.object.npairs);
+out->data.object.npairs = in->data.object.npairs;
+
for (i = 0; i < in->data.object.npairs; i++) {
-virJSONValuePtr val = NULL;
-if (!(val = virJSONValueCopy(in->data.object.pairs[i].value)))
-goto error;
-if (virJSONValueObjectAppend(out, in->data.object.pairs[i].key,
- val) < 0) {
-virJSONValueFree(val);
-goto error;
-}
+out->data.object.pairs[i].key =
g_strdup(in->data.object.pairs[i].key);
+out->data.object.pairs[i].value =
virJSONValueCopy(in->data.object.pairs[i].value);
}
break;
case VIR_JSON_TYPE_ARRAY:
out = virJSONValueNewArray();
+
+out->data.array.values = g_new0(virJSONValuePtr,
in->data.array.nvalues);
+out->data.array.nvalues = in->data.array.nvalues;
+
for (i = 0; i < in->data.array.nvalues; i++) {
-virJSONValuePtr val = NULL;
-if (!(val = virJSONValueCopy(in->data.array.values[i])))
-goto error;
-if (virJSONValueArrayAppend(out, val) < 0) {
-virJSONValueFree(val);
-goto error;
-}
+out->data.array.values[i] =
virJSONValueCopy(in->data.array.values[i]);
}
break;
@@ -1570,10 +1566,6 @@ virJSONValueCopy(const virJSONValue *in)
}
return out;
-
- error:
-virJSONValueFree(out);
-return NULL;
}
--
2.29.2
[PATCH 16/25] virJSONValueObjectAppend: Clear pointer when taking ownership of passed value
The parent object takes ownership of the inserted value once all checks
pass. Don't make the callers second-guess when that happens and modify
the function to take a double pointer so that it can be cleared once the
ownership is taken.
Signed-off-by: Peter Krempa
---
src/locking/lock_daemon.c| 9 +++
src/logging/log_daemon.c | 6 ++---
src/logging/log_handler.c| 3 +--
src/node_device/node_device_driver.c | 2 +-
src/qemu/qemu_block.c| 3 +--
src/qemu/qemu_firmware.c | 23 +---
src/qemu/qemu_monitor_json.c | 22 ++--
src/rpc/virnetdaemon.c | 6 ++---
src/rpc/virnetserver.c | 6 ++---
src/rpc/virnetserverclient.c | 6 ++---
src/rpc/virnetserverservice.c| 3 +--
src/util/virjson.c | 39
src/util/virjson.h | 4 ++-
src/util/virlockspace.c | 6 ++---
src/util/virmacmap.c | 3 +--
15 files changed, 52 insertions(+), 89 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 5913c0cb9c..26905a462b 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -711,16 +711,14 @@ virLockDaemonPreExecRestart(const char *state_file,
if (!(daemon = virNetDaemonPreExecRestart(dmn)))
return -1;
-if (virJSONValueObjectAppend(object, "daemon", daemon) < 0)
+if (virJSONValueObjectAppend(object, "daemon", &daemon) < 0)
return -1;
-daemon = NULL;
if (!(defaultLockspace =
virLockSpacePreExecRestart(lockDaemon->defaultLockspace)))
return -1;
-if (virJSONValueObjectAppend(object, "defaultLockspace", defaultLockspace)
< 0)
+if (virJSONValueObjectAppend(object, "defaultLockspace",
&defaultLockspace) < 0)
return -1;
-defaultLockspace = NULL;
tmp = pairs = virHashGetItems(lockDaemon->lockspaces, NULL, false);
while (tmp && tmp->key) {
@@ -737,9 +735,8 @@ virLockDaemonPreExecRestart(const char *state_file,
tmp++;
}
-if (virJSONValueObjectAppend(object, "lockspaces", lockspaces) < 0)
+if (virJSONValueObjectAppend(object, "lockspaces", &lockspaces) < 0)
return -1;
-lockspaces = NULL;
if (!(magic = virLockDaemonGetExecRestartMagic()))
return -1;
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index ceffa6a368..cc0b96ea05 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -516,9 +516,8 @@ virLogDaemonPreExecRestart(const char *state_file,
if (!(daemon = virNetDaemonPreExecRestart(dmn)))
return -1;
-if (virJSONValueObjectAppend(object, "daemon", daemon) < 0)
+if (virJSONValueObjectAppend(object, "daemon", &daemon) < 0)
return -1;
-daemon = NULL;
if (!(magic = virLogDaemonGetExecRestartMagic()))
return -1;
@@ -529,9 +528,8 @@ virLogDaemonPreExecRestart(const char *state_file,
if (!(handler = virLogHandlerPreExecRestart(logDaemon->handler)))
return -1;
-if (virJSONValueObjectAppend(object, "handler", handler) < 0)
+if (virJSONValueObjectAppend(object, "handler", &handler) < 0)
return -1;
-handler = NULL;
if (!(state = virJSONValueToString(object, true)))
return -1;
diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
index cacf9584cd..d649c4d132 100644
--- a/src/logging/log_handler.c
+++ b/src/logging/log_handler.c
@@ -647,9 +647,8 @@ virLogHandlerPreExecRestart(virLogHandlerPtr handler)
file = NULL;
}
-if (virJSONValueObjectAppend(ret, "files", files) < 0)
+if (virJSONValueObjectAppend(ret, "files", &files) < 0)
return NULL;
-files = NULL;
return g_steal_pointer(&ret);
}
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index 51b20848ce..d946e64ad6 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -610,7 +610,7 @@ nodeDeviceDefToMdevctlConfig(virNodeDeviceDefPtr def, char
**buf)
return -1;
}
-if (virJSONValueObjectAppend(json, "attrs",
g_steal_pointer(&attributes)) < 0)
+if (virJSONValueObjectAppend(json, "attrs", &attributes) < 0)
return -1;
}
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 3d88e701b2..1df51ba97b 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1097,9 +1097,8 @@
qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSourcePtr src,
NULL) < 0)
return -1;
-if (virJSONValueObjectAppend(props, "cache", cacheobj) < 0)
+if (virJSONValueObjectAppend(props, "cache", &cacheobj) < 0)
return -1;
-cacheobj = NULL;
return 0;
}
diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index c571fdecf7..5e8fdd0ff1 100644
--- a/src/qemu/qemu_firmware.c
+++
[PATCH 09/25] virLockSpacePreExecRestart: Refactor memory cleanup
Switch to using the 'g_auto*' helpers.
Signed-off-by: Peter Krempa
---
src/util/virlockspace.c | 51 +
1 file changed, 21 insertions(+), 30 deletions(-)
diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
index 7df319004e..1b6b51b649 100644
--- a/src/util/virlockspace.c
+++ b/src/util/virlockspace.c
@@ -418,9 +418,10 @@ virLockSpacePtr
virLockSpaceNewPostExecRestart(virJSONValuePtr object)
virJSONValuePtr virLockSpacePreExecRestart(virLockSpacePtr lockspace)
{
-virJSONValuePtr object = virJSONValueNewObject();
-virJSONValuePtr resources;
-virHashKeyValuePairPtr pairs = NULL, tmp;
+g_autoptr(virJSONValue) object = virJSONValueNewObject();
+g_autoptr(virJSONValue) resources = virJSONValueNewArray();
+g_autofree virHashKeyValuePairPtr pairs = NULL;
+virHashKeyValuePairPtr tmp;
virMutexLock(&lockspace->lock);
@@ -428,25 +429,14 @@ virJSONValuePtr
virLockSpacePreExecRestart(virLockSpacePtr lockspace)
virJSONValueObjectAppendString(object, "directory", lockspace->dir) <
0)
goto error;
-resources = virJSONValueNewArray();
-
-if (virJSONValueObjectAppend(object, "resources", resources) < 0) {
-virJSONValueFree(resources);
-goto error;
-}
tmp = pairs = virHashGetItems(lockspace->resources, NULL, false);
while (tmp && tmp->value) {
virLockSpaceResourcePtr res = (virLockSpaceResourcePtr)tmp->value;
-virJSONValuePtr child = virJSONValueNewObject();
-virJSONValuePtr owners = NULL;
+g_autoptr(virJSONValue) child = virJSONValueNewObject();
+g_autoptr(virJSONValue) owners = virJSONValueNewArray();
size_t i;
-if (virJSONValueArrayAppend(resources, child) < 0) {
-virJSONValueFree(child);
-goto error;
-}
-
if (virJSONValueObjectAppendString(child, "name", res->name) < 0 ||
virJSONValueObjectAppendString(child, "path", res->path) < 0 ||
virJSONValueObjectAppendNumberInt(child, "fd", res->fd) < 0 ||
@@ -460,34 +450,35 @@ virJSONValuePtr
virLockSpacePreExecRestart(virLockSpacePtr lockspace)
goto error;
}
-owners = virJSONValueNewArray();
-
-if (virJSONValueObjectAppend(child, "owners", owners) < 0) {
-virJSONValueFree(owners);
-goto error;
-}
-
for (i = 0; i < res->nOwners; i++) {
-virJSONValuePtr owner = virJSONValueNewNumberUlong(res->owners[i]);
+g_autoptr(virJSONValue) owner =
virJSONValueNewNumberUlong(res->owners[i]);
if (!owner)
goto error;
-if (virJSONValueArrayAppend(owners, owner) < 0) {
-virJSONValueFree(owner);
+if (virJSONValueArrayAppend(owners, owner) < 0)
goto error;
-}
+owner = NULL;
}
+if (virJSONValueObjectAppend(child, "owners", owners) < 0)
+goto error;
+owners = NULL;
+
+if (virJSONValueArrayAppend(resources, child) < 0)
+goto error;
+child = NULL;
+
tmp++;
}
-VIR_FREE(pairs);
+
+if (virJSONValueObjectAppend(object, "resources", resources) < 0)
+goto error;
+resources = NULL;
virMutexUnlock(&lockspace->lock);
return object;
error:
-VIR_FREE(pairs);
-virJSONValueFree(object);
virMutexUnlock(&lockspace->lock);
return NULL;
}
--
2.29.2
[PATCH 10/25] qemuAgentMakeCommand: Refactor memory cleanup
Switch to using the 'g_auto*' helpers.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_agent.c | 29 -
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 51cc00c618..4712aeb529 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1149,31 +1149,26 @@ static virJSONValuePtr G_GNUC_NULL_TERMINATED
qemuAgentMakeCommand(const char *cmdname,
...)
{
-virJSONValuePtr obj = virJSONValueNewObject();
-virJSONValuePtr jargs = NULL;
+g_autoptr(virJSONValue) obj = NULL;
+g_autoptr(virJSONValue) jargs = NULL;
va_list args;
va_start(args, cmdname);
-if (virJSONValueObjectAppendString(obj, "execute", cmdname) < 0)
-goto error;
-
-if (virJSONValueObjectCreateVArgs(&jargs, args) < 0)
-goto error;
-
-if (jargs &&
-virJSONValueObjectAppend(obj, "arguments", jargs) < 0)
-goto error;
+if (virJSONValueObjectCreateVArgs(&jargs, args) < 0) {
+va_end(args);
+return NULL;
+}
va_end(args);
-return obj;
+if (virJSONValueObjectCreate(&obj,
+ "s:execute", cmdname,
+ "A:arguments", &jargs,
+ NULL) < 0)
+return NULL;
- error:
-virJSONValueFree(obj);
-virJSONValueFree(jargs);
-va_end(args);
-return NULL;
+return g_steal_pointer(&obj);
}
static virJSONValuePtr
--
2.29.2
[PATCH 07/25] virNetServerPreExecRestart: Drop error reporting from virJSONValueObjectAppend* calls
The functions report errors already and the error can nowadays only
happen on programmer errors (if the passed virJSONValue isn't an
object), which won't happen. Remove the reporting.
Signed-off-by: Peter Krempa
---
src/rpc/virnetserver.c | 42 ++
1 file changed, 10 insertions(+), 32 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index f0b866a962..ab8dafb1bb 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -556,51 +556,29 @@ virJSONValuePtr
virNetServerPreExecRestart(virNetServerPtr srv)
virObjectLock(srv);
if (virJSONValueObjectAppendNumberUint(object, "min_workers",
-
virThreadPoolGetMinWorkers(srv->workers)) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot set min_workers data in JSON document"));
+
virThreadPoolGetMinWorkers(srv->workers)) < 0)
goto error;
-}
if (virJSONValueObjectAppendNumberUint(object, "max_workers",
-
virThreadPoolGetMaxWorkers(srv->workers)) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot set max_workers data in JSON document"));
+
virThreadPoolGetMaxWorkers(srv->workers)) < 0)
goto error;
-}
if (virJSONValueObjectAppendNumberUint(object, "priority_workers",
-
virThreadPoolGetPriorityWorkers(srv->workers)) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot set priority_workers data in JSON document"));
+
virThreadPoolGetPriorityWorkers(srv->workers)) < 0)
goto error;
-}
-if (virJSONValueObjectAppendNumberUint(object, "max_clients",
srv->nclients_max) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot set max_clients data in JSON document"));
+
+if (virJSONValueObjectAppendNumberUint(object, "max_clients",
srv->nclients_max) < 0)
goto error;
-}
if (virJSONValueObjectAppendNumberUint(object, "max_anonymous_clients",
- srv->nclients_unauth_max) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot set max_anonymous_clients data in JSON
document"));
+ srv->nclients_unauth_max) < 0)
goto error;
-}
-if (virJSONValueObjectAppendNumberUint(object, "keepaliveInterval",
srv->keepaliveInterval) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot set keepaliveInterval data in JSON
document"));
+
+if (virJSONValueObjectAppendNumberUint(object, "keepaliveInterval",
srv->keepaliveInterval) < 0)
goto error;
-}
-if (virJSONValueObjectAppendNumberUint(object, "keepaliveCount",
srv->keepaliveCount) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot set keepaliveCount data in JSON document"));
+if (virJSONValueObjectAppendNumberUint(object, "keepaliveCount",
srv->keepaliveCount) < 0)
goto error;
-}
if (virJSONValueObjectAppendNumberUlong(object, "next_client_id",
-srv->next_client_id) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot set next_client_id data in JSON document"));
+srv->next_client_id) < 0)
goto error;
-}
services = virJSONValueNewArray();
--
2.29.2
[PATCH 08/25] virNetServerPreExecRestart: Refactor memory cleanup
Switch to using the 'g_auto*' helpers.
Signed-off-by: Peter Krempa
---
src/rpc/virnetserver.c | 41 -
1 file changed, 16 insertions(+), 25 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index ab8dafb1bb..ee402e8ea0 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -548,9 +548,9 @@ virNetServerPtr
virNetServerNewPostExecRestart(virJSONValuePtr object,
virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv)
{
-virJSONValuePtr object = virJSONValueNewObject();
-virJSONValuePtr clients;
-virJSONValuePtr services;
+g_autoptr(virJSONValue) object = virJSONValueNewObject();
+g_autoptr(virJSONValue) clients = virJSONValueNewArray();
+g_autoptr(virJSONValue) services = virJSONValueNewArray();
size_t i;
virObjectLock(srv);
@@ -580,48 +580,39 @@ virJSONValuePtr
virNetServerPreExecRestart(virNetServerPtr srv)
srv->next_client_id) < 0)
goto error;
-services = virJSONValueNewArray();
-
-if (virJSONValueObjectAppend(object, "services", services) < 0) {
-virJSONValueFree(services);
-goto error;
-}
-
for (i = 0; i < srv->nservices; i++) {
-virJSONValuePtr child;
+g_autoptr(virJSONValue) child = NULL;
if (!(child = virNetServerServicePreExecRestart(srv->services[i])))
goto error;
-if (virJSONValueArrayAppend(services, child) < 0) {
-virJSONValueFree(child);
+if (virJSONValueArrayAppend(services, child) < 0)
goto error;
-}
+child = NULL;
}
-clients = virJSONValueNewArray();
-
-if (virJSONValueObjectAppend(object, "clients", clients) < 0) {
-virJSONValueFree(clients);
+if (virJSONValueObjectAppend(object, "services", services) < 0)
goto error;
-}
+services = NULL;
for (i = 0; i < srv->nclients; i++) {
-virJSONValuePtr child;
+g_autoptr(virJSONValue) child = NULL;
if (!(child = virNetServerClientPreExecRestart(srv->clients[i])))
goto error;
-if (virJSONValueArrayAppend(clients, child) < 0) {
-virJSONValueFree(child);
+if (virJSONValueArrayAppend(clients, child) < 0)
goto error;
-}
+child = NULL;
}
+if (virJSONValueObjectAppend(object, "clients", clients) < 0)
+goto error;
+clients = NULL;
+
virObjectUnlock(srv);
-return object;
+return g_steal_pointer(&object);
error:
-virJSONValueFree(object);
virObjectUnlock(srv);
return NULL;
}
--
2.29.2
[PATCH 06/25] virNetServerClientPreExecRestart: Refactor memory cleanup
Switch to using the 'g_auto*' helpers.
Signed-off-by: Peter Krempa
---
src/rpc/virnetserverclient.c | 26 +++---
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 4d01e87e21..0789ad9154 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -585,15 +585,14 @@ virNetServerClientPtr
virNetServerClientNewPostExecRestart(virNetServerPtr srv,
virJSONValuePtr virNetServerClientPreExecRestart(virNetServerClientPtr client)
{
-virJSONValuePtr object = virJSONValueNewObject();
-virJSONValuePtr child;
+g_autoptr(virJSONValue) object = virJSONValueNewObject();
+g_autoptr(virJSONValue) sock = NULL;
+g_autoptr(virJSONValue) priv = NULL;
virObjectLock(client);
-if (virJSONValueObjectAppendNumberUlong(object, "id",
-client->id) < 0)
+if (virJSONValueObjectAppendNumberUlong(object, "id", client->id) < 0)
goto error;
-
if (virJSONValueObjectAppendNumberInt(object, "auth", client->auth) < 0)
goto error;
if (virJSONValueObjectAppendBoolean(object, "auth_pending",
client->auth_pending) < 0)
@@ -608,28 +607,25 @@ virJSONValuePtr
virNetServerClientPreExecRestart(virNetServerClientPtr client)
client->conn_time) < 0)
goto error;
-if (!(child = virNetSocketPreExecRestart(client->sock)))
+if (!(sock = virNetSocketPreExecRestart(client->sock)))
goto error;
-if (virJSONValueObjectAppend(object, "sock", child) < 0) {
-virJSONValueFree(child);
+if (virJSONValueObjectAppend(object, "sock", sock) < 0)
goto error;
-}
+sock = NULL;
-if (!(child = client->privateDataPreExecRestart(client,
client->privateData)))
+if (!(priv = client->privateDataPreExecRestart(client,
client->privateData)))
goto error;
-if (virJSONValueObjectAppend(object, "privateData", child) < 0) {
-virJSONValueFree(child);
+if (virJSONValueObjectAppend(object, "privateData", priv) < 0)
goto error;
-}
+priv = NULL;
virObjectUnlock(client);
-return object;
+return g_steal_pointer(&object);
error:
virObjectUnlock(client);
-virJSONValueFree(object);
return NULL;
}
--
2.29.2
[PATCH 05/25] virNetServerServicePreExecRestart: Refactor memory cleanup
Switch to using the 'g_auto*' helpers.
Signed-off-by: Peter Krempa
---
src/rpc/virnetserverservice.c | 36 ++-
1 file changed, 14 insertions(+), 22 deletions(-)
diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
index 73232e3747..a72277226a 100644
--- a/src/rpc/virnetserverservice.c
+++ b/src/rpc/virnetserverservice.c
@@ -336,40 +336,32 @@ virNetServerServicePtr
virNetServerServiceNewPostExecRestart(virJSONValuePtr obj
virJSONValuePtr virNetServerServicePreExecRestart(virNetServerServicePtr svc)
{
-virJSONValuePtr object = virJSONValueNewObject();
-virJSONValuePtr socks;
+g_autoptr(virJSONValue) object = virJSONValueNewObject();
+g_autoptr(virJSONValue) socks = virJSONValueNewArray();
size_t i;
if (virJSONValueObjectAppendNumberInt(object, "auth", svc->auth) < 0)
-goto error;
+return NULL;
if (virJSONValueObjectAppendBoolean(object, "readonly", svc->readonly) < 0)
-goto error;
+return NULL;
if (virJSONValueObjectAppendNumberUint(object, "nrequests_client_max",
svc->nrequests_client_max) < 0)
-goto error;
-
-socks = virJSONValueNewArray();
-
-if (virJSONValueObjectAppend(object, "socks", socks) < 0) {
-virJSONValueFree(socks);
-goto error;
-}
+return NULL;
for (i = 0; i < svc->nsocks; i++) {
-virJSONValuePtr child;
+g_autoptr(virJSONValue) child = NULL;
if (!(child = virNetSocketPreExecRestart(svc->socks[i])))
-goto error;
+return NULL;
-if (virJSONValueArrayAppend(socks, child) < 0) {
-virJSONValueFree(child);
-goto error;
-}
+if (virJSONValueArrayAppend(socks, child) < 0)
+return NULL;
+child = NULL;
}
-return object;
+if (virJSONValueObjectAppend(object, "socks", socks) < 0)
+return NULL;
+socks = NULL;
- error:
-virJSONValueFree(object);
-return NULL;
+return g_steal_pointer(&object);
}
--
2.29.2
[PATCH 04/25] virNetDaemonPreExecRestart: Refactor memory cleanup
Switch to using the 'g_auto*' helpers.
Signed-off-by: Peter Krempa
---
src/rpc/virnetdaemon.c | 27 +++
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 2c18da790b..6132615093 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -384,23 +384,18 @@ virJSONValuePtr
virNetDaemonPreExecRestart(virNetDaemonPtr dmn)
{
size_t i = 0;
-virJSONValuePtr object = virJSONValueNewObject();
-virJSONValuePtr srvObj = virJSONValueNewObject();
-virHashKeyValuePairPtr srvArray = NULL;
+g_autoptr(virJSONValue) object = virJSONValueNewObject();
+g_autoptr(virJSONValue) srvObj = virJSONValueNewObject();
+g_autofree virHashKeyValuePairPtr srvArray = NULL;
virObjectLock(dmn);
-if (virJSONValueObjectAppend(object, "servers", srvObj) < 0) {
-virJSONValueFree(srvObj);
-goto error;
-}
-
if (!(srvArray = virHashGetItems(dmn->servers, NULL, true)))
goto error;
for (i = 0; srvArray[i].key; i++) {
virNetServerPtr server = virHashLookup(dmn->servers, srvArray[i].key);
-virJSONValuePtr srvJSON;
+g_autoptr(virJSONValue) srvJSON = NULL;
if (!server)
goto error;
@@ -409,20 +404,20 @@ virNetDaemonPreExecRestart(virNetDaemonPtr dmn)
if (!srvJSON)
goto error;
-if (virJSONValueObjectAppend(srvObj, srvArray[i].key, srvJSON) < 0) {
-virJSONValueFree(srvJSON);
+if (virJSONValueObjectAppend(srvObj, srvArray[i].key, srvJSON) < 0)
goto error;
-}
+srvJSON = NULL;
}
-VIR_FREE(srvArray);
virObjectUnlock(dmn);
-return object;
+if (virJSONValueObjectAppend(object, "servers", srvObj) < 0)
+return NULL;
+srvObj = NULL;
+
+return g_steal_pointer(&object);
error:
-VIR_FREE(srvArray);
-virJSONValueFree(object);
virObjectUnlock(dmn);
return NULL;
}
--
2.29.2
[PATCH 03/25] virLogHandlerPreExecRestart: Refactor memory cleanup
Switch to using the 'g_auto*' helpers.
Signed-off-by: Peter Krempa
---
src/logging/log_handler.c | 42 ---
1 file changed, 17 insertions(+), 25 deletions(-)
diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
index a77c1e0250..cacf9584cd 100644
--- a/src/logging/log_handler.c
+++ b/src/logging/log_handler.c
@@ -608,56 +608,48 @@ virLogHandlerDomainAppendLogFile(virLogHandlerPtr handler,
virJSONValuePtr
virLogHandlerPreExecRestart(virLogHandlerPtr handler)
{
-virJSONValuePtr ret = virJSONValueNewObject();
-virJSONValuePtr files;
+g_autoptr(virJSONValue) ret = virJSONValueNewObject();
+g_autoptr(virJSONValue) files = virJSONValueNewArray();
size_t i;
char domuuid[VIR_UUID_STRING_BUFLEN];
-files = virJSONValueNewArray();
-
-if (virJSONValueObjectAppend(ret, "files", files) < 0) {
-virJSONValueFree(files);
-goto error;
-}
-
for (i = 0; i < handler->nfiles; i++) {
-virJSONValuePtr file = virJSONValueNewObject();
-
-if (virJSONValueArrayAppend(files, file) < 0) {
-virJSONValueFree(file);
-goto error;
-}
+g_autoptr(virJSONValue) file = virJSONValueNewObject();
if (virJSONValueObjectAppendNumberInt(file, "pipefd",
handler->files[i]->pipefd) < 0)
-goto error;
+return NULL;
if (virJSONValueObjectAppendString(file, "path",
virRotatingFileWriterGetPath(handler->files[i]->file)) < 0)
-goto error;
+return NULL;
if (virJSONValueObjectAppendString(file, "driver",
handler->files[i]->driver) < 0)
-goto error;
+return NULL;
if (virJSONValueObjectAppendString(file, "domname",
handler->files[i]->domname) < 0)
-goto error;
+return NULL;
virUUIDFormat(handler->files[i]->domuuid, domuuid);
if (virJSONValueObjectAppendString(file, "domuuid", domuuid) < 0)
-goto error;
+return NULL;
if (virSetInherit(handler->files[i]->pipefd, true) < 0) {
virReportSystemError(errno, "%s",
_("Cannot disable close-on-exec flag"));
-goto error;
+return NULL;
}
+
+if (virJSONValueArrayAppend(files, file) < 0)
+return NULL;
+file = NULL;
}
-return ret;
+if (virJSONValueObjectAppend(ret, "files", files) < 0)
+return NULL;
+files = NULL;
- error:
-virJSONValueFree(ret);
-return NULL;
+return g_steal_pointer(&ret);
}
--
2.29.2
[PATCH 01/25] virLockDaemonPreExecRestart: Refactor memory cleanup
Switch to using the 'g_auto*' helpers.
Signed-off-by: Peter Krempa
---
src/locking/lock_daemon.c | 81 +--
1 file changed, 35 insertions(+), 46 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index 94fe374df6..5913c0cb9c 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -697,87 +697,76 @@ virLockDaemonPreExecRestart(const char *state_file,
virNetDaemonPtr dmn,
char **argv)
{
-virJSONValuePtr child;
-char *state = NULL;
-virJSONValuePtr object = virJSONValueNewObject();
-char *magic = NULL;
-virHashKeyValuePairPtr pairs = NULL, tmp;
-virJSONValuePtr lockspaces;
+g_autoptr(virJSONValue) object = virJSONValueNewObject();
+g_autoptr(virJSONValue) lockspaces = virJSONValueNewArray();
+g_autoptr(virJSONValue) defaultLockspace = NULL;
+g_autoptr(virJSONValue) daemon = NULL;
+g_autofree char *state = NULL;
+g_autofree char *magic = NULL;
+g_autofree virHashKeyValuePairPtr pairs = NULL;
+virHashKeyValuePairPtr tmp;
VIR_DEBUG("Running pre-restart exec");
-if (!(child = virNetDaemonPreExecRestart(dmn)))
-goto cleanup;
-
-if (virJSONValueObjectAppend(object, "daemon", child) < 0) {
-virJSONValueFree(child);
-goto cleanup;
-}
-
-if (!(child = virLockSpacePreExecRestart(lockDaemon->defaultLockspace)))
-goto cleanup;
-
-if (virJSONValueObjectAppend(object, "defaultLockspace", child) < 0) {
-virJSONValueFree(child);
-goto cleanup;
-}
+if (!(daemon = virNetDaemonPreExecRestart(dmn)))
+return -1;
-lockspaces = virJSONValueNewArray();
+if (virJSONValueObjectAppend(object, "daemon", daemon) < 0)
+return -1;
+daemon = NULL;
-if (virJSONValueObjectAppend(object, "lockspaces", lockspaces) < 0) {
-virJSONValueFree(lockspaces);
-goto cleanup;
-}
+if (!(defaultLockspace =
virLockSpacePreExecRestart(lockDaemon->defaultLockspace)))
+return -1;
+if (virJSONValueObjectAppend(object, "defaultLockspace", defaultLockspace)
< 0)
+return -1;
+defaultLockspace = NULL;
tmp = pairs = virHashGetItems(lockDaemon->lockspaces, NULL, false);
while (tmp && tmp->key) {
virLockSpacePtr lockspace = (virLockSpacePtr)tmp->value;
+g_autoptr(virJSONValue) child = NULL;
if (!(child = virLockSpacePreExecRestart(lockspace)))
-goto cleanup;
+return -1;
-if (virJSONValueArrayAppend(lockspaces, child) < 0) {
-virJSONValueFree(child);
-goto cleanup;
-}
+if (virJSONValueArrayAppend(lockspaces, child) < 0)
+return -1;
+child = NULL;
tmp++;
}
+if (virJSONValueObjectAppend(object, "lockspaces", lockspaces) < 0)
+return -1;
+lockspaces = NULL;
+
if (!(magic = virLockDaemonGetExecRestartMagic()))
-goto cleanup;
+return -1;
if (virJSONValueObjectAppendString(object, "magic", magic) < 0)
-goto cleanup;
+return -1;
if (!(state = virJSONValueToString(object, true)))
-goto cleanup;
+return -1;
VIR_DEBUG("Saving state %s", state);
-if (virFileWriteStr(state_file,
-state, 0700) < 0) {
+if (virFileWriteStr(state_file, state, 0700) < 0) {
virReportSystemError(errno,
- _("Unable to save state file %s"),
- state_file);
-goto cleanup;
+ _("Unable to save state file %s"), state_file);
+return -1;
}
if (execvp(argv[0], argv) < 0) {
virReportSystemError(errno, "%s",
_("Unable to restart self"));
-goto cleanup;
+return -1;
}
abort(); /* This should be impossible to reach */
- cleanup:
-VIR_FREE(magic);
-VIR_FREE(pairs);
-VIR_FREE(state);
-virJSONValueFree(object);
-return -1;
+return 0;
}
--
2.29.2
[PATCH 00/25] Clear pointers in virJSONValue(Object|Array)Append and other cleanups
Peter Krempa (25): virLockDaemonPreExecRestart: Refactor memory cleanup virLogDaemonPreExecRestart: Refactor memory cleanup virLogHandlerPreExecRestart: Refactor memory cleanup virNetDaemonPreExecRestart: Refactor memory cleanup virNetServerServicePreExecRestart: Refactor memory cleanup virNetServerClientPreExecRestart: Refactor memory cleanup virNetServerPreExecRestart: Drop error reporting from virJSONValueObjectAppend* calls virNetServerPreExecRestart: Refactor memory cleanup virLockSpacePreExecRestart: Refactor memory cleanup qemuAgentMakeCommand: Refactor memory cleanup virJSONValueObjectInsert: Clear @value on successful insertion virJSONValueCopy: Don't use virJSONValue(Object|Array)Append virJSONValue(Array|Object)Append*: Simplify handling of appended object virJSONValueNewArrayFromBitmap: Refactor cleanup virJSONValueObjectAddVArgs: Use autofree for the temporary bitmap virJSONValueObjectAppend: Clear pointer when taking ownership of passed value qemuAgentMakeStringsArray: Refactor cleanup virMACMapHashDumper: Refactor array addition testQEMUSchemaValidateObjectMergeVariantMember: Fix theoretical leak virJSONValueArrayAppend: Clear pointer when taking ownership of passed value qemuMonitorJSONTransactionAdd: Refactor cleanup qemuAgentSetVCPUsCommand: Refactor cleanup virJSONParserHandle*: Refactor memory cleanup and drop NULL checks virJSONValueNewNumber: Take ownership of passed string virJSONParserInsertValue: Take double pointer for @value src/locking/lock_daemon.c| 77 src/logging/log_daemon.c | 52 +++--- src/logging/log_handler.c| 40 ++-- src/network/leaseshelper.c | 3 +- src/node_device/node_device_driver.c | 4 +- src/qemu/qemu_agent.c| 89 - src/qemu/qemu_block.c| 19 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_firmware.c | 27 +-- src/qemu/qemu_migration_params.c | 4 +- src/qemu/qemu_monitor_json.c | 58 +++--- src/rpc/virnetdaemon.c | 25 +-- src/rpc/virnetserver.c | 79 +++- src/rpc/virnetserverclient.c | 24 +-- src/rpc/virnetserverservice.c| 34 ++-- src/util/virjson.c | 261 +++ src/util/virjson.h | 7 +- src/util/virlease.c | 2 +- src/util/virlockspace.c | 47 ++--- src/util/virmacmap.c | 13 +- tests/testutilsqemuschema.c | 4 +- 21 files changed, 322 insertions(+), 550 deletions(-) -- 2.29.2
[PATCH 02/25] virLogDaemonPreExecRestart: Refactor memory cleanup
Switch to using the 'g_auto*' helpers.
Signed-off-by: Peter Krempa
---
src/logging/log_daemon.c | 54 +---
1 file changed, 23 insertions(+), 31 deletions(-)
diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
index 770f6dd273..ceffa6a368 100644
--- a/src/logging/log_daemon.c
+++ b/src/logging/log_daemon.c
@@ -505,62 +505,54 @@ virLogDaemonPreExecRestart(const char *state_file,
virNetDaemonPtr dmn,
char **argv)
{
-virJSONValuePtr child;
-char *state = NULL;
-virJSONValuePtr object = virJSONValueNewObject();
-char *magic = NULL;
+g_autoptr(virJSONValue) object = virJSONValueNewObject();
+g_autoptr(virJSONValue) daemon = NULL;
+g_autoptr(virJSONValue) handler = NULL;
+g_autofree char *state = NULL;
+g_autofree char *magic = NULL;
VIR_DEBUG("Running pre-restart exec");
-if (!(child = virNetDaemonPreExecRestart(dmn)))
-goto cleanup;
+if (!(daemon = virNetDaemonPreExecRestart(dmn)))
+return -1;
-if (virJSONValueObjectAppend(object, "daemon", child) < 0) {
-virJSONValueFree(child);
-goto cleanup;
-}
+if (virJSONValueObjectAppend(object, "daemon", daemon) < 0)
+return -1;
+daemon = NULL;
if (!(magic = virLogDaemonGetExecRestartMagic()))
-goto cleanup;
+return -1;
if (virJSONValueObjectAppendString(object, "magic", magic) < 0)
-goto cleanup;
-
-if (!(child = virLogHandlerPreExecRestart(logDaemon->handler)))
-goto cleanup;
+return -1;
-if (virJSONValueObjectAppend(object, "handler", child) < 0) {
-virJSONValueFree(child);
-goto cleanup;
-}
+if (!(handler = virLogHandlerPreExecRestart(logDaemon->handler)))
+return -1;
+if (virJSONValueObjectAppend(object, "handler", handler) < 0)
+return -1;
+handler = NULL;
if (!(state = virJSONValueToString(object, true)))
-goto cleanup;
+return -1;
VIR_DEBUG("Saving state %s", state);
-if (virFileWriteStr(state_file,
-state, 0700) < 0) {
+if (virFileWriteStr(state_file, state, 0700) < 0) {
virReportSystemError(errno,
- _("Unable to save state file %s"),
- state_file);
-goto cleanup;
+ _("Unable to save state file %s"), state_file);
+return -1;
}
if (execvp(argv[0], argv) < 0) {
virReportSystemError(errno, "%s",
_("Unable to restart self"));
-goto cleanup;
+return -1;
}
abort(); /* This should be impossible to reach */
- cleanup:
-VIR_FREE(magic);
-VIR_FREE(state);
-virJSONValueFree(object);
-return -1;
+return 0;
}
--
2.29.2
Re: [PATCH] qemu_shim: URI escape root directory
On Fri, Feb 12, 2021 at 05:42:19PM +0100, Michal Privoznik wrote: > The root directory can be provided by user (or a temporary one is > generated) and is always formatted into connection URI for both > secret driver and QEMU driver, like this: > > qemu:///embed?root=$root > > But if it so happens that there is an URI unfriendly character in > root directory or path to it (say a space) then invalid URI is > formatted which results in unexpected results. We can trust > g_dir_make_tmp() to generate valid URI but we can't trust user. > Escape user provided root directory. Always. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1920400 > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_shim.c | 9 ++--- > 1 file changed, 6 insertions(+), 3 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 :|
[PATCH] qemu_shim: URI escape root directory
The root directory can be provided by user (or a temporary one is
generated) and is always formatted into connection URI for both
secret driver and QEMU driver, like this:
qemu:///embed?root=$root
But if it so happens that there is an URI unfriendly character in
root directory or path to it (say a space) then invalid URI is
formatted which results in unexpected results. We can trust
g_dir_make_tmp() to generate valid URI but we can't trust user.
Escape user provided root directory. Always.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1920400
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_shim.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_shim.c b/src/qemu/qemu_shim.c
index 18bdc99256..c10598df4b 100644
--- a/src/qemu/qemu_shim.c
+++ b/src/qemu/qemu_shim.c
@@ -140,7 +140,8 @@ int main(int argc, char **argv)
g_autofree char *xml = NULL;
g_autofree char *uri = NULL;
g_autofree char *suri = NULL;
-char *root = NULL;
+const char *root = NULL;
+g_autofree char *escaped = NULL;
bool tmproot = false;
int ret = 1;
g_autoptr(GError) error = NULL;
@@ -216,6 +217,8 @@ int main(int argc, char **argv)
}
}
+escaped = g_uri_escape_string(root, NULL, true);
+
virFileActivateDirOverrideForProg(argv[0]);
if (verbose)
@@ -242,7 +245,7 @@ int main(int argc, char **argv)
eventLoopThread = g_thread_new("event-loop", qemuShimEventLoop, NULL);
if (secrets && *secrets) {
-suri = g_strdup_printf("secret:///embed?root=%s", root);
+suri = g_strdup_printf("secret:///embed?root=%s", escaped);
if (verbose)
g_printerr("%s: %lld: opening %s\n",
@@ -303,7 +306,7 @@ int main(int argc, char **argv)
}
}
-uri = g_strdup_printf("qemu:///embed?root=%s", root);
+uri = g_strdup_printf("qemu:///embed?root=%s", escaped);
if (verbose)
g_printerr("%s: %lld: opening %s\n",
--
2.26.2
[libvirt PATCH v2 2/3] ci: Add temporary workaround for Fedora Rawhide
The .repo files for Fedora Rawhide are already pointing to the Fedora 35 key, but all RPMs are still signed with the Fedora 34 key, resulting in GPG key at file:///etc/pki/rpm-gpg/RPM-GPG-KEY-fedora-35-x86_64 (0x9867C58F) is already installed The GPG keys listed for the "Fedora - Rawhide - Developmental packages for the next Fedora release" repository are already installed but they are not correct for this package. Check that the correct key URLs are configured for this repository.. Failing package is: nosync-1.1-10.fc34.x86_64 GPG Keys are configured as: file:///etc/pki/rpm-gpg/RPM-GPG-KEY-fedora-35-x86_64 The downloaded packages were saved in cache until the next successful transaction. You can remove cached packages by executing 'dnf clean packages'. Error: GPG check FAILED Temporarily tweak the .repo files so that the Fedora 34 key is used for validation. We should be able to revert this in a few days. Signed-off-by: Andrea Bolognani --- Test pipeline showing that, if this commit is reverted, building the Fedora Rawhide container images still fails even though we're updating fedora-gpg-keys first: https://gitlab.com/abologna/libvirt/-/pipelines/255494585 ci/containers/ci-fedora-rawhide-cross-mingw32.Dockerfile | 3 ++- ci/containers/ci-fedora-rawhide-cross-mingw64.Dockerfile | 3 ++- ci/containers/ci-fedora-rawhide.Dockerfile | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/ci/containers/ci-fedora-rawhide-cross-mingw32.Dockerfile b/ci/containers/ci-fedora-rawhide-cross-mingw32.Dockerfile index c718778acb..fd03378bdf 100644 --- a/ci/containers/ci-fedora-rawhide-cross-mingw32.Dockerfile +++ b/ci/containers/ci-fedora-rawhide-cross-mingw32.Dockerfile @@ -5,7 +5,8 @@ # https://gitlab.com/libvirt/libvirt-ci/-/commit/891c7d56be1d0eb5adaf78fced7d1d882d6f0b6a FROM registry.fedoraproject.org/fedora:rawhide -RUN dnf update -y --nogpgcheck fedora-gpg-keys && \ +RUN sed -Ei 's|^gpgkey=.*$|gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-fedora-34-x86_64|g' /etc/yum.repos.d/*.repo && \ +dnf update -y --nogpgcheck fedora-gpg-keys && \ dnf install -y nosync && \ echo -e '#!/bin/sh\n\ if test -d /usr/lib64\n\ diff --git a/ci/containers/ci-fedora-rawhide-cross-mingw64.Dockerfile b/ci/containers/ci-fedora-rawhide-cross-mingw64.Dockerfile index 6058d0c0b2..8a66d5e375 100644 --- a/ci/containers/ci-fedora-rawhide-cross-mingw64.Dockerfile +++ b/ci/containers/ci-fedora-rawhide-cross-mingw64.Dockerfile @@ -5,7 +5,8 @@ # https://gitlab.com/libvirt/libvirt-ci/-/commit/891c7d56be1d0eb5adaf78fced7d1d882d6f0b6a FROM registry.fedoraproject.org/fedora:rawhide -RUN dnf update -y --nogpgcheck fedora-gpg-keys && \ +RUN sed -Ei 's|^gpgkey=.*$|gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-fedora-34-x86_64|g' /etc/yum.repos.d/*.repo && \ +dnf update -y --nogpgcheck fedora-gpg-keys && \ dnf install -y nosync && \ echo -e '#!/bin/sh\n\ if test -d /usr/lib64\n\ diff --git a/ci/containers/ci-fedora-rawhide.Dockerfile b/ci/containers/ci-fedora-rawhide.Dockerfile index 027e8a7c41..fa6a824242 100644 --- a/ci/containers/ci-fedora-rawhide.Dockerfile +++ b/ci/containers/ci-fedora-rawhide.Dockerfile @@ -5,7 +5,8 @@ # https://gitlab.com/libvirt/libvirt-ci/-/commit/891c7d56be1d0eb5adaf78fced7d1d882d6f0b6a FROM registry.fedoraproject.org/fedora:rawhide -RUN dnf update -y --nogpgcheck fedora-gpg-keys && \ +RUN sed -Ei 's|^gpgkey=.*$|gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-fedora-34-x86_64|g' /etc/yum.repos.d/*.repo && \ +dnf update -y --nogpgcheck fedora-gpg-keys && \ dnf install -y nosync && \ echo -e '#!/bin/sh\n\ if test -d /usr/lib64\n\ -- 2.26.2
[libvirt PATCH v2 3/3] ci: Build on FreeBSD 12.2
The FreeBSD 12.1 image on Cirrus CI is currently broken, but that's okay because a FreeBSD 12.2 image is also available and we'd rather build on the more up-to-date target anyway. Signed-off-by: Andrea Bolognani --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index bb1d4d46b5..4563bccdf1 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -418,7 +418,7 @@ x64-freebsd-12-build: NAME: freebsd-12 CIRRUS_VM_INSTANCE_TYPE: freebsd_instance CIRRUS_VM_IMAGE_SELECTOR: image_family -CIRRUS_VM_IMAGE_NAME: freebsd-12-1 +CIRRUS_VM_IMAGE_NAME: freebsd-12-2 INSTALL_COMMAND: pkg install -y x64-macos-1015-build: -- 2.26.2
[libvirt PATCH v2 1/3] ci: Refresh Dockerfiles
Signed-off-by: Andrea Bolognani --- ci/containers/ci-centos-7.Dockerfile | 3 ++- ci/containers/ci-centos-8.Dockerfile | 3 ++- ci/containers/ci-centos-stream.Dockerfile| 3 ++- ci/containers/ci-debian-10-cross-aarch64.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-armv6l.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-armv7l.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-i686.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-mips.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-mips64el.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-mipsel.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-ppc64le.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-s390x.Dockerfile| 3 ++- ci/containers/ci-debian-10.Dockerfile| 3 ++- ci/containers/ci-debian-sid-cross-aarch64.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-armv6l.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-armv7l.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-i686.Dockerfile| 3 ++- ci/containers/ci-debian-sid-cross-mips64el.Dockerfile| 3 ++- ci/containers/ci-debian-sid-cross-mipsel.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-ppc64le.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-s390x.Dockerfile | 3 ++- ci/containers/ci-debian-sid.Dockerfile | 3 ++- ci/containers/ci-fedora-32.Dockerfile| 3 ++- ci/containers/ci-fedora-33.Dockerfile| 3 ++- ci/containers/ci-fedora-rawhide-cross-mingw32.Dockerfile | 7 --- ci/containers/ci-fedora-rawhide-cross-mingw64.Dockerfile | 7 --- ci/containers/ci-fedora-rawhide.Dockerfile | 7 --- ci/containers/ci-opensuse-152.Dockerfile | 3 ++- ci/containers/ci-ubuntu-1804.Dockerfile | 3 ++- ci/containers/ci-ubuntu-2004.Dockerfile | 3 ++- 30 files changed, 66 insertions(+), 36 deletions(-) diff --git a/ci/containers/ci-centos-7.Dockerfile b/ci/containers/ci-centos-7.Dockerfile index a847e9135b..c499e7a19d 100644 --- a/ci/containers/ci-centos-7.Dockerfile +++ b/ci/containers/ci-centos-7.Dockerfile @@ -2,7 +2,7 @@ # # $ lcitool dockerfile centos-7 libvirt # -# https://gitlab.com/libvirt/libvirt-ci/-/commit/740f5254f607de914a92d664196d045149edb45a +# https://gitlab.com/libvirt/libvirt-ci/-/commit/891c7d56be1d0eb5adaf78fced7d1d882d6f0b6a FROM docker.io/library/centos:7 RUN yum update -y && \ @@ -65,6 +65,7 @@ WEiJKtQrZDJloqtyi/mmRa1VsV7RYR0VPJjhK/R8EQ7Ysshy\n\ glusterfs-api-devel \ gnutls-devel \ iproute \ +iptables \ iscsi-initiator-utils \ kmod \ libacl-devel \ diff --git a/ci/containers/ci-centos-8.Dockerfile b/ci/containers/ci-centos-8.Dockerfile index 5b81fb8f27..e600598329 100644 --- a/ci/containers/ci-centos-8.Dockerfile +++ b/ci/containers/ci-centos-8.Dockerfile @@ -2,7 +2,7 @@ # # $ lcitool dockerfile centos-8 libvirt # -# https://gitlab.com/libvirt/libvirt-ci/-/commit/740f5254f607de914a92d664196d045149edb45a +# https://gitlab.com/libvirt/libvirt-ci/-/commit/891c7d56be1d0eb5adaf78fced7d1d882d6f0b6a FROM docker.io/library/centos:8 RUN dnf update -y && \ @@ -37,6 +37,7 @@ RUN dnf update -y && \ gnutls-devel \ iproute \ iproute-tc \ +iptables \ iscsi-initiator-utils \ kmod \ libacl-devel \ diff --git a/ci/containers/ci-centos-stream.Dockerfile b/ci/containers/ci-centos-stream.Dockerfile index e286857e00..2b51eccc8d 100644 --- a/ci/containers/ci-centos-stream.Dockerfile +++ b/ci/containers/ci-centos-stream.Dockerfile @@ -2,7 +2,7 @@ # # $ lcitool dockerfile centos-stream libvirt # -# https://gitlab.com/libvirt/libvirt-ci/-/commit/740f5254f607de914a92d664196d045149edb45a +# https://gitlab.com/libvirt/libvirt-ci/-/commit/891c7d56be1d0eb5adaf78fced7d1d882d6f0b6a FROM docker.io/library/centos:8 RUN dnf install -y centos-release-stream && \ @@ -39,6 +39,7 @@ RUN dnf install -y centos-release-stream && \ gnutls-devel \ iproute \ iproute-tc \ +iptables \ iscsi-initiator-utils \ kmod \ libacl-devel \ diff --git a/ci/containers/ci-debian-10-cross-aarch64.Dockerfile b/ci/containers/ci-debian-10-cross-aarch64.Dockerfile index 53a3e42951..272c809f17 100644 --- a/ci/containers/ci-debian-10-cross-aarch64.Dockerfile +++ b/ci/containers/ci-debian-10-cross-aarch64.Dockerfile @@ -2,7 +2,7 @@ # # $ lcitool dockerfile --cross aarch64 debian-10 libvirt # -# https://gitlab.com/libvirt/libvirt-ci/-/commit/740f5254f607de914a92d664196d045149edb45a +# https://gitlab.com/libvirt/libvirt-ci/-/commit/891c7d56be1d0eb5adaf78fced7d1d882d6f0b6a FROM docker.io/library/debian:10-slim RUN export DEBIAN_FRONTEND=noninteractive && \ @@ -25,6 +25,7 @@ RUN expor
[libvirt PATCH v2 0/3] ci: Paint the pipeline green
Various workarounds that are necessary due to breakages in external services and distribution archives, plus fixes for a couple of issues that were discovered in the process. Changes from [v1]: * the first three patches have been dropped from the series as they've been pushed already; * Dockerfiles have been refreshed using a more recent version of lcitool. [v1] https://listman.redhat.com/archives/libvir-list/2021-February/msg00664.html Andrea Bolognani (3): ci: Refresh Dockerfiles ci: Add temporary workaround for Fedora Rawhide ci: Build on FreeBSD 12.2 .gitlab-ci.yml | 2 +- ci/containers/ci-centos-7.Dockerfile | 3 ++- ci/containers/ci-centos-8.Dockerfile | 3 ++- ci/containers/ci-centos-stream.Dockerfile| 3 ++- ci/containers/ci-debian-10-cross-aarch64.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-armv6l.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-armv7l.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-i686.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-mips.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-mips64el.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-mipsel.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-ppc64le.Dockerfile | 3 ++- ci/containers/ci-debian-10-cross-s390x.Dockerfile| 3 ++- ci/containers/ci-debian-10.Dockerfile| 3 ++- ci/containers/ci-debian-sid-cross-aarch64.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-armv6l.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-armv7l.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-i686.Dockerfile| 3 ++- ci/containers/ci-debian-sid-cross-mips64el.Dockerfile| 3 ++- ci/containers/ci-debian-sid-cross-mipsel.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-ppc64le.Dockerfile | 3 ++- ci/containers/ci-debian-sid-cross-s390x.Dockerfile | 3 ++- ci/containers/ci-debian-sid.Dockerfile | 3 ++- ci/containers/ci-fedora-32.Dockerfile| 3 ++- ci/containers/ci-fedora-33.Dockerfile| 3 ++- ci/containers/ci-fedora-rawhide-cross-mingw32.Dockerfile | 8 +--- ci/containers/ci-fedora-rawhide-cross-mingw64.Dockerfile | 8 +--- ci/containers/ci-fedora-rawhide.Dockerfile | 8 +--- ci/containers/ci-opensuse-152.Dockerfile | 3 ++- ci/containers/ci-ubuntu-1804.Dockerfile | 3 ++- ci/containers/ci-ubuntu-2004.Dockerfile | 3 ++- 31 files changed, 70 insertions(+), 37 deletions(-) -- 2.26.2
Re: [libvirt PATCH 00/10] [RFC] clang-tidy CI integration
On Fri, 2021-02-12 at 14:12 +, Daniel P. Berrangé wrote:
> On Fri, Feb 12, 2021 at 02:25:24PM +0100, Tim Wiederhake wrote:
> > "clang-tidy" is a static code analysis tool for c and c++ code. See
> > https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html
> > for some issues found by clang-tidy and more background
> > information.
> >
> > Meson has support for clang-tidy and generates target named "clang-
> > tidy" if
> > it finds a ".clang-tidy" configuration file in the project's root
> > directory.
> >
> > There are some problems with this approach though, with regards to
> > inclusion
> > in GitLab's CI:
> >
> > * Single-threaded runtime of a complete scan takes between 95 and
> > 110 minutes,
> > depending on the enabled checks, which is significantly longer than
> > GitLab's
> > maximum run time of 60 minutes, after which jobs are aborted.
>
> IIUC, you can override the timeout setting per job...
IMO, extending the "hard" timeout for the job is not an option anyway,
for the reason presented in the next point:
>
> > * Even without this limit on runtime, this new check would double
> > to triple
> > the run time of the libVirt pipeline in GitLab.
>
↑ This one.
> Yeah that's a problem, but bear in mind there are two separate
> scenarios
>
> Running post merge we need to check all files, but the length of time
> is a non-issue since no one is waiting for a post-merge check to
> complete.
I am vary of the possibility of the pre-merge job running successfully
(e.g. because it ran into timeout before finding a certain issue) and
then failing for the post-merge job. One idea would be to set the post-
merge job to "allow-failure: true", i.e. making it a "notification"
only. But I am absolutely unsure on what the best approach is here,
hence the "RFC".
> Running pre- merge we only need to check files which are modified by
> the patches on the current branch. That ought to be orders of
> magnitude
> faster.
I see where you are coming from, but I believe this does not catch
cases where e.g. a change in a header file leads to a clang-tidy issue
in an otherwise untouched .c file, or vice versa.
The approach to cache files keyed on the result of the preprocessor
stage ("hash(gcc -E file.c)") on the other hand does catch this kind of
changes and reruns clang-tidy for all affected files.
> > * clang-tidy finds a lot of false positives (see link above for
> > explanation)
> > and has checks that contradict the libVirt code style (e.g. braces
> > around
> > blocks). This makes a quite complex configuration in ".clang-tidy"
> > necessary.
>
> IMHO for code checkers to add real value to people you need the
> existing
> codebase in git to be 100% clean.
>
> If you're not going to make the existing code compliant, then users
> will
> never be sure of what they should do for new code. We see this all
> the
> time in QEMU where its patch checker complains about problems that
> were
> pre-existing.
>
> If we could get clang-tidy to do a subset of checks where we can make
> libvirt 100% clean
I believe there is a misunderstanding here, please see explanation
below.
> > * I was unable to make clang-tidy recognize the settings from the
> > configuration file for generated files, leading clang-tidy to
> > always add some
> > checks. These checks were among those that produced false
> > positives.
> >
> > * The list of enabled / disabled checks in the yaml configuration
> > file is a
> > quite long string, making it hard to weave in some comments /
> > documentation
> > on which checks are enabled / disabled for what reason.
> >
> > This series introduces a new script, "run-clang-tidy.py". This is a
> > replacement for the script of the same name from clang-tools-extra.
> > It offers
> > parallel execution, caching of results and a configurable soft-
> > timeout.
> >
> > Please see the individual commits for more details. Comments
> > welcome.
> >
> > https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang-
> > tidy".
>
> What am I missing here - that job just seems to show a list of
> filenames, but isn't reporting any issues for them ?
There are three "types" of checks:
* Irrelevant checks that we want to disable permanently, e.g. checks
for some c++ specific issue that does not apply to c code,
* Checks that the libvirt code base currently passes,
* Checks that the libvirt code base currently fails.
Path #09 disables the first and last category of checks. Note that the
list is not defined in terms of "enable these checks" but "disable
these checks". The idea is to have checks that are introduced in new
versions of clang-tidy enabled by default and disabled checks commented
on and justified.
This is why you see no findings in the output -- there are none,
currently only "passing" checks are enabled. I disabled all failing
checks indiscriminately, as a discussion of which particular checks are
actually useful and which are not would distract from the general
question of
Re: [libvirt PATCH 00/10] [RFC] clang-tidy CI integration
On Fri, 2021-02-12 at 14:48 +0100, Ján Tomko wrote: > On a Friday in 2021, Tim Wiederhake wrote: > > "clang-tidy" is a static code analysis tool for c and c++ code. See > > https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html > > for some issues found by clang-tidy and more background > > information. > > > > Meson has support for clang-tidy and generates target named "clang- > > tidy" if > > it finds a ".clang-tidy" configuration file in the project's root > > directory. > > > > There are some problems with this approach though, with regards to > > inclusion > > in GitLab's CI: > > > > * Single-threaded runtime of a complete scan takes between 95 and > > 110 minutes, > > depending on the enabled checks, which is significantly longer than > > GitLab's > > maximum run time of 60 minutes, after which jobs are aborted. > > > > That does seem like a deal-breaker. Did you manage to do a successful > run by iteratively building up the cache over multiple jobs? Or by > doing > a multi-threaded run? > I combined the soft-timeout (./scripts/run-clang-tidy.py --timeout ...) together with caching. What this does is to stop scanning after the specified amount of time, but will keep the cache. Consecutive runs will pick up the cache and hopefully get further down the list of scanned files before running out of time. This process converges at a point where there are so few (if any) uncached results left, that the scan completes. The benefit of this approach is, that it actually works, takes no longer than 30 minutes and does not require one to click "re-run" several times until the job becomes green. And in the best case where no code file changed aka. all cache files are recent, this reduces the run time of the clang-tidy job to roughly three minutes; see link to pipeline below. The first iteration did run into timeout, the second one finished within the time window. I clicked "rerun" a second time to get the minimum required time I quoted above. The downside of this approach is, that for "incomplete" runs, aka when we hit the timeout, not all files are scanned and the code may still contain issues that would be flagged in a consecutive run. > > * Even without this limit on runtime, this new check would double > > to triple > > the run time of the libVirt pipeline in GitLab. > > > > Running it for every job sounds wasteful - it can be run on schedule > like the coverity job - daily or even weekly. > > Also, please never write libvirt like that again. > > Jano > I have no idea what I mixed up libvirt with, which is capitalized this way. Good thing it's Friday. Tim > > * clang-tidy finds a lot of false positives (see link above for > > explanation) > > and has checks that contradict the libVirt code style (e.g. braces > > around > > blocks). This makes a quite complex configuration in ".clang-tidy" > > neccessary. > > > > * I was unable to make clang-tidy recognize the settings from the > > configuration file for generated files, leading clang-tidy to > > always add some > > checks. These checks were among those that produced false > > positives. > > > > * The list of enabled / disabled checks in the yaml configuration > > file is a > > quite long string, making it hard to weave in some comments / > > documentation > > on which checks are enabled / disabled for what reason. > > > > This series introduces a new script, "run-clang-tidy.py". This is a > > replacement for the script of the same name from clang-tools-extra. > > It offers > > parallel execution, caching of results and a configurable soft- > > timeout. > > > > Please see the individual commits for more details. Comments > > welcome. > > > > https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang- > > tidy". > > > > Tim > > > > Tim Wiederhake (10): > > clang-tidy: Add a simple runner > > clang-tidy: Run in parallel > > clang-tidy: Filter output > > clang-tidy: Add cache > > clang-tidy: Add timeout > > clang-tidy: Allow timeouts > > clang-tidy: Add shuffle > > clang-tidy: Make list of checks explicit > > clang-tidy: Disable irrelevant and failing checks > > clang-tidy: Add CI integration > > > > .gitlab-ci.yml| 88 ++ > > scripts/run-clang-tidy.py | 557 > > ++ > > 2 files changed, 645 insertions(+) > > create mode 100755 scripts/run-clang-tidy.py > > > > -- > > 2.26.2 > > > >
Re: [libvirt PATCH v2 4/4] ci: Makefile: Expose the CI_USER_LOGIN variable for users to use
On Fri, Feb 12, 2021 at 04:04:04PM +0100, Andrea Bolognani wrote: > On Fri, 2021-02-12 at 15:19 +0100, Erik Skultety wrote: > > @@ -239,6 +241,7 @@ ci-help: > > @echo "CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after > > completion" > > @echo "CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' > > content" > > @echo "CI_ENGINE=auto - container engine to use (podman, > > docker)" > > + @echo "CI_USER_LOGIN=$$USER - which user should run in the > > container" > > Alignment is completely broken here. But I just realized that $USER > will get expanded here, so we have basically no way of keeping that > consistent... I didn't want to point it out earlier, since to me it's not a deal breaker :). > > Basically, your original version of the help message was better, > please go back to that one :) Sure, will do, thanks for the review. Erik > > Reviewed-by: Andrea Bolognani > > -- > Andrea Bolognani / Red Hat / Virtualization >
Re: [libvirt PATCH v2 4/4] ci: Makefile: Expose the CI_USER_LOGIN variable for users to use
On Fri, 2021-02-12 at 15:19 +0100, Erik Skultety wrote: > @@ -239,6 +241,7 @@ ci-help: > @echo "CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after > completion" > @echo "CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' > content" > @echo "CI_ENGINE=auto - container engine to use (podman, > docker)" > + @echo "CI_USER_LOGIN=$$USER - which user should run in the > container" Alignment is completely broken here. But I just realized that $USER will get expanded here, so we have basically no way of keeping that consistent... Basically, your original version of the help message was better, please go back to that one :) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
[libvirt PATCH v2 3/4] ci: Drop the prepare.sh script
The purpose of this script was to prepare a customized environment in the container, but was actually never used and it required the usage of sudo to switch the environment from root's context to a regular user's one. The thing is that once someone needs a custom script they would very likely to debug something and would also benefit from root privileges in general, so the usage of 'sudo' in such case was a bit cumbersome. Signed-off-by: Erik Skultety Reviewed-by: Andrea Bolognani --- ci/prepare.sh | 13 - 1 file changed, 13 deletions(-) delete mode 100644 ci/prepare.sh diff --git a/ci/prepare.sh b/ci/prepare.sh deleted file mode 100644 index da6fc9a1b5..00 --- a/ci/prepare.sh +++ /dev/null @@ -1,13 +0,0 @@ -# This script is used to prepare the environment that will be used -# to build libvirt inside the container. -# -# You can customize it to your liking, or alternatively use a -# completely different script by passing -# -# CI_PREPARE_SCRIPT=/path/to/your/prepare/script -# -# to make. -# -# Note that this script will have root privileges inside the -# container, so it can be used for things like installing additional -# packages. -- 2.29.2
[libvirt PATCH v2 4/4] ci: Makefile: Expose the CI_USER_LOGIN variable for users to use
More often than not I find myself debugging in the containers which means that I need to have root inside, but without manually tweaking the Makefile each time the execution would simply fail thanks to the uid/gid mapping we do. What if we expose the CI_USER_LOGIN variable, so that when needed, the root can be simply passed with this variable and voila - you have a root shell inside the container with CWD=~root. Signed-off-by: Erik Skultety --- ci/Makefile | 35 +++ 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/ci/Makefile b/ci/Makefile index 9308738d2d..05a50c021c 100644 --- a/ci/Makefile +++ b/ci/Makefile @@ -45,15 +45,15 @@ CI_CLEAN = 1 # preserved env CI_REUSE = 0 -# We need the container process to run with current host IDs -# so that it can access the passed in build directory -CI_UID = $(shell id -u) -CI_GID = $(shell id -g) - -# We also need the user's login and home directory to prepare the +# We need the user's login and home directory to prepare the # environment the way some programs expect it -CI_USER_LOGIN = $(shell echo "$$USER") -CI_USER_HOME = $(shell echo "$$HOME") +CI_USER_LOGIN = $(shell whoami) +CI_USER_HOME = $(shell eval echo "~$(CI_USER_LOGIN)") + +# We also need the container process to run with current host IDs +# so that it can access the passed in build directory +CI_UID = $(shell id -u "$(CI_USER_LOGIN)") +CI_GID = $(shell id -g "$(CI_USER_LOGIN)") CI_ENGINE = auto # Container engine we are going to use, can be overridden per make @@ -124,14 +124,16 @@ ifeq ($(CI_ENGINE),podman) CI_UID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_UID)-$(CI_UID CI_GID_OTHER_RANGE = $(shell echo $$(($(CI_MAX_GID)-$(CI_GID - CI_PODMAN_ARGS = \ - --uidmap 0:1:$(CI_UID) \ - --uidmap $(CI_UID):0:1 \ - --uidmap $(CI_UID_OTHER):$(CI_UID_OTHER):$(CI_UID_OTHER_RANGE) \ - --gidmap 0:1:$(CI_GID) \ - --gidmap $(CI_GID):0:1 \ - --gidmap $(CI_GID_OTHER):$(CI_GID_OTHER):$(CI_GID_OTHER_RANGE) \ - $(NULL) + ifneq ($(CI_UID), 0) + CI_PODMAN_ARGS = \ + --uidmap 0:1:$(CI_UID) \ + --uidmap $(CI_UID):0:1 \ + --uidmap $(CI_UID_OTHER):$(CI_UID_OTHER):$(CI_UID_OTHER_RANGE) \ + --gidmap 0:1:$(CI_GID) \ + --gidmap $(CI_GID):0:1 \ + --gidmap $(CI_GID_OTHER):$(CI_GID_OTHER):$(CI_GID_OTHER_RANGE) \ + $(NULL) + endif endif # Args to use when cloning a git repo. @@ -239,6 +241,7 @@ ci-help: @echo "CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after completion" @echo "CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' content" @echo "CI_ENGINE=auto - container engine to use (podman, docker)" + @echo "CI_USER_LOGIN=$$USER - which user should run in the container" @echo "CI_MESON_ARGS= - extra arguments passed to meson" @echo "CI_NINJA_ARGS= - extra arguments passed to ninja" @echo -- 2.29.2
[libvirt PATCH v2 2/4] ci: Run podman command directly without wrapping it with prepare.sh
The prepare.sh script isn't currently used and forces us to make use
of sudo to switch the user inside the container from root to $USER
which created a problem on our Debian Slim-based containers which don't
have the 'sudo' package installed.
This patch removes the sudo invocation and instead runs the CMD
directly with podman.
Summary of the changes:
- move the corresponding env variables which we need to be set in the
environment from the sudo invocation to the podman invocation
- pass --workdir to podman to retain the original behaviour we had with
sudo spawning a login shell.
- MESON_OPTS env variable doesn't need to propagated to the execution
environment anymore (like we had to do with sudo), because it's
defined in the Dockerfile
Signed-off-by: Erik Skultety
Reviewed-by: Andrea Bolognani
---
ci/Makefile | 27 ---
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/ci/Makefile b/ci/Makefile
index 7938e14c15..9308738d2d 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -82,7 +82,6 @@ CI_HOME_MOUNTS = \
$(NULL)
CI_SCRIPT_MOUNTS = \
- --volume $(CI_SCRATCHDIR)/prepare:$(CI_USER_HOME)/prepare:z \
--volume $(CI_SCRATCHDIR)/build:$(CI_USER_HOME)/build:z \
$(NULL)
@@ -150,6 +149,8 @@ CI_GIT_ARGS = \
# --userwe execute as the same user & group account
# as dev so that file ownership matches host
# instead of root:root
+# --workdir we change to user's home dir in the container
+# before running the workload
# --volume to pass in the cloned git repo & config
# --ulimit lower files limit for performance reasons
# --interactive
@@ -158,6 +159,11 @@ CI_ENGINE_ARGS = \
--rm \
--interactive \
--tty \
+ --user "$(CI_UID)":"$(CI_GID)" \
+ --workdir "$(CI_USER_HOME)" \
+ --env CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \
+ --env CI_MESON_ARGS="$(CI_MESON_ARGS)" \
+ --env CI_NINJA_ARGS="$(CI_NINJA_ARGS)" \
$(CI_PODMAN_ARGS) \
$(CI_PWDB_MOUNTS) \
$(CI_HOME_MOUNTS) \
@@ -178,9 +184,8 @@ ci-prepare-tree: ci-check-engine
cp /etc/passwd $(CI_SCRATCHDIR); \
cp /etc/group $(CI_SCRATCHDIR); \
mkdir -p $(CI_SCRATCHDIR)/home; \
- cp "$(CI_PREPARE_SCRIPT)" $(CI_SCRATCHDIR)/prepare; \
cp "$(CI_BUILD_SCRIPT)" $(CI_SCRATCHDIR)/build; \
- chmod +x "$(CI_SCRATCHDIR)/prepare" "$(CI_SCRATCHDIR)/build"; \
+ chmod +x "$(CI_SCRATCHDIR)/build"; \
echo "Cloning $(CI_GIT_ROOT) to $(CI_HOST_SRCDIR)"; \
git clone $(CI_GIT_ARGS) $(CI_GIT_ROOT) $(CI_HOST_SRCDIR) ||
exit 1; \
for mod in $$(git submodule | awk '{ print $$2 }' | sed -E
's,^../,,g') ; \
@@ -192,18 +197,10 @@ ci-prepare-tree: ci-check-engine
fi
ci-run-command@%: ci-prepare-tree
- $(CI_ENGINE) run $(CI_ENGINE_ARGS) $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \
- /bin/bash -c ' \
- $(CI_USER_HOME)/prepare || exit 1; \
- sudo \
- --login \
- --user="#$(CI_UID)" \
- --group="#$(CI_GID)" \
- MESON_OPTS="$$MESON_OPTS" \
- CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \
- CI_MESON_ARGS="$(CI_MESON_ARGS)" \
- CI_NINJA_ARGS="$(CI_NINJA_ARGS)" \
- $(CI_COMMAND) || exit 1'
+ $(CI_ENGINE) run \
+ $(CI_ENGINE_ARGS) \
+ $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \
+ $(CI_COMMAND)
@test "$(CI_CLEAN)" = "1" && rm -rf $(CI_SCRATCHDIR) || :
ci-shell@%:
--
2.29.2
[libvirt PATCH v2 0/4] ci: Adjustments to remove our dependency on sudo
Our Debian containers don't have sudo pre-installed and the only reason we actually needed it was to run a custom prepare script which as it turns out does nothing really. Since v1: - applied quoting to the variables as asked during the review process - simplified setting of CI_PODMAN_ARGS - moved 4/4 to 3/4 Erik Skultety (4): ci: Specify the shebang sequence for build.sh ci: Run podman command directly without wrapping it with prepare.sh ci: Drop the prepare.sh script ci: Makefile: Expose the CI_USER_LOGIN variable for users to use ci/Makefile | 62 +-- ci/build.sh | 2 ++ ci/prepare.sh | 13 --- 3 files changed, 33 insertions(+), 44 deletions(-) delete mode 100644 ci/prepare.sh -- 2.29.2
[libvirt PATCH v2 1/4] ci: Specify the shebang sequence for build.sh
This is necessary for the follow up patch, because the default entrypoint for a Dockerfile is exec. Signed-off-by: Erik Skultety Reviewed-by: Andrea Bolognani --- ci/build.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/build.sh b/ci/build.sh index 740b46a935..0f23df1fa3 100644 --- a/ci/build.sh +++ b/ci/build.sh @@ -1,3 +1,5 @@ +#!/bin/sh + # This script is used to build libvirt inside the container. # # You can customize it to your liking, or alternatively use a -- 2.29.2
Re: [libvirt PATCH 00/10] [RFC] clang-tidy CI integration
On Fri, Feb 12, 2021 at 02:25:24PM +0100, Tim Wiederhake wrote: > "clang-tidy" is a static code analysis tool for c and c++ code. See > https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html > for some issues found by clang-tidy and more background information. > > Meson has support for clang-tidy and generates target named "clang-tidy" if > it finds a ".clang-tidy" configuration file in the project's root directory. > > There are some problems with this approach though, with regards to inclusion > in GitLab's CI: > > * Single-threaded runtime of a complete scan takes between 95 and 110 minutes, > depending on the enabled checks, which is significantly longer than GitLab's > maximum run time of 60 minutes, after which jobs are aborted. IIUC, you can override the timeout setting per job... > > * Even without this limit on runtime, this new check would double to triple > the run time of the libVirt pipeline in GitLab. Yeah that's a problem, but bear in mind there are two separate scenarios Running post merge we need to check all files, but the length of time is a non-issue since no one is waiting for a post-merge check to complete. Running pre- merge we only need to check files which are modified by the patches on the current branch. That ought to be orders of magnitude faster. > > * clang-tidy finds a lot of false positives (see link above for explanation) > and has checks that contradict the libVirt code style (e.g. braces around > blocks). This makes a quite complex configuration in ".clang-tidy" neccessary. IMHO for code checkers to add real value to people you need the existing codebase in git to be 100% clean. If you're not going to make the existing code compliant, then users will never be sure of what they should do for new code. We see this all the time in QEMU where its patch checker complains about problems that were pre-existing. If we could get clang-tidy to do a subset of checks where we can malke libvirt 100% clean > * I was unable to make clang-tidy recognize the settings from the > configuration file for generated files, leading clang-tidy to always add some > checks. These checks were among those that produced false positives. > > * The list of enabled / disabled checks in the yaml configuration file is a > quite long string, making it hard to weave in some comments / documentation > on which checks are enabled / disabled for what reason. > > This series introduces a new script, "run-clang-tidy.py". This is a > replacement for the script of the same name from clang-tools-extra. It offers > parallel execution, caching of results and a configurable soft-timeout. > > Please see the individual commits for more details. Comments welcome. > > https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang-tidy". What am I missing here - that job just seems to show a list of filenames, but isn't reporting any issues for them ? 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: [libvirt PATCH 3/4] ci: Expose CI_USER_LOGIN as a Makefile variable for users to use
On Fri, 2021-02-12 at 13:41 +0100, Erik Skultety wrote: > On Fri, Feb 12, 2021 at 01:27:42PM +0100, Andrea Bolognani wrote: > > On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote: > > > # We also need the user's login and home directory to prepare the > > > # environment the way some programs expect it > > > -CI_USER_LOGIN = $(shell echo "$$USER") > > > -CI_USER_HOME = $(shell echo "$$HOME") > > > +CI_USER_LOGIN = $(shell whoami) > > > +CI_USER_HOME = $(shell eval echo "~$(CI_USER_LOGIN)") > > > > This use of eval makes me a bit uncomfortable. Can we do > > Can you elaborate what the problem is? The argument is even quoted so I > sincerely don't see a problem and find it much clearer than what you suggest. It's just a code smell. In general, I prefer straightforward constructs to "fancy" ones, especially in languages where quoting and the like matter so much. But I agree with you that it's safe in this specific scenario, so if you'd rather keep it this way I won't NACK the patch because of that. -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 00/10] [RFC] clang-tidy CI integration
On a Friday in 2021, Tim Wiederhake wrote: "clang-tidy" is a static code analysis tool for c and c++ code. See https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html for some issues found by clang-tidy and more background information. Meson has support for clang-tidy and generates target named "clang-tidy" if it finds a ".clang-tidy" configuration file in the project's root directory. There are some problems with this approach though, with regards to inclusion in GitLab's CI: * Single-threaded runtime of a complete scan takes between 95 and 110 minutes, depending on the enabled checks, which is significantly longer than GitLab's maximum run time of 60 minutes, after which jobs are aborted. That does seem like a deal-breaker. Did you manage to do a successful run by iteratively building up the cache over multiple jobs? Or by doing a multi-threaded run? * Even without this limit on runtime, this new check would double to triple the run time of the libVirt pipeline in GitLab. Running it for every job sounds wasteful - it can be run on schedule like the coverity job - daily or even weekly. Also, please never write libvirt like that again. Jano * clang-tidy finds a lot of false positives (see link above for explanation) and has checks that contradict the libVirt code style (e.g. braces around blocks). This makes a quite complex configuration in ".clang-tidy" neccessary. * I was unable to make clang-tidy recognize the settings from the configuration file for generated files, leading clang-tidy to always add some checks. These checks were among those that produced false positives. * The list of enabled / disabled checks in the yaml configuration file is a quite long string, making it hard to weave in some comments / documentation on which checks are enabled / disabled for what reason. This series introduces a new script, "run-clang-tidy.py". This is a replacement for the script of the same name from clang-tools-extra. It offers parallel execution, caching of results and a configurable soft-timeout. Please see the individual commits for more details. Comments welcome. https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang-tidy". Tim Tim Wiederhake (10): clang-tidy: Add a simple runner clang-tidy: Run in parallel clang-tidy: Filter output clang-tidy: Add cache clang-tidy: Add timeout clang-tidy: Allow timeouts clang-tidy: Add shuffle clang-tidy: Make list of checks explicit clang-tidy: Disable irrelevant and failing checks clang-tidy: Add CI integration .gitlab-ci.yml| 88 ++ scripts/run-clang-tidy.py | 557 ++ 2 files changed, 645 insertions(+) create mode 100755 scripts/run-clang-tidy.py -- 2.26.2 signature.asc Description: PGP signature
[libvirt PATCH 08/10] clang-tidy: Make list of checks explicit
Preparation for next patch.
Signed-off-by: Tim Wiederhake
---
scripts/run-clang-tidy.py | 24
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 1d1038df0f..945a1f75d4 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -75,6 +75,7 @@ def run_clang_tidy(item):
cmd = (
"clang-tidy",
"--warnings-as-errors=*",
+"--checks=-*,%s" % ",".join(checks),
"-p",
item["directory"],
item["file"])
@@ -90,7 +91,7 @@ def run_clang_tidy(item):
}
-def cache_name(item):
+def cache_name(item, checks):
if not args.cache:
return None
@@ -117,6 +118,7 @@ def cache_name(item):
hashsum = hashlib.sha256()
hashsum.update(item["command"].encode())
+hashsum.update("\n".join(checks).encode())
hashsum.update(result.stdout.encode())
basename = "".join([c if c.isalnum() else "_" for c in item["output"]])
@@ -145,7 +147,7 @@ def cache_write(filename, result):
json.dump(result, f)
-def worker():
+def worker(checks):
while True:
item = items.get()
if args.timeout and args.timeout < time.time():
@@ -156,7 +158,7 @@ def worker():
os.chdir(item["directory"])
-cache = cache_name(item)
+cache = cache_name(item, checks)
result = cache_read(cache)
with lock:
print(item["file"], "" if result is None else "(from cache)")
@@ -177,9 +179,19 @@ def worker():
items.task_done()
+def list_checks():
+output = subprocess.check_output(
+["clang-tidy", "-checks=*", "-list-checks"],
+universal_newlines=True).split("\n")[1:]
+
+output = [line.strip() for line in output]
+return output
+
+
args = parse_args()
items = queue.Queue()
lock = threading.Lock()
+checks = list_checks()
findings = list()
if args.cache:
@@ -189,8 +201,12 @@ if args.cache:
if args.timeout:
args.timeout = time.time() + args.timeout * 60
+print("Enabled checks:")
+for check in checks:
+print("%s" % check)
+
for _ in range(args.thread_num):
-threading.Thread(target=worker, daemon=True).start()
+threading.Thread(target=worker, daemon=True, args=[checks]).start()
with open(os.path.join(args.build_dir, "compile_commands.json")) as f:
compile_commands = json.load(f)
--
2.26.2
[libvirt PATCH 06/10] clang-tidy: Allow timeouts
With an empty cache, the first run of the clang-tidy scan in the CI will
fail. While the instinctive reaction "press the rerun button" will eventually
lead to the situation where enough files are cached that the entire scan fits
within the time window, this creates friction for e.g. new contributors.
By allowing timeouts to be non-fatal events, we reduce scanning to a "best
effort" approach, that might miss newly introduced regressions.
Signed-off-by: Tim Wiederhake
---
scripts/run-clang-tidy.py | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 19b8640982..54eb0ea584 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -56,6 +56,11 @@ def parse_args():
dest="timeout",
type=int,
help="Timeout in minutes")
+parser.add_argument(
+"--allow-timeout",
+dest="allow_timeout",
+action="store_true",
+help="Do not treat timeout as failure if set")
return parser.parse_args()
@@ -138,7 +143,8 @@ def worker():
while True:
item = items.get()
if args.timeout and args.timeout < time.time():
-findings.append("%s (timeout)" % item["file"])
+if not args.allow_timeout:
+findings.append("%s (timeout)" % item["file"])
items.task_done()
continue
--
2.26.2
[libvirt PATCH 10/10] clang-tidy: Add CI integration
A better solution would be to have an explicit target that creates all generated files, similar to libvirt-pot-dep. Signed-off-by: Tim Wiederhake --- .gitlab-ci.yml | 88 ++ 1 file changed, 88 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5779b1b8b2..313563 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -617,3 +617,91 @@ coverity: - curl https://scan.coverity.com/builds?project=$COVERITY_SCAN_PROJECT_NAME --form token=$COVERITY_SCAN_TOKEN --form email=$GITLAB_USER_EMAIL --form file=@cov-int.tar.gz --form version="$(git describe --tags)" --form description="$(git describe --tags) / $CI_COMMIT_TITLE / $CI_COMMIT_REF_NAME:$CI_PIPELINE_ID" rules: - if: "$CI_PIPELINE_SOURCE == 'schedule' && $COVERITY_SCAN_PROJECT_NAME && $COVERITY_SCAN_TOKEN" + +clang-tidy: + image: $CI_REGISTRY_IMAGE/ci-fedora-32:latest + needs: +- x64-fedora-32-container + stage: builds + cache: +key: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG" +when: always +paths: + - clang-tidy-cache + script: +- meson build +- ninja -C build src/access/viraccessapicheck.c +- ninja -C build src/access/viraccessapicheck.h +- ninja -C build src/access/viraccessapichecklxc.c +- ninja -C build src/access/viraccessapichecklxc.h +- ninja -C build src/access/viraccessapicheckqemu.c +- ninja -C build src/access/viraccessapicheckqemu.h +- ninja -C build src/admin/admin_client.h +- ninja -C build src/admin/admin_protocol.c +- ninja -C build src/admin/admin_protocol.h +- ninja -C build src/admin/admin_server_dispatch_stubs.h +- ninja -C build src/esx/esx_vi.generated.c +- ninja -C build src/esx/esx_vi.generated.h +- ninja -C build src/esx/esx_vi_methods.generated.c +- ninja -C build src/esx/esx_vi_methods.generated.h +- ninja -C build src/esx/esx_vi_methods.generated.macro +- ninja -C build src/esx/esx_vi_types.generated.c +- ninja -C build src/esx/esx_vi_types.generated.h +- ninja -C build src/esx/esx_vi_types.generated.typedef +- ninja -C build src/esx/esx_vi_types.generated.typeenum +- ninja -C build src/esx/esx_vi_types.generated.typefromstring +- ninja -C build src/esx/esx_vi_types.generated.typetostring +- ninja -C build src/hyperv/hyperv_wmi_classes.generated.c +- ninja -C build src/hyperv/hyperv_wmi_classes.generated.h +- ninja -C build src/hyperv/hyperv_wmi_classes.generated.typedef +- ninja -C build src/libvirt_functions.stp +- ninja -C build src/libvirt_probes.h +- ninja -C build src/libvirt_probes.stp +- ninja -C build src/locking/lock_daemon_dispatch_stubs.h +- ninja -C build src/locking/lock_protocol.c +- ninja -C build src/locking/lock_protocol.h +- ninja -C build src/logging/log_daemon_dispatch_stubs.h +- ninja -C build src/logging/log_protocol.c +- ninja -C build src/logging/log_protocol.h +- ninja -C build src/lxc/lxc_controller_dispatch.h +- ninja -C build src/lxc/lxc_monitor_dispatch.h +- ninja -C build src/lxc/lxc_monitor_protocol.c +- ninja -C build src/lxc/lxc_monitor_protocol.h +- ninja -C build src/qemu/libvirt_qemu_probes.h +- ninja -C build src/qemu/libvirt_qemu_probes.stp +- ninja -C build src/remote/lxc_client_bodies.h +- ninja -C build src/remote/lxc_daemon_dispatch_stubs.h +- ninja -C build src/remote/lxc_protocol.c +- ninja -C build src/remote/lxc_protocol.h +- ninja -C build src/remote/qemu_client_bodies.h +- ninja -C build src/remote/qemu_daemon_dispatch_stubs.h +- ninja -C build src/remote/qemu_protocol.c +- ninja -C build src/remote/qemu_protocol.h +- ninja -C build src/remote/remote_client_bodies.h +- ninja -C build src/remote/remote_daemon_dispatch_stubs.h +- ninja -C build src/remote/remote_protocol.c +- ninja -C build src/remote/remote_protocol.h +- ninja -C build src/rpc/virkeepaliveprotocol.c +- ninja -C build src/rpc/virkeepaliveprotocol.h +- ninja -C build src/rpc/virnetprotocol.c +- ninja -C build src/rpc/virnetprotocol.h +- ninja -C build src/util/virkeycodetable_atset1.h +- ninja -C build src/util/virkeycodetable_atset2.h +- ninja -C build src/util/virkeycodetable_atset3.h +- ninja -C build src/util/virkeycodetable_linux.h +- ninja -C build src/util/virkeycodetable_osx.h +- ninja -C build src/util/virkeycodetable_qnum.h +- ninja -C build src/util/virkeycodetable_usb.h +- ninja -C build src/util/virkeycodetable_win32.h +- ninja -C build src/util/virkeycodetable_xtkbd.h +- ninja -C build src/util/virkeynametable_linux.h +- ninja -C build src/util/virkeynametable_osx.h +- ninja -C build src/util/virkeynametable_win32.h +- ninja -C build tools/wireshark/src/libvirt/keepalive.h +- ninja -C build tools/wireshark/src/libvirt/lxc.h +- ninja -C build tools/wireshark/src/libvirt/protocol.h +- ninja -C build tools/wireshark/src/libvirt
[libvirt PATCH 07/10] clang-tidy: Add shuffle
Randomizing the order of files to scan has no impact for local use of the script. The same holds true for use in the CI, if the amount of cached files is big enough for the entire scan to finish before timeout. If the cache is empty or not filled enough to ensure timely completion, randomizing the order of files makes it more likely to spent time on caching new files rather than hashing already cached files to check for the presence of a cache file. Signed-off-by: Tim Wiederhake --- scripts/run-clang-tidy.py | 8 1 file changed, 8 insertions(+) diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py index 54eb0ea584..1d1038df0f 100755 --- a/scripts/run-clang-tidy.py +++ b/scripts/run-clang-tidy.py @@ -6,6 +6,7 @@ import json import multiprocessing import os import queue +import random import re import shlex import subprocess @@ -61,6 +62,11 @@ def parse_args(): dest="allow_timeout", action="store_true", help="Do not treat timeout as failure if set") +parser.add_argument( +"--shuffle-input", +dest="shuffle_input", +action="store_true", +help="Randomize order of files to check") return parser.parse_args() @@ -188,6 +194,8 @@ for _ in range(args.thread_num): with open(os.path.join(args.build_dir, "compile_commands.json")) as f: compile_commands = json.load(f) +if args.shuffle_input: +random.shuffle(compile_commands) for compile_command in compile_commands: items.put(compile_command) -- 2.26.2
[libvirt PATCH 09/10] clang-tidy: Disable irrelevant and failing checks
clang-tidy's focus is on c++. Disable all checks that do not apply to the libVirt code base. Also disable all checks that are currently failing, to prevent introduction of new issues, while we work down the list of existing issues and / or decide on disabling some checks permanently. Signed-off-by: Tim Wiederhake --- scripts/run-clang-tidy.py | 326 ++ 1 file changed, 326 insertions(+) diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py index 945a1f75d4..24bc034682 100755 --- a/scripts/run-clang-tidy.py +++ b/scripts/run-clang-tidy.py @@ -25,6 +25,331 @@ spam = [ ] +disabled_checks = [ +# aliases for other checks +"bugprone-narrowing-conversions", +"cert-dcl03-c", +"cert-dcl16-c", +"cert-dcl54-cpp", +"cert-dcl59-cpp", +"cert-err61-cpp", +"cert-fio38-c", +"cert-msc30-c", +"cert-msc32-c", +"cert-oop11-cpp", +"cert-oop54-cpp", +"cert-pos44-c", +"cppcoreguidelines-avoid-c-arrays", +"cppcoreguidelines-avoid-magic-numbers", +"cppcoreguidelines-c-copy-assignment-signature", +"cppcoreguidelines-explicit-virtual-functions", +"cppcoreguidelines-non-private-member-variables-in-classes", +"fuchsia-header-anon-namespaces", +"google-readability-braces-around-statements", +"google-readability-function-size", +"google-readability-namespace-comments", +"hicpp-avoid-c-arrays", +"hicpp-avoid-goto", +"hicpp-braces-around-statements", +"hicpp-deprecated-headers", +"hicpp-explicit-conversions", +"hicpp-function-size", +"hicpp-invalid-access-moved", +"hicpp-member-init", +"hicpp-move-const-arg", +"hicpp-named-parameter", +"hicpp-new-delete-operators", +"hicpp-no-array-decay", +"hicpp-no-malloc", +"hicpp-special-member-functions", +"hicpp-static-assert", +"hicpp-uppercase-literal-suffix", +"hicpp-use-auto", +"hicpp-use-emplace", +"hicpp-use-equals-default", +"hicpp-use-equals-delete", +"hicpp-use-noexcept", +"hicpp-use-nullptr", +"hicpp-use-override", +"hicpp-vararg", +"llvm-qualified-auto", + +# only relevant for c++ +"bugprone-copy-constructor-init", +"bugprone-dangling-handle", +"bugprone-exception-escape", +"bugprone-fold-init-type", +"bugprone-forward-declaration-namespace", +"bugprone-forwarding-reference-overload", +"bugprone-inaccurate-erase", +"bugprone-lambda-function-name", +"bugprone-move-forwarding-reference", +"bugprone-parent-virtual-call", +"bugprone-sizeof-container", +"bugprone-string-constructor", +"bugprone-string-integer-assignment", +"bugprone-swapped-arguments", +"bugprone-throw-keyword-missing", +"bugprone-undelegated-constructor", +"bugprone-unhandled-self-assignment", +"bugprone-unused-raii", +"bugprone-unused-return-value", +"bugprone-use-after-move", +"bugprone-virtual-near-miss", +"cert-dcl21-cpp", +"cert-dcl50-cpp", +"cert-dcl58-cpp", +"cert-err09-cpp", +"cert-err52-cpp", +"cert-err58-cpp", +"cert-err60-cpp", +"cert-mem57-cpp", +"cert-msc50-cpp", +"cert-msc51-cpp", +"cert-oop58-cpp", +"clang-analyzer-cplusplus.InnerPointer", +"clang-analyzer-cplusplus.Move", +"clang-analyzer-cplusplus.NewDelete", +"clang-analyzer-cplusplus.NewDeleteLeaks", +"clang-analyzer-cplusplus.PureVirtualCall", +"clang-analyzer-cplusplus.SelfAssignment", +"clang-analyzer-cplusplus.SmartPtr", +"clang-analyzer-cplusplus.VirtualCallModeling", +"clang-analyzer-optin.cplusplus.UninitializedObject", +"clang-analyzer-optin.cplusplus.VirtualCall", +"cppcoreguidelines-no-malloc", +"cppcoreguidelines-owning-memory", +"cppcoreguidelines-pro-bounds-array-to-pointer-decay", +"cppcoreguidelines-pro-bounds-constant-array-index", +"cppcoreguidelines-pro-bounds-pointer-arithmetic", +"cppcoreguidelines-pro-type-const-cast", +"cppcoreguidelines-pro-type-cstyle-cast", +"cppcoreguidelines-pro-type-member-init", +"cppcoreguidelines-pro-type-reinterpret-cast", +"cppcoreguidelines-pro-type-static-cast-downcast", +"cppcoreguidelines-pro-type-union-access", +"cppcoreguidelines-pro-type-vararg", +"cppcoreguidelines-slicing", +"cppcoreguidelines-special-member-functions", +"fuchsia-default-arguments-calls", +"fuchsia-default-arguments-declarations", +"fuchsia-multiple-inheritance", +"fuchsia-overloaded-operator", +"fuchsia-statically-constructed-objects", +"fuchsia-trailing-return", +"fuchsia-virtual-inheritance", +"google-build-explicit-make-pair", +"google-build-namespaces", +"google-build-using-namespace", +"google-default-arguments", +"google-explicit-constructor", +"google-global-names-in-headers", +"google-readability-casting", +"google-runtime-operator", +"google-runtime-references", +"hicpp-exception-baseclass", +
[libvirt PATCH 03/10] clang-tidy: Filter output
GitLab's CI output is capped at a certain size. Filter out all status
messages that do not add value.
Signed-off-by: Tim Wiederhake
---
scripts/run-clang-tidy.py | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 79d9fb38b2..dc5880878b 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -5,11 +5,32 @@ import json
import multiprocessing
import os
import queue
+import re
import subprocess
import sys
import threading
+spam = [
+re.compile("^[0-9]+ (warning|error)[s]? .*generated"),
+re.compile("^[0-9]+ warning[s]? treated as error"),
+re.compile("^Suppressed [0-9]+ warning[s]?"),
+re.compile("^Use .* to display errors from all non-system headers"),
+re.compile("Error while processing "),
+re.compile("Found compiler error"),
+]
+
+
+def remove_spam(output):
+retval = list()
+for line in output.split("\n"):
+if any([s.match(line) for s in spam]):
+continue
+retval.append(line)
+
+return "\n".join(retval)
+
+
def parse_args():
parser = argparse.ArgumentParser(description="caching clang-tidy runner")
parser.add_argument(
@@ -41,8 +62,8 @@ def run_clang_tidy(item):
universal_newlines=True)
return {
"returncode": result.returncode,
-"stdout": result.stdout.strip(),
-"stderr": result.stderr.strip(),
+"stdout": remove_spam(result.stdout.strip()),
+"stderr": remove_spam(result.stderr.strip()),
}
--
2.26.2
[libvirt PATCH 05/10] clang-tidy: Add timeout
Defining a custom timeout allows the scan to finish (albeit unsuccessfully)
before GitLab's 60 minute limit and thus preserve the cache of already
scanned files.
A successive run, e.g. when the "rerun" button in GitLab's web interface is
clicked, roughly picks up where the previous run stopped, gradually increasing
the amount of cached results and eventually succeeds.
Signed-off-by: Tim Wiederhake
---
scripts/run-clang-tidy.py | 13 +
1 file changed, 13 insertions(+)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index cc9c20ea32..19b8640982 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -51,6 +51,11 @@ def parse_args():
"--cache",
dest="cache",
help="Path to cache directory")
+parser.add_argument(
+"--timeout",
+dest="timeout",
+type=int,
+help="Timeout in minutes")
return parser.parse_args()
@@ -132,6 +137,11 @@ def cache_write(filename, result):
def worker():
while True:
item = items.get()
+if args.timeout and args.timeout < time.time():
+findings.append("%s (timeout)" % item["file"])
+items.task_done()
+continue
+
os.chdir(item["directory"])
cache = cache_name(item)
@@ -164,6 +174,9 @@ if args.cache:
args.cache = os.path.abspath(args.cache)
os.makedirs(args.cache, exist_ok=True)
+if args.timeout:
+args.timeout = time.time() + args.timeout * 60
+
for _ in range(args.thread_num):
threading.Thread(target=worker, daemon=True).start()
--
2.26.2
[libvirt PATCH 02/10] clang-tidy: Run in parallel
Similar to the "official" run-clang-tidy script. Defaults to run one thread
per core, making the tool more pleasant to use locally.
Signed-off-by: Tim Wiederhake
---
scripts/run-clang-tidy.py | 38 +-
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index 10c8b80fe0..79d9fb38b2 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -2,9 +2,12 @@
import argparse
import json
+import multiprocessing
import os
+import queue
import subprocess
import sys
+import threading
def parse_args():
@@ -14,6 +17,12 @@ def parse_args():
dest="build_dir",
default=".",
help="Path to build directory")
+parser.add_argument(
+"-j",
+dest="thread_num",
+default=multiprocessing.cpu_count(),
+type=int,
+help="Number of threads to run")
return parser.parse_args()
@@ -38,28 +47,39 @@ def run_clang_tidy(item):
def worker():
-for item in items:
+while True:
+item = items.get()
os.chdir(item["directory"])
print(item["file"])
result = run_clang_tidy(item)
-if result["returncode"] != 0:
-findings.append(item["file"])
-if result["stdout"]:
-print(result["stdout"])
-if result["stderr"]:
-print(result["stderr"])
+with lock:
+if result["returncode"] != 0:
+findings.append(item["file"])
+if result["stdout"]:
+print(result["stdout"])
+if result["stderr"]:
+print(result["stderr"])
+
+items.task_done()
args = parse_args()
+items = queue.Queue()
+lock = threading.Lock()
findings = list()
+for _ in range(args.thread_num):
+threading.Thread(target=worker, daemon=True).start()
+
with open(os.path.join(args.build_dir, "compile_commands.json")) as f:
-items = json.load(f)
+compile_commands = json.load(f)
+for compile_command in compile_commands:
+items.put(compile_command)
-worker()
+items.join()
if findings:
print("Findings in %s file(s):" % len(findings))
--
2.26.2
[libvirt PATCH 01/10] clang-tidy: Add a simple runner
Run clang-tidy with default configuration on all files listed in the
compilation database. Note that the generated files in the build directory
have to be built first. The simplest way to achieve this is to build libVirt
first.
Example:
$ meson build && ninja -C build
$ ./scripts/run-clang-tidy.py -p build
Signed-off-by: Tim Wiederhake
---
scripts/run-clang-tidy.py | 68 +++
1 file changed, 68 insertions(+)
create mode 100755 scripts/run-clang-tidy.py
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
new file mode 100755
index 00..10c8b80fe0
--- /dev/null
+++ b/scripts/run-clang-tidy.py
@@ -0,0 +1,68 @@
+#!/usr/bin/env python3
+
+import argparse
+import json
+import os
+import subprocess
+import sys
+
+
+def parse_args():
+parser = argparse.ArgumentParser(description="caching clang-tidy runner")
+parser.add_argument(
+"-p",
+dest="build_dir",
+default=".",
+help="Path to build directory")
+
+return parser.parse_args()
+
+
+def run_clang_tidy(item):
+cmd = (
+"clang-tidy",
+"--warnings-as-errors=*",
+"-p",
+item["directory"],
+item["file"])
+result = subprocess.run(
+cmd,
+stdout=subprocess.PIPE,
+stderr=subprocess.PIPE,
+universal_newlines=True)
+return {
+"returncode": result.returncode,
+"stdout": result.stdout.strip(),
+"stderr": result.stderr.strip(),
+}
+
+
+def worker():
+for item in items:
+os.chdir(item["directory"])
+
+print(item["file"])
+
+result = run_clang_tidy(item)
+
+if result["returncode"] != 0:
+findings.append(item["file"])
+if result["stdout"]:
+print(result["stdout"])
+if result["stderr"]:
+print(result["stderr"])
+
+
+args = parse_args()
+findings = list()
+
+with open(os.path.join(args.build_dir, "compile_commands.json")) as f:
+items = json.load(f)
+
+worker()
+
+if findings:
+print("Findings in %s file(s):" % len(findings))
+for finding in findings:
+print(" %s" % finding)
+sys.exit(1)
--
2.26.2
[libvirt PATCH 00/10] [RFC] clang-tidy CI integration
"clang-tidy" is a static code analysis tool for c and c++ code. See https://listman.redhat.com/archives/libvir-list/2021-January/msg01152.html for some issues found by clang-tidy and more background information. Meson has support for clang-tidy and generates target named "clang-tidy" if it finds a ".clang-tidy" configuration file in the project's root directory. There are some problems with this approach though, with regards to inclusion in GitLab's CI: * Single-threaded runtime of a complete scan takes between 95 and 110 minutes, depending on the enabled checks, which is significantly longer than GitLab's maximum run time of 60 minutes, after which jobs are aborted. * Even without this limit on runtime, this new check would double to triple the run time of the libVirt pipeline in GitLab. * clang-tidy finds a lot of false positives (see link above for explanation) and has checks that contradict the libVirt code style (e.g. braces around blocks). This makes a quite complex configuration in ".clang-tidy" neccessary. * I was unable to make clang-tidy recognize the settings from the configuration file for generated files, leading clang-tidy to always add some checks. These checks were among those that produced false positives. * The list of enabled / disabled checks in the yaml configuration file is a quite long string, making it hard to weave in some comments / documentation on which checks are enabled / disabled for what reason. This series introduces a new script, "run-clang-tidy.py". This is a replacement for the script of the same name from clang-tools-extra. It offers parallel execution, caching of results and a configurable soft-timeout. Please see the individual commits for more details. Comments welcome. https://gitlab.com/twiederh/libvirt/-/pipelines/255321968 → "clang-tidy". Tim Tim Wiederhake (10): clang-tidy: Add a simple runner clang-tidy: Run in parallel clang-tidy: Filter output clang-tidy: Add cache clang-tidy: Add timeout clang-tidy: Allow timeouts clang-tidy: Add shuffle clang-tidy: Make list of checks explicit clang-tidy: Disable irrelevant and failing checks clang-tidy: Add CI integration .gitlab-ci.yml| 88 ++ scripts/run-clang-tidy.py | 557 ++ 2 files changed, 645 insertions(+) create mode 100755 scripts/run-clang-tidy.py -- 2.26.2
[libvirt PATCH 04/10] clang-tidy: Add cache
Cache the results of clang-tidy. The cache is keyed, for each file, on:
* the file name,
* the exact command used to compile the file to detect changes in arguments,
* the hash of the preprocessor stage output to detect changes in includes.
A later patch also adds the list of enabled checks to the cache key.
Running clang-tidy uncached takes between 95 and 110 minutes single threaded
(just over 9 minutes wall time on a 12 core builder), depending on the set of
enabled checks. In the ideal case, where no source files changes, this number
is reduced to 80 seconds (9 seconds on a 12 core builder), when caching is
enabled.
This makes clang-tidy much more pleasant to work with locally, but is not
enough to guarantee painless CI operation: While GitLab does support caching
between builds and can be configured to retain the cache even when the job
fails, this does not happen when the job times out after 60 minutes or the
job is manually aborted.
Signed-off-by: Tim Wiederhake
---
scripts/run-clang-tidy.py | 83 ++-
1 file changed, 81 insertions(+), 2 deletions(-)
diff --git a/scripts/run-clang-tidy.py b/scripts/run-clang-tidy.py
index dc5880878b..cc9c20ea32 100755
--- a/scripts/run-clang-tidy.py
+++ b/scripts/run-clang-tidy.py
@@ -1,14 +1,17 @@
#!/usr/bin/env python3
import argparse
+import hashlib
import json
import multiprocessing
import os
import queue
import re
+import shlex
import subprocess
import sys
import threading
+import time
spam = [
@@ -44,6 +47,10 @@ def parse_args():
default=multiprocessing.cpu_count(),
type=int,
help="Number of threads to run")
+parser.add_argument(
+"--cache",
+dest="cache",
+help="Path to cache directory")
return parser.parse_args()
@@ -67,14 +74,75 @@ def run_clang_tidy(item):
}
+def cache_name(item):
+if not args.cache:
+return None
+
+cmd = shlex.split(item["command"])
+for index, element in enumerate(cmd):
+if element == "-o":
+cmd[index + 1] = "/dev/stdout"
+continue
+if element == "-MD":
+cmd[index] = None
+if element in ("-MQ", "-MF"):
+cmd[index] = None
+cmd[index + 1] = None
+cmd = [c for c in cmd if c is not None]
+cmd.append("-E")
+
+result = subprocess.run(
+cmd,
+stdout=subprocess.PIPE,
+universal_newlines=True)
+
+if result.returncode != 0:
+return None
+
+hashsum = hashlib.sha256()
+hashsum.update(item["command"].encode())
+hashsum.update(result.stdout.encode())
+
+basename = "".join([c if c.isalnum() else "_" for c in item["output"]])
+return os.path.join(args.cache, "%s-%s" % (basename, hashsum.hexdigest()))
+
+
+def cache_read(filename):
+if filename is None:
+return None
+
+try:
+with open(filename) as f:
+return json.load(f)
+except FileNotFoundError:
+pass
+except json.decoder.JSONDecodeError:
+pass
+return None
+
+
+def cache_write(filename, result):
+if filename is None:
+return
+
+with open(filename, "w") as f:
+json.dump(result, f)
+
+
def worker():
while True:
item = items.get()
os.chdir(item["directory"])
-print(item["file"])
+cache = cache_name(item)
+result = cache_read(cache)
+with lock:
+print(item["file"], "" if result is None else "(from cache)")
+
+if result is None:
+result = run_clang_tidy(item)
-result = run_clang_tidy(item)
+cache_write(cache, result)
with lock:
if result["returncode"] != 0:
@@ -92,6 +160,10 @@ items = queue.Queue()
lock = threading.Lock()
findings = list()
+if args.cache:
+args.cache = os.path.abspath(args.cache)
+os.makedirs(args.cache, exist_ok=True)
+
for _ in range(args.thread_num):
threading.Thread(target=worker, daemon=True).start()
@@ -102,6 +174,13 @@ with open(os.path.join(args.build_dir,
"compile_commands.json")) as f:
items.join()
+if args.cache:
+cutoffdate = time.time() - 7 * 24 * 60 * 60
+for filename in os.listdir(args.cache):
+pathname = os.path.join(args.cache, filename)
+if os.path.getmtime(pathname) < cutoffdate:
+os.remove(pathname)
+
if findings:
print("Findings in %s file(s):" % len(findings))
for finding in findings:
--
2.26.2
Re: [libvirt PATCH 3/4] ci: Expose CI_USER_LOGIN as a Makefile variable for users to use
On Fri, Feb 12, 2021 at 01:27:42PM +0100, Andrea Bolognani wrote: > On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote: > > # We need the container process to run with current host IDs > > # so that it can access the passed in build directory > > -CI_UID = $(shell id -u) > > -CI_GID = $(shell id -g) > > +CI_UID = $(shell id -u $(CI_USER_LOGIN)) > > +CI_GID = $(shell id -g $(CI_USER_LOGIN)) > > Please quote uses of $(CI_USER_LOGIN) in the shell. > > Also, since you're using the variable here, it would be IMHO cleaner > if you moved the block that contains its definition before this one. Sure I can do that, I just didn't feel like moving around code to achieve the same thing in a declarative language. > > > # We also need the user's login and home directory to prepare the > > # environment the way some programs expect it > > -CI_USER_LOGIN = $(shell echo "$$USER") > > -CI_USER_HOME = $(shell echo "$$HOME") > > +CI_USER_LOGIN = $(shell whoami) > > +CI_USER_HOME = $(shell eval echo "~$(CI_USER_LOGIN)") > > This use of eval makes me a bit uncomfortable. Can we do Can you elaborate what the problem is? The argument is even quoted so I sincerely don't see a problem and find it much clearer than what you suggest. Erik
Re: [libvirt RFC PATCH 0/5] eliminating VIR_FREE in the *Clear() functions
On Fri, Feb 12, 2021 at 12:14:27PM +0100, Michal Privoznik wrote: > On 2/12/21 11:07 AM, Erik Skultety wrote: > > On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote: > > > On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote: > > > > I've looked at a few of these, and one thing I've found is that very > > > > often we have a function called somethingSomethingClear(), and: > > > > > > > > 1) The only places it is ever called will immediately free the memory > > > > of the object as soon as they clear it. > > > > > > > > and very possibly > > > > > > > > 2) It doesn't actually *clear* everything. Some items are cleared via > > > > VIR_FREE(), but then some of the other pointers call > > > > > > > > bobLoblawFree(def->bobloblaw) > > > > > > > > and then don't actually set def->bobloblaw to NULL - so the functions > > > > aren't actually "Clearing", they're "Freeing and then clearing a few > > > > things, but not everything". > > > > > > > > > > One thing I am wondering is whether this is really only used where it > > > makes > > > sense. As far as I understand, and please correct me if I am way off, the > > > purpose of the Clear functions is to: > > > > > > a) provide a way to remove everything from a structure that the current > > > function cannot recreate (there is a pointer to it somewhere else > > > which > > > would not be updated) and > > > > > > b) provide a way to reset a structure so that it can be filled again > > > without > > > needless reallocation. > > > > > > I think (b) is obviously pointless, especially lately, so the only > > > reasonable > > > usage would be for the scenario (a). However, I think I remember this > > > also > > > being used in places where it would be perfectly fine to free the > > > variable and > > > recreate it. Maybe it could ease up the decision, at least by > > > eliminating some > > > of the code, if my hunch is correct. > > > > > > In my quick search I only found virDomainVideoDefClear to be used in this > > > manner > > > and I am not convinced that it is worth having this extra function with > > > extra > > > > You could always memset it explicitly, someone might find the code more > > readable then. IMO I'd vote for an explicit memset just for the sake of > > better > > security practice (since we'll have to wait a little while for something > > like > > SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure > > how > > many cycles exactly would be wasted, but IIRC a recent discussion memset > > can be > > optimized away (correct me if I don't remember it well!), so Dan P.B. > > suggested to gradually convert to some platform-specific ways on how to > > sanitize the memory safely - with that in mind, I'd say we use an explicit > > memset in all the functions in question and convert them later? > > I think one can argue that if there's a memset() called inside a function > that is supposed to clear out a member of a struct and later the member is > tested against NULL, but compiler decides to "optimize" memset out - it's a > compiler bug. I agree - it is, especially now that GCC employs a static analyzer it should be smart and not optimize it away, not sure whether the memset optimization happened only with GCC or was a generic thing hence I won't try to make any comments about Clang. Erik
Re: [libvirt PATCH 4/4] ci: Drop the prepare.sh script
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote: > The purpose of this script was to prepare a customized environment in > the container, but was actually never used and it required the usage of > sudo to switch the environment from root's context to a regular user's > one. > The thing is that once someone needs a custom script they would very > likely to debug something and would also benefit from root privileges > in general, so the usage of 'sudo' in such case was a bit cumbersome. > > Signed-off-by: Erik Skultety > --- > ci/prepare.sh | 13 - > 1 file changed, 13 deletions(-) > delete mode 100644 ci/prepare.sh This makes IMHO more sense either as 3/4 or even squashed directly into 2/4. Either way, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 3/4] ci: Expose CI_USER_LOGIN as a Makefile variable for users to use
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote: > # We need the container process to run with current host IDs > # so that it can access the passed in build directory > -CI_UID = $(shell id -u) > -CI_GID = $(shell id -g) > +CI_UID = $(shell id -u $(CI_USER_LOGIN)) > +CI_GID = $(shell id -g $(CI_USER_LOGIN)) Please quote uses of $(CI_USER_LOGIN) in the shell. Also, since you're using the variable here, it would be IMHO cleaner if you moved the block that contains its definition before this one. > # We also need the user's login and home directory to prepare the > # environment the way some programs expect it > -CI_USER_LOGIN = $(shell echo "$$USER") > -CI_USER_HOME = $(shell echo "$$HOME") > +CI_USER_LOGIN = $(shell whoami) > +CI_USER_HOME = $(shell eval echo "~$(CI_USER_LOGIN)") This use of eval makes me a bit uncomfortable. Can we do CI_USER_HOME = $(shell getent passwd "$(CI_USER_LOGIN)" | cut -d: -f6) instead? > + # In case we want to debug in the container, having root is actually > + # preferable, so reset the CI_PODMAN_ARGS and don't actually perform > + # any uid/gid mapping > + ifeq ($(CI_UID), 0) > + CI_PODMAN_ARGS= > + endif Setting $(CI_PODMAN_ARGS) only to reset it moments later seems worse than just doing ifneq ($(CI_UID, 0) CI_PODMAN_ARGS = \ ... $(NULL) endif The comment is also somewhat misleading: whether or not you're going to debug in the container, whatever that means, is not really relevant - the point is simply that performing these mappings is only necessary when the container process is running as non-root. > @echo "CI_CLEAN=0 - do not delete '$(CI_SCRATCHDIR)' after > completion" > @echo "CI_REUSE=1 - re-use existing '$(CI_SCRATCHDIR)' > content" > + @echo "CI_USER_LOGIN= - which user should run in the container > (default is $$USER)" > @echo "CI_ENGINE=auto - container engine to use (podman, > docker)" > @echo "CI_MESON_ARGS= - extra arguments passed to meson" > @echo "CI_NINJA_ARGS= - extra arguments passed to ninja" We document the default, so this should be CI_USER_LOGIN=$$USER - which user should run in the container And please document this after CI_ENGINE. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 08/19] storage: Format qcow2v3 volumes by default
On Fri, Feb 12, 2021 at 12:13:58 +, Daniel P. Berrangé wrote: > On Fri, Feb 12, 2021 at 11:55:36AM +0100, Peter Krempa wrote: > > On Fri, Feb 12, 2021 at 10:49:02 +, Daniel Berrange wrote: > > > On Thu, Feb 11, 2021 at 04:37:47PM +0100, Peter Krempa wrote: > > > > Format the new volumes with 'compat=1.1' since the minimum supported > > > > qemu version is now 1.5 rather the pre-historic compat=0.10. > > > > > > I understand the desire to do this, but this is none the less a > > > semantic change to the behaviour of the APIs. It is the same > > > situation as arbitrarily changing the defaults for any part of > > > the domain XML. > > > > I'm aware of that, but at certain point it IMO doesn't make sense to try > > to stick with a prehistoric format just for the sake of it and IMO this > > is it. > > Well that's a policy decision and it is upto the user or mgmt app to > decide when they wish to drop compatibility with old distros. RHEL > had continued to publish its cloud images with old format until very > recently for sake of compat. I don't think libvirt should be forcing > that decision onto people as it sabotages libvirt's value of providing > long term stable behaviour to applications. The oldest QEMU release we support understands v3 so there's no reason to use an older format in this respect. But you're right there might be other use cases which would still need v2 format for compatibility. I guess I was too focused on our usage and libvirt/QEMU compatibility when reviewing the patch. Jirka
Re: [PATCH 08/19] storage: Format qcow2v3 volumes by default
On Fri, Feb 12, 2021 at 11:55:36AM +0100, Peter Krempa wrote: > On Fri, Feb 12, 2021 at 10:49:02 +, Daniel Berrange wrote: > > On Thu, Feb 11, 2021 at 04:37:47PM +0100, Peter Krempa wrote: > > > Format the new volumes with 'compat=1.1' since the minimum supported > > > qemu version is now 1.5 rather the pre-historic compat=0.10. > > > > I understand the desire to do this, but this is none the less a > > semantic change to the behaviour of the APIs. It is the same > > situation as arbitrarily changing the defaults for any part of > > the domain XML. > > I'm aware of that, but at certain point it IMO doesn't make sense to try > to stick with a prehistoric format just for the sake of it and IMO this > is it. Well that's a policy decision and it is upto the user or mgmt app to decide when they wish to drop compatibility with old distros. RHEL had continued to publish its cloud images with old format until very recently for sake of compat. I don't think libvirt should be forcing that decision onto people as it sabotages libvirt's value of providing long term stable behaviour to applications. 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: [libvirt PATCH 2/4] ci: Run podman command directly without wrapping it with prepare.sh
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote: > The prepare.sh script isn't currently used and forces us to make use > of sudo to switch the user inside the container from root to $USER > which created a problem on our Debian Slim-based containers which don't > have the 'sudo' package installed. > This patch removes the sudo invocation and instead runs the CMD > directly with podman. > > Summary of the changes: > - move the corresponding env variables which we need to be set in the > environment from the sudo invocation to the podman invocation > - pass --workdir to podman to retain the original behaviour we had with > sudo spawning a login shell. > - MESON_ARGS env variable doesn't need to propagated to the execution s/MESON_ARGS/MESON_OPTS/ > @@ -158,6 +159,11 @@ CI_ENGINE_ARGS = \ > --rm \ > --interactive \ > --tty \ > + --user $(CI_UID):$(CI_GID) \ > + --workdir $(CI_USER_HOME) \ Please add quotes around the arguments here. Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH 1/4] ci: Specify the shebang sequence for build.sh
On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote: > This is necessary for the follow up patch, because the default > entrypoint for a Dockerfile is exec. > > Signed-off-by: Erik Skultety > --- > ci/build.sh | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt RFC PATCH 0/5] eliminating VIR_FREE in the *Clear() functions
On 2/12/21 11:07 AM, Erik Skultety wrote: On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote: On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote: I've looked at a few of these, and one thing I've found is that very often we have a function called somethingSomethingClear(), and: 1) The only places it is ever called will immediately free the memory of the object as soon as they clear it. and very possibly 2) It doesn't actually *clear* everything. Some items are cleared via VIR_FREE(), but then some of the other pointers call bobLoblawFree(def->bobloblaw) and then don't actually set def->bobloblaw to NULL - so the functions aren't actually "Clearing", they're "Freeing and then clearing a few things, but not everything". One thing I am wondering is whether this is really only used where it makes sense. As far as I understand, and please correct me if I am way off, the purpose of the Clear functions is to: a) provide a way to remove everything from a structure that the current function cannot recreate (there is a pointer to it somewhere else which would not be updated) and b) provide a way to reset a structure so that it can be filled again without needless reallocation. I think (b) is obviously pointless, especially lately, so the only reasonable usage would be for the scenario (a). However, I think I remember this also being used in places where it would be perfectly fine to free the variable and recreate it. Maybe it could ease up the decision, at least by eliminating some of the code, if my hunch is correct. In my quick search I only found virDomainVideoDefClear to be used in this manner and I am not convinced that it is worth having this extra function with extra You could always memset it explicitly, someone might find the code more readable then. IMO I'd vote for an explicit memset just for the sake of better security practice (since we'll have to wait a little while for something like SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how many cycles exactly would be wasted, but IIRC a recent discussion memset can be optimized away (correct me if I don't remember it well!), so Dan P.B. suggested to gradually convert to some platform-specific ways on how to sanitize the memory safely - with that in mind, I'd say we use an explicit memset in all the functions in question and convert them later? I think one can argue that if there's a memset() called inside a function that is supposed to clear out a member of a struct and later the member is tested against NULL, but compiler decides to "optimize" memset out - it's a compiler bug. Michal
Re: [PATCH 08/19] storage: Format qcow2v3 volumes by default
On Fri, Feb 12, 2021 at 10:49:02 +, Daniel Berrange wrote: > On Thu, Feb 11, 2021 at 04:37:47PM +0100, Peter Krempa wrote: > > Format the new volumes with 'compat=1.1' since the minimum supported > > qemu version is now 1.5 rather the pre-historic compat=0.10. > > I understand the desire to do this, but this is none the less a > semantic change to the behaviour of the APIs. It is the same > situation as arbitrarily changing the defaults for any part of > the domain XML. I'm aware of that, but at certain point it IMO doesn't make sense to try to stick with a prehistoric format just for the sake of it and IMO this is it. Nonetheless, it's not strictly required. I can just use the new format for the non-shared-storage migration rather than globally if we want to stay in the cave.
Re: [PATCH 08/19] storage: Format qcow2v3 volumes by default
On Thu, Feb 11, 2021 at 04:37:47PM +0100, Peter Krempa wrote: > Format the new volumes with 'compat=1.1' since the minimum supported > qemu version is now 1.5 rather the pre-historic compat=0.10. I understand the desire to do this, but this is none the less a semantic change to the behaviour of the APIs. It is the same situation as arbitrarily changing the defaults for any part of the domain XML. 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 08/19] storage: Format qcow2v3 volumes by default
On Thu, Feb 11, 2021 at 16:37:47 +0100, Peter Krempa wrote: > Format the new volumes with 'compat=1.1' since the minimum supported > qemu version is now 1.5 rather the pre-historic compat=0.10. > > Signed-off-by: Peter Krempa > --- > src/storage/storage_util.c | 2 +- > .../storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv | 2 +- > tests/storagevolxml2argvdata/qcow2-compat.argv | 2 +- > tests/storagevolxml2argvdata/qcow2-from-logical-compat.argv | 2 +- > tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt.argv| 2 +- > .../qcow2-luks-convert-encrypt2fileqcow2.argv | 2 +- > tests/storagevolxml2argvdata/qcow2-luks.argv| 2 +- > .../qcow2-nobacking-convert-prealloc-compat.argv| 2 +- > .../storagevolxml2argvdata/qcow2-nobacking-prealloc-compat.argv | 2 +- > .../qcow2-nocapacity-convert-prealloc.argv | 2 +- > tests/storagevolxml2argvdata/qcow2-nocapacity.argv | 2 +- > tests/storagevolxml2argvdata/qcow2-nocow-compat.argv| 2 +- > tests/storagevolxml2argvdata/qcow2-zerocapacity.argv| 2 +- > 13 files changed, 13 insertions(+), 13 deletions(-) Reviewed-by: Jiri Denemark
Re: [PATCH 07/19] storagevolxml2argvdata: Rewrap all output files
On Thu, Feb 11, 2021 at 16:37:46 +0100, Peter Krempa wrote: > Use scripts/test-wrap-argv.py to rewrap the output files so that any > further changes don't introduce churn since we are rewrapping the output > automatically now. > > Signed-off-by: Peter Krempa > --- > tests/storagevolxml2argvdata/iso-input.argv | 6 +++-- > tests/storagevolxml2argvdata/iso.argv | 4 +++- > .../logical-from-qcow2.argv | 6 +++-- > tests/storagevolxml2argvdata/luks-cipher.argv | 8 --- > .../luks-convert-encrypt.argv | 23 --- > .../luks-convert-encrypt2fileqcow2.argv | 19 ++- > .../luks-convert-encrypt2fileraw.argv | 20 ++-- > .../luks-convert-qcow2.argv | 21 +++-- > .../storagevolxml2argvdata/luks-convert.argv | 20 ++-- > tests/storagevolxml2argvdata/luks.argv| 8 --- > tests/storagevolxml2argvdata/qcow2-1.1.argv | 8 --- > .../storagevolxml2argvdata/qcow2-compat.argv | 8 --- > .../qcow2-from-logical-compat.argv| 8 --- > tests/storagevolxml2argvdata/qcow2-lazy.argv | 9 +--- > ...ow2-nobacking-convert-prealloc-compat.argv | 10 +--- > .../qcow2-nobacking-prealloc-compat.argv | 8 --- > .../qcow2-nocapacity-convert-prealloc.argv| 9 +--- > .../qcow2-nocapacity.argv | 6 ++--- > .../qcow2-nocow-compat.argv | 9 +--- > tests/storagevolxml2argvdata/qcow2-nocow.argv | 9 +--- > .../qcow2-zerocapacity.argv | 5 +++- > 21 files changed, 147 insertions(+), 77 deletions(-) Reviewed-by: Jiri Denemark
Re: [PATCH 06/19] testutils: virTestRewrapFile: Rewrap also '.argv' files
On Thu, Feb 11, 2021 at 16:37:45 +0100, Peter Krempa wrote: > The suffix is used for output files of 'storagevolxml2argvtest. > > Signed-off-by: Peter Krempa > --- > tests/testutils.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tests/testutils.c b/tests/testutils.c > index 7ecf7923b8..8734790457 100644 > --- a/tests/testutils.c > +++ b/tests/testutils.c > @@ -332,6 +332,7 @@ virTestRewrapFile(const char *filename) > g_autoptr(virCommand) cmd = NULL; > > if (!(virStringHasSuffix(filename, ".args") || > + virStringHasSuffix(filename, ".argv") || >virStringHasSuffix(filename, ".ldargs"))) > return 0; > Reviewed-by: Jiri Denemark
Re: [libvirt RFC PATCH 0/5] eliminating VIR_FREE in the *Clear() functions
On Fri, Feb 12, 2021 at 11:07:21AM +0100, Erik Skultety wrote: > On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote: > > On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote: > > > I've looked at a few of these, and one thing I've found is that very > > > often we have a function called somethingSomethingClear(), and: > > > > > > 1) The only places it is ever called will immediately free the memory > > > of the object as soon as they clear it. > > > > > > and very possibly > > > > > > 2) It doesn't actually *clear* everything. Some items are cleared via > > > VIR_FREE(), but then some of the other pointers call > > > > > >bobLoblawFree(def->bobloblaw) > > > > > > and then don't actually set def->bobloblaw to NULL - so the functions > > > aren't actually "Clearing", they're "Freeing and then clearing a few > > > things, but not everything". > > > > > > > One thing I am wondering is whether this is really only used where it makes > > sense. As far as I understand, and please correct me if I am way off, the > > purpose of the Clear functions is to: > > > > a) provide a way to remove everything from a structure that the current > > function cannot recreate (there is a pointer to it somewhere else which > > would not be updated) and > > > > b) provide a way to reset a structure so that it can be filled again > > without > > needless reallocation. > > > > I think (b) is obviously pointless, especially lately, so the only > > reasonable > > usage would be for the scenario (a). However, I think I remember this also > > being used in places where it would be perfectly fine to free the variable > > and > > recreate it. Maybe it could ease up the decision, at least by eliminating > > some > > of the code, if my hunch is correct. > > > > In my quick search I only found virDomainVideoDefClear to be used in this > > manner > > and I am not convinced that it is worth having this extra function with > > extra > > You could always memset it explicitly, someone might find the code more > readable then. IMO I'd vote for an explicit memset just for the sake of better > security practice (since we'll have to wait a little while for something like > SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how > many cycles exactly would be wasted, but IIRC a recent discussion memset can > be > optimized away (correct me if I don't remember it well!), so Dan P.B. > suggested to gradually convert to some platform-specific ways on how to > sanitize the memory safely - with that in mind, I'd say we use an explicit > memset in all the functions in question and convert them later? I only suggest that for places where security is required. ie to scrub passwords. If the compiler wants to cull memsets in places unrelated to security that's fine by me, or at least, not our problem to worry about. 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 05/19] qemuMigrationSrcPerformPeer2Peer3: Don't leak 'dom_xml' on cleanup
On Thu, Feb 11, 2021 at 16:37:44 +0100, Peter Krempa wrote: > Use g_autofree for 'dom_xml' to free it on some of the (unlikely) code > paths jumping to cleanup prior to the deallocation which is done right > after it's not needed any more since it's a big string. > > Noticed when running under valgrind: > > ==2204780== 8,192 bytes in 1 blocks are definitely lost in loss record 2,539 > of 2,551 > ==2204780==at 0x483BCE8: realloc (vg_replace_malloc.c:834) > ==2204780==by 0x4D890DF: g_realloc (in /usr/lib64/libglib-2.0.so.0.6600.4) > ==2204780==by 0x4DA3AF0: g_string_append_vprintf (in > /usr/lib64/libglib-2.0.so.0.6600.4) > ==2204780==by 0x4917293: virBufferAsprintf (virbuffer.c:307) > ==2204780==by 0x49B0B75: virDomainChrDefFormat (domain_conf.c:26109) > ==2204780==by 0x49E25EF: virDomainDefFormatInternalSetRootName > (domain_conf.c:28956) > ==2204780==by 0x15F81D24: qemuDomainDefFormatBufInternal > (qemu_domain.c:6204) > ==2204780==by 0x15F8270D: qemuDomainDefFormatXMLInternal > (qemu_domain.c:6229) > ==2204780==by 0x15F8270D: qemuDomainDefFormatLive (qemu_domain.c:6279) > ==2204780==by 0x15FD8100: qemuMigrationSrcBeginPhase > (qemu_migration.c:2395) > ==2204780==by 0x15FE0F0D: qemuMigrationSrcPerformPeer2Peer3 > (qemu_migration.c:4640) > ==2204780==by 0x15FE0F0D: qemuMigrationSrcPerformPeer2Peer > (qemu_migration.c:5093) > ==2204780==by 0x15FE0F0D: qemuMigrationSrcPerformJob > (qemu_migration.c:5168) > ==2204780==by 0x15FE280E: qemuMigrationSrcPerform (qemu_migration.c:5372) > ==2204780==by 0x15F9BA3D: qemuDomainMigratePerform3Params > (qemu_driver.c:11841) > > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_migration.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index f44d31c971..37f0d43d24 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -4347,7 +4347,7 @@ qemuMigrationSrcPerformPeer2Peer3(virQEMUDriverPtr > driver, > char *uri_out = NULL; > char *cookiein = NULL; > char *cookieout = NULL; > -char *dom_xml = NULL; > +g_autofree char *dom_xml = NULL; > int cookieinlen = 0; > int cookieoutlen = 0; > int ret = -1; Oh wow, the leak has been with us for 10 years since v3 migration protocol was introduced... Reviewed-by: Jiri Denemark
Re: [PATCH 04/19] virDomainMigrateVersion3Full: Don't set 'cancelled' to the same value
On Thu, Feb 11, 2021 at 16:37:43 +0100, Peter Krempa wrote:
> It's already initialized to '1'.
>
> Signed-off-by: Peter Krempa
> ---
> src/libvirt-domain.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index dba89a7d3a..e9688a15b4 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -3089,7 +3089,6 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
> virTypedParamsReplaceString(¶ms, &nparams,
> VIR_MIGRATE_PARAM_URI,
> uri_out) < 0) {
> -cancelled = 1;
> virErrorPreserveLast(&orig_err);
> goto finish;
> }
> @@ -3098,7 +3097,6 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
> VIR_MIGRATE_PARAM_URI, &uri) <= 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("domainMigratePrepare3 did not set uri"));
> -cancelled = 1;
> virErrorPreserveLast(&orig_err);
> goto finish;
> }
Reviewed-by: Jiri Denemark
Re: [PATCH 03/19] qemu: Probe whether an image is 'qcow2 v2' from query-named-block-nodes
On Thu, Feb 11, 2021 at 16:37:42 +0100, Peter Krempa wrote: > Such images don't support stuff like dirty bitmaps. Note that the > synthetic test for detecting bitmaps is used as an example to prevent > adding additional test cases. > > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_monitor.h | 3 +++ > src/qemu/qemu_monitor_json.c | 11 +++ > tests/qemublocktest.c | 2 ++ > tests/qemublocktestdata/bitmap/synthetic.json | 2 +- > tests/qemublocktestdata/bitmap/synthetic.out | 1 + > 5 files changed, 18 insertions(+), 1 deletion(-) Reviewed-by: Jiri Denemark
Re: [PATCH 02/19] qemu: capabilities: Introduce QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING
On Fri, Feb 12, 2021 at 09:30:25 +0100, Peter Krempa wrote:
> On Fri, Feb 12, 2021 at 08:57:08 +0100, Jiri Denemark wrote:
> > On Thu, Feb 11, 2021 at 16:37:41 +0100, Peter Krempa wrote:
> > > The capability represents qemu's ability to setup mappings for migrating
> > > block dirty bitmaps and is based on presence of the 'transform' property
> > > of the 'block-bitmap-mapping' property of 'migrate-set-parameters' QMP
> > > command.
> > >
> > > Signed-off-by: Peter Krempa
> > > ---
> > > src/qemu/qemu_capabilities.c | 2 ++
> > > src/qemu/qemu_capabilities.h | 1 +
> > > 2 files changed, 3 insertions(+)
> > >
> > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > > index ccf810ff96..38555dde98 100644
> > > --- a/src/qemu/qemu_capabilities.c
> > > +++ b/src/qemu/qemu_capabilities.c
> > > @@ -616,6 +616,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
> > >"vhost-user-blk",
> > >"cpu-max",
> > >"memory-backend-file.x-use-canonical-path-for-ramblock-id",
> > > + "migration-param.block-bitmap-mapping",
> > > );
> > >
> > >
> > > @@ -1549,6 +1550,7 @@ static struct virQEMUCapsStringFlags
> > > virQEMUCapsQMPSchemaQueries[] = {
> > > { "migrate-set-parameters/arg-type/xbzrle-cache-size",
> > > QEMU_CAPS_MIGRATION_PARAM_XBZRLE_CACHE_SIZE },
> > > { "set-numa-node/arg-type/+hmat-lb", QEMU_CAPS_NUMA_HMAT },
> > > { "netdev_add/arg-type/+vhost-vdpa", QEMU_CAPS_NETDEV_VHOST_VDPA },
> > > +{ "migrate-set-parameters/arg-type/block-bitmap-mapping/transform",
> > > QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING },
> > > };
> > >
> >
> > So how is it possible this change is not reflected in
> > tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml after the previous
> > patch updated QEMU replies? Interestingly enough, tests pass after this
> > patch so either the capability detection is not working or QEMU replies
> > do not actually contain what you're looking for here.
>
> Oops, there's a mistake in the query string where I've missed 'bitmaps'
> subcomponent. Well actually I've already noticed this before, but
> squashed the changes to a patch that I've ultimately removed from the
> series ...
>
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index c7ab144a8e..0e0926d0e5 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -1550,7 +1550,8 @@ static struct virQEMUCapsStringFlags
> virQEMUCapsQMPSchemaQueries[] = {
> { "migrate-set-parameters/arg-type/xbzrle-cache-size",
> QEMU_CAPS_MIGRATION_PARAM_XBZRLE_CACHE_SIZE },
> { "set-numa-node/arg-type/+hmat-lb", QEMU_CAPS_NUMA_HMAT },
> { "netdev_add/arg-type/+vhost-vdpa", QEMU_CAPS_NETDEV_VHOST_VDPA },
> -{ "migrate-set-parameters/arg-type/block-bitmap-mapping/transform",
> QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING },
> +{
> "migrate-set-parameters/arg-type/block-bitmap-mapping/bitmaps/transform",
> + QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING },
> };
>
> typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
> diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
> b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
> index f2ec32e46b..b9a36cb71e 100644
> --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
> @@ -255,6 +255,7 @@
>
>
>
> +
>5002050
>0
>43100242
With these two hunks squashed in
Reviewed-by: Jiri Denemark
Re: [PATCH 01/19] qemucapabilitiesdata: Update test data for qemu-6.0 on x86_64
On Thu, Feb 11, 2021 at 16:37:40 +0100, Peter Krempa wrote: > Include the 'transform' member of 'block-bitmap-mapping'. > > Note that this is based on uncommited patches and will be updated once > they are merged. > --- > .../caps_6.0.0.x86_64.replies | 510 ++ > .../caps_6.0.0.x86_64.xml | 16 +- > 2 files changed, 306 insertions(+), 220 deletions(-) Looks ok, but obviously we need to wait for the QEMU patches to be merged first. Jirka
Re: [libvirt RFC PATCH 0/5] eliminating VIR_FREE in the *Clear() functions
On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote: > On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote: > > I've looked at a few of these, and one thing I've found is that very > > often we have a function called somethingSomethingClear(), and: > > > > 1) The only places it is ever called will immediately free the memory > > of the object as soon as they clear it. > > > > and very possibly > > > > 2) It doesn't actually *clear* everything. Some items are cleared via > > VIR_FREE(), but then some of the other pointers call > > > >bobLoblawFree(def->bobloblaw) > > > > and then don't actually set def->bobloblaw to NULL - so the functions > > aren't actually "Clearing", they're "Freeing and then clearing a few > > things, but not everything". > > > > One thing I am wondering is whether this is really only used where it makes > sense. As far as I understand, and please correct me if I am way off, the > purpose of the Clear functions is to: > > a) provide a way to remove everything from a structure that the current > function cannot recreate (there is a pointer to it somewhere else which > would not be updated) and > > b) provide a way to reset a structure so that it can be filled again without > needless reallocation. > > I think (b) is obviously pointless, especially lately, so the only reasonable > usage would be for the scenario (a). However, I think I remember this also > being used in places where it would be perfectly fine to free the variable and > recreate it. Maybe it could ease up the decision, at least by eliminating > some > of the code, if my hunch is correct. > > In my quick search I only found virDomainVideoDefClear to be used in this > manner > and I am not convinced that it is worth having this extra function with extra You could always memset it explicitly, someone might find the code more readable then. IMO I'd vote for an explicit memset just for the sake of better security practice (since we'll have to wait a little while for something like SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how many cycles exactly would be wasted, but IIRC a recent discussion memset can be optimized away (correct me if I don't remember it well!), so Dan P.B. suggested to gradually convert to some platform-specific ways on how to sanitize the memory safely - with that in mind, I'd say we use an explicit memset in all the functions in question and convert them later? Erik
Re: [libvirt RFC PATCH 0/5] eliminating VIR_FREE in the *Clear() functions
On Fri, Feb 12, 2021 at 12:54:02AM -0500, Laine Stump wrote: I've looked at a few of these, and one thing I've found is that very often we have a function called somethingSomethingClear(), and: 1) The only places it is ever called will immediately free the memory of the object as soon as they clear it. and very possibly 2) It doesn't actually *clear* everything. Some items are cleared via VIR_FREE(), but then some of the other pointers call bobLoblawFree(def->bobloblaw) and then don't actually set def->bobloblaw to NULL - so the functions aren't actually "Clearing", they're "Freeing and then clearing a few things, but not everything". One thing I am wondering is whether this is really only used where it makes sense. As far as I understand, and please correct me if I am way off, the purpose of the Clear functions is to: a) provide a way to remove everything from a structure that the current function cannot recreate (there is a pointer to it somewhere else which would not be updated) and b) provide a way to reset a structure so that it can be filled again without needless reallocation. I think (b) is obviously pointless, especially lately, so the only reasonable usage would be for the scenario (a). However, I think I remember this also being used in places where it would be perfectly fine to free the variable and recreate it. Maybe it could ease up the decision, at least by eliminating some of the code, if my hunch is correct. In my quick search I only found virDomainVideoDefClear to be used in this manner and I am not convinced that it is worth having this extra function with extra memset(). Just food for thought. So I'm wondering if it is worthwhile to A) audit all the *Clear() functions and rename the functions that don't actually need to clear the contents to be, e.g. bobLobLawFreeContents(), while also replacing VIR_FREE with g_free(). (this is what I've done in these 5 patches) or if we should B) just do the wholesale approach and (as danpb suggested last week) change all VIR_FREE in *Clear() functions to g_free(), and put a "memset(obj, 0, sizeof(*obj))" at the end of each function, ignoring whether or not we actually need that. (B) would obviously be much faster to get done, and simpler for everyone to keep track of what's happening, but of course it is less efficient. Very likely this efficiency is completely meaningless in the large scheme (even in the medium or small scheme). (I actually like the idea of 0'ing out *everything*[*] when we're done with it, extra cycles be damned! I think of the two choices above, after going through this exercise, I'd say (B) is a more reasonable choice, but since I tried this, I thought I'd send it and see if anyone else has an opinion (or different idea) [*](including all those places I've replaced VIR_FREE with g_free in the name of "progress?") Laine Stump (5): conf: rename and narrow scope of virDomainHostdevDefClear() conf: rename virDomainHostdevSubsysSCSIClear conf: replace pointless VIR_FREEs with g_free in virDomainVideoDefClear() conf: don't call virDomainDeviceInfoClear from virDomainDeviceInfoParseXML conf: replace virDomainDeviceInfoClear with virDomainDeviceInfoFreeContents src/conf/device_conf.c | 15 +++- src/conf/device_conf.h | 2 +- src/conf/domain_conf.c | 81 src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - 5 files changed, 47 insertions(+), 53 deletions(-) -- 2.29.2 signature.asc Description: PGP signature
Re: [libvirt RFC PATCH 0/5] eliminating VIR_FREE in the *Clear() functions
On 2/12/21 6:54 AM, Laine Stump wrote: I've looked at a few of these, and one thing I've found is that very often we have a function called somethingSomethingClear(), and: 1) The only places it is ever called will immediately free the memory of the object as soon as they clear it. and very possibly 2) It doesn't actually *clear* everything. Some items are cleared via VIR_FREE(), but then some of the other pointers call bobLoblawFree(def->bobloblaw) and then don't actually set def->bobloblaw to NULL - so the functions aren't actually "Clearing", they're "Freeing and then clearing a few things, but not everything". So I'm wondering if it is worthwhile to A) audit all the *Clear() functions and rename the functions that don't actually need to clear the contents to be, e.g. bobLobLawFreeContents(), while also replacing VIR_FREE with g_free(). (this is what I've done in these 5 patches) or if we should B) just do the wholesale approach and (as danpb suggested last week) change all VIR_FREE in *Clear() functions to g_free(), and put a "memset(obj, 0, sizeof(*obj))" at the end of each function, ignoring whether or not we actually need that. (B) would obviously be much faster to get done, and simpler for everyone to keep track of what's happening, but of course it is less efficient. Very likely this efficiency is completely meaningless in the large scheme (even in the medium or small scheme). (I actually like the idea of 0'ing out *everything*[*] when we're done with it, extra cycles be damned! I think of the two choices above, after going through this exercise, I'd say (B) is a more reasonable choice, but since I tried this, I thought I'd send it and see if anyone else has an opinion (or different idea) [*](including all those places I've replaced VIR_FREE with g_free in the name of "progress?") Laine Stump (5): conf: rename and narrow scope of virDomainHostdevDefClear() conf: rename virDomainHostdevSubsysSCSIClear conf: replace pointless VIR_FREEs with g_free in virDomainVideoDefClear() conf: don't call virDomainDeviceInfoClear from virDomainDeviceInfoParseXML conf: replace virDomainDeviceInfoClear with virDomainDeviceInfoFreeContents src/conf/device_conf.c | 15 +++- src/conf/device_conf.h | 2 +- src/conf/domain_conf.c | 81 src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - 5 files changed, 47 insertions(+), 53 deletions(-) I don't like change and thus prefer keeping *Clear() with explicit memset(0) - if needed, but don't want to stop progress. Michal
Re: [libvirt PATCH 0/8] More VIR_FREE removals
On 2/12/21 6:28 AM, Laine Stump wrote: Only 90 this time. These are all functions that behave similar to the *Free() functions, but their names don't end in "Free" so I missed them last time. Laine Stump (8): esx: replace VIR_FREE with g_free in any ESX_VI__TEMPLATE__FREE conf: convert VIR_FREE to g_free in other functions that free their arg locking: convert VIR_FREE to g_free in other functions that free their arg openvz: convert VIR_FREE to g_free in other functions that free their arg remote: convert VIR_FREE to g_free in other functions that free their arg qemu: convert VIR_FREE to g_free in other functions that free their arg util: convert VIR_FREE to g_free in other functions that free their arg vmware: convert VIR_FREE to g_free in other functions that free their arg src/conf/capabilities.c | 38 +++ src/esx/esx_vi.c | 18 +++ src/esx/esx_vi_types.c| 24 +-- src/locking/lock_manager.c| 4 ++-- src/openvz/openvz_conf.c | 2 +- src/qemu/qemu_domain.c| 2 +- src/qemu/qemu_driver.c| 8 +++ src/remote/remote_daemon_stream.c | 2 +- src/util/virconf.c| 4 ++-- src/util/virerror.c | 4 ++-- src/util/virobject.c | 2 +- src/util/virstring.c | 4 ++-- src/vmware/vmware_conf.c | 4 ++-- src/vmware/vmware_driver.c| 4 ++-- 14 files changed, 60 insertions(+), 60 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH 02/19] qemu: capabilities: Introduce QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING
On Fri, Feb 12, 2021 at 08:57:08 +0100, Jiri Denemark wrote:
> On Thu, Feb 11, 2021 at 16:37:41 +0100, Peter Krempa wrote:
> > The capability represents qemu's ability to setup mappings for migrating
> > block dirty bitmaps and is based on presence of the 'transform' property
> > of the 'block-bitmap-mapping' property of 'migrate-set-parameters' QMP
> > command.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/qemu/qemu_capabilities.c | 2 ++
> > src/qemu/qemu_capabilities.h | 1 +
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index ccf810ff96..38555dde98 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -616,6 +616,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
> >"vhost-user-blk",
> >"cpu-max",
> >"memory-backend-file.x-use-canonical-path-for-ramblock-id",
> > + "migration-param.block-bitmap-mapping",
> > );
> >
> >
> > @@ -1549,6 +1550,7 @@ static struct virQEMUCapsStringFlags
> > virQEMUCapsQMPSchemaQueries[] = {
> > { "migrate-set-parameters/arg-type/xbzrle-cache-size",
> > QEMU_CAPS_MIGRATION_PARAM_XBZRLE_CACHE_SIZE },
> > { "set-numa-node/arg-type/+hmat-lb", QEMU_CAPS_NUMA_HMAT },
> > { "netdev_add/arg-type/+vhost-vdpa", QEMU_CAPS_NETDEV_VHOST_VDPA },
> > +{ "migrate-set-parameters/arg-type/block-bitmap-mapping/transform",
> > QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING },
> > };
> >
>
> So how is it possible this change is not reflected in
> tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml after the previous
> patch updated QEMU replies? Interestingly enough, tests pass after this
> patch so either the capability detection is not working or QEMU replies
> do not actually contain what you're looking for here.
Oops, there's a mistake in the query string where I've missed 'bitmaps'
subcomponent. Well actually I've already noticed this before, but
squashed the changes to a patch that I've ultimately removed from the
series ...
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c7ab144a8e..0e0926d0e5 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1550,7 +1550,8 @@ static struct virQEMUCapsStringFlags
virQEMUCapsQMPSchemaQueries[] = {
{ "migrate-set-parameters/arg-type/xbzrle-cache-size",
QEMU_CAPS_MIGRATION_PARAM_XBZRLE_CACHE_SIZE },
{ "set-numa-node/arg-type/+hmat-lb", QEMU_CAPS_NUMA_HMAT },
{ "netdev_add/arg-type/+vhost-vdpa", QEMU_CAPS_NETDEV_VHOST_VDPA },
-{ "migrate-set-parameters/arg-type/block-bitmap-mapping/transform",
QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING },
+{ "migrate-set-parameters/arg-type/block-bitmap-mapping/bitmaps/transform",
+ QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING },
};
typedef struct _virQEMUCapsObjectTypeProps virQEMUCapsObjectTypeProps;
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index f2ec32e46b..b9a36cb71e 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -255,6 +255,7 @@
+
5002050
0
43100242
