Re: [libvirt PATCH] qemu: fix potential resource leak
On Thu, Oct 22, 2020 at 14:08:51 -0500, Jonathon Jongsma wrote:
> On Thu, 22 Oct 2020 09:01:13 +0200
> Peter Krempa wrote:
>
> > On Wed, Oct 21, 2020 at 22:31:09 -0400, Laine Stump wrote:
> > > On 10/21/20 5:50 PM, Jonathon Jongsma wrote:
> > > > Coverity reported a potential resource leak. While it's probably
> > > > not a real-world scenario, the code could technically jump to
> > > > cleanup between the time that vdpafd is opened and when it is
> > > > used. Ensure that it gets cleaned up in that case.
> > > >
> > > > Signed-off-by: Jonathon Jongsma
> > > > ---
> > > > src/qemu/qemu_command.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > > index 5c4e37bd9e..cbe7a6e331 100644
> > > > --- a/src/qemu/qemu_command.c
> > > > +++ b/src/qemu/qemu_command.c
> > > > @@ -8135,6 +8135,7 @@
> > > > qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, addfdarg =
> > > > g_strdup_printf("%s,opaque=%s", fdset, net->data.vdpa.devicepath);
> > > > virCommandAddArgList(cmd, "-add-fd", addfdarg, NULL);
> > > > +vdpafd = -1;
> > > > }
> > > > if (chardev)
> > > > @@ -8204,6 +8205,8 @@
> > > > qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
> > > > VIR_FREE(tapfdName); VIR_FREE(vhostfd);
> > > > VIR_FREE(tapfd);
> > > > +if (vdpafd >= 0)
> > > > +VIR_FORCE_CLOSE(vdpafd);
> > >
> > >
> > > VIR_FORCE_CLOSE() ==>virForceCloseHelper() ==> virFileClose()
> > >
> > > and virFileClose() is a NOP if fd < 0, so this doesn't need the
> > > conditional.
> > >
> > >
> > > I *was* going to say "With that change,
> > >
> > >
> > > Reviewed-by: Laine Stump
> > >
> > > "
> > >
> > >
> > > but this got me looking at the code of the entire function rather
> > > than just the diffs in the patch, and I've got a question - is
> > > there any reason that you only ope n the vdpa device inside the
> > > switch, and save everything else related to it until later (at the
> > > "if (vdpafd)")? You could then device
> >
> > I'd like to point out that opening anything in the command line
> > formatters is IMO bad practice. The command line formatter should
> > format the commandline and nothing more. This is visible by the
> > necessity to have a lot of mock override functions which prevent the
> > command line formatter from touching resources on the host in the
> > first place.
> >
> > Better approach is to open resources in 'qemuProcessPrepareHost' and
> > store them in private data for command line formatting. Fake data for
> > tests are added in 'testCompareXMLToArgvCreateArgs'.
> >
> > I'm aware though that there's a lot of "prior art" in this area
> > though.
>
> These are good points. I fell into the trap of modeling the new code on
> some existing code that did similar things rather than thinking
> critically enough about the best way to do it. I'll look into a better
> approach.
Just keep this as a note for future code. No need to refactor your
addition, since there's a lot of other code which does the same. The
fix in this patch is fine once you drop the unneeded conditional.
Re: [libvirt PATCH] qemu: fix potential resource leak
On Thu, Oct 22, 2020 at 23:20:20 -0400, Laine Stump wrote:
> On 10/22/20 3:01 AM, Peter Krempa wrote:
> > On Wed, Oct 21, 2020 at 22:31:09 -0400, Laine Stump wrote:
> > > On 10/21/20 5:50 PM, Jonathon Jongsma wrote:
> > > > Coverity reported a potential resource leak. While it's probably not
> > > > a real-world scenario, the code could technically jump to cleanup
> > > > between the time that vdpafd is opened and when it is used. Ensure that
> > > > it gets cleaned up in that case.
> > > >
> > > > Signed-off-by: Jonathon Jongsma
> > > > ---
> > > >src/qemu/qemu_command.c | 3 +++
> > > >1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > > index 5c4e37bd9e..cbe7a6e331 100644
> > > > --- a/src/qemu/qemu_command.c
> > > > +++ b/src/qemu/qemu_command.c
> > > > @@ -8135,6 +8135,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr
> > > > driver,
> > > >addfdarg = g_strdup_printf("%s,opaque=%s", fdset,
> > > > net->data.vdpa.devicepath);
> > > >virCommandAddArgList(cmd, "-add-fd", addfdarg, NULL);
> > > > +vdpafd = -1;
> > > >}
> > > >if (chardev)
> > > > @@ -8204,6 +8205,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr
> > > > driver,
> > > >VIR_FREE(tapfdName);
> > > >VIR_FREE(vhostfd);
> > > >VIR_FREE(tapfd);
> > > > +if (vdpafd >= 0)
> > > > +VIR_FORCE_CLOSE(vdpafd);
> > >
> > > VIR_FORCE_CLOSE() ==>virForceCloseHelper() ==> virFileClose()
> > >
> > > and virFileClose() is a NOP if fd < 0, so this doesn't need the
> > > conditional.
> > >
> > >
> > > I *was* going to say "With that change,
> > >
> > >
> > > Reviewed-by: Laine Stump
> > >
> > > "
> > >
> > >
> > > but this got me looking at the code of the entire function rather than
> > > just
> > > the diffs in the patch, and I've got a question - is there any reason that
> > > you only ope n the vdpa device inside the switch, and save everything else
> > > related to it until later (at the "if (vdpafd)")? You could then device
> > I'd like to point out that opening anything in the command line
> > formatters is IMO bad practice.
>
>
> Well... I agree that it is an ugly design, but that's pretty much what's in
> place for almost everything.
Sure, but it shouldn't be used as an argument to not use a better
approach.
> > The command line formatter should format
> > the commandline and nothing more.
>
>
> It would be nice if that was the case, but it already isn't. :-/
No. Please don't put it this way. My message is that while the old code
isn't compliant, it shouldn't be an excuse to make it worse!
In this instace the code is commited. We don't need to change it, but
any further change should be encouraged to use the better approach.
> > This is visible by the necessity to
> > have a lot of mock override functions which prevent the command line
> > formatter from touching resources on the host in the first place.
> >
> > Better approach is to open resources in 'qemuProcessPrepareHost' and
> > store them in private data for command line formatting. Fake data for
> > tests are added in 'testCompareXMLToArgvCreateArgs'.
>
>
> That's nice in the fact that it eliminates the need for mock overrides
> (would be even *nicer* if that function had even a single line of
> documentation included that described its purpose, and what code in the qemu
> driver it should be mimicking, amirite?).
I agree, documentation is lacking in many parts. This is the
not-so-obvious techincal debt.
> But it's bad because the code in qemuProcessPrepareHost() won't be tested
> (can't be tested if there are no mocks for the system functions it calls).
IMO this statement is misrepresenting what's happening. Sure
qemuProcessPrepareHost can't be tested by unit testing. It's replaced by
another function which fakes inputs. But that is EXACTLY what the mock
functions preloaded into our tests are doing!
With qemuProcessPrepareHost() you at least have one place and one
function where all the code is aggregated rather than spreading it
trhough the command line generator and it being very hard to audit
afterwards.
> Basically you're trading the extra work of mocking system-level functions
> for the extra work of filling in stuff in the privateData (and the
> maintenance of that code), and eliminating testing of the code that's been
> moved (pretty *lame* testing, I'll admit, since it's getting back canned
> results from the fake system calls).
As noted before the extent of testing is exactly identical. The
difference is that all the code which is touching the host is aggregated
in one place and replaced by another function vs scattered accross the
whole file and LD_PRELOAD-ed.
> So it's not really the panacea your advocacy implies :-)
>
>
> (Don't get me wrong! I've always disliked the mixing of device/file/whatever
> init with the commandline generating functio
Re: [libvirt PATCH] qemu: fix potential resource leak
On 10/22/20 3:01 AM, Peter Krempa wrote:
On Wed, Oct 21, 2020 at 22:31:09 -0400, Laine Stump wrote:
On 10/21/20 5:50 PM, Jonathon Jongsma wrote:
Coverity reported a potential resource leak. While it's probably not
a real-world scenario, the code could technically jump to cleanup
between the time that vdpafd is opened and when it is used. Ensure that
it gets cleaned up in that case.
Signed-off-by: Jonathon Jongsma
---
src/qemu/qemu_command.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5c4e37bd9e..cbe7a6e331 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8135,6 +8135,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
addfdarg = g_strdup_printf("%s,opaque=%s", fdset,
net->data.vdpa.devicepath);
virCommandAddArgList(cmd, "-add-fd", addfdarg, NULL);
+vdpafd = -1;
}
if (chardev)
@@ -8204,6 +8205,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
VIR_FREE(tapfdName);
VIR_FREE(vhostfd);
VIR_FREE(tapfd);
+if (vdpafd >= 0)
+VIR_FORCE_CLOSE(vdpafd);
VIR_FORCE_CLOSE() ==>virForceCloseHelper() ==> virFileClose()
and virFileClose() is a NOP if fd < 0, so this doesn't need the conditional.
I *was* going to say "With that change,
Reviewed-by: Laine Stump
"
but this got me looking at the code of the entire function rather than just
the diffs in the patch, and I've got a question - is there any reason that
you only ope n the vdpa device inside the switch, and save everything else
related to it until later (at the "if (vdpafd)")? You could then device
I'd like to point out that opening anything in the command line
formatters is IMO bad practice.
Well... I agree that it is an ugly design, but that's pretty much what's
in place for almost everything.
The command line formatter should format
the commandline and nothing more.
It would be nice if that was the case, but it already isn't. :-/
This is visible by the necessity to
have a lot of mock override functions which prevent the command line
formatter from touching resources on the host in the first place.
Better approach is to open resources in 'qemuProcessPrepareHost' and
store them in private data for command line formatting. Fake data for
tests are added in 'testCompareXMLToArgvCreateArgs'.
That's nice in the fact that it eliminates the need for mock overrides
(would be even *nicer* if that function had even a single line of
documentation included that described its purpose, and what code in the
qemu driver it should be mimicking, amirite?).
But it's bad because the code in qemuProcessPrepareHost() won't be
tested (can't be tested if there are no mocks for the system functions
it calls). Basically you're trading the extra work of mocking
system-level functions for the extra work of filling in stuff in the
privateData (and the maintenance of that code), and eliminating testing
of the code that's been moved (pretty *lame* testing, I'll admit, since
it's getting back canned results from the fake system calls).
So it's not really the panacea your advocacy implies :-)
(Don't get me wrong! I've always disliked the mixing of
device/file/whatever init with the commandline generating functions.)
(actually a couple months ago I looked into putting the network
interface "prepare" stuff into privateData similar to what's done with
the slirp stuff now. In the end I gave up because it didn't provide the
result I wanted - I was trying to keep track of what device prep actions
had been done for which devices during domain startup so that the
shutdown in case of startup failure would only shutdown those things
that had actually been setup; it ended up being too complicated to make
it work correctly both in the case of an aborted startup and a normal
shutdown, once you took into account the possibility of libvirtd being
restarted as part of a libvirt package update.
I'll point out that during all my searches through the code during the
experiment referenced in the previous paragraph, I never ran across
testCompareXMLToArgvCreateArgs(), and didn't know of its existence (or
at least didn't remember it, if I had known about it before). Is this
documented somewhere? Or is it expected to be learned by reading every
patch coming across the mailing list (I unfortunately fail at that in a
major way)?
I'm aware though that there's a lot of "prior art" in this area though.
... and nothing in the code or the coding practices to warn against it,
point people in the other direction.
This sounds like another "saga" in the making - split all commandline
generating functions into separate "prepare device" and "generate
commandline" parts. I don't know that we should require Jonathon to
change his code that much just to fix a memory leak though ... (too bad
I hadn't kept up with the latest c
Re: [External] Re: [PATCH v3 0/2]support memory failure
On 10/23/20 1:00 AM, Michal Privoznik wrote: On 10/14/20 12:37 PM, zhenwei pi wrote: v2->v3: Rework patches to make each patch could be compiled, v1->v2: Seperate a 'all in one' patch into 4 patches. Use a 'flags' with bit definition instead of 'action_required' & 'recursive' for extention. Queue event directly without internal job. Add full test method in commit. v1: Since QEMU 5.2 (commit-77b285f7f6), QEMU supports 'memory failure' event, posts event to monitor if hitting a hardware memory error. zhenwei pi (2): libvirt: support memory failure event qemu: implement memory failure event include/libvirt/libvirt-domain.h | 82 + src/conf/domain_event.c | 80 src/conf/domain_event.h | 12 ++ src/libvirt_private.syms | 2 + src/remote/remote_daemon_dispatch.c | 32 +++ src/remote/remote_driver.c | 32 +++ src/remote/remote_protocol.x | 16 +++- src/remote_protocol-structs | 8 examples/c/misc/event-test.c | 16 tools/virsh-domain.c | 40 ++ src/qemu/qemu_monitor.c | 21 +- src/qemu/qemu_monitor.h | 39 ++ src/qemu/qemu_monitor_json.c | 49 ++ src/qemu/qemu_process.c | 59 ++ 14 files changed, 486 insertions(+), 2 deletions(-) Sorry for not replying earlier, had this marked for review but got sidetracked with some other work. Patches look good to me, but I'm suggesting couple of fixups. If you agree with them I can put my reviewed by tag and merge them. Michal I agree with these changes. Thanks a lot. -- zhenwei pi
[PATCH 4/6] hyperv: remove unneeded braces in hypervDomainGetInfo() and hypervDomainGetXMLDesc()
Signed-off-by: Matt Coleman
---
src/hyperv/hyperv_driver.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 6d5e899535..68835cad91 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -1053,9 +1053,8 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr
info)
if (hypervGetProcSDByVSSDInstanceId(priv,
virtualSystemSettingData->data.common->InstanceID,
-&processorSettingData) < 0) {
+&processorSettingData) < 0)
goto cleanup;
-}
if (hypervGetMsvmMemorySettingDataFromVSSD(priv,
virtualSystemSettingData->data.common->InstanceID,
@@ -1137,9 +1136,8 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int
flags)
if (hypervGetProcSDByVSSDInstanceId(priv,
virtualSystemSettingData->data.common->InstanceID,
-&processorSettingData) < 0) {
+&processorSettingData) < 0)
goto cleanup;
-}
if (hypervGetMsvmMemorySettingDataFromVSSD(priv,
virtualSystemSettingData->data.common->InstanceID,
--
2.27.0
[PATCH 6/6] hyperv: do not overwrite errors from hypervInvokeMethod()
Signed-off-by: Matt Coleman
---
src/hyperv/hyperv_driver.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index fba1e355db..a71d0d6261 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -1800,11 +1800,8 @@ hypervDomainSendKey(virDomainPtr domain, unsigned int
codeset,
if (hypervAddSimpleParam(params, "keyCode", keycodeStr) < 0)
goto cleanup;
-if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not press key %d"),
- translatedKeycodes[i]);
+if (hypervInvokeMethod(priv, ¶ms, NULL) < 0)
goto cleanup;
-}
}
/* simulate holdtime by sleeping */
@@ -1823,11 +1820,8 @@ hypervDomainSendKey(virDomainPtr domain, unsigned int
codeset,
if (hypervAddSimpleParam(params, "keyCode", keycodeStr) < 0)
goto cleanup;
-if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not release key %s"), keycodeStr);
+if (hypervInvokeMethod(priv, ¶ms, NULL) < 0)
goto cleanup;
-}
}
result = 0;
@@ -1919,10 +1913,8 @@ hypervDomainSetMemoryFlags(virDomainPtr domain, unsigned
long memory,
}
}
-if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not set
memory"));
+if (hypervInvokeMethod(priv, ¶ms, NULL) < 0)
goto cleanup;
-}
result = 0;
--
2.27.0
[PATCH 3/6] hyperv: remove duplicate function hypervGetMemSDByVSSDInstanceId()
Signed-off-by: Matt Coleman
---
src/hyperv/hyperv_driver.c | 39 ++
1 file changed, 6 insertions(+), 33 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 3935320ea9..6d5e899535 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -246,31 +246,6 @@ hypervGetProcSDByVSSDInstanceId(hypervPrivate *priv, const
char *id,
}
-static int
-hypervGetMemSDByVSSDInstanceId(hypervPrivate *priv, const char *id,
- Msvm_MemorySettingData **data)
-{
-g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
-virBufferEscapeSQL(&query,
- "ASSOCIATORS OF
{Msvm_VirtualSystemSettingData.InstanceID='%s'} "
- "WHERE AssocClass =
Msvm_VirtualSystemSettingDataComponent "
- "ResultClass = Msvm_MemorySettingData",
- id);
-
-if (hypervGetWmiClass(Msvm_MemorySettingData, data) < 0)
-return -1;
-
-if (!*data) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not look up memory setting data with virtual
system instance ID '%s'"),
- id);
-return -1;
-}
-
-return 0;
-}
-
-
static int
hypervRequestStateChange(virDomainPtr domain, int state)
{
@@ -1082,11 +1057,10 @@ hypervDomainGetInfo(virDomainPtr domain,
virDomainInfoPtr info)
goto cleanup;
}
-if (hypervGetMemSDByVSSDInstanceId(priv,
-
virtualSystemSettingData->data.common->InstanceID,
- &memorySettingData) < 0) {
+if (hypervGetMsvmMemorySettingDataFromVSSD(priv,
+
virtualSystemSettingData->data.common->InstanceID,
+ &memorySettingData) < 0)
goto cleanup;
-}
/* Fill struct */
info->state =
hypervMsvmComputerSystemEnabledStateToDomainState(computerSystem);
@@ -1167,11 +1141,10 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned
int flags)
goto cleanup;
}
-if (hypervGetMemSDByVSSDInstanceId(priv,
-
virtualSystemSettingData->data.common->InstanceID,
- &memorySettingData) < 0) {
+if (hypervGetMsvmMemorySettingDataFromVSSD(priv,
+
virtualSystemSettingData->data.common->InstanceID,
+ &memorySettingData) < 0)
goto cleanup;
-}
/* Fill struct */
def->virtType = VIR_DOMAIN_VIRT_HYPERV;
--
2.27.0
[PATCH 5/6] hyperv: reduce duplicate code for Msvm_ComputerSystem lookups
Signed-off-by: Matt Coleman
---
src/hyperv/hyperv_driver.c | 26 +-
src/hyperv/hyperv_wmi.c| 36 ++--
src/hyperv/hyperv_wmi.h| 4
3 files changed, 27 insertions(+), 39 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 68835cad91..fba1e355db 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -172,30 +172,6 @@ hypervGetVirtualSystemByID(hypervPrivate *priv, int id,
}
-static int
-hypervGetVirtualSystemByUUID(hypervPrivate *priv, const char *uuid,
- Msvm_ComputerSystem **computerSystemList)
-{
-g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
-virBufferEscapeSQL(&query,
- MSVM_COMPUTERSYSTEM_WQL_SELECT
- "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL
- "AND Name = '%s'",
- uuid);
-
-if (hypervGetWmiClass(Msvm_ComputerSystem, computerSystemList) < 0)
-return -1;
-
-if (*computerSystemList == NULL) {
-virReportError(VIR_ERR_NO_DOMAIN,
- _("No domain with UUID %s"), uuid);
-return -1;
-}
-
-return 0;
-}
-
-
static int
hypervGetVirtualSystemByName(hypervPrivate *priv, const char *name,
Msvm_ComputerSystem **computerSystemList)
@@ -806,7 +782,7 @@ hypervDomainLookupByUUID(virConnectPtr conn, const unsigned
char *uuid)
virUUIDFormat(uuid, uuid_string);
-if (hypervGetVirtualSystemByUUID(priv, uuid_string, &computerSystem) < 0)
+if (hypervMsvmComputerSystemFromUUID(priv, uuid_string, &computerSystem) <
0)
goto cleanup;
hypervMsvmComputerSystemToDomain(conn, computerSystem, &domain);
diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c
index d804558fe4..66aed01832 100644
--- a/src/hyperv/hyperv_wmi.c
+++ b/src/hyperv/hyperv_wmi.c
@@ -1508,32 +1508,27 @@ hypervMsvmComputerSystemToDomain(virConnectPtr conn,
int
-hypervMsvmComputerSystemFromDomain(virDomainPtr domain,
- Msvm_ComputerSystem **computerSystem)
+hypervMsvmComputerSystemFromUUID(hypervPrivate *priv, const char *uuid,
+ Msvm_ComputerSystem **computerSystem)
{
-hypervPrivate *priv = domain->conn->privateData;
-char uuid_string[VIR_UUID_STRING_BUFLEN];
g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
-if (computerSystem == NULL || *computerSystem != NULL) {
+if (!computerSystem || *computerSystem) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
return -1;
}
-virUUIDFormat(domain->uuid, uuid_string);
-
-virBufferAsprintf(&query,
- MSVM_COMPUTERSYSTEM_WQL_SELECT
- "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL
- "AND Name = '%s'", uuid_string);
+virBufferEscapeSQL(&query,
+ MSVM_COMPUTERSYSTEM_WQL_SELECT
+ "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL
+ "AND Name = '%s'", uuid);
if (hypervGetWmiClassList(priv, Msvm_ComputerSystem_WmiInfo, &query,
(hypervObject **)computerSystem) < 0)
return -1;
-if (*computerSystem == NULL) {
-virReportError(VIR_ERR_NO_DOMAIN,
- _("No domain with UUID %s"), uuid_string);
+if (!*computerSystem) {
+virReportError(VIR_ERR_NO_DOMAIN, _("No domain with UUID %s"), uuid);
return -1;
}
@@ -1541,6 +1536,19 @@ hypervMsvmComputerSystemFromDomain(virDomainPtr domain,
}
+int
+hypervMsvmComputerSystemFromDomain(virDomainPtr domain,
+ Msvm_ComputerSystem **computerSystem)
+{
+hypervPrivate *priv = domain->conn->privateData;
+char uuidString[VIR_UUID_STRING_BUFLEN];
+
+virUUIDFormat(domain->uuid, uuidString);
+
+return hypervMsvmComputerSystemFromUUID(priv, uuidString, computerSystem);
+}
+
+
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
* Msvm_VirtualSystemSettingData
*/
diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h
index 570aa07eb8..5b97ab3db9 100644
--- a/src/hyperv/hyperv_wmi.h
+++ b/src/hyperv/hyperv_wmi.h
@@ -233,5 +233,9 @@ int hypervMsvmComputerSystemToDomain(virConnectPtr conn,
Msvm_ComputerSystem *computerSystem,
virDomainPtr *domain);
+int
+hypervMsvmComputerSystemFromUUID(hypervPrivate *priv, const char *uuid,
+ Msvm_ComputerSystem **computerSystem);
+
int hypervMsvmComputerSystemFromDomain(virDomainPtr domain,
Msvm_ComputerSystem **computerSystem);
--
2.27.0
[PATCH 2/6] hyperv: remove duplicate function hypervGetVSSDFromUUID()
Signed-off-by: Matt Coleman
---
src/hyperv/hyperv_driver.c | 42 --
1 file changed, 9 insertions(+), 33 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index dbc864b6fa..3935320ea9 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -220,30 +220,6 @@ hypervGetVirtualSystemByName(hypervPrivate *priv, const
char *name,
}
-static int
-hypervGetVSSDFromUUID(hypervPrivate *priv, const char *uuid,
- Msvm_VirtualSystemSettingData **data)
-{
-g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
-virBufferEscapeSQL(&query,
- "ASSOCIATORS OF
{Msvm_ComputerSystem.CreationClassName='Msvm_ComputerSystem',Name='%s'} "
- "WHERE AssocClass = Msvm_SettingsDefineState "
- "ResultClass = Msvm_VirtualSystemSettingData",
- uuid);
-
-if (hypervGetWmiClass(Msvm_VirtualSystemSettingData, data) < 0)
-return -1;
-
-if (!*data) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not look up virtual system setting data with
UUID '%s'"),
- uuid);
-return -1;
-}
-
-return 0;
-}
-
static int
hypervGetProcSDByVSSDInstanceId(hypervPrivate *priv, const char *id,
@@ -1096,10 +1072,9 @@ hypervDomainGetInfo(virDomainPtr domain,
virDomainInfoPtr info)
if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0)
goto cleanup;
-if (hypervGetVSSDFromUUID(priv, uuid_string,
- &virtualSystemSettingData) < 0) {
+if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string,
+
&virtualSystemSettingData) < 0)
goto cleanup;
-}
if (hypervGetProcSDByVSSDInstanceId(priv,
virtualSystemSettingData->data.common->InstanceID,
@@ -1182,10 +1157,9 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int
flags)
if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0)
goto cleanup;
-if (hypervGetVSSDFromUUID(priv, uuid_string,
- &virtualSystemSettingData) < 0) {
+if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string,
+
&virtualSystemSettingData) < 0)
goto cleanup;
-}
if (hypervGetProcSDByVSSDInstanceId(priv,
virtualSystemSettingData->data.common->InstanceID,
@@ -1397,7 +1371,7 @@ hypervDomainGetAutostart(virDomainPtr domain, int
*autostart)
*autostart = vsgsd->data.common->AutomaticStartupAction == 2;
result = 0;
} else {
-if (hypervGetVSSDFromUUID(priv, uuid_string, &vssd) < 0)
+if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string,
&vssd) < 0)
goto cleanup;
*autostart = vssd->data.v2->AutomaticStartupAction == 4;
@@ -1446,7 +1420,7 @@ hypervDomainSetAutostart(virDomainPtr domain, int
autostart)
virUUIDFormat(domain->uuid, uuid_string);
-if (hypervGetVSSDFromUUID(priv, uuid_string, &vssd) < 0)
+if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string,
&vssd) < 0)
goto cleanup;
params = hypervCreateInvokeParamsList(priv, methodName,
@@ -1721,7 +1695,9 @@ hypervConnectListAllDomains(virConnectPtr conn,
goto cleanup;
}
-virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT "WHERE "
MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
+virBufferAddLit(&query,
+MSVM_COMPUTERSYSTEM_WQL_SELECT
+"WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL);
/* construct query with filter depending on flags */
if (!(MATCH(VIR_CONNECT_LIST_DOMAINS_ACTIVE) &&
--
2.27.0
[PATCH 1/6] hyperv: reformat WQL query strings
Signed-off-by: Matt Coleman
---
src/hyperv/hyperv_driver.c | 50 +-
src/hyperv/hyperv_wmi.c| 29 +++---
2 files changed, 36 insertions(+), 43 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 002434c56a..dbc864b6fa 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -68,7 +68,7 @@ hypervGetProcessorsByName(hypervPrivate *priv, const char
*name,
{
g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
virBufferEscapeSQL(&query,
- "ASSOCIATORS OF {Win32_ComputerSystem.Name=\"%s\"} "
+ "ASSOCIATORS OF {Win32_ComputerSystem.Name='%s'} "
"WHERE AssocClass = Win32_ComputerSystemProcessor "
"ResultClass = Win32_Processor",
name);
@@ -180,7 +180,7 @@ hypervGetVirtualSystemByUUID(hypervPrivate *priv, const
char *uuid,
virBufferEscapeSQL(&query,
MSVM_COMPUTERSYSTEM_WQL_SELECT
"WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL
- "AND Name = \"%s\"",
+ "AND Name = '%s'",
uuid);
if (hypervGetWmiClass(Msvm_ComputerSystem, computerSystemList) < 0)
@@ -204,7 +204,7 @@ hypervGetVirtualSystemByName(hypervPrivate *priv, const
char *name,
virBufferEscapeSQL(&query,
MSVM_COMPUTERSYSTEM_WQL_SELECT
"WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL
- "AND ElementName = \"%s\"",
+ "AND ElementName = '%s'",
name);
if (hypervGetWmiClass(Msvm_ComputerSystem, computerSystemList) < 0)
@@ -226,7 +226,7 @@ hypervGetVSSDFromUUID(hypervPrivate *priv, const char *uuid,
{
g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
virBufferEscapeSQL(&query,
- "ASSOCIATORS OF
{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\",Name=\"%s\"} "
+ "ASSOCIATORS OF
{Msvm_ComputerSystem.CreationClassName='Msvm_ComputerSystem',Name='%s'} "
"WHERE AssocClass = Msvm_SettingsDefineState "
"ResultClass = Msvm_VirtualSystemSettingData",
uuid);
@@ -251,7 +251,7 @@ hypervGetProcSDByVSSDInstanceId(hypervPrivate *priv, const
char *id,
{
g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
virBufferEscapeSQL(&query,
- "ASSOCIATORS OF
{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
+ "ASSOCIATORS OF
{Msvm_VirtualSystemSettingData.InstanceID='%s'} "
"WHERE AssocClass =
Msvm_VirtualSystemSettingDataComponent "
"ResultClass = Msvm_ProcessorSettingData",
id);
@@ -276,7 +276,7 @@ hypervGetMemSDByVSSDInstanceId(hypervPrivate *priv, const
char *id,
{
g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
virBufferEscapeSQL(&query,
- "ASSOCIATORS OF
{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} "
+ "ASSOCIATORS OF
{Msvm_VirtualSystemSettingData.InstanceID='%s'} "
"WHERE AssocClass =
Msvm_VirtualSystemSettingDataComponent "
"ResultClass = Msvm_MemorySettingData",
id);
@@ -346,10 +346,9 @@ static int
hypervLookupHostSystemBiosUuid(hypervPrivate *priv, unsigned char *uuid)
{
Win32_ComputerSystemProduct *computerSystem = NULL;
-g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER;
+g_auto(virBuffer) query = {
g_string_new(WIN32_COMPUTERSYSTEMPRODUCT_WQL_SELECT), 0 };
int result = -1;
-virBufferAddLit(&query, WIN32_COMPUTERSYSTEMPRODUCT_WQL_SELECT);
if (hypervGetWmiClass(Win32_ComputerSystemProduct, &computerSystem) < 0)
goto cleanup;
@@ -459,18 +458,18 @@ hypervInitConnection(virConnectPtr conn, hypervPrivate
*priv,
wqlQuery.info = Msvm_ComputerSystem_WmiInfo;
wqlQuery.query = &query;
-virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT);
-virBufferAddLit(&query, "WHERE ");
-virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_PHYSICAL);
+virBufferAddLit(&query,
+MSVM_COMPUTERSYSTEM_WQL_SELECT
+"WHERE " MSVM_COMPUTERSYSTEM_WQL_PHYSICAL);
/* try query using V2 namespace (for Hyper-V 2012+) */
priv->wmiVersion = HYPERV_WMI_VERSION_V2;
if (hypervEnumAndPull(priv, &wqlQuery, &computerSystem) < 0) {
/* rebuild query because hypervEnumAndPull consumes it */
-virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT);
-virBufferAddLit(&query, "WHERE ");
-virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_PHYSICAL);
+virBufferAddLit(&query,
+MSVM_COMPUTERSYSTEM_WQL_SELECT
+"WHERE " MSVM_COMPUTERSYSTEM_WQL_PHYSICAL);
[PATCH 0/6] Hyper-V code cleanup
Here's a draft GitLab MR if you'd prefer to review the changes there: https://gitlab.com/iammattcoleman/libvirt/-/merge_requests/3/diffs Matt Coleman (6): hyperv: reformat WQL query strings hyperv: remove duplicate function hypervGetVSSDFromUUID() hyperv: remove duplicate function hypervGetMemSDByVSSDInstanceId() hyperv: remove unneeded braces in hypervDomainGetInfo() and hypervDomainGetXMLDesc() hyperv: reduce duplicate code for Msvm_ComputerSystem lookups hyperv: do not overwrite errors from hypervInvokeMethod() src/hyperv/hyperv_driver.c | 169 + src/hyperv/hyperv_wmi.c| 57 +++-- src/hyperv/hyperv_wmi.h| 4 + 3 files changed, 75 insertions(+), 155 deletions(-) -- 2.27.0
[PATCH 1/1] virt-aa-helper: allow hard links for mounts
Guests should be allowed to create hard links on mounted pathes, since many applications rely on this functionality and would error on guest with current "rw" AppArmor permission with 9pfs. Signed-off-by: Christian Schoenebeck --- src/security/virt-aa-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 12429278fb..5a6f4a5f7d 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1142,7 +1142,7 @@ get_files(vahControl * ctl) /* We don't need to add deny rw rules for readonly mounts, * this can only lead to troubles when mounting / readonly. */ -if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", true) != 0) +if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwl", true) != 0) goto cleanup; } } -- 2.20.1
[PATCH 0/1] virt-aa-helper: allow hard links for mounts
I'm suggesting to add the AppArmor permission "l" on libvirt export pathes to allow guests creating hard links, which is currently a problem for 9pfs mounts. Since the suggested patch would affect virtiofs as well, I would ask David and Stefan for feedback. If necessary I would change the patch to exclude virtiofs from this change. Christian Schoenebeck (1): virt-aa-helper: allow hard links for mounts src/security/virt-aa-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.20.1
Re: [libvirt PATCH] qemu: fix potential resource leak
On Thu, 22 Oct 2020 09:01:13 +0200
Peter Krempa wrote:
> On Wed, Oct 21, 2020 at 22:31:09 -0400, Laine Stump wrote:
> > On 10/21/20 5:50 PM, Jonathon Jongsma wrote:
> > > Coverity reported a potential resource leak. While it's probably
> > > not a real-world scenario, the code could technically jump to
> > > cleanup between the time that vdpafd is opened and when it is
> > > used. Ensure that it gets cleaned up in that case.
> > >
> > > Signed-off-by: Jonathon Jongsma
> > > ---
> > > src/qemu/qemu_command.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index 5c4e37bd9e..cbe7a6e331 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -8135,6 +8135,7 @@
> > > qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, addfdarg =
> > > g_strdup_printf("%s,opaque=%s", fdset, net->data.vdpa.devicepath);
> > > virCommandAddArgList(cmd, "-add-fd", addfdarg, NULL);
> > > +vdpafd = -1;
> > > }
> > > if (chardev)
> > > @@ -8204,6 +8205,8 @@
> > > qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
> > > VIR_FREE(tapfdName); VIR_FREE(vhostfd);
> > > VIR_FREE(tapfd);
> > > +if (vdpafd >= 0)
> > > +VIR_FORCE_CLOSE(vdpafd);
> >
> >
> > VIR_FORCE_CLOSE() ==>virForceCloseHelper() ==> virFileClose()
> >
> > and virFileClose() is a NOP if fd < 0, so this doesn't need the
> > conditional.
> >
> >
> > I *was* going to say "With that change,
> >
> >
> > Reviewed-by: Laine Stump
> >
> > "
> >
> >
> > but this got me looking at the code of the entire function rather
> > than just the diffs in the patch, and I've got a question - is
> > there any reason that you only ope n the vdpa device inside the
> > switch, and save everything else related to it until later (at the
> > "if (vdpafd)")? You could then device
>
> I'd like to point out that opening anything in the command line
> formatters is IMO bad practice. The command line formatter should
> format the commandline and nothing more. This is visible by the
> necessity to have a lot of mock override functions which prevent the
> command line formatter from touching resources on the host in the
> first place.
>
> Better approach is to open resources in 'qemuProcessPrepareHost' and
> store them in private data for command line formatting. Fake data for
> tests are added in 'testCompareXMLToArgvCreateArgs'.
>
> I'm aware though that there's a lot of "prior art" in this area
> though.
These are good points. I fell into the trap of modeling the new code on
some existing code that did similar things rather than thinking
critically enough about the best way to do it. I'll look into a better
approach.
Jonathon
Re: [PATCH 1/3] qemu: add option to process offloaded legacy blockjob event ealier
On 22.10.2020 15:12, Peter Krempa wrote:
> On Tue, Oct 20, 2020 at 16:44:07 +0300, Nikolay Shirokovskiy wrote:
>> Currently in qemuProcessHandleBlockJob we either offload blockjob event
>> processing to the worker thread or notify another thread that waits for
>> blockjob event and that thread processes the event. But sometimes after event
>> is offloaded to the worker thread we need to process the event in a different
>> thread.
>>
>> To be able to to do it let's always set newstate/errmsg and then let whatever
>> thread is first process it.
>>
>> Signed-off-by: Nikolay Shirokovskiy
>> ---
>> src/qemu/qemu_driver.c | 17 -
>> src/qemu/qemu_process.c | 20
>> 2 files changed, 16 insertions(+), 21 deletions(-)
>>
>
> [...]
>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 6422881..4d63e7d 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -953,13 +953,19 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon
>> G_GNUC_UNUSED,
>> if (!(disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, diskAlias,
>> NULL)))
>> goto cleanup;
>>
>> -job = qemuBlockJobDiskGetJob(disk);
>> +if (!(job = qemuBlockJobDiskGetJob(disk))) {
>> +VIR_DEBUG("creating new block job object for '%s'", diskAlias);
>> +if (!(job = qemuBlockJobDiskNew(vm, disk, type, diskAlias)))
>
> So this actually writes out the status XML. I'm not sure if that is a
> good idea to do if we don't hold the domain job. The premise is that
> threads holding the job lock might have partially modified it.
>
Hi, Peter.
Yeah, I did not notice that.
There is another reason to have this patch (modified of course to prevent the
issue
you mentioned). Without it is just hard to synchronize block jobs correctly on
reconnect.
We call "query-block-jobs" to do the sync and set block jobs state based on
query result. But first we can have pending events that we received before
quering. So after the reconnect we have to deal with this outdated events.
Second we can receive events after querying but before reconnection is finished
and these events also will be offloaded. This can be an issue if we use sync
mode like as in qemuMigrationSrcCancel because we won't see this events as they
offloaded. I don't think qemuMigrationSrcCancel is really affected as all we
want to do it just cancel block jobs.
So we can miss some block job events in sync mode on reconnection and can have
outdated block job events after reconnection. Probably it can be handled
another way but with the approach as in this patch we can eliminate these
possibilities.
Also "new" block job code uses approach just as this patch namely save
state changes in job so other threads can see it not just the worker thread.
And this option is used by reconnection code for new block jobs.
Nikolay
Re: [PATCH 0/4] hyperv: Deduplicate and reformat
> On Oct 21, 2020, at 12:11 PM, Matt Coleman wrote: > >> On Oct 21, 2020, at 11:44 AM, Michal Privoznik wrote: >> >> The more I look into the code the more things to fix I find. Well, here >> are some fixes. > > Thanks! This whole patchset looks good. Reviewed-by: Matt Coleman -- Matt
Re: [PATCH 0/6] qemu: Fix cdrom as SCSI hostdev via -blockdev
Greetings Peter, > Sent: Tuesday, October 20, 2020 at 2:54 PM > From: "daggs" > To: "daggs" > Cc: "Peter Krempa" , libvir-list@redhat.com > Subject: Re: [PATCH 0/6] qemu: Fix cdrom as SCSI hostdev via -blockdev > > Greetings Peter, > > so apparently, the os I'm using in the vm comes with scsi virtio disabled in > the kernel... > will rebuild the os and report. > > thanks for all your work. > > Dagg. > I've built the os with scsi virtio, now I do see the cdrom, see: # ls -lia /sys/bus/scsi/devices total 0 7017 drwxr-xr-x2 root root 0 Oct 22 2020 . 7015 drwxr-xr-x4 root root 0 Oct 22 2020 .. 18874 lrwxrwxrwx1 root root 0 Oct 22 2020 0:0:0:0 -> ../../../devices/pci:00/:00:1e.0/:06:00.0/:07:01.0/virtio2/host0/target0:0:0/0:0:0:0 17468 lrwxrwxrwx1 root root 0 Oct 22 2020 host0 -> ../../../devices/pci:00/:00:1e.0/:06:00.0/:07:01.0/virtio2/host0 18051 lrwxrwxrwx1 root root 0 Oct 22 2020 host1 -> ../../../devices/pci:00/:00:1f.2/ata1/host1 18099 lrwxrwxrwx1 root root 0 Oct 22 2020 host2 -> ../../../devices/pci:00/:00:1f.2/ata2/host2 18147 lrwxrwxrwx1 root root 0 Oct 22 2020 host3 -> ../../../devices/pci:00/:00:1f.2/ata3/host3 18195 lrwxrwxrwx1 root root 0 Oct 22 2020 host4 -> ../../../devices/pci:00/:00:1f.2/ata4/host4 18243 lrwxrwxrwx1 root root 0 Oct 22 2020 host5 -> ../../../devices/pci:00/:00:1f.2/ata5/host5 18291 lrwxrwxrwx1 root root 0 Oct 22 2020 host6 -> ../../../devices/pci:00/:00:1f.2/ata6/host6 18835 lrwxrwxrwx1 root root 0 Oct 22 2020 target0:0:0 -> ../../../devices/pci:00/:00:1e.0/:06:00.0/:07:01.0/virtio2/host0/target0:0:0 I've tested with 6.7.0 and 6.8.0 + your patchset, in both the cdrom is visible, in 6.7.0, the guest's dmesg is filled with theses errors: [ 57.510366] sr 0:0:0:0: ioctl_internal_command return code = 802 [ 57.510400] sr 0:0:0:0: Sense Key : 0xb [current] [ 57.510403] sr 0:0:0:0: ASC=0x0 ASCQ=0x6 every few seconds these lines are added to the log. with 6.8.0 + your patchset, I see these outputs only few times in the boot process. here is the dmesg: https://dpaste.com/F32A9B92H I've inserted a audio cd and expected the os ui to detected it but it didn't. I assume there is another bug there. anything I can do to help hunting this bug? Thanks. Dagg.
Re: [PATCH 1/1] virt-aa-helper: allow hard links for mounts
[Please don't CC random people on patches until asked to, we are all subscribed to the list] On 10/22/20 4:58 PM, Christian Schoenebeck wrote: Guests should be allowed to create hard links on mounted pathes, since many applications rely on this functionality and would error on guest with current "rw" AppArmor permission with 9pfs. Signed-off-by: Christian Schoenebeck --- src/security/virt-aa-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 12429278fb..5a6f4a5f7d 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1142,7 +1142,7 @@ get_files(vahControl * ctl) /* We don't need to add deny rw rules for readonly mounts, * this can only lead to troubles when mounting / readonly. */ -if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", true) != 0) +if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwl", true) != 0) goto cleanup; } } Reviewed-by: Michal Privoznik but I will give a day or two for other developers to chime in. Michal
Re: [PATCH v3 1/2] libvirt: support memory failure event
On 10/14/20 12:37 PM, zhenwei pi wrote:
Introduce memory failure event. Libvirt should monitor domain's
event, then posts it to uplayer. According to the hardware memory
corrupted message, a cloud scheduler could migrate domain to another
health physical server.
Several changes in this patch:
public API:
include/*
src/conf/*
src/remote/*
src/remote_protocol-structs
client:
examples/c/misc/event-test.c
tools/virsh-domain.c
With this patch, each driver could implement its own method to run
this new event.
Signed-off-by: zhenwei pi
---
include/libvirt/libvirt-domain.h| 82 +
src/conf/domain_event.c | 80
src/conf/domain_event.h | 12 ++
src/libvirt_private.syms| 2 +
src/remote/remote_daemon_dispatch.c | 32 +++
src/remote/remote_driver.c | 32 +++
src/remote/remote_protocol.x| 16 +++-
src/remote_protocol-structs | 8
examples/c/misc/event-test.c| 16
tools/virsh-domain.c| 40 ++
10 files changed, 319 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 77f9116675..5138843a56 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -3196,6 +3196,64 @@ typedef enum {
} virDomainEventCrashedDetailType;
/**
+ * virDomainMemoryFailureRecipientType:
+ *
+ * Recipient of a memory failure event.
+ */
+typedef enum {
+/* memory failure at hypersivor memory address space */
+VIR_DOMAIN_EVENT_MEMORY_FAILURE_RECIPIENT_HYPERVISOR = 0,
+
+/* memory failure at guest memory address space */
+VIR_DOMAIN_EVENT_MEMORY_FAILURE_RECIPIENT_GUEST = 1,
+
+# ifdef VIR_ENUM_SENTINELS
+VIR_DOMAIN_EVENT_MEMORY_FAILURE_RECIPIENT_LAST
+# endif
+} virDomainMemoryFailureRecipientType;
+
+
+/**
+ * virDomainMemoryFailureActionType:
+ *
+ * Action of a memory failure event.
+ */
+typedef enum {
+/* the memory failure could be ignored. This will only be the case for
+ * action-optional failures. */
+VIR_DOMAIN_EVENT_MEMORY_FAILURE_ACTION_IGNORE = 0,
+
+/* memory failure occurred in guest memory, the guest enabled MCE handling
+ * mechanism, and hypervisor could inject the MCE into the guest
+ * successfully. */
+VIR_DOMAIN_EVENT_MEMORY_FAILURE_ACTION_INJECT = 1,
+
+/* the failure is unrecoverable. This occurs for action-required failures
+ * if the recipient is the hypervisor; hypervisor will exit. */
+VIR_DOMAIN_EVENT_MEMORY_FAILURE_ACTION_FATAL = 2,
+
+/* the failure is unrecoverable but confined to the guest. This occurs if
+ * the recipient is a guest which is not ready to handle memory failures.
*/
+VIR_DOMAIN_EVENT_MEMORY_FAILURE_ACTION_RESET = 3,
+
+# ifdef VIR_ENUM_SENTINELS
+VIR_DOMAIN_EVENT_MEMORY_FAILURE_ACTION_LAST
+# endif
+} virDomainMemoryFailureActionType;
+
+
+typedef enum {
+/* whether a memory failure event is action-required or action-optional
+ * (e.g. a failure during memory scrub). */
+VIR_DOMAIN_MEMORY_FAILURE_ACTION_REQUIRED = (1 << 0),
+
+/* whether the failure occurred while the previous failure was still in
+ * progress. */
+VIR_DOMAIN_MEMORY_FAILURE_RECURSIVE = (1 << 1),
+} virDomainMemoryFailureFlags;
+
+
+/**
* virConnectDomainEventCallback:
* @conn: virConnect connection
* @dom: The domain on which the event occurred
@@ -4565,6 +4623,29 @@ typedef void
(*virConnectDomainEventBlockThresholdCallback)(virConnectPtr conn,
void *opaque);
/**
+ * virConnectDomainEventMemoryFailureCallback:
+ * @conn: connection object
+ * @dom: domain on which the event occurred
+ * @recipient: the recipient of hardware memory failure
+ * @action: the action of hardware memory failure
+ * @flags: the flags of hardware memory failure
+ * @opaque: application specified data
+ *
+ * The callback occurs when the hypervisor handles the hardware memory
+ * corrupted event.
+ *
+ * The callback signature to use when registering for an event of type
+ * VIR_DOMAIN_EVENT_ID_MEMORY_FAILURE with virConnectDomainEventRegisterAny()
+ */
+typedef void (*virConnectDomainEventMemoryFailureCallback)(virConnectPtr conn,
+ virDomainPtr dom,
+
virDomainMemoryFailureRecipientType recipient,
+
virDomainMemoryFailureActionType action,
While this works for now, it's not as future proof as it could be. We
try to avoid enums in public APIs because if we ever add a new member
into the enum its size might change and thus break the ABI. Use 'int'
instead. That is also the reason why we use int on the RPC level. Then
we merely document i
Re: [PATCH v3 0/2]support memory failure
On 10/14/20 12:37 PM, zhenwei pi wrote: v2->v3: Rework patches to make each patch could be compiled, v1->v2: Seperate a 'all in one' patch into 4 patches. Use a 'flags' with bit definition instead of 'action_required' & 'recursive' for extention. Queue event directly without internal job. Add full test method in commit. v1: Since QEMU 5.2 (commit-77b285f7f6), QEMU supports 'memory failure' event, posts event to monitor if hitting a hardware memory error. zhenwei pi (2): libvirt: support memory failure event qemu: implement memory failure event include/libvirt/libvirt-domain.h| 82 + src/conf/domain_event.c | 80 src/conf/domain_event.h | 12 ++ src/libvirt_private.syms| 2 + src/remote/remote_daemon_dispatch.c | 32 +++ src/remote/remote_driver.c | 32 +++ src/remote/remote_protocol.x| 16 +++- src/remote_protocol-structs | 8 examples/c/misc/event-test.c| 16 tools/virsh-domain.c| 40 ++ src/qemu/qemu_monitor.c | 21 +- src/qemu/qemu_monitor.h | 39 ++ src/qemu/qemu_monitor_json.c| 49 ++ src/qemu/qemu_process.c | 59 ++ 14 files changed, 486 insertions(+), 2 deletions(-) Sorry for not replying earlier, had this marked for review but got sidetracked with some other work. Patches look good to me, but I'm suggesting couple of fixups. If you agree with them I can put my reviewed by tag and merge them. Michal
Re: [PATCH v3 2/2] qemu: implement memory failure event
On 10/14/20 12:37 PM, zhenwei pi wrote:
Since QEMU 5.2 (commit-77b285f7f6), QEMU supports 'memory failure'
event, posts event to monitor if hitting a hardware memory error.
Fully support this feature for QEMU.
Test with commit 'libvirt: support memory failure event', build a
little complex environment(nested KVM):
1, install newly built libvirt in L1, and start a L2 vm. run command
in L1:
~# virsh event l2 --event memory-failure
2, run command in L0 to inject MCE to L1:
~# virsh qemu-monitor-command l1 --hmp mce 0 9 0xbdc0 0xd
0x6200 0x8c
Test result in l1(recipient hypervisor case):
event 'memory-failure' for domain l2:
recipient: hypervisor
action: ignore
flags:
action required: 0
recursive: 0
Test result in l1(recipient guest case):
event 'memory-failure' for domain l2:
recipient: guest
action: inject
flags:
action required: 0
recursive: 0
Signed-off-by: zhenwei pi
---
src/qemu/qemu_monitor.c | 21 +++-
src/qemu/qemu_monitor.h | 39 +
src/qemu/qemu_monitor_json.c | 49
src/qemu/qemu_process.c | 59
4 files changed, 167 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 8c991fefbb..189b789bb8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -159,7 +159,6 @@ static int qemuMonitorOnceInit(void)
VIR_ONCE_GLOBAL_INIT(qemuMonitor);
-
VIR_ENUM_IMPL(qemuMonitorMigrationStatus,
QEMU_MONITOR_MIGRATION_STATUS_LAST,
"inactive", "setup",
@@ -197,6 +196,14 @@ VIR_ENUM_IMPL(qemuMonitorDumpStatus,
"none", "active", "completed", "failed",
);
+VIR_ENUM_IMPL(qemuMonitorMemoryFailureRecipient,
+ QEMU_MONITOR_MEMORY_FAILURE_RECIPIENT_LAST,
+ "hypervisor", "guest");
+
+VIR_ENUM_IMPL(qemuMonitorMemoryFailureAction,
+ QEMU_MONITOR_MEMORY_FAILURE_ACTION_LAST,
+ "ignore", "inject",
+ "fatal", "reset");
#if DEBUG_RAW_IO
static char *
@@ -1428,6 +1435,18 @@ qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon)
int
+qemuMonitorEmitMemoryFailure(qemuMonitorPtr mon,
+ qemuMonitorEventMemoryFailurePtr mfp)
+{
+int ret = -1;
+
+QEMU_MONITOR_CALLBACK(mon, ret, domainMemoryFailure, mon->vm, mfp);
+
+return ret;
+}
+
+
+int
qemuMonitorEmitMigrationStatus(qemuMonitorPtr mon,
int status)
{
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index a744c8975b..17ba006a2f 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -340,6 +340,40 @@ typedef int
(*qemuMonitorDomainGuestCrashloadedCallback)(qemuMonitorPtr mon,
virDomainObjPtr vm,
void *opaque);
+typedef enum {
+QEMU_MONITOR_MEMORY_FAILURE_RECIPIENT_HYPERVISOR,
+QEMU_MONITOR_MEMORY_FAILURE_RECIPIENT_GUEST,
+
+QEMU_MONITOR_MEMORY_FAILURE_RECIPIENT_LAST
+} qemuMonitorMemoryFailureRecipient;
+
+VIR_ENUM_DECL(qemuMonitorMemoryFailureRecipient);
+
+typedef enum {
+QEMU_MONITOR_MEMORY_FAILURE_ACTION_IGNORE,
+QEMU_MONITOR_MEMORY_FAILURE_ACTION_INJECT,
+QEMU_MONITOR_MEMORY_FAILURE_ACTION_FATAL,
+QEMU_MONITOR_MEMORY_FAILURE_ACTION_RESET,
+
+QEMU_MONITOR_MEMORY_FAILURE_ACTION_LAST
+} qemuMonitorMemoryFailureAction;
+
+VIR_ENUM_DECL(qemuMonitorMemoryFailureAction);
+
+typedef struct _qemuMonitorEventMemoryFailure qemuMonitorEventMemoryFailure;
+typedef qemuMonitorEventMemoryFailure *qemuMonitorEventMemoryFailurePtr;
+struct _qemuMonitorEventMemoryFailure {
+qemuMonitorMemoryFailureRecipient recipient;
+qemuMonitorMemoryFailureAction action;
+bool action_required;
+bool recursive;
+};
+
+typedef int (*qemuMonitorDomainMemoryFailureCallback)(qemuMonitorPtr mon,
+ virDomainObjPtr vm,
+
qemuMonitorEventMemoryFailurePtr mfp,
+ void *opaque);
+
typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks;
typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr;
struct _qemuMonitorCallbacks {
@@ -376,6 +410,7 @@ struct _qemuMonitorCallbacks {
qemuMonitorDomainPRManagerStatusChangedCallback
domainPRManagerStatusChanged;
qemuMonitorDomainRdmaGidStatusChangedCallback domainRdmaGidStatusChanged;
qemuMonitorDomainGuestCrashloadedCallback domainGuestCrashloaded;
+qemuMonitorDomainMemoryFailureCallback domainMemoryFailure;
};
qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm,
@@ -475,6 +510,10 @@ int qemuMonitorEmitSerialChange(qemuMonitorPtr mon,
const char *devAlias,
bool
[PATCH v2] virsh: Allow listing just domain IDs
Some completers for libvirt related tools might want to list
domain IDs only. Just like the one I've implemented for
virt-viewer [1]. I've worked around it using some awk magic,
but if it was possible to just 'virsh list --id' then I could
drop awk.
1: https://www.redhat.com/archives/virt-tools-list/2019-May/msg00014.html
Signed-off-by: Michal Privoznik
---
Diff to v1:
- Documented the new switch in the manpage
- Allowed --id to be used with --all
docs/manpages/virsh.rst | 21 +-
tools/virsh-domain-monitor.c | 42 +---
2 files changed, 39 insertions(+), 24 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index d34a1c8684..848e1a6458 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -621,7 +621,7 @@ list
list [--inactive | --all]
[--managed-save] [--title]
-{ [--table] | --name | --uuid }
+{ [--table] | --name | --uuid | --id }
[--persistent] [--transient]
[--with-managed-save] [--without-managed-save]
[--autostart] [--no-autostart]
@@ -758,16 +758,15 @@ If *--managed-save* is specified, then domains that have
managed save state
in the listing. This flag is usable only with the default *--table* output.
Note that this flag does not filter the list of domains.
-If *--name* is specified, domain names are printed instead of the table
-formatted one per line. If *--uuid* is specified domain's UUID's are printed
-instead of names. Flag *--table* specifies that the legacy table-formatted
-output should be used. This is the default.
-
-If both *--name* and *--uuid* are specified, domain UUID's and names
-are printed side by side without any header. Flag *--table* specifies
-that the legacy table-formatted output should be used. This is the
-default if neither *--name* nor *--uuid* are specified. Option
-*--table* is mutually exclusive with options *--uuid* and *--name*.
+If *--name* is specified, domain names are printed instead of the
+table formatted one per line. If *--uuid* is specified domain's UUID's
+are printed instead of names. If *--id* is specified then domain's ID's
+are printed indead of names. However, it is possible to combine
+*--name*, *--uuid* and *--id* to select only desired fields for
+printing. Flag *--table* specifies that the legacy table-formatted
+output should be used, but it is mutually exclusive with *--name*,
+*--uuid* and *--id*. This is the default and will be used if neither of
+*--name*, *--uuid* or *--id* is specified.
If *--title* is specified, then the short domain description (title) is
printed in an extra column. This flag is usable only with the default
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index e0491d48ac..5d0a03afa9 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1919,6 +1919,10 @@ static const vshCmdOptDef opts_list[] = {
.type = VSH_OT_BOOL,
.help = N_("list domain names only")
},
+{.name = "id",
+ .type = VSH_OT_BOOL,
+ .help = N_("list domain IDs only")
+},
{.name = "table",
.type = VSH_OT_BOOL,
.help = N_("list table (default)")
@@ -1945,6 +1949,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
bool optTable = vshCommandOptBool(cmd, "table");
bool optUUID = vshCommandOptBool(cmd, "uuid");
bool optName = vshCommandOptBool(cmd, "name");
+bool optID = vshCommandOptBool(cmd, "id");
size_t i;
char *title;
char uuid[VIR_UUID_STRING_BUFLEN];
@@ -1988,8 +1993,9 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
VSH_EXCLUSIVE_OPTIONS("table", "name");
VSH_EXCLUSIVE_OPTIONS("table", "uuid");
+VSH_EXCLUSIVE_OPTIONS("table", "id");
-if (!optUUID && !optName)
+if (!optUUID && !optName && !optID)
optTable = true;
if (!(list = virshDomainListCollect(ctl, flags)))
@@ -2007,6 +2013,8 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
}
for (i = 0; i < list->ndomains; i++) {
+const char *sep = "";
+
dom = list->domains[i];
id = virDomainGetID(dom);
if (id != (unsigned int) -1)
@@ -2044,20 +2052,28 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
}
-} else if (optUUID && optName) {
-if (virDomainGetUUIDString(dom, uuid) < 0) {
-vshError(ctl, "%s", _("Failed to get domain's UUID"));
-goto cleanup;
+} else {
+if (optUUID) {
+if (virDomainGetUUIDString(dom, uuid) < 0) {
+vshError(ctl, "%s", _("Failed to get domain's UUID"));
+goto cleanup;
+}
+vshPrint(ctl, "%s", uuid);
+sep = " ";
}
-vshPrint(ctl, "%-36s %-30s\n", uuid, virDomainGetName(dom));
-} else if (optUUID) {
-if (virDomainGetUUIDString(dom, uuid) < 0) {
-
Re: [PATCH] os: deprecate the -enable-fips option and QEMU's FIPS enforcement
On 10/21/20 6:17 AM, Daniel P. Berrangé wrote: Claiming QEMU is FIPS compliant without using libgcrypt is a bit of joke since we don't do any self-tests of ciphers, hence this deprecation notice is warning people that libgcrypt is going to be mandatory if you care about FIPS. FWIW this is my main problem with this flag: we read the value in procfs and then use this to change precisely one behavior for one of our components. It doesn't really ... do what the name might imply it does. Leaving that business to the crypto libraries is indeed the correct thing to do. So: Reviewed-by: John Snow
Re: [PATCH 03/15] util: cgroup: Use GHashTable instead of virHashTable
On Thu, Oct 22, 2020 at 02:17:01PM +0100, Daniel P. Berrangé wrote:
> On Thu, Oct 22, 2020 at 03:11:59PM +0200, Pavel Hrdina wrote:
> > On Thu, Oct 22, 2020 at 11:34:54AM +0200, Peter Krempa wrote:
> > > Rewrite using GHashTable which already has interfaces for using a number
> > > as hash key. Glib's implementation doesn't copy the key by default, so
> > > we need to allocate it, but overal the interface is more suited for this
> > > case.
> > >
> > > Signed-off-by: Peter Krempa
> > > ---
> > > src/util/vircgroup.c| 61 -
> > > src/util/vircgroupbackend.h | 3 +-
> > > src/util/vircgrouppriv.h| 2 +-
> > > src/util/vircgroupv1.c | 2 +-
> > > src/util/vircgroupv2.c | 2 +-
> > > 5 files changed, 17 insertions(+), 53 deletions(-)
> > >
> > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> > > index 5f4cb01bc0..b74ec1a8fa 100644
> > > --- a/src/util/vircgroup.c
> > > +++ b/src/util/vircgroup.c
> > > @@ -42,7 +42,6 @@
> > > #include "virlog.h"
> > > #include "virfile.h"
> > > #include "virhash.h"
> > > -#include "virhashcode.h"
> > > #include "virstring.h"
> > > #include "virsystemd.h"
> > > #include "virtypedparam.h"
> > > @@ -2382,7 +2381,7 @@ virCgroupRemove(virCgroupPtr group)
> > > static int
> > > virCgroupKillInternal(virCgroupPtr group,
> > >int signum,
> > > - virHashTablePtr pids,
> > > + GHashTable *pids,
> > >int controller,
> > >const char *taskFile)
> > > {
> > > @@ -2415,8 +2414,9 @@ virCgroupKillInternal(virCgroupPtr group,
> > > goto cleanup;
> > > } else {
> > > while (!feof(fp)) {
> > > -long pid_value;
> > > -if (fscanf(fp, "%ld", &pid_value) != 1) {
> > > +g_autofree long long *pid_value = g_new0(long long, 1);
> >
> > I would rather use gint64 here as the exact type of gint64 changes with
> > the hardware. For example on my AMD x86_84 it is 'signed long'.
>
> If you use gint64, then you need to start using PRIu64 macro
> to deal with the fact that the data type changes for printf
> formatting.
>
> Using long long is simpler as you can unconditionally use %ll
> which is a good thing IMHO.
>
>
> > We should do this every time we pass pointers to GLib APIs because for
> > example bool and gboolean are different and when I used bool type in
> > GLib dbus APIs it randomly crashed.
>
> bool vs gboolean is a special case because of the different types.
>
> For all the other g* basic types, we should not use them. GLib has
> a ticket open considering deprecating them, because they re-invent
> stdint.h
Good to know. I personally don't like these types as it complicates
thinks especially with the gboolean which is not that obvious.
I checked the GLib code and it handles it reasonably so using long long
should not be an issue.
Reviewed-by: Pavel Hrdina
signature.asc
Description: PGP signature
Re: [PATCH 03/15] util: cgroup: Use GHashTable instead of virHashTable
On Thu, Oct 22, 2020 at 14:17:01 +0100, Daniel Berrange wrote:
> On Thu, Oct 22, 2020 at 03:11:59PM +0200, Pavel Hrdina wrote:
> > On Thu, Oct 22, 2020 at 11:34:54AM +0200, Peter Krempa wrote:
> > > Rewrite using GHashTable which already has interfaces for using a number
> > > as hash key. Glib's implementation doesn't copy the key by default, so
> > > we need to allocate it, but overal the interface is more suited for this
> > > case.
> > >
> > > Signed-off-by: Peter Krempa
> > > ---
> > > src/util/vircgroup.c| 61 -
> > > src/util/vircgroupbackend.h | 3 +-
> > > src/util/vircgrouppriv.h| 2 +-
> > > src/util/vircgroupv1.c | 2 +-
> > > src/util/vircgroupv2.c | 2 +-
> > > 5 files changed, 17 insertions(+), 53 deletions(-)
[...]
> > > @@ -2415,8 +2414,9 @@ virCgroupKillInternal(virCgroupPtr group,
> > > goto cleanup;
> > > } else {
> > > while (!feof(fp)) {
> > > -long pid_value;
> > > -if (fscanf(fp, "%ld", &pid_value) != 1) {
> > > +g_autofree long long *pid_value = g_new0(long long, 1);
> >
> > I would rather use gint64 here as the exact type of gint64 changes with
> > the hardware. For example on my AMD x86_84 it is 'signed long'.
>
> If you use gint64, then you need to start using PRIu64 macro
> to deal with the fact that the data type changes for printf
> formatting.
>
> Using long long is simpler as you can unconditionally use %ll
> which is a good thing IMHO.
Yup, simplicity of using %ll along with the fact that 'long long' is 64
bit signed integer on all of our supported platform lead me to use it
without trying to get into the glib types.
Re: [PATCH 03/15] util: cgroup: Use GHashTable instead of virHashTable
On Thu, Oct 22, 2020 at 03:11:59PM +0200, Pavel Hrdina wrote:
> On Thu, Oct 22, 2020 at 11:34:54AM +0200, Peter Krempa wrote:
> > Rewrite using GHashTable which already has interfaces for using a number
> > as hash key. Glib's implementation doesn't copy the key by default, so
> > we need to allocate it, but overal the interface is more suited for this
> > case.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/util/vircgroup.c| 61 -
> > src/util/vircgroupbackend.h | 3 +-
> > src/util/vircgrouppriv.h| 2 +-
> > src/util/vircgroupv1.c | 2 +-
> > src/util/vircgroupv2.c | 2 +-
> > 5 files changed, 17 insertions(+), 53 deletions(-)
> >
> > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> > index 5f4cb01bc0..b74ec1a8fa 100644
> > --- a/src/util/vircgroup.c
> > +++ b/src/util/vircgroup.c
> > @@ -42,7 +42,6 @@
> > #include "virlog.h"
> > #include "virfile.h"
> > #include "virhash.h"
> > -#include "virhashcode.h"
> > #include "virstring.h"
> > #include "virsystemd.h"
> > #include "virtypedparam.h"
> > @@ -2382,7 +2381,7 @@ virCgroupRemove(virCgroupPtr group)
> > static int
> > virCgroupKillInternal(virCgroupPtr group,
> >int signum,
> > - virHashTablePtr pids,
> > + GHashTable *pids,
> >int controller,
> >const char *taskFile)
> > {
> > @@ -2415,8 +2414,9 @@ virCgroupKillInternal(virCgroupPtr group,
> > goto cleanup;
> > } else {
> > while (!feof(fp)) {
> > -long pid_value;
> > -if (fscanf(fp, "%ld", &pid_value) != 1) {
> > +g_autofree long long *pid_value = g_new0(long long, 1);
>
> I would rather use gint64 here as the exact type of gint64 changes with
> the hardware. For example on my AMD x86_84 it is 'signed long'.
If you use gint64, then you need to start using PRIu64 macro
to deal with the fact that the data type changes for printf
formatting.
Using long long is simpler as you can unconditionally use %ll
which is a good thing IMHO.
> We should do this every time we pass pointers to GLib APIs because for
> example bool and gboolean are different and when I used bool type in
> GLib dbus APIs it randomly crashed.
bool vs gboolean is a special case because of the different types.
For all the other g* basic types, we should not use them. GLib has
a ticket open considering deprecating them, because they re-invent
stdint.h
> >
> > -VIR_DEBUG("pid=%ld", pid_value);
> > +VIR_DEBUG("pid=%lld", *pid_value);
>
> Using gint64 would require to use G_GINT64_FORMAT here.
That is exactly why we should not use guint64 - it makes the
debug code more verbose for no benefit.
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 03/15] util: cgroup: Use GHashTable instead of virHashTable
On Thu, Oct 22, 2020 at 11:34:54AM +0200, Peter Krempa wrote:
> Rewrite using GHashTable which already has interfaces for using a number
> as hash key. Glib's implementation doesn't copy the key by default, so
> we need to allocate it, but overal the interface is more suited for this
> case.
>
> Signed-off-by: Peter Krempa
> ---
> src/util/vircgroup.c| 61 -
> src/util/vircgroupbackend.h | 3 +-
> src/util/vircgrouppriv.h| 2 +-
> src/util/vircgroupv1.c | 2 +-
> src/util/vircgroupv2.c | 2 +-
> 5 files changed, 17 insertions(+), 53 deletions(-)
>
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 5f4cb01bc0..b74ec1a8fa 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -42,7 +42,6 @@
> #include "virlog.h"
> #include "virfile.h"
> #include "virhash.h"
> -#include "virhashcode.h"
> #include "virstring.h"
> #include "virsystemd.h"
> #include "virtypedparam.h"
> @@ -2382,7 +2381,7 @@ virCgroupRemove(virCgroupPtr group)
> static int
> virCgroupKillInternal(virCgroupPtr group,
>int signum,
> - virHashTablePtr pids,
> + GHashTable *pids,
>int controller,
>const char *taskFile)
> {
> @@ -2415,8 +2414,9 @@ virCgroupKillInternal(virCgroupPtr group,
> goto cleanup;
> } else {
> while (!feof(fp)) {
> -long pid_value;
> -if (fscanf(fp, "%ld", &pid_value) != 1) {
> +g_autofree long long *pid_value = g_new0(long long, 1);
I would rather use gint64 here as the exact type of gint64 changes with
the hardware. For example on my AMD x86_84 it is 'signed long'.
We should do this every time we pass pointers to GLib APIs because for
example bool and gboolean are different and when I used bool type in
GLib dbus APIs it randomly crashed.
> +if (fscanf(fp, "%lld", pid_value) != 1) {
> if (feof(fp))
> break;
> virReportSystemError(errno,
> @@ -2424,16 +2424,17 @@ virCgroupKillInternal(virCgroupPtr group,
> keypath);
> goto cleanup;
> }
> -if (virHashLookup(pids, (void*)pid_value))
> +
> +if (g_hash_table_lookup(pids, pid_value))
> continue;
>
> -VIR_DEBUG("pid=%ld", pid_value);
> +VIR_DEBUG("pid=%lld", *pid_value);
Using gint64 would require to use G_GINT64_FORMAT here.
Otherwise looks good.
Pavel
signature.asc
Description: PGP signature
Re: [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy needed
22.10.2020 10:50, Andrey Shinkevich wrote:
On 21.10.2020 23:43, Andrey Shinkevich wrote:
On 14.10.2020 18:22, Vladimir Sementsov-Ogievskiy wrote:
14.10.2020 15:51, Max Reitz wrote:
On 12.10.20 19:43, Andrey Shinkevich wrote:
If the flag BDRV_REQ_PREFETCH was set, pass it further to the
COR-driver to skip unneeded reading. It can be taken into account for
the COR-algorithms optimization. That check is being made during the
block stream job by the moment.
Signed-off-by: Andrey Shinkevich
---
[...]
diff --git a/block/io.c b/block/io.c
index 11df188..bff1808 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1512,7 +1512,8 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild
*child,
max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
if (bytes <= max_bytes && bytes <= max_transfer) {
- ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset, 0);
+ ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset,
+ flags & bs->supported_read_flags);
When BDRV_REQ_PREFETCH is passed, qiov may be (and generally should be) NULL.
This means, that we can't just drop the flag when call the driver that doesn't
support it.
Actually, if driver doesn't support the PREFETCH flag we should do nothing.
Ah, OK. I see. I expected this to be a separate patch. I still wonder
why it isn’t.
Could it be part of patch 07? I mean introduce new field supported_read_flags
and handle it in generic code in one patch, prior to implementing support for
it in COR driver.
We have to add the supported flags for the COR driver in the same patch. Or
before handling the supported_read_flags at the generic layer (handling zero
does not make a sence). Otherwise, the test #216 (where the COR-filter is
applied) will not pass.
Andrey
I have found a workaround and am going to send all the related patches as a
separate series.
What is the problem?
If in a separate patch prior to modifying COR driver, we add new field
supported_read_flags and add a support for it in generic layer, when no driver
support it yet, nothing should be changed and all tests should pass..
--
Best regards,
Vladimir
Re: [PATCH v2] virCgroupKillRecursive: Refactor cleanup
On Thu, Oct 22, 2020 at 02:29:49PM +0200, Peter Krempa wrote: > Remove 'cleanup' label and simplify remembering of the returned value > from the callback. > > Signed-off-by: Peter Krempa > --- > src/util/vircgroup.c | 21 ++--- > 1 file changed, 10 insertions(+), 11 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
[PATCH v2] virCgroupKillRecursive: Refactor cleanup
Remove 'cleanup' label and simplify remembering of the returned value
from the callback.
Signed-off-by: Peter Krempa
---
src/util/vircgroup.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index b74ec1a8fa..b1caf11f47 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2532,8 +2532,8 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
int
virCgroupKillRecursive(virCgroupPtr group, int signum)
{
-int ret = 0;
int rc;
+bool success = false;
size_t i;
bool backendAvailable = false;
virCgroupBackendPtr *backends = virCgroupBackendGetAll();
@@ -2544,25 +2544,24 @@ virCgroupKillRecursive(virCgroupPtr group, int signum)
for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
if (backends && backends[i] && backends[i]->available()) {
backendAvailable = true;
-rc = backends[i]->killRecursive(group, signum, pids);
-if (rc < 0) {
-ret = -1;
-goto cleanup;
-}
+if ((rc = backends[i]->killRecursive(group, signum, pids)) < 0)
+return -1;
+
if (rc > 0)
-ret = rc;
+success = true;
}
}
+if (success)
+return 1;
+
if (!backends || !backendAvailable) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("no cgroup backend available"));
-ret = -1;
-goto cleanup;
+return -1;
}
- cleanup:
-return ret;
+return 0;
}
--
2.26.2
Re: [PATCH 3/3] qemu: sync backing chain update in virDomainGetBlockJobInfo
On Tue, Oct 20, 2020 at 16:44:09 +0300, Nikolay Shirokovskiy wrote:
> Some mgmt still use polling for block job completion. After job completion the
> job failure/success is infered by inspecting domain xml. With legacy block job
> processing this does not always work.
So, fix your app? :)
> The issue deals with how libvirt processes events. If no other thread is
> waiting for blockjob event then event processing if offloaded to worker
> thread.
> If now virDomainGetBlockJobInfo API is called then as block job is already
> dismissed in legacy scheme the API returns 0 but backing chain is not yet
> updated as processing yet be done in worker thread. Now mgmt checks backing
> chain right after return from the API call and detects error.
>
> This happens quite often under load. I guess because of we have only one
> worker
> thread for all the domains.
So basically the problem is that qemuDomainGetBlockJobInfo returns
nothing, but the job is still stuck.
> The event delivery is synchronous in qemu and block job completed event is
> sent
> in job finalize step so if block job is absent the event is already delivered
> and we just need to process it.
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/qemu/qemu_driver.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c06db3a..7189a29 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14672,8 +14672,15 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
> ret = qemuMonitorGetBlockJobInfo(qemuDomainGetMonitor(vm), job->name,
> &rawInfo);
> if (qemuDomainObjExitMonitor(driver, vm) < 0)
> ret = -1;
> -if (ret <= 0)
> +if (ret < 0)
> +goto endjob;
> +if (ret == 0) {
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))
> +qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE);
> goto endjob;
> +}
>
> if (qemuBlockJobInfoTranslate(&rawInfo, info, disk,
>flags &
> VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES) < 0) {
My alternate proposal is to not return 0 if qemuMonitorGetBlockJobInfo
returns 0 jobs, but rather continue to qemuBlockJobInfoTranslate which
will report fake data for the job.
That will prevent the ugly changes to the handling code and will pretend
that the job still isn't complete for broken apps.
Re: [PATCH 1/3] qemu: add option to process offloaded legacy blockjob event ealier
On Tue, Oct 20, 2020 at 16:44:07 +0300, Nikolay Shirokovskiy wrote:
> Currently in qemuProcessHandleBlockJob we either offload blockjob event
> processing to the worker thread or notify another thread that waits for
> blockjob event and that thread processes the event. But sometimes after event
> is offloaded to the worker thread we need to process the event in a different
> thread.
>
> To be able to to do it let's always set newstate/errmsg and then let whatever
> thread is first process it.
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/qemu/qemu_driver.c | 17 -
> src/qemu/qemu_process.c | 20
> 2 files changed, 16 insertions(+), 21 deletions(-)
>
[...]
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 6422881..4d63e7d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -953,13 +953,19 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon
> G_GNUC_UNUSED,
> if (!(disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, diskAlias, NULL)))
> goto cleanup;
>
> -job = qemuBlockJobDiskGetJob(disk);
> +if (!(job = qemuBlockJobDiskGetJob(disk))) {
> +VIR_DEBUG("creating new block job object for '%s'", diskAlias);
> +if (!(job = qemuBlockJobDiskNew(vm, disk, type, diskAlias)))
So this actually writes out the status XML. I'm not sure if that is a
good idea to do if we don't hold the domain job. The premise is that
threads holding the job lock might have partially modified it.
Re: [PATCH 15/15] virHashRemoveAll: Don't return number of removed items
On Thu, Oct 22, 2020 at 11:35:06AM +0200, Peter Krempa wrote: > Nobody uses the return value. > > Signed-off-by: Peter Krempa > --- > src/util/virhash.c | 8 ++-- > src/util/virhash.h | 2 +- > 2 files changed, 3 insertions(+), 7 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 14/15] util: virhash: Remove key handling callbacks
On Thu, Oct 22, 2020 at 11:35:05AM +0200, Peter Krempa wrote: > Since we use virHashTable for string-keyed values only, we can remove > all the callbacks which allowed universal keys. > > Code which wishes to use non-string keys should use glib's GHashTable. > > Signed-off-by: Peter Krempa > --- > src/util/virhash.c | 68 +++--- > src/util/virhash.h | 54 > 2 files changed, 10 insertions(+), 112 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 05/15] conf: domain_addr: Refactor hash usage in zpci reservation code
On Thu, Oct 22, 2020 at 13:44:10 +0200, Bjoern Walk wrote:
> Peter Krempa [2020-10-22, 11:34AM +0200]:
> > Rewrite using GHashTable which already has interfaces for using a number
> > as hash key.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/conf/domain_addr.c | 123 +
> > src/conf/domain_addr.h | 4 +-
> > 2 files changed, 27 insertions(+), 100 deletions(-)
[...]
> > @@ -123,40 +122,19 @@ virDomainZPCIAddressAssignFid(virHashTablePtr set,
> >
> >
> > static void
> > -virDomainZPCIAddressReleaseId(virHashTablePtr set,
> > - virZPCIDeviceAddressID *id,
> > - const char *name)
> > +virDomainZPCIAddressReleaseId(GHashTable *set,
> > + virZPCIDeviceAddressID *id)
> > {
> > if (!id->isSet)
> > return;
> >
> > -if (virHashRemoveEntry(set, &id->value) < 0) {
> > -virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("Release %s %o failed"),
> > - name, id->value);
> > -}
> > +g_hash_table_remove(set, &id->value);
>
> Here I'm not so sure, IIUC the original code reported an error when the
> value was not found in the hash table. This will be dropped with the new
> code. But since this should only occur in pathological cases anyways, so
Specifically, here the error is only logged, but the caller doesn't take
any action/return any value.
That means that there wouldn't be any immediate notification of the
user. It's very unlikely that the error will be noticed in logs.
g_hash_table_remove returns whether it released the value or not, but I
don't feel it's worth keeping it.
> I guess this is fine.
Thanks!
Re: [PATCH 13/15] util: hash: Change type of hash table name/key to 'char'
On Thu, Oct 22, 2020 at 11:35:04AM +0200, Peter Krempa wrote: > All users of virHashTable pass strings as the name/key of the entry. > Make this an official requirement by turning the variables to 'const > char *'. > > For any other case it's better to use glib's GHashTable. > > Signed-off-by: Peter Krempa > --- > src/conf/nwfilter_params.c | 2 +- > src/conf/virchrdev.c | 2 +- > src/conf/virdomainmomentobjlist.c | 6 ++-- > src/conf/virdomainobjlist.c| 12 > src/conf/virinterfaceobj.c | 10 +++--- > src/conf/virnetworkobj.c | 16 +- > src/conf/virnodedeviceobj.c| 18 +-- > src/conf/virnwfilterbindingobjlist.c | 4 +-- > src/conf/virsecretobj.c| 8 ++--- > src/conf/virstorageobj.c | 22 +++--- > src/hypervisor/virclosecallbacks.c | 2 +- > src/locking/lock_daemon.c | 2 +- > src/nwfilter/nwfilter_dhcpsnoop.c | 6 ++-- > src/nwfilter/nwfilter_gentech_driver.c | 6 ++-- > src/qemu/qemu_blockjob.c | 2 +- > src/qemu/qemu_capabilities.c | 2 +- > src/qemu/qemu_checkpoint.c | 2 +- > src/qemu/qemu_domain.c | 6 ++-- > src/qemu/qemu_domain.h | 2 +- > src/qemu/qemu_process.c| 2 +- > src/qemu/qemu_snapshot.c | 4 +-- > src/rpc/virnetdaemon.c | 14 - > src/test/test_driver.c | 6 ++-- > src/util/virfilecache.c| 2 +- > src/util/virhash.c | 42 +- > src/util/virhash.h | 32 ++-- > src/util/virlockspace.c| 2 +- > src/util/virmacmap.c | 4 +-- > tests/qemumonitorjsontest.c| 2 +- > tests/qemusecuritymock.c | 8 ++--- > tests/virhashtest.c| 10 +++--- > 31 files changed, 129 insertions(+), 129 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 12/15] util: hash: Remove virHashCreateFull
On Thu, Oct 22, 2020 at 11:35:03AM +0200, Peter Krempa wrote: > The only place we call it is in virHashNew. Move the code to virHashNew > and remove virHashCreateFull. > > Code wishing to use non-strings as hash table keys will be better off > using glib's GHashTable directly. > > Signed-off-by: Peter Krempa > --- > src/util/virhash.c | 53 +- > src/util/virhash.h | 7 -- > 2 files changed, 10 insertions(+), 50 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy needed
On 21.10.2020 23:43, Andrey Shinkevich wrote:
On 14.10.2020 18:22, Vladimir Sementsov-Ogievskiy wrote:
14.10.2020 15:51, Max Reitz wrote:
On 12.10.20 19:43, Andrey Shinkevich wrote:
If the flag BDRV_REQ_PREFETCH was set, pass it further to the
COR-driver to skip unneeded reading. It can be taken into account for
the COR-algorithms optimization. That check is being made during the
block stream job by the moment.
Signed-off-by: Andrey Shinkevich
---
[...]
diff --git a/block/io.c b/block/io.c
index 11df188..bff1808 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1512,7 +1512,8 @@ static int coroutine_fn
bdrv_aligned_preadv(BdrvChild *child,
max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
if (bytes <= max_bytes && bytes <= max_transfer) {
- ret = bdrv_driver_preadv(bs, offset, bytes, qiov,
qiov_offset, 0);
+ ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset,
+ flags & bs->supported_read_flags);
When BDRV_REQ_PREFETCH is passed, qiov may be (and generally should
be) NULL. This means, that we can't just drop the flag when call the
driver that doesn't support it.
Actually, if driver doesn't support the PREFETCH flag we should do
nothing.
Ah, OK. I see. I expected this to be a separate patch. I still wonder
why it isn’t.
Could it be part of patch 07? I mean introduce new field
supported_read_flags and handle it in generic code in one patch, prior
to implementing support for it in COR driver.
We have to add the supported flags for the COR driver in the same patch.
Or before handling the supported_read_flags at the generic layer
(handling zero does not make a sence). Otherwise, the test #216 (where
the COR-filter is applied) will not pass.
Andrey
I have found a workaround and am going to send all the related patches
as a separate series.
Andrey
Re: [PATCH 11/15] util: hash: Remove virHashValueFree
On Thu, Oct 22, 2020 at 11:35:02AM +0200, Peter Krempa wrote: > Use 'g_free' directly. > > Signed-off-by: Peter Krempa > --- > src/conf/domain_addr.c | 2 +- > src/hypervisor/virclosecallbacks.c | 2 +- > src/libvirt_private.syms| 1 - > src/nwfilter/nwfilter_learnipaddr.c | 2 +- > src/qemu/qemu_interop_config.c | 2 +- > src/qemu/qemu_migration_cookie.c| 2 +- > src/qemu/qemu_monitor.c | 6 +++--- > src/qemu/qemu_monitor_json.c| 2 +- > src/util/virhash.c | 7 --- > src/util/virhash.h | 3 --- > tests/qemumonitorjsontest.c | 6 +++--- > tests/qemusecuritymock.c| 4 ++-- > tests/testutilsqemu.c | 2 +- > 13 files changed, 15 insertions(+), 26 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 05/15] conf: domain_addr: Refactor hash usage in zpci reservation code
Peter Krempa [2020-10-22, 11:34AM +0200]:
> Rewrite using GHashTable which already has interfaces for using a number
> as hash key.
>
> Signed-off-by: Peter Krempa
> ---
> src/conf/domain_addr.c | 123 +
> src/conf/domain_addr.h | 4 +-
> 2 files changed, 27 insertions(+), 100 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index c7419916aa..4e47c2547c 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -25,17 +25,18 @@
> #include "virlog.h"
> #include "virstring.h"
> #include "domain_addr.h"
> -#include "virhashcode.h"
>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN
>
> VIR_LOG_INIT("conf.domain_addr");
>
> static int
> -virDomainZPCIAddressReserveId(virHashTablePtr set,
> +virDomainZPCIAddressReserveId(GHashTable *set,
>virZPCIDeviceAddressID *id,
>const char *name)
> {
> +int *idval;
> +
> if (!id->isSet) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("No zPCI %s to reserve"),
> @@ -43,26 +44,24 @@ virDomainZPCIAddressReserveId(virHashTablePtr set,
> return -1;
> }
>
> -if (virHashLookup(set, &id->value)) {
> +if (g_hash_table_lookup(set, &id->value)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("zPCI %s %o is already reserved"),
> name, id->value);
> return -1;
> }
>
> -if (virHashAddEntry(set, &id->value, (void*)1) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to reserve %s %o"),
> - name, id->value);
> -return -1;
> -}
> +idval = g_new0(int, 1);
> +*idval = (int) id->value;
> +
> +g_hash_table_add(set, idval);
g_hash_table_add() can't fail, only abort(), right? Then dropping the
error message should be fine.
>
> return 0;
> }
>
>
> static int
> -virDomainZPCIAddressReserveUid(virHashTablePtr set,
> +virDomainZPCIAddressReserveUid(GHashTable *set,
> virZPCIDeviceAddressPtr addr)
> {
> return virDomainZPCIAddressReserveId(set, &addr->uid, "uid");
> @@ -70,7 +69,7 @@ virDomainZPCIAddressReserveUid(virHashTablePtr set,
>
>
> static int
> -virDomainZPCIAddressReserveFid(virHashTablePtr set,
> +virDomainZPCIAddressReserveFid(GHashTable *set,
> virZPCIDeviceAddressPtr addr)
> {
> return virDomainZPCIAddressReserveId(set, &addr->fid, "fid");
> @@ -78,7 +77,7 @@ virDomainZPCIAddressReserveFid(virHashTablePtr set,
>
>
> static int
> -virDomainZPCIAddressAssignId(virHashTablePtr set,
> +virDomainZPCIAddressAssignId(GHashTable *set,
> virZPCIDeviceAddressID *id,
> unsigned int min,
> unsigned int max,
> @@ -87,7 +86,7 @@ virDomainZPCIAddressAssignId(virHashTablePtr set,
> if (id->isSet)
> return 0;
>
> -while (virHashLookup(set, &min)) {
> +while (g_hash_table_lookup(set, &min)) {
> if (min == max) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("There is no more free %s."),
> @@ -105,7 +104,7 @@ virDomainZPCIAddressAssignId(virHashTablePtr set,
>
>
> static int
> -virDomainZPCIAddressAssignUid(virHashTablePtr set,
> +virDomainZPCIAddressAssignUid(GHashTable *set,
>virZPCIDeviceAddressPtr addr)
> {
> return virDomainZPCIAddressAssignId(set, &addr->uid, 1,
> @@ -114,7 +113,7 @@ virDomainZPCIAddressAssignUid(virHashTablePtr set,
>
>
> static int
> -virDomainZPCIAddressAssignFid(virHashTablePtr set,
> +virDomainZPCIAddressAssignFid(GHashTable *set,
>virZPCIDeviceAddressPtr addr)
> {
> return virDomainZPCIAddressAssignId(set, &addr->fid, 0,
> @@ -123,40 +122,19 @@ virDomainZPCIAddressAssignFid(virHashTablePtr set,
>
>
> static void
> -virDomainZPCIAddressReleaseId(virHashTablePtr set,
> - virZPCIDeviceAddressID *id,
> - const char *name)
> +virDomainZPCIAddressReleaseId(GHashTable *set,
> + virZPCIDeviceAddressID *id)
> {
> if (!id->isSet)
> return;
>
> -if (virHashRemoveEntry(set, &id->value) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Release %s %o failed"),
> - name, id->value);
> -}
> +g_hash_table_remove(set, &id->value);
Here I'm not so sure, IIUC the original code reported an error when the
value was not found in the hash table. This will be dropped with the new
code. But since this should only occur in pathological cases anyways, so
I guess this is fine.
>
> id->value = 0;
> id->isSet = false;
> }
>
>
> -static void
> -virDomainZPCIAddressReleaseUid(virHashTablePtr set,
> -
Re: [PATCH 10/15] Replace all instances of 'virHashCreate' with 'virHashNew'
On Thu, Oct 22, 2020 at 11:35:01AM +0200, Peter Krempa wrote: > It doesn't make much sense to configure the bucket count in the hash > table for each case specifically. Replace all calls of virHashCreate > with virHashNew which has a pre-set size and remove virHashCreate > completely. > > Signed-off-by: Peter Krempa > --- > src/conf/domain_addr.c| 2 +- > src/conf/domain_conf.c| 4 ++-- > src/conf/virchrdev.c | 2 +- > src/conf/virdomainmomentobjlist.c | 2 +- > src/conf/virdomainobjlist.c | 4 ++-- > src/conf/virinterfaceobj.c| 2 +- > src/conf/virnetworkobj.c | 5 ++--- > src/conf/virnodedeviceobj.c | 2 +- > src/conf/virnwfilterbindingobjlist.c | 2 +- > src/conf/virsecretobj.c | 2 +- > src/conf/virstorageobj.c | 10 +- > src/hyperv/hyperv_wmi.c | 2 +- > src/hypervisor/virclosecallbacks.c| 2 +- > src/libvirt_private.syms | 1 - > src/libxl/libxl_logger.c | 2 +- > src/locking/lock_daemon.c | 8 ++-- > src/nwfilter/nwfilter_dhcpsnoop.c | 6 +++--- > src/nwfilter/nwfilter_ebiptables_driver.c | 4 ++-- > src/nwfilter/nwfilter_gentech_driver.c| 2 +- > src/nwfilter/nwfilter_learnipaddr.c | 4 ++-- > src/qemu/qemu_block.c | 6 +++--- > src/qemu/qemu_capabilities.c | 4 ++-- > src/qemu/qemu_driver.c| 2 +- > src/qemu/qemu_interop_config.c| 2 +- > src/qemu/qemu_monitor.c | 10 +- > src/qemu/qemu_monitor_json.c | 2 +- > src/qemu/qemu_qapi.c | 2 +- > src/rpc/virnetdaemon.c| 2 +- > src/security/security_selinux.c | 4 ++-- > src/util/virfilecache.c | 2 +- > src/util/virhash.c| 21 - > src/util/virhash.h| 2 -- > src/util/viriptables.c| 4 ++-- > src/util/virlockspace.c | 6 ++ > src/util/virmacmap.c | 2 +- > src/util/virstoragefile.c | 4 ++-- > src/util/virsystemd.c | 2 +- > tests/qemumonitorjsontest.c | 10 +- > tests/qemusecuritymock.c | 4 ++-- > tests/testutilsqemu.c | 2 +- > tests/virhashtest.c | 8 > 41 files changed, 69 insertions(+), 100 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 09/15] qemuDomainObjPrivateAlloc: Use virHashNew instead of virHashCreate
On Thu, Oct 22, 2020 at 11:35:00AM +0200, Peter Krempa wrote: > virHashCreate will be removed in upcoming patches. This change has an > impact on ordering of the blockjob entries in one of the status XML->XML > tests. > > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_domain.c| 2 +- > .../blockjob-blockdev-in.xml | 110 +- > 2 files changed, 56 insertions(+), 56 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 08/15] tests: hash: Prepare for replacement of virHashCreate
On Thu, Oct 22, 2020 at 11:34:59AM +0200, Peter Krempa wrote: > Most callers pass a random number. We have virHashNew which doesn't give > the callers the option to configure the table. Since we are going to > switch to virHashNew replace it in tests and remove multiple instances > of the 'testHashGrow' case as it doesn't make sense with the new > semantics. > > Signed-off-by: Peter Krempa > --- > tests/virhashtest.c | 28 +++- > 1 file changed, 11 insertions(+), 17 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 07/15] conf: nwfilter: Replace 'virNWFilterHashTableCreate' with 'virHashNew'
On Thu, Oct 22, 2020 at 11:34:58AM +0200, Peter Krempa wrote: > Export the freeing function rather than having a wrapper for the hash > creation function. > > Signed-off-by: Peter Krempa > --- > src/conf/domain_nwfilter.c | 2 +- > src/conf/nwfilter_ipaddrmap.c | 2 +- > src/conf/nwfilter_params.c | 12 +++- > src/conf/nwfilter_params.h | 2 +- > src/conf/virnwfilterbindingdef.c | 2 +- > src/libvirt_private.syms | 2 +- > src/nwfilter/nwfilter_gentech_driver.c | 6 +++--- > tests/nwfilterxml2firewalltest.c | 6 +++--- > 8 files changed, 14 insertions(+), 20 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 06/15] virHashAtomicNew: Remove 'size' argument
On Thu, Oct 22, 2020 at 11:34:57AM +0200, Peter Krempa wrote: > Use 'virHashNew' internally which uses a default size. > > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_migration.c | 2 +- > src/util/virhash.c| 5 ++--- > src/util/virhash.h| 3 +-- > 3 files changed, 4 insertions(+), 6 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 05/15] conf: domain_addr: Refactor hash usage in zpci reservation code
On Thu, Oct 22, 2020 at 11:34:56AM +0200, Peter Krempa wrote: > Rewrite using GHashTable which already has interfaces for using a number > as hash key. > > Signed-off-by: Peter Krempa > --- > src/conf/domain_addr.c | 123 + > src/conf/domain_addr.h | 4 +- > 2 files changed, 27 insertions(+), 100 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 0/2] parthelper: Initialize error object
On a Thursday in 2020, Michal Privoznik wrote:
To observe the error fixed in 1/2 apply the following patch and run
parthelper under valgrind, e.g. like this:
valgrind libvirt_parthelper /dev/sda -g
diff --git i/src/util/virdevmapper.c w/src/util/virdevmapper.c
index 4d27c9f104..fad76f35a1 100644
--- i/src/util/virdevmapper.c
+++ w/src/util/virdevmapper.c
@@ -57,6 +57,8 @@ virDevMapperGetMajor(unsigned int *major)
VIR_AUTOSTRINGLIST lines = NULL;
size_t i;
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("blah"));
+
if (!virFileExists(CONTROL_PATH))
return -2;
Michal Prívozník (2):
parthelper: Initialize error object
parthelper: Don't leak @canonical_path
src/storage/parthelper.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
[PATCH 0/2] parthelper: Initialize error object
To observe the error fixed in 1/2 apply the following patch and run
parthelper under valgrind, e.g. like this:
valgrind libvirt_parthelper /dev/sda -g
diff --git i/src/util/virdevmapper.c w/src/util/virdevmapper.c
index 4d27c9f104..fad76f35a1 100644
--- i/src/util/virdevmapper.c
+++ w/src/util/virdevmapper.c
@@ -57,6 +57,8 @@ virDevMapperGetMajor(unsigned int *major)
VIR_AUTOSTRINGLIST lines = NULL;
size_t i;
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("blah"));
+
if (!virFileExists(CONTROL_PATH))
return -2;
Michal Prívozník (2):
parthelper: Initialize error object
parthelper: Don't leak @canonical_path
src/storage/parthelper.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--
2.26.2
[PATCH 1/2] parthelper: Initialize error object
Some functions called from parthelper can report an error. But
that means that the error object must be initialized otherwise
virResetError() (which happens as a part of virReportError())
will free random pointers.
Reported-by: Katerina Koukiou
Signed-off-by: Michal Privoznik
---
src/storage/parthelper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c
index 812e90d3cb..29a01d3dd5 100644
--- a/src/storage/parthelper.c
+++ b/src/storage/parthelper.c
@@ -66,7 +66,8 @@ int main(int argc, char **argv)
const char *partsep;
bool devmap_partsep = false;
-if (virGettextInitialize() < 0)
+if (virGettextInitialize() < 0 ||
+virErrorInitialize() < 0)
exit(EXIT_FAILURE);
if (argc == 3 && STREQ(argv[2], "-g")) {
--
2.26.2
[PATCH 2/2] parthelper: Don't leak @canonical_path
The @canonical_path variable holds canonicalized path passed as argv[1]. The canonicalized path is obtained either via virFileResolveLink() or plain g_strdup(). Nevertheless, in both cases it must be freed. Signed-off-by: Michal Privoznik --- src/storage/parthelper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 29a01d3dd5..caa2e8fa62 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -62,7 +62,7 @@ int main(int argc, char **argv) PedPartition *part; int cmd = DISK_LAYOUT; const char *path; -char *canonical_path; +g_autofree char *canonical_path = NULL; const char *partsep; bool devmap_partsep = false; -- 2.26.2
Re: [PATCH 02/15] util: virhash: Remove virHashTableSize
On Thu, Oct 22, 2020 at 11:34:53AM +0200, Peter Krempa wrote: > It's used only in one place in tests which isn't even automatically > evaluated. > > Signed-off-by: Peter Krempa > --- > src/libvirt_private.syms | 1 - > src/util/virhash.c | 17 - > src/util/virhash.h | 1 - > tests/virhashtest.c | 6 -- > 4 files changed, 25 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH 04/15] virCgroupKillRecursive: Refactor cleanup
On Thu, Oct 22, 2020 at 11:55:06AM +0200, Peter Krempa wrote:
> On Thu, Oct 22, 2020 at 11:50:01 +0200, Pavel Hrdina wrote:
> > On Thu, Oct 22, 2020 at 11:34:55AM +0200, Peter Krempa wrote:
> > > Remove 'cleanup' label and simplify remembering of the returned value
> > > from the callback.
> > >
> > > Signed-off-by: Peter Krempa
> > > ---
> > > src/util/vircgroup.c | 16
> > > 1 file changed, 4 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> > > index b74ec1a8fa..dafc6c98c0 100644
> > > --- a/src/util/vircgroup.c
> > > +++ b/src/util/vircgroup.c
> > > @@ -2532,8 +2532,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
> > > int
> > > virCgroupKillRecursive(virCgroupPtr group, int signum)
> > > {
> > > -int ret = 0;
> > > -int rc;
> > > +int ret = -1;
> > > size_t i;
> > > bool backendAvailable = false;
> > > virCgroupBackendPtr *backends = virCgroupBackendGetAll();
> > > @@ -2544,24 +2543,17 @@ virCgroupKillRecursive(virCgroupPtr group, int
> > > signum)
> > > for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
> > > if (backends && backends[i] && backends[i]->available()) {
> > > backendAvailable = true;
> > > -rc = backends[i]->killRecursive(group, signum, pids);
> > > -if (rc < 0) {
> > > -ret = -1;
> > > -goto cleanup;
> > > -}
> > > -if (rc > 0)
> > > -ret = rc;
> > > +if ((ret = backends[i]->killRecursive(group, signum, pids))
> > > < 0)
> > > +return -1;
> >
> > This doesn't look correct. In case that both cgroups v1 and v2 are used
> > the first call could return 1 meaning that it managed to kill some
> > process but the second call would probably return 0 because the process
> > would be already gone.
>
> Does it in such case even make sense to call the second callback?
>
> If yes, then I'll probably rather change it such, that a boolean
> variable will be set to true if any of the callbacks returns 1 to make
> it more obvious.
Good question, if the list of processes is the same in cgroups v1 and v2
it should be enough to call it only for one the cgroups, but I would
rather call it for both just to be on a safe side.
Using boolean sounds good.
Pavel
signature.asc
Description: PGP signature
Re: [PATCH 04/15] virCgroupKillRecursive: Refactor cleanup
On Thu, Oct 22, 2020 at 11:50:01 +0200, Pavel Hrdina wrote:
> On Thu, Oct 22, 2020 at 11:34:55AM +0200, Peter Krempa wrote:
> > Remove 'cleanup' label and simplify remembering of the returned value
> > from the callback.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/util/vircgroup.c | 16
> > 1 file changed, 4 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> > index b74ec1a8fa..dafc6c98c0 100644
> > --- a/src/util/vircgroup.c
> > +++ b/src/util/vircgroup.c
> > @@ -2532,8 +2532,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
> > int
> > virCgroupKillRecursive(virCgroupPtr group, int signum)
> > {
> > -int ret = 0;
> > -int rc;
> > +int ret = -1;
> > size_t i;
> > bool backendAvailable = false;
> > virCgroupBackendPtr *backends = virCgroupBackendGetAll();
> > @@ -2544,24 +2543,17 @@ virCgroupKillRecursive(virCgroupPtr group, int
> > signum)
> > for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
> > if (backends && backends[i] && backends[i]->available()) {
> > backendAvailable = true;
> > -rc = backends[i]->killRecursive(group, signum, pids);
> > -if (rc < 0) {
> > -ret = -1;
> > -goto cleanup;
> > -}
> > -if (rc > 0)
> > -ret = rc;
> > +if ((ret = backends[i]->killRecursive(group, signum, pids)) <
> > 0)
> > +return -1;
>
> This doesn't look correct. In case that both cgroups v1 and v2 are used
> the first call could return 1 meaning that it managed to kill some
> process but the second call would probably return 0 because the process
> would be already gone.
Does it in such case even make sense to call the second callback?
If yes, then I'll probably rather change it such, that a boolean
variable will be set to true if any of the callbacks returns 1 to make
it more obvious.
Re: [PATCH 04/15] virCgroupKillRecursive: Refactor cleanup
On Thu, Oct 22, 2020 at 11:34:55AM +0200, Peter Krempa wrote:
> Remove 'cleanup' label and simplify remembering of the returned value
> from the callback.
>
> Signed-off-by: Peter Krempa
> ---
> src/util/vircgroup.c | 16
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index b74ec1a8fa..dafc6c98c0 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -2532,8 +2532,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
> int
> virCgroupKillRecursive(virCgroupPtr group, int signum)
> {
> -int ret = 0;
> -int rc;
> +int ret = -1;
> size_t i;
> bool backendAvailable = false;
> virCgroupBackendPtr *backends = virCgroupBackendGetAll();
> @@ -2544,24 +2543,17 @@ virCgroupKillRecursive(virCgroupPtr group, int signum)
> for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
> if (backends && backends[i] && backends[i]->available()) {
> backendAvailable = true;
> -rc = backends[i]->killRecursive(group, signum, pids);
> -if (rc < 0) {
> -ret = -1;
> -goto cleanup;
> -}
> -if (rc > 0)
> -ret = rc;
> +if ((ret = backends[i]->killRecursive(group, signum, pids)) < 0)
> +return -1;
This doesn't look correct. In case that both cgroups v1 and v2 are used
the first call could return 1 meaning that it managed to kill some
process but the second call would probably return 0 because the process
would be already gone.
> }
> }
>
> if (!backends || !backendAvailable) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("no cgroup backend available"));
> -ret = -1;
> -goto cleanup;
> +return -1;
> }
>
> - cleanup:
> return ret;
> }
>
> --
> 2.26.2
>
signature.asc
Description: PGP signature
Re: [PATCH 01/15] virCgroupKillRecursive: Return -1 on failure condition
On Thu, Oct 22, 2020 at 11:34:52AM +0200, Peter Krempa wrote: > virCgroupKillRecursive sneakily initializes 'ret' to 0 rather than the > usual -1. 401030499bf moved an error condition but didn't actually > modify 'ret' return the proper error code. > > Fixes: 401030499bf > Signed-off-by: Peter Krempa > --- > src/util/vircgroup.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
[PATCH 15/15] virHashRemoveAll: Don't return number of removed items
Nobody uses the return value.
Signed-off-by: Peter Krempa
---
src/util/virhash.c | 8 ++--
src/util/virhash.h | 2 +-
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/src/util/virhash.c b/src/util/virhash.c
index 7a20b28379..301e485e69 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -579,15 +579,11 @@ _virHashRemoveAllIter(const void *payload G_GNUC_UNUSED,
*
* Free the hash @table's contents. The userdata is
* deallocated with the function provided at creation time.
- *
- * Returns the number of items removed on success, -1 on failure
*/
-ssize_t
+void
virHashRemoveAll(virHashTablePtr table)
{
-return virHashRemoveSet(table,
-_virHashRemoveAllIter,
-NULL);
+virHashRemoveSet(table, _virHashRemoveAllIter, NULL);
}
/**
diff --git a/src/util/virhash.h b/src/util/virhash.h
index d7cea75c35..029e6e83b7 100644
--- a/src/util/virhash.h
+++ b/src/util/virhash.h
@@ -83,7 +83,7 @@ int virHashRemoveEntry(virHashTablePtr table,
/*
* Remove all entries from the hash table.
*/
-ssize_t virHashRemoveAll(virHashTablePtr table);
+void virHashRemoveAll(virHashTablePtr table);
/*
* Retrieve the userdata.
--
2.26.2
[PATCH 11/15] util: hash: Remove virHashValueFree
Use 'g_free' directly.
Signed-off-by: Peter Krempa
---
src/conf/domain_addr.c | 2 +-
src/hypervisor/virclosecallbacks.c | 2 +-
src/libvirt_private.syms| 1 -
src/nwfilter/nwfilter_learnipaddr.c | 2 +-
src/qemu/qemu_interop_config.c | 2 +-
src/qemu/qemu_migration_cookie.c| 2 +-
src/qemu/qemu_monitor.c | 6 +++---
src/qemu/qemu_monitor_json.c| 2 +-
src/util/virhash.c | 7 ---
src/util/virhash.h | 3 ---
tests/qemumonitorjsontest.c | 6 +++---
tests/qemusecuritymock.c| 4 ++--
tests/testutilsqemu.c | 2 +-
13 files changed, 15 insertions(+), 26 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index de344186ec..37dad20ade 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1389,7 +1389,7 @@ virDomainCCWAddressSetCreate(void)
addrs = g_new0(virDomainCCWAddressSet, 1);
-if (!(addrs->defined = virHashNew(virHashValueFree)))
+if (!(addrs->defined = virHashNew(g_free)))
goto error;
/* must use cssid = 0xfe (254) for virtio-ccw devices */
diff --git a/src/hypervisor/virclosecallbacks.c
b/src/hypervisor/virclosecallbacks.c
index 176dd5c263..a73ab818da 100644
--- a/src/hypervisor/virclosecallbacks.c
+++ b/src/hypervisor/virclosecallbacks.c
@@ -69,7 +69,7 @@ virCloseCallbacksNew(void)
if (!(closeCallbacks = virObjectLockableNew(virCloseCallbacksClass)))
return NULL;
-closeCallbacks->list = virHashNew(virHashValueFree);
+closeCallbacks->list = virHashNew(g_free);
if (!closeCallbacks->list) {
virObjectUnref(closeCallbacks);
return NULL;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 36e2c66d93..926e982e0b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2213,7 +2213,6 @@ virHashSearch;
virHashSize;
virHashSteal;
virHashUpdateEntry;
-virHashValueFree;
# util/virhashcode.h
diff --git a/src/nwfilter/nwfilter_learnipaddr.c
b/src/nwfilter/nwfilter_learnipaddr.c
index 82797d9a64..6dc535a4fe 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -781,7 +781,7 @@ virNWFilterLearnInit(void)
if (!pendingLearnReq)
return -1;
-ifaceLockMap = virHashNew(virHashValueFree);
+ifaceLockMap = virHashNew(g_free);
if (!ifaceLockMap) {
virNWFilterLearnShutdown();
return -1;
diff --git a/src/qemu/qemu_interop_config.c b/src/qemu/qemu_interop_config.c
index 080674ce2d..53b251f056 100644
--- a/src/qemu/qemu_interop_config.c
+++ b/src/qemu/qemu_interop_config.c
@@ -125,7 +125,7 @@ qemuInteropFetchConfigs(const char *name,
homeConfig = g_strdup_printf("%s/qemu/%s", xdgConfig, name);
}
-if (!(files = virHashNew(virHashValueFree)))
+if (!(files = virHashNew(g_free)))
return -1;
if (qemuBuildFileList(files, sysLocation) < 0)
diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c
index abe797759d..708c2cced7 100644
--- a/src/qemu/qemu_migration_cookie.c
+++ b/src/qemu/qemu_migration_cookie.c
@@ -435,7 +435,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
-g_autoptr(virHashTable) stats = virHashNew(virHashValueFree);
+g_autoptr(virHashTable) stats = virHashNew(g_free);
bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
size_t i;
int rc;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 51de72b5bf..69d81b20c2 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2137,7 +2137,7 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
-if (!(*ret_stats = virHashNew(virHashValueFree)))
+if (!(*ret_stats = virHashNew(g_free)))
goto error;
ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, *ret_stats,
@@ -4289,7 +4289,7 @@ qemuMonitorGetMemoryDeviceInfo(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
-if (!(*info = virHashNew(virHashValueFree)))
+if (!(*info = virHashNew(g_free)))
return -1;
if ((ret = qemuMonitorJSONGetMemoryDeviceInfo(mon, *info)) < 0) {
@@ -4593,7 +4593,7 @@ qemuMonitorGetPRManagerInfo(qemuMonitorPtr mon,
QEMU_CHECK_MONITOR(mon);
-if (!(info = virHashNew(virHashValueFree)))
+if (!(info = virHashNew(g_free)))
goto cleanup;
if (qemuMonitorJSONGetPRManagerInfo(mon, info) < 0)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 91cc0c9046..2689ad50b9 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5126,7 +5126,7 @@ qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr mon,
}
nr_results = virJSONValueArraySize(data);
-if (!(blockJobs = virHashNew(virHashValueFree)))
+if (!(blockJobs = virHashNew(g_free)))
goto cleanup;
[PATCH 12/15] util: hash: Remove virHashCreateFull
The only place we call it is in virHashNew. Move the code to virHashNew
and remove virHashCreateFull.
Code wishing to use non-strings as hash table keys will be better off
using glib's GHashTable directly.
Signed-off-by: Peter Krempa
---
src/util/virhash.c | 53 +-
src/util/virhash.h | 7 --
2 files changed, 10 insertions(+), 50 deletions(-)
diff --git a/src/util/virhash.c b/src/util/virhash.c
index c781e1d5a5..8d56b4bb85 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -118,43 +118,31 @@ virHashComputeKey(const virHashTable *table, const void
*name)
return value % table->size;
}
+
/**
- * virHashCreateFull:
- * @size: the size of the hash table
+ * virHashNew:
* @dataFree: callback to free data
- * @keyCode: callback to compute hash code
- * @keyEqual: callback to compare hash keys
- * @keyCopy: callback to copy hash keys
- * @keyFree: callback to free keys
*
* Create a new virHashTablePtr.
*
* Returns the newly created object.
*/
-virHashTablePtr virHashCreateFull(ssize_t size,
- virHashDataFree dataFree,
- virHashKeyCode keyCode,
- virHashKeyEqual keyEqual,
- virHashKeyCopy keyCopy,
- virHashKeyPrintHuman keyPrint,
- virHashKeyFree keyFree)
+virHashTablePtr
+virHashNew(virHashDataFree dataFree)
{
virHashTablePtr table = NULL;
-if (size <= 0)
-size = 256;
-
table = g_new0(virHashTable, 1);
table->seed = virRandomBits(32);
-table->size = size;
+table->size = 32;
table->nbElems = 0;
table->dataFree = dataFree;
-table->keyCode = keyCode;
-table->keyEqual = keyEqual;
-table->keyCopy = keyCopy;
-table->keyPrint = keyPrint;
-table->keyFree = keyFree;
+table->keyCode = virHashStrCode;
+table->keyEqual = virHashStrEqual;
+table->keyCopy = virHashStrCopy;
+table->keyPrint = virHashStrPrintHuman;
+table->keyFree = virHashStrFree;
table->table = g_new0(virHashEntryPtr, table->size);
@@ -162,27 +150,6 @@ virHashTablePtr virHashCreateFull(ssize_t size,
}
-/**
- * virHashNew:
- * @dataFree: callback to free data
- *
- * Create a new virHashTablePtr.
- *
- * Returns the newly created object.
- */
-virHashTablePtr
-virHashNew(virHashDataFree dataFree)
-{
-return virHashCreateFull(32,
- dataFree,
- virHashStrCode,
- virHashStrEqual,
- virHashStrCopy,
- virHashStrPrintHuman,
- virHashStrFree);
-}
-
-
virHashAtomicPtr
virHashAtomicNew(virHashDataFree dataFree)
{
diff --git a/src/util/virhash.h b/src/util/virhash.h
index 2d221dce25..a9b022f362 100644
--- a/src/util/virhash.h
+++ b/src/util/virhash.h
@@ -113,13 +113,6 @@ typedef void (*virHashKeyFree)(void *name);
*/
virHashTablePtr virHashNew(virHashDataFree dataFree);
virHashAtomicPtr virHashAtomicNew(virHashDataFree dataFree);
-virHashTablePtr virHashCreateFull(ssize_t size,
- virHashDataFree dataFree,
- virHashKeyCode keyCode,
- virHashKeyEqual keyEqual,
- virHashKeyCopy keyCopy,
- virHashKeyPrintHuman keyPrint,
- virHashKeyFree keyFree);
void virHashFree(virHashTablePtr table);
ssize_t virHashSize(const virHashTable *table);
--
2.26.2
[PATCH 06/15] virHashAtomicNew: Remove 'size' argument
Use 'virHashNew' internally which uses a default size.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_migration.c | 2 +-
src/util/virhash.c| 5 ++---
src/util/virhash.h| 3 +--
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index e18216824c..2f5d61f8e7 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5743,7 +5743,7 @@ qemuMigrationDstErrorFree(void *data)
int
qemuMigrationDstErrorInit(virQEMUDriverPtr driver)
{
-driver->migrationErrors = virHashAtomicNew(64, qemuMigrationDstErrorFree);
+driver->migrationErrors = virHashAtomicNew(qemuMigrationDstErrorFree);
if (driver->migrationErrors)
return 0;
else
diff --git a/src/util/virhash.c b/src/util/virhash.c
index 7dd2d9f81d..23e12e0255 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -212,8 +212,7 @@ virHashTablePtr virHashCreate(ssize_t size, virHashDataFree
dataFree)
virHashAtomicPtr
-virHashAtomicNew(ssize_t size,
- virHashDataFree dataFree)
+virHashAtomicNew(virHashDataFree dataFree)
{
virHashAtomicPtr hash;
@@ -223,7 +222,7 @@ virHashAtomicNew(ssize_t size,
if (!(hash = virObjectLockableNew(virHashAtomicClass)))
return NULL;
-if (!(hash->hash = virHashCreate(size, dataFree))) {
+if (!(hash->hash = virHashNew(dataFree))) {
virObjectUnref(hash);
return NULL;
}
diff --git a/src/util/virhash.h b/src/util/virhash.h
index 37853aba36..baf92996a3 100644
--- a/src/util/virhash.h
+++ b/src/util/virhash.h
@@ -114,8 +114,7 @@ typedef void (*virHashKeyFree)(void *name);
virHashTablePtr virHashNew(virHashDataFree dataFree);
virHashTablePtr virHashCreate(ssize_t size,
virHashDataFree dataFree);
-virHashAtomicPtr virHashAtomicNew(ssize_t size,
- virHashDataFree dataFree);
+virHashAtomicPtr virHashAtomicNew(virHashDataFree dataFree);
virHashTablePtr virHashCreateFull(ssize_t size,
virHashDataFree dataFree,
virHashKeyCode keyCode,
--
2.26.2
[PATCH 14/15] util: virhash: Remove key handling callbacks
Since we use virHashTable for string-keyed values only, we can remove
all the callbacks which allowed universal keys.
Code which wishes to use non-string keys should use glib's GHashTable.
Signed-off-by: Peter Krempa
---
src/util/virhash.c | 68 +++---
src/util/virhash.h | 54
2 files changed, 10 insertions(+), 112 deletions(-)
diff --git a/src/util/virhash.c b/src/util/virhash.c
index f386839d6b..7a20b28379 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -56,11 +56,6 @@ struct _virHashTable {
size_t size;
size_t nbElems;
virHashDataFree dataFree;
-virHashKeyCode keyCode;
-virHashKeyEqual keyEqual;
-virHashKeyCopy keyCopy;
-virHashKeyPrintHuman keyPrint;
-virHashKeyFree keyFree;
};
struct _virHashAtomic {
@@ -81,40 +76,10 @@ static int virHashAtomicOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virHashAtomic);
-
-static uint32_t virHashStrCode(const char *name, uint32_t seed)
-{
-return virHashCodeGen(name, strlen(name), seed);
-}
-
-static bool virHashStrEqual(const char *namea, const char *nameb)
-{
-return STREQ(namea, nameb);
-}
-
-static char *virHashStrCopy(const char *name)
-{
-return g_strdup(name);
-}
-
-
-static char *
-virHashStrPrintHuman(const char *name)
-{
-return g_strdup(name);
-}
-
-
-static void virHashStrFree(char *name)
-{
-VIR_FREE(name);
-}
-
-
static size_t
virHashComputeKey(const virHashTable *table, const char *name)
{
-uint32_t value = table->keyCode(name, table->seed);
+uint32_t value = virHashCodeGen(name, strlen(name), table->seed);
return value % table->size;
}
@@ -138,11 +103,6 @@ virHashNew(virHashDataFree dataFree)
table->size = 32;
table->nbElems = 0;
table->dataFree = dataFree;
-table->keyCode = virHashStrCode;
-table->keyEqual = virHashStrEqual;
-table->keyCopy = virHashStrCopy;
-table->keyPrint = virHashStrPrintHuman;
-table->keyFree = virHashStrFree;
table->table = g_new0(virHashEntryPtr, table->size);
@@ -260,8 +220,7 @@ virHashFree(virHashTablePtr table)
if (table->dataFree)
table->dataFree(iter->payload);
-if (table->keyFree)
-table->keyFree(iter->name);
+g_free(iter->name);
VIR_FREE(iter);
iter = next;
}
@@ -287,20 +246,15 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const char
*name,
/* Check for duplicate entry */
for (entry = table->table[key]; entry; entry = entry->next) {
-if (table->keyEqual(entry->name, name)) {
+if (STREQ(entry->name, name)) {
if (is_update) {
if (table->dataFree)
table->dataFree(entry->payload);
entry->payload = userdata;
return 0;
} else {
-g_autofree char *keystr = NULL;
-
-if (table->keyPrint)
-keystr = table->keyPrint(name);
-
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Duplicate hash table key '%s'"),
NULLSTR(keystr));
+ _("Duplicate hash table key '%s'"), name);
return -1;
}
}
@@ -309,7 +263,7 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const char
*name,
}
entry = g_new0(virHashEntry, 1);
-entry->name = table->keyCopy(name);
+entry->name = g_strdup(name);
entry->payload = userdata;
if (last)
@@ -388,7 +342,7 @@ virHashGetEntry(const virHashTable *table,
key = virHashComputeKey(table, name);
for (entry = table->table[key]; entry; entry = entry->next) {
-if (table->keyEqual(entry->name, name))
+if (STREQ(entry->name, name))
return entry;
}
@@ -510,11 +464,10 @@ virHashRemoveEntry(virHashTablePtr table, const char
*name)
nextptr = table->table + virHashComputeKey(table, name);
for (entry = *nextptr; entry; entry = entry->next) {
-if (table->keyEqual(entry->name, name)) {
+if (STREQ(entry->name, name)) {
if (table->dataFree)
table->dataFree(entry->payload);
-if (table->keyFree)
-table->keyFree(entry->name);
+g_free(entry->name);
*nextptr = entry->next;
VIR_FREE(entry);
table->nbElems--;
@@ -601,8 +554,7 @@ virHashRemoveSet(virHashTablePtr table,
count++;
if (table->dataFree)
table->dataFree(entry->payload);
-if (table->keyFree)
-table->keyFree(entry->name);
+g_free(entry->name);
*nextptr = entry->next;
VIR_FREE(entry);
table->nbElems--;
@@ -669,7 +621,7 @@ void *virHashSearch(const virHashTable *ctable,
for (entry = table->table[i]; entry; entry = entry->next
[PATCH 13/15] util: hash: Change type of hash table name/key to 'char'
All users of virHashTable pass strings as the name/key of the entry.
Make this an official requirement by turning the variables to 'const
char *'.
For any other case it's better to use glib's GHashTable.
Signed-off-by: Peter Krempa
---
src/conf/nwfilter_params.c | 2 +-
src/conf/virchrdev.c | 2 +-
src/conf/virdomainmomentobjlist.c | 6 ++--
src/conf/virdomainobjlist.c| 12
src/conf/virinterfaceobj.c | 10 +++---
src/conf/virnetworkobj.c | 16 +-
src/conf/virnodedeviceobj.c| 18 +--
src/conf/virnwfilterbindingobjlist.c | 4 +--
src/conf/virsecretobj.c| 8 ++---
src/conf/virstorageobj.c | 22 +++---
src/hypervisor/virclosecallbacks.c | 2 +-
src/locking/lock_daemon.c | 2 +-
src/nwfilter/nwfilter_dhcpsnoop.c | 6 ++--
src/nwfilter/nwfilter_gentech_driver.c | 6 ++--
src/qemu/qemu_blockjob.c | 2 +-
src/qemu/qemu_capabilities.c | 2 +-
src/qemu/qemu_checkpoint.c | 2 +-
src/qemu/qemu_domain.c | 6 ++--
src/qemu/qemu_domain.h | 2 +-
src/qemu/qemu_process.c| 2 +-
src/qemu/qemu_snapshot.c | 4 +--
src/rpc/virnetdaemon.c | 14 -
src/test/test_driver.c | 6 ++--
src/util/virfilecache.c| 2 +-
src/util/virhash.c | 42 +-
src/util/virhash.h | 32 ++--
src/util/virlockspace.c| 2 +-
src/util/virmacmap.c | 4 +--
tests/qemumonitorjsontest.c| 2 +-
tests/qemusecuritymock.c | 8 ++---
tests/virhashtest.c| 10 +++---
31 files changed, 129 insertions(+), 129 deletions(-)
diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c
index fd05b45ca3..73160a38a4 100644
--- a/src/conf/nwfilter_params.c
+++ b/src/conf/nwfilter_params.c
@@ -620,7 +620,7 @@ struct addToTableStruct {
static int
-addToTable(void *payload, const void *name, void *data)
+addToTable(void *payload, const char *name, void *data)
{
struct addToTableStruct *atts = (struct addToTableStruct *)data;
virNWFilterVarValuePtr val;
diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c
index 0d2c9503ab..5e5c03d03b 100644
--- a/src/conf/virchrdev.c
+++ b/src/conf/virchrdev.c
@@ -279,7 +279,7 @@ virChrdevsPtr virChrdevAlloc(void)
* Helper to clear stream callbacks when freeing the hash
*/
static int virChrdevFreeClearCallbacks(void *payload,
- const void *name G_GNUC_UNUSED,
+ const char *name G_GNUC_UNUSED,
void *data G_GNUC_UNUSED)
{
virChrdevHashEntry *ent = payload;
diff --git a/src/conf/virdomainmomentobjlist.c
b/src/conf/virdomainmomentobjlist.c
index 43c77e6c54..511bf1d415 100644
--- a/src/conf/virdomainmomentobjlist.c
+++ b/src/conf/virdomainmomentobjlist.c
@@ -76,7 +76,7 @@ struct moment_act_on_descendant {
static int
virDomainMomentActOnDescendant(void *payload,
- const void *name,
+ const char *name,
void *data)
{
virDomainMomentObjPtr obj = payload;
@@ -307,7 +307,7 @@ struct virDomainMomentNameData {
static int virDomainMomentObjListCopyNames(void *payload,
- const void *name G_GNUC_UNUSED,
+ const char *name G_GNUC_UNUSED,
void *opaque)
{
virDomainMomentObjPtr obj = payload;
@@ -491,7 +491,7 @@ struct moment_set_relation {
};
static int
virDomainMomentSetRelations(void *payload,
-const void *name G_GNUC_UNUSED,
+const char *name G_GNUC_UNUSED,
void *data)
{
virDomainMomentObjPtr obj = payload;
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 9e8757eff9..e9a4b271df 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -95,7 +95,7 @@ static void virDomainObjListDispose(void *obj)
static int virDomainObjListSearchID(const void *payload,
-const void *name G_GNUC_UNUSED,
+const char *name G_GNUC_UNUSED,
const void *data)
{
virDomainObjPtr obj = (virDomainObjPtr)payload;
@@ -649,7 +649,7 @@ struct virDomainObjListData {
static int
virDomainObjListCount(void *payload,
- const void *name G_GNUC_UNUSED,
+ const char *name G_GNUC_UNUSED,
void *opaque)
{
virDomainObjPtr obj = payload;
@@ -696,7 +696,7 @@ struct virDomainIDData {
static int
[PATCH 08/15] tests: hash: Prepare for replacement of virHashCreate
Most callers pass a random number. We have virHashNew which doesn't give
the callers the option to configure the table. Since we are going to
switch to virHashNew replace it in tests and remove multiple instances
of the 'testHashGrow' case as it doesn't make sense with the new
semantics.
Signed-off-by: Peter Krempa
---
tests/virhashtest.c | 28 +++-
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/tests/virhashtest.c b/tests/virhashtest.c
index 7b1b8dd38c..bc93c07d1f 100644
--- a/tests/virhashtest.c
+++ b/tests/virhashtest.c
@@ -15,12 +15,12 @@
VIR_LOG_INIT("tests.hashtest");
static virHashTablePtr
-testHashInit(int size)
+testHashInit(void)
{
virHashTablePtr hash;
ssize_t i;
-if (!(hash = virHashCreate(size, NULL)))
+if (!(hash = virHashNew(NULL)))
return NULL;
/* entries are added in reverse order so that they will be linked in
@@ -83,13 +83,12 @@ struct testInfo {
static int
-testHashGrow(const void *data)
+testHashGrow(const void *data G_GNUC_UNUSED)
{
-const struct testInfo *info = data;
virHashTablePtr hash;
int ret = -1;
-if (!(hash = testHashInit(info->count)))
+if (!(hash = testHashInit()))
return -1;
if (testHashCheckCount(hash, G_N_ELEMENTS(uuids)) < 0)
@@ -111,7 +110,7 @@ testHashUpdate(const void *data G_GNUC_UNUSED)
size_t i;
int ret = -1;
-if (!(hash = testHashInit(0)))
+if (!(hash = testHashInit()))
return -1;
for (i = 0; i < G_N_ELEMENTS(uuids_subset); i++) {
@@ -149,7 +148,7 @@ testHashRemove(const void *data G_GNUC_UNUSED)
size_t i;
int ret = -1;
-if (!(hash = testHashInit(0)))
+if (!(hash = testHashInit()))
return -1;
for (i = 0; i < G_N_ELEMENTS(uuids_subset); i++) {
@@ -216,7 +215,7 @@ testHashRemoveForEach(const void *data)
virHashTablePtr hash;
int ret = -1;
-if (!(hash = testHashInit(0)))
+if (!(hash = testHashInit()))
return -1;
if (virHashForEach(hash, (virHashIterator) info->data, hash)) {
@@ -243,7 +242,7 @@ testHashSteal(const void *data G_GNUC_UNUSED)
size_t i;
int ret = -1;
-if (!(hash = testHashInit(0)))
+if (!(hash = testHashInit()))
return -1;
for (i = 0; i < G_N_ELEMENTS(uuids_subset); i++) {
@@ -297,7 +296,7 @@ testHashRemoveSet(const void *data G_GNUC_UNUSED)
int rcount;
int ret = -1;
-if (!(hash = testHashInit(0)))
+if (!(hash = testHashInit()))
return -1;
/* seed the generator so that rand() provides reproducible sequence */
@@ -340,7 +339,7 @@ testHashSearch(const void *data G_GNUC_UNUSED)
void *entry;
int ret = -1;
-if (!(hash = testHashInit(0)))
+if (!(hash = testHashInit()))
return -1;
entry = virHashSearch(hash, testHashSearchIter, NULL, NULL);
@@ -544,15 +543,10 @@ mymain(void)
testHash ## cmd ## data, \
testHashCount ## cmd ## data)
-#define DO_TEST_COUNT(name, cmd, count) \
-DO_TEST_FULL(name "(" #count ")", cmd, NULL, count)
-
#define DO_TEST(name, cmd) \
DO_TEST_FULL(name, cmd, NULL, -1)
-DO_TEST_COUNT("Grow", Grow, 1);
-DO_TEST_COUNT("Grow", Grow, 10);
-DO_TEST_COUNT("Grow", Grow, 42);
+DO_TEST("Grow", Grow);
DO_TEST("Update", Update);
DO_TEST("Remove", Remove);
DO_TEST_DATA("Remove in ForEach", RemoveForEach, Some);
--
2.26.2
[PATCH 05/15] conf: domain_addr: Refactor hash usage in zpci reservation code
Rewrite using GHashTable which already has interfaces for using a number
as hash key.
Signed-off-by: Peter Krempa
---
src/conf/domain_addr.c | 123 +
src/conf/domain_addr.h | 4 +-
2 files changed, 27 insertions(+), 100 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index c7419916aa..4e47c2547c 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -25,17 +25,18 @@
#include "virlog.h"
#include "virstring.h"
#include "domain_addr.h"
-#include "virhashcode.h"
#define VIR_FROM_THIS VIR_FROM_DOMAIN
VIR_LOG_INIT("conf.domain_addr");
static int
-virDomainZPCIAddressReserveId(virHashTablePtr set,
+virDomainZPCIAddressReserveId(GHashTable *set,
virZPCIDeviceAddressID *id,
const char *name)
{
+int *idval;
+
if (!id->isSet) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("No zPCI %s to reserve"),
@@ -43,26 +44,24 @@ virDomainZPCIAddressReserveId(virHashTablePtr set,
return -1;
}
-if (virHashLookup(set, &id->value)) {
+if (g_hash_table_lookup(set, &id->value)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("zPCI %s %o is already reserved"),
name, id->value);
return -1;
}
-if (virHashAddEntry(set, &id->value, (void*)1) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to reserve %s %o"),
- name, id->value);
-return -1;
-}
+idval = g_new0(int, 1);
+*idval = (int) id->value;
+
+g_hash_table_add(set, idval);
return 0;
}
static int
-virDomainZPCIAddressReserveUid(virHashTablePtr set,
+virDomainZPCIAddressReserveUid(GHashTable *set,
virZPCIDeviceAddressPtr addr)
{
return virDomainZPCIAddressReserveId(set, &addr->uid, "uid");
@@ -70,7 +69,7 @@ virDomainZPCIAddressReserveUid(virHashTablePtr set,
static int
-virDomainZPCIAddressReserveFid(virHashTablePtr set,
+virDomainZPCIAddressReserveFid(GHashTable *set,
virZPCIDeviceAddressPtr addr)
{
return virDomainZPCIAddressReserveId(set, &addr->fid, "fid");
@@ -78,7 +77,7 @@ virDomainZPCIAddressReserveFid(virHashTablePtr set,
static int
-virDomainZPCIAddressAssignId(virHashTablePtr set,
+virDomainZPCIAddressAssignId(GHashTable *set,
virZPCIDeviceAddressID *id,
unsigned int min,
unsigned int max,
@@ -87,7 +86,7 @@ virDomainZPCIAddressAssignId(virHashTablePtr set,
if (id->isSet)
return 0;
-while (virHashLookup(set, &min)) {
+while (g_hash_table_lookup(set, &min)) {
if (min == max) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("There is no more free %s."),
@@ -105,7 +104,7 @@ virDomainZPCIAddressAssignId(virHashTablePtr set,
static int
-virDomainZPCIAddressAssignUid(virHashTablePtr set,
+virDomainZPCIAddressAssignUid(GHashTable *set,
virZPCIDeviceAddressPtr addr)
{
return virDomainZPCIAddressAssignId(set, &addr->uid, 1,
@@ -114,7 +113,7 @@ virDomainZPCIAddressAssignUid(virHashTablePtr set,
static int
-virDomainZPCIAddressAssignFid(virHashTablePtr set,
+virDomainZPCIAddressAssignFid(GHashTable *set,
virZPCIDeviceAddressPtr addr)
{
return virDomainZPCIAddressAssignId(set, &addr->fid, 0,
@@ -123,40 +122,19 @@ virDomainZPCIAddressAssignFid(virHashTablePtr set,
static void
-virDomainZPCIAddressReleaseId(virHashTablePtr set,
- virZPCIDeviceAddressID *id,
- const char *name)
+virDomainZPCIAddressReleaseId(GHashTable *set,
+ virZPCIDeviceAddressID *id)
{
if (!id->isSet)
return;
-if (virHashRemoveEntry(set, &id->value) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Release %s %o failed"),
- name, id->value);
-}
+g_hash_table_remove(set, &id->value);
id->value = 0;
id->isSet = false;
}
-static void
-virDomainZPCIAddressReleaseUid(virHashTablePtr set,
- virZPCIDeviceAddressPtr addr)
-{
-virDomainZPCIAddressReleaseId(set, &addr->uid, "uid");
-}
-
-
-static void
-virDomainZPCIAddressReleaseFid(virHashTablePtr set,
- virZPCIDeviceAddressPtr addr)
-{
-virDomainZPCIAddressReleaseId(set, &addr->fid, "fid");
-}
-
-
static void
virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
virZPCIDeviceAddressPtr addr)
@@ -164,8 +142,8 @@ virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr
zpciIds,
if (!zpciIds)
return;
-virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
-virDomainZPCIAddres
[PATCH 01/15] virCgroupKillRecursive: Return -1 on failure condition
virCgroupKillRecursive sneakily initializes 'ret' to 0 rather than the
usual -1. 401030499bf moved an error condition but didn't actually
modify 'ret' return the proper error code.
Fixes: 401030499bf
Signed-off-by: Peter Krempa
---
src/util/vircgroup.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index d408e3366f..5f4cb01bc0 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2591,6 +2591,7 @@ virCgroupKillRecursive(virCgroupPtr group, int signum)
if (!backends || !backendAvailable) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("no cgroup backend available"));
+ret = -1;
goto cleanup;
}
--
2.26.2
[PATCH 03/15] util: cgroup: Use GHashTable instead of virHashTable
Rewrite using GHashTable which already has interfaces for using a number
as hash key. Glib's implementation doesn't copy the key by default, so
we need to allocate it, but overal the interface is more suited for this
case.
Signed-off-by: Peter Krempa
---
src/util/vircgroup.c| 61 -
src/util/vircgroupbackend.h | 3 +-
src/util/vircgrouppriv.h| 2 +-
src/util/vircgroupv1.c | 2 +-
src/util/vircgroupv2.c | 2 +-
5 files changed, 17 insertions(+), 53 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 5f4cb01bc0..b74ec1a8fa 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -42,7 +42,6 @@
#include "virlog.h"
#include "virfile.h"
#include "virhash.h"
-#include "virhashcode.h"
#include "virstring.h"
#include "virsystemd.h"
#include "virtypedparam.h"
@@ -2382,7 +2381,7 @@ virCgroupRemove(virCgroupPtr group)
static int
virCgroupKillInternal(virCgroupPtr group,
int signum,
- virHashTablePtr pids,
+ GHashTable *pids,
int controller,
const char *taskFile)
{
@@ -2415,8 +2414,9 @@ virCgroupKillInternal(virCgroupPtr group,
goto cleanup;
} else {
while (!feof(fp)) {
-long pid_value;
-if (fscanf(fp, "%ld", &pid_value) != 1) {
+g_autofree long long *pid_value = g_new0(long long, 1);
+
+if (fscanf(fp, "%lld", pid_value) != 1) {
if (feof(fp))
break;
virReportSystemError(errno,
@@ -2424,16 +2424,17 @@ virCgroupKillInternal(virCgroupPtr group,
keypath);
goto cleanup;
}
-if (virHashLookup(pids, (void*)pid_value))
+
+if (g_hash_table_lookup(pids, pid_value))
continue;
-VIR_DEBUG("pid=%ld", pid_value);
+VIR_DEBUG("pid=%lld", *pid_value);
/* Cgroups is a Linux concept, so this cast is safe. */
-if (kill((pid_t)pid_value, signum) < 0) {
+if (kill((pid_t)*pid_value, signum) < 0) {
if (errno != ESRCH) {
virReportSystemError(errno,
- _("Failed to kill process %ld"),
- pid_value);
+ _("Failed to kill process %lld"),
+ *pid_value);
goto cleanup;
}
/* Leave RC == 0 since we didn't kill one */
@@ -2442,7 +2443,7 @@ virCgroupKillInternal(virCgroupPtr group,
done = false;
}
-ignore_value(virHashAddEntry(pids, (void*)pid_value,
(void*)1));
+g_hash_table_add(pids, g_steal_pointer(&pid_value));
}
VIR_FORCE_FCLOSE(fp);
}
@@ -2458,39 +2459,10 @@ virCgroupKillInternal(virCgroupPtr group,
}
-static uint32_t
-virCgroupPidCode(const void *name, uint32_t seed)
-{
-long pid_value = (long)(intptr_t)name;
-return virHashCodeGen(&pid_value, sizeof(pid_value), seed);
-}
-
-
-static bool
-virCgroupPidEqual(const void *namea, const void *nameb)
-{
-return namea == nameb;
-}
-
-
-static void *
-virCgroupPidCopy(const void *name)
-{
-return (void*)name;
-}
-
-
-static char *
-virCgroupPidPrintHuman(const void *name)
-{
-return g_strdup_printf("%ld", (const long)name);
-}
-
-
int
virCgroupKillRecursiveInternal(virCgroupPtr group,
int signum,
- virHashTablePtr pids,
+ GHashTable *pids,
int controller,
const char *taskFile,
bool dormdir)
@@ -2565,13 +2537,7 @@ virCgroupKillRecursive(virCgroupPtr group, int signum)
size_t i;
bool backendAvailable = false;
virCgroupBackendPtr *backends = virCgroupBackendGetAll();
-virHashTablePtr pids = virHashCreateFull(100,
- NULL,
- virCgroupPidCode,
- virCgroupPidEqual,
- virCgroupPidCopy,
- virCgroupPidPrintHuman,
- NULL);
+g_autoptr(GHashTable) pids = g_hash_table_new_full(g_int64_hash,
g_int64_equal, g_free, NULL);
VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum);
@@ -2596,7 +2562,6 @@ virCgroupKillRecursive(virCgroupPtr group, int signum)
}
cleanup:
-virHashFree(pids);
return ret;
}
diff --git a/src/util/v
[PATCH 02/15] util: virhash: Remove virHashTableSize
It's used only in one place in tests which isn't even automatically
evaluated.
Signed-off-by: Peter Krempa
---
src/libvirt_private.syms | 1 -
src/util/virhash.c | 17 -
src/util/virhash.h | 1 -
tests/virhashtest.c | 6 --
4 files changed, 25 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b2d786409b..79c8403208 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2213,7 +2213,6 @@ virHashRemoveSet;
virHashSearch;
virHashSize;
virHashSteal;
-virHashTableSize;
virHashUpdateEntry;
virHashValueFree;
diff --git a/src/util/virhash.c b/src/util/virhash.c
index 4b503a0313..7dd2d9f81d 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -549,23 +549,6 @@ virHashSize(const virHashTable *table)
return table->nbElems;
}
-/**
- * virHashTableSize:
- * @table: the hash table
- *
- * Query the size of the hash @table, i.e., number of buckets in the table.
- *
- * Returns the number of keys in the hash table or
- * -1 in case of error
- */
-ssize_t
-virHashTableSize(const virHashTable *table)
-{
-if (table == NULL)
-return -1;
-return table->size;
-}
-
/**
* virHashRemoveEntry:
diff --git a/src/util/virhash.h b/src/util/virhash.h
index cb59fb639b..37853aba36 100644
--- a/src/util/virhash.h
+++ b/src/util/virhash.h
@@ -125,7 +125,6 @@ virHashTablePtr virHashCreateFull(ssize_t size,
virHashKeyFree keyFree);
void virHashFree(virHashTablePtr table);
ssize_t virHashSize(const virHashTable *table);
-ssize_t virHashTableSize(const virHashTable *table);
/*
* Add a new entry to the hash table.
diff --git a/tests/virhashtest.c b/tests/virhashtest.c
index af30791241..7b1b8dd38c 100644
--- a/tests/virhashtest.c
+++ b/tests/virhashtest.c
@@ -27,16 +27,10 @@ testHashInit(int size)
* collision list in the same order as in the uuids array
*/
for (i = G_N_ELEMENTS(uuids) - 1; i >= 0; i--) {
-ssize_t oldsize = virHashTableSize(hash);
if (virHashAddEntry(hash, uuids[i], (void *) uuids[i]) < 0) {
virHashFree(hash);
return NULL;
}
-
-if (virHashTableSize(hash) != oldsize) {
-VIR_TEST_DEBUG("hash grown from %zu to %zu",
- (size_t)oldsize, (size_t)virHashTableSize(hash));
-}
}
for (i = 0; i < G_N_ELEMENTS(uuids); i++) {
--
2.26.2
[PATCH 07/15] conf: nwfilter: Replace 'virNWFilterHashTableCreate' with 'virHashNew'
Export the freeing function rather than having a wrapper for the hash
creation function.
Signed-off-by: Peter Krempa
---
src/conf/domain_nwfilter.c | 2 +-
src/conf/nwfilter_ipaddrmap.c | 2 +-
src/conf/nwfilter_params.c | 12 +++-
src/conf/nwfilter_params.h | 2 +-
src/conf/virnwfilterbindingdef.c | 2 +-
src/libvirt_private.syms | 2 +-
src/nwfilter/nwfilter_gentech_driver.c | 6 +++---
tests/nwfilterxml2firewalltest.c | 6 +++---
8 files changed, 14 insertions(+), 20 deletions(-)
diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
index ef388d5d02..f32122b8a3 100644
--- a/src/conf/domain_nwfilter.c
+++ b/src/conf/domain_nwfilter.c
@@ -59,7 +59,7 @@ virNWFilterBindingDefForNet(const char *vmname,
ret->filter = g_strdup(net->filter);
-if (!(ret->filterparams = virNWFilterHashTableCreate(0)))
+if (!(ret->filterparams = virHashNew(virNWFilterVarValueHashFree)))
goto error;
if (net->filterparams &&
diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c
index 672a58be3a..487e7f22bd 100644
--- a/src/conf/nwfilter_ipaddrmap.c
+++ b/src/conf/nwfilter_ipaddrmap.c
@@ -149,7 +149,7 @@ virNWFilterIPAddrMapGetIPAddr(const char *ifname)
int
virNWFilterIPAddrMapInit(void)
{
-ipAddressMap = virNWFilterHashTableCreate(0);
+ipAddressMap = virHashNew(virNWFilterVarValueHashFree);
if (!ipAddressMap)
return -1;
diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c
index 1210079954..fd05b45ca3 100644
--- a/src/conf/nwfilter_params.c
+++ b/src/conf/nwfilter_params.c
@@ -606,19 +606,13 @@
virNWFilterVarCombIterGetVarValue(virNWFilterVarCombIterPtr ci,
return res;
}
-static void
-hashDataFree(void *payload)
+void
+virNWFilterVarValueHashFree(void *payload)
{
virNWFilterVarValueFree(payload);
}
-virHashTablePtr
-virNWFilterHashTableCreate(int n)
-{
-return virHashCreate(n, hashDataFree);
-}
-
struct addToTableStruct {
virHashTablePtr target;
int errOccurred;
@@ -708,7 +702,7 @@ virNWFilterParseParamAttributes(xmlNodePtr cur)
char *nam, *val;
virNWFilterVarValuePtr value;
-virHashTablePtr table = virNWFilterHashTableCreate(0);
+virHashTablePtr table = virHashNew(virNWFilterVarValueHashFree);
if (!table)
return NULL;
diff --git a/src/conf/nwfilter_params.h b/src/conf/nwfilter_params.h
index d51f3f7f9f..4962a86864 100644
--- a/src/conf/nwfilter_params.h
+++ b/src/conf/nwfilter_params.h
@@ -52,6 +52,7 @@ virNWFilterVarValuePtr virNWFilterVarValueCreateSimple(char
*);
virNWFilterVarValuePtr virNWFilterVarValueCreateSimpleCopyValue(const char *);
virNWFilterVarValuePtr virNWFilterVarValueCopy(const virNWFilterVarValue *);
void virNWFilterVarValueFree(virNWFilterVarValuePtr val);
+void virNWFilterVarValueHashFree(void *payload);
const char *virNWFilterVarValueGetSimple(const virNWFilterVarValue *val);
const char *virNWFilterVarValueGetNthValue(const virNWFilterVarValue *val,
unsigned int idx);
@@ -67,7 +68,6 @@ int virNWFilterFormatParamAttributes(virBufferPtr buf,
virHashTablePtr table,
const char *filterref);
-virHashTablePtr virNWFilterHashTableCreate(int n);
int virNWFilterHashTablePutAll(virHashTablePtr src,
virHashTablePtr dest);
bool virNWFilterHashTableEqual(virHashTablePtr a,
diff --git a/src/conf/virnwfilterbindingdef.c b/src/conf/virnwfilterbindingdef.c
index 81f8e72182..28c1c3938c 100644
--- a/src/conf/virnwfilterbindingdef.c
+++ b/src/conf/virnwfilterbindingdef.c
@@ -65,7 +65,7 @@ virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src)
ret->filter = g_strdup(src->filter);
-if (!(ret->filterparams = virNWFilterHashTableCreate(0)))
+if (!(ret->filterparams = virHashNew(virNWFilterVarValueHashFree)))
goto error;
if (virNWFilterHashTablePutAll(src->filterparams, ret->filterparams) < 0)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 79c8403208..4f9e7f6048 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -927,7 +927,6 @@ virNWFilterIPAddrMapShutdown;
# conf/nwfilter_params.h
-virNWFilterHashTableCreate;
virNWFilterHashTableEqual;
virNWFilterHashTablePutAll;
virNWFilterVarAccessGetVarName;
@@ -948,6 +947,7 @@ virNWFilterVarValueFree;
virNWFilterVarValueGetCardinality;
virNWFilterVarValueGetNthValue;
virNWFilterVarValueGetSimple;
+virNWFilterVarValueHashFree;
# conf/object_event.h
diff --git a/src/nwfilter/nwfilter_gentech_driver.c
b/src/nwfilter/nwfilter_gentech_driver.c
index c93f2ed18f..99d2c0fce4 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -199,7 +199,7 @@ static virHashTablePtr
virNWFilterCreateVarsFrom(virHashTablePtr vars1,
virHashTableP
[PATCH 04/15] virCgroupKillRecursive: Refactor cleanup
Remove 'cleanup' label and simplify remembering of the returned value
from the callback.
Signed-off-by: Peter Krempa
---
src/util/vircgroup.c | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index b74ec1a8fa..dafc6c98c0 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2532,8 +2532,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group,
int
virCgroupKillRecursive(virCgroupPtr group, int signum)
{
-int ret = 0;
-int rc;
+int ret = -1;
size_t i;
bool backendAvailable = false;
virCgroupBackendPtr *backends = virCgroupBackendGetAll();
@@ -2544,24 +2543,17 @@ virCgroupKillRecursive(virCgroupPtr group, int signum)
for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
if (backends && backends[i] && backends[i]->available()) {
backendAvailable = true;
-rc = backends[i]->killRecursive(group, signum, pids);
-if (rc < 0) {
-ret = -1;
-goto cleanup;
-}
-if (rc > 0)
-ret = rc;
+if ((ret = backends[i]->killRecursive(group, signum, pids)) < 0)
+return -1;
}
}
if (!backends || !backendAvailable) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("no cgroup backend available"));
-ret = -1;
-goto cleanup;
+return -1;
}
- cleanup:
return ret;
}
--
2.26.2
[PATCH 10/15] Replace all instances of 'virHashCreate' with 'virHashNew'
It doesn't make much sense to configure the bucket count in the hash
table for each case specifically. Replace all calls of virHashCreate
with virHashNew which has a pre-set size and remove virHashCreate
completely.
Signed-off-by: Peter Krempa
---
src/conf/domain_addr.c| 2 +-
src/conf/domain_conf.c| 4 ++--
src/conf/virchrdev.c | 2 +-
src/conf/virdomainmomentobjlist.c | 2 +-
src/conf/virdomainobjlist.c | 4 ++--
src/conf/virinterfaceobj.c| 2 +-
src/conf/virnetworkobj.c | 5 ++---
src/conf/virnodedeviceobj.c | 2 +-
src/conf/virnwfilterbindingobjlist.c | 2 +-
src/conf/virsecretobj.c | 2 +-
src/conf/virstorageobj.c | 10 +-
src/hyperv/hyperv_wmi.c | 2 +-
src/hypervisor/virclosecallbacks.c| 2 +-
src/libvirt_private.syms | 1 -
src/libxl/libxl_logger.c | 2 +-
src/locking/lock_daemon.c | 8 ++--
src/nwfilter/nwfilter_dhcpsnoop.c | 6 +++---
src/nwfilter/nwfilter_ebiptables_driver.c | 4 ++--
src/nwfilter/nwfilter_gentech_driver.c| 2 +-
src/nwfilter/nwfilter_learnipaddr.c | 4 ++--
src/qemu/qemu_block.c | 6 +++---
src/qemu/qemu_capabilities.c | 4 ++--
src/qemu/qemu_driver.c| 2 +-
src/qemu/qemu_interop_config.c| 2 +-
src/qemu/qemu_monitor.c | 10 +-
src/qemu/qemu_monitor_json.c | 2 +-
src/qemu/qemu_qapi.c | 2 +-
src/rpc/virnetdaemon.c| 2 +-
src/security/security_selinux.c | 4 ++--
src/util/virfilecache.c | 2 +-
src/util/virhash.c| 21 -
src/util/virhash.h| 2 --
src/util/viriptables.c| 4 ++--
src/util/virlockspace.c | 6 ++
src/util/virmacmap.c | 2 +-
src/util/virstoragefile.c | 4 ++--
src/util/virsystemd.c | 2 +-
tests/qemumonitorjsontest.c | 10 +-
tests/qemusecuritymock.c | 4 ++--
tests/testutilsqemu.c | 2 +-
tests/virhashtest.c | 8
41 files changed, 69 insertions(+), 100 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 4e47c2547c..de344186ec 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1389,7 +1389,7 @@ virDomainCCWAddressSetCreate(void)
addrs = g_new0(virDomainCCWAddressSet, 1);
-if (!(addrs->defined = virHashCreate(10, virHashValueFree)))
+if (!(addrs->defined = virHashNew(virHashValueFree)))
goto error;
/* must use cssid = 0xfe (254) for virtio-ccw devices */
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 37efd104f1..f4f017cf83 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5821,7 +5821,7 @@ virDomainDefBootOrderPostParse(virDomainDefPtr def)
virHashTablePtr bootHash = NULL;
int ret = -1;
-if (!(bootHash = virHashCreate(5, NULL)))
+if (!(bootHash = virHashNew(NULL)))
goto cleanup;
if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder,
bootHash) < 0)
@@ -6927,7 +6927,7 @@ virDomainDefValidateAliases(const virDomainDef *def,
int ret = -1;
/* We are not storing copies of aliases. Don't free them. */
-if (!(data.aliases = virHashCreate(10, NULL)))
+if (!(data.aliases = virHashNew(NULL)))
goto cleanup;
if (virDomainDeviceInfoIterateInternal((virDomainDefPtr) def,
diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c
index c50f9ca720..0d2c9503ab 100644
--- a/src/conf/virchrdev.c
+++ b/src/conf/virchrdev.c
@@ -266,7 +266,7 @@ virChrdevsPtr virChrdevAlloc(void)
/* there will hardly be any devices most of the time, the hash
* does not have to be huge */
-if (!(devs->hash = virHashCreate(3, virChrdevHashEntryFree)))
+if (!(devs->hash = virHashNew(virChrdevHashEntryFree)))
goto error;
return devs;
diff --git a/src/conf/virdomainmomentobjlist.c
b/src/conf/virdomainmomentobjlist.c
index 8a1eb6b734..43c77e6c54 100644
--- a/src/conf/virdomainmomentobjlist.c
+++ b/src/conf/virdomainmomentobjlist.c
@@ -275,7 +275,7 @@ virDomainMomentObjListNew(void)
virDomainMomentObjListPtr moments;
moments = g_new0(virDomainMomentObjList, 1);
-moments->objs = virHashCreate(50, virDomainMomentObjListDataFree);
+moments->objs = virHashNew(virDomainMomentObjListDataFree);
if (!moments->objs) {
VIR_FREE(moments);
return NULL;
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 46b28cc9a6..9e8757eff9 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@
[PATCH 00/15] util: hash: Use glib's GHashTable - preparation (part 1)
Glib provides a hash table implementation called GHashTable. In this part of the series we'll refactor two instances of code which use non-string keys for hashtable to use GHashTable directly which simplifies the code (glib provides int hashing functions). Since GHashTable is not a direct replacement for virHashTable without code modification (glib's functions don't accept NULL hash table, ours do) the next step will be to use virHashTable as a shim to provide NULL compatibility and adapt to our slightly different style of iterators. For this step we modify the variable type for the key to be 'char *' as there's no other option left and remove various internals which won't be compatible with GHashTable. Second part (WIP, [1]) will then rewrite virHashTable internals to use GHashTable, which will be used as an intermediate step before removal which requires audit of all callers. [1]: https://gitlab.com/pipo.sk/libvirt/-/commits/glib-hash-part2 - needs auditing of all callers of virHashForeach to ensure that they don't modify the hash table itself from the callback Peter Krempa (15): virCgroupKillRecursive: Return -1 on failure condition util: virhash: Remove virHashTableSize util: cgroup: Use GHashTable instead of virHashTable virCgroupKillRecursive: Refactor cleanup conf: domain_addr: Refactor hash usage in zpci reservation code virHashAtomicNew: Remove 'size' argument conf: nwfilter: Replace 'virNWFilterHashTableCreate' with 'virHashNew' tests: hash: Prepare for replacement of virHashCreate qemuDomainObjPrivateAlloc: Use virHashNew instead of virHashCreate Replace all instances of 'virHashCreate' with 'virHashNew' util: hash: Remove virHashValueFree util: hash: Remove virHashCreateFull util: hash: Change type of hash table name/key to 'char' util: virhash: Remove key handling callbacks virHashRemoveAll: Don't return number of removed items src/conf/domain_addr.c| 125 +++ src/conf/domain_addr.h| 4 +- src/conf/domain_conf.c| 4 +- src/conf/domain_nwfilter.c| 2 +- src/conf/nwfilter_ipaddrmap.c | 2 +- src/conf/nwfilter_params.c| 14 +- src/conf/nwfilter_params.h| 2 +- src/conf/virchrdev.c | 4 +- src/conf/virdomainmomentobjlist.c | 8 +- src/conf/virdomainobjlist.c | 16 +- src/conf/virinterfaceobj.c| 12 +- src/conf/virnetworkobj.c | 21 +- src/conf/virnodedeviceobj.c | 20 +- src/conf/virnwfilterbindingdef.c | 2 +- src/conf/virnwfilterbindingobjlist.c | 6 +- src/conf/virsecretobj.c | 10 +- src/conf/virstorageobj.c | 32 +-- src/hyperv/hyperv_wmi.c | 2 +- src/hypervisor/virclosecallbacks.c| 4 +- src/libvirt_private.syms | 5 +- src/libxl/libxl_logger.c | 2 +- src/locking/lock_daemon.c | 10 +- src/nwfilter/nwfilter_dhcpsnoop.c | 12 +- src/nwfilter/nwfilter_ebiptables_driver.c | 4 +- src/nwfilter/nwfilter_gentech_driver.c| 14 +- src/nwfilter/nwfilter_learnipaddr.c | 4 +- src/qemu/qemu_block.c | 6 +- src/qemu/qemu_blockjob.c | 2 +- src/qemu/qemu_capabilities.c | 6 +- src/qemu/qemu_checkpoint.c| 2 +- src/qemu/qemu_domain.c| 8 +- src/qemu/qemu_domain.h| 2 +- src/qemu/qemu_driver.c| 2 +- src/qemu/qemu_interop_config.c| 2 +- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_migration_cookie.c | 2 +- src/qemu/qemu_monitor.c | 10 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_qapi.c | 2 +- src/qemu/qemu_snapshot.c | 4 +- src/rpc/virnetdaemon.c| 16 +- src/security/security_selinux.c | 4 +- src/test/test_driver.c| 6 +- src/util/vircgroup.c | 76 ++- src/util/vircgroupbackend.h | 3 +- src/util/vircgrouppriv.h | 2 +- src/util/vircgroupv1.c| 2 +- src/util/vircgroupv2.c| 2 +- src/util/virfilecache.c | 4 +- src/util/virhash.c| 201 +++--- src/util/virhash.h| 94 ++-- src/util/viriptables.c| 4 +- src/util/virlockspace.c | 8 +- src/util/virmacmap.c
[PATCH 09/15] qemuDomainObjPrivateAlloc: Use virHashNew instead of virHashCreate
virHashCreate will be removed in upcoming patches. This change has an impact on ordering of the blockjob entries in one of the status XML->XML tests. Signed-off-by: Peter Krempa --- src/qemu/qemu_domain.c| 2 +- .../blockjob-blockdev-in.xml | 110 +- 2 files changed, 56 insertions(+), 56 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index eeceb44348..bea43a1aba 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1697,7 +1697,7 @@ qemuDomainObjPrivateAlloc(void *opaque) if (!(priv->devs = virChrdevAlloc())) goto error; -if (!(priv->blockjobs = virHashCreate(5, virObjectFreeHashData))) +if (!(priv->blockjobs = virHashNew(virObjectFreeHashData))) goto error; /* agent commands block by default, user can choose different behavior */ diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml index ca6d110179..8ffc91bdcb 100644 --- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml +++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml @@ -244,64 +244,9 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @@ -339,6 +284,61 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + -2 -- 2.26.2
Re: [libvirt PATCH] qemu: fix potential resource leak
On Wed, Oct 21, 2020 at 22:31:09 -0400, Laine Stump wrote:
> On 10/21/20 5:50 PM, Jonathon Jongsma wrote:
> > Coverity reported a potential resource leak. While it's probably not
> > a real-world scenario, the code could technically jump to cleanup
> > between the time that vdpafd is opened and when it is used. Ensure that
> > it gets cleaned up in that case.
> >
> > Signed-off-by: Jonathon Jongsma
> > ---
> > src/qemu/qemu_command.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 5c4e37bd9e..cbe7a6e331 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -8135,6 +8135,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
> > addfdarg = g_strdup_printf("%s,opaque=%s", fdset,
> > net->data.vdpa.devicepath);
> > virCommandAddArgList(cmd, "-add-fd", addfdarg, NULL);
> > +vdpafd = -1;
> > }
> > if (chardev)
> > @@ -8204,6 +8205,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
> > VIR_FREE(tapfdName);
> > VIR_FREE(vhostfd);
> > VIR_FREE(tapfd);
> > +if (vdpafd >= 0)
> > +VIR_FORCE_CLOSE(vdpafd);
>
>
> VIR_FORCE_CLOSE() ==>virForceCloseHelper() ==> virFileClose()
>
> and virFileClose() is a NOP if fd < 0, so this doesn't need the conditional.
>
>
> I *was* going to say "With that change,
>
>
> Reviewed-by: Laine Stump
>
> "
>
>
> but this got me looking at the code of the entire function rather than just
> the diffs in the patch, and I've got a question - is there any reason that
> you only ope n the vdpa device inside the switch, and save everything else
> related to it until later (at the "if (vdpafd)")? You could then device
I'd like to point out that opening anything in the command line
formatters is IMO bad practice. The command line formatter should format
the commandline and nothing more. This is visible by the necessity to
have a lot of mock override functions which prevent the command line
formatter from touching resources on the host in the first place.
Better approach is to open resources in 'qemuProcessPrepareHost' and
store them in private data for command line formatting. Fake data for
tests are added in 'testCompareXMLToArgvCreateArgs'.
I'm aware though that there's a lot of "prior art" in this area though.
