[libvirt] [PATCH libvirt v2 0/9] Fix virConnectRegisterCloseCallback and get rid of global variables
7:40 a.m.
The first part of this patch series fixes the behavior of
virConnectRegisterCloseCallback and converts the testDriver to
virObjectLockable.
The subsequent patches remove the need to have the global variables
'qemuProgram' and 'remoteProgram' in libvirtd.[ch]. They only work in
combination with the fixed behavior of virConnectRegisterCloseCallback.
Changelog:
+ v1->v2:
- Removed accepted patches
- Removed NACKed patches
- Added r-b to patch 5
- Worked in comments
- Rebased
- Added patches 7-9
Marc Hartmayer (9):
virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no
connectRegisterCloseCallback
test: Convert testDriver to virObjectLockable
remote: Add the information which program has to be used to
daemonClientEventCallback
remote: Use domainClientEventCallbacks for
remoteReplayConnectionClosedEvent
rpc: Introduce virNetServerGetProgramLocked helper function
remote/rpc: Use virNetServerGetProgram() to determine the program
rpc: use the return value of virObjectRef directly
remote: remove ATTRIBUTE_UNUSED when attribute is actually used
remote: shrink the critical sections
src/libvirt-host.c | 10 +-
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 3 -
src/remote/remote_daemon_dispatch.c | 264 +++++++++++++++++++++++-------------
src/rpc/gendispatch.pl | 6 +
src/rpc/virnetserver.c | 57 ++++++--
src/rpc/virnetserver.h | 2 +
src/test/test_driver.c | 200 ++++++++++++---------------
9 files changed, 328 insertions(+), 219 deletions(-)
--
2.13.4
7:40 a.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
The commit 'close callback: move it to driver' (88f09b75eb99) moved
the responsibility for the close callback to the driver. But if the
driver doesn't support the connectRegisterCloseCallback API this
function does nothing, even no unsupported error report. This behavior
may lead to problems, for example memory leaks, as the caller cannot
differentiate whether the close callback was 'really' registered or
not.
Therefore, call directly @freecb if the connectRegisterCloseCallback
API is not supported by the driver used by the connection.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/libvirt-host.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 7ff7407a0874..cb2ace7d9778 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1221,9 +1221,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
virCheckNonNullArgGoto(cb, error);
- if (conn->driver->connectRegisterCloseCallback &&
- conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) <
0)
- goto error;
+ if (conn->driver->connectRegisterCloseCallback) {
+ if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb)
< 0)
+ goto error;
+ } else {
+ if (freecb)
+ freecb(opaque);
+ }
return 0;
--
2.13.4
10:06 a.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
The commit 'close callback: move it to driver' (88f09b75eb99)
moved
the responsibility for the close callback to the driver. But if the
driver doesn't support the connectRegisterCloseCallback API this
function does nothing, even no unsupported error report. This behavior
may lead to problems, for example memory leaks, as the caller cannot
differentiate whether the close callback was 'really' registered or
not.
Therefore, call directly @freecb if the connectRegisterCloseCallback
API is not supported by the driver used by the connection.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/libvirt-host.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 7ff7407a0874..cb2ace7d9778 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1221,9 +1221,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
virCheckNonNullArgGoto(cb, error);
- if (conn->driver->connectRegisterCloseCallback &&
- conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) <
0)
- goto error;
+ if (conn->driver->connectRegisterCloseCallback) {
+ if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb)
< 0)
+ goto error;
+ } else {
+ if (freecb)
+ freecb(opaque);
+ }
I see this follows what Daniel suggests from v1:
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
but I guess it still baffles me about calling @freecb w/ @opaque. If
there was a @freecb routine supplied it'd be called and free @opaque
which may actually be used afterwards - after all this is a
RegisterCloseCallback which would conceptually be called after open, but
before perhaps using the @opaque. E.G., the virsh code uses this - with
virshReconnect being called from virshInit before entering the loop
processing commands. So if @ctl was free'd - things wouldn't be good!
Running in debug using '-c test:///default' dumps you into the else.
I would think a little loss of memory is better than using memory you
didn't expect to be free'd when virConnectRegisterCloseCallback was
successful.
John
return 0;
11:09 a.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
> The commit 'close callback: move it to driver' (88f09b75eb99) moved
> the responsibility for the close callback to the driver. But if the
> driver doesn't support the connectRegisterCloseCallback API this
> function does nothing, even no unsupported error report. This behavior
> may lead to problems, for example memory leaks, as the caller cannot
> differentiate whether the close callback was 'really' registered or
> not.
>
> Therefore, call directly @freecb if the connectRegisterCloseCallback
> API is not supported by the driver used by the connection.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> ---
> src/libvirt-host.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
> index 7ff7407a0874..cb2ace7d9778 100644
> --- a/src/libvirt-host.c
> +++ b/src/libvirt-host.c
> @@ -1221,9 +1221,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
> virCheckConnectReturn(conn, -1);
> virCheckNonNullArgGoto(cb, error);
>
> - if (conn->driver->connectRegisterCloseCallback &&
> - conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb)
< 0)
> - goto error;
> + if (conn->driver->connectRegisterCloseCallback) {
> + if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
freecb) < 0)
> + goto error;
> + } else {
> + if (freecb)
> + freecb(opaque);
> + }
I see this follows what Daniel suggests from v1:
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
but I guess it still baffles me about calling @freecb w/ @opaque. If
there was a @freecb routine supplied it'd be called and free @opaque
which may actually be used afterwards - after all this is a
RegisterCloseCallback which would conceptually be called after open, but
before perhaps using the @opaque.
I understand your concerns.
E.G., the virsh code uses this - with
virshReconnect being called from virshInit before entering the loop
processing commands. So if @ctl was free'd - things wouldn't be good!
Running in debug using '-c test:///default' dumps you into the else.
virshReconnect doesn’t pass a @freecb != NULL argument, does it? So the
registered callback has not the responsibility to free the memory.
But I can understand, there is surely someone who might think "hey, the
loop is not running yet, the call was successful => I can still use the
data".
I would think a little loss of memory is better than using memory you
didn't expect to be free'd when virConnectRegisterCloseCallback was
successful.
Yes, definitely. But a memory leak is still a memory leak… So I'd like
it fixed :) Especially, as 'remoteDispatchConnectSupportsFeature' has an
hard coded “VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK” is supported. If we
would change this (as I suggested in v1), at least our remote_driver
would be fenced against a memory leak (even without this patch). But
this would be an API change :(
doRemoteOpen:
…
priv->serverCloseCallback = remoteConnectSupportsFeatureUnlocked(conn,
priv,
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK);
if (!priv->serverCloseCallback) {
VIR_INFO("Close callback registering isn't supported "
"by the remote side.");
}
…
Thanks for taking a look!
John
>
> return 0;
>
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
7:16 p.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On 04/26/2018 12:09 PM, Marc Hartmayer wrote:
On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan
<jferlan(a)redhat.com> wrote:
> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>> The commit 'close callback: move it to driver' (88f09b75eb99) moved
>> the responsibility for the close callback to the driver. But if the
>> driver doesn't support the connectRegisterCloseCallback API this
>> function does nothing, even no unsupported error report. This behavior
>> may lead to problems, for example memory leaks, as the caller cannot
>> differentiate whether the close callback was 'really' registered or
>> not.
>>
>> Therefore, call directly @freecb if the connectRegisterCloseCallback
>> API is not supported by the driver used by the connection.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>> ---
>> src/libvirt-host.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
>> index 7ff7407a0874..cb2ace7d9778 100644
>> --- a/src/libvirt-host.c
>> +++ b/src/libvirt-host.c
>> @@ -1221,9 +1221,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
>> virCheckConnectReturn(conn, -1);
>> virCheckNonNullArgGoto(cb, error);
>>
>> - if (conn->driver->connectRegisterCloseCallback &&
>> - conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
freecb) < 0)
>> - goto error;
>> + if (conn->driver->connectRegisterCloseCallback) {
>> + if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
freecb) < 0)
>> + goto error;
>> + } else {
>> + if (freecb)
>> + freecb(opaque);
>> + }
>
> I see this follows what Daniel suggests from v1:
>
> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>
> but I guess it still baffles me about calling @freecb w/ @opaque. If
> there was a @freecb routine supplied it'd be called and free @opaque
> which may actually be used afterwards - after all this is a
> RegisterCloseCallback which would conceptually be called after open, but
> before perhaps using the @opaque.
I understand your concerns.
> E.G., the virsh code uses this - with
> virshReconnect being called from virshInit before entering the loop
> processing commands. So if @ctl was free'd - things wouldn't be good!
> Running in debug using '-c test:///default' dumps you into the else.
virshReconnect doesn’t pass a @freecb != NULL argument, does it? So the
registered callback has not the responsibility to free the memory.
True virsh uses NULL so it's fine; however, I was thinking about more
generically - why would a Register routine with a callback to free
memory free the memory upon successful register.
I'm still not sure I understand why the API cannot return a failure, but
Daniel says it cannot.
I'm really not sure what to do - maybe Daniel will pipe it - I'll try to
ping him in the morning...
John
But I can understand, there is surely someone who might think
"hey, the
loop is not running yet, the call was successful => I can still use the
data".
>
> I would think a little loss of memory is better than using memory you
> didn't expect to be free'd when virConnectRegisterCloseCallback was
> successful.
Yes, definitely. But a memory leak is still a memory leak… So I'd like
it fixed :) Especially, as 'remoteDispatchConnectSupportsFeature' has an
hard coded “VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK” is supported. If we
would change this (as I suggested in v1), at least our remote_driver
would be fenced against a memory leak (even without this patch). But
this would be an API change :(
doRemoteOpen:
…
priv->serverCloseCallback = remoteConnectSupportsFeatureUnlocked(conn,
priv,
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK);
if (!priv->serverCloseCallback) {
VIR_INFO("Close callback registering isn't supported "
"by the remote side.");
}
…
Thanks for taking a look!
>
> John
>
>>
>> return 0;
>>
>>
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
12:59 a.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Fri, Apr 27, 2018 at 02:16 AM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 04/26/2018 12:09 PM, Marc Hartmayer wrote:
> On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan(a)redhat.com>
wrote:
>> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>>> The commit 'close callback: move it to driver' (88f09b75eb99) moved
>>> the responsibility for the close callback to the driver. But if the
>>> driver doesn't support the connectRegisterCloseCallback API this
>>> function does nothing, even no unsupported error report. This behavior
>>> may lead to problems, for example memory leaks, as the caller cannot
>>> differentiate whether the close callback was 'really' registered or
>>> not.
>>>
>>> Therefore, call directly @freecb if the connectRegisterCloseCallback
>>> API is not supported by the driver used by the connection.
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>>> ---
>>> src/libvirt-host.c | 10 +++++++---
>>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
>>> index 7ff7407a0874..cb2ace7d9778 100644
>>> --- a/src/libvirt-host.c
>>> +++ b/src/libvirt-host.c
>>> @@ -1221,9 +1221,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
>>> virCheckConnectReturn(conn, -1);
>>> virCheckNonNullArgGoto(cb, error);
>>>
>>> - if (conn->driver->connectRegisterCloseCallback &&
>>> - conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
freecb) < 0)
>>> - goto error;
>>> + if (conn->driver->connectRegisterCloseCallback) {
>>> + if (conn->driver->connectRegisterCloseCallback(conn, cb,
opaque, freecb) < 0)
>>> + goto error;
>>> + } else {
>>> + if (freecb)
>>> + freecb(opaque);
>>> + }
>>
>> I see this follows what Daniel suggests from v1:
>>
>> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>>
>> but I guess it still baffles me about calling @freecb w/ @opaque. If
>> there was a @freecb routine supplied it'd be called and free @opaque
>> which may actually be used afterwards - after all this is a
>> RegisterCloseCallback which would conceptually be called after open, but
>> before perhaps using the @opaque.
>
> I understand your concerns.
>
>> E.G., the virsh code uses this - with
>> virshReconnect being called from virshInit before entering the loop
>> processing commands. So if @ctl was free'd - things wouldn't be good!
>> Running in debug using '-c test:///default' dumps you into the else.
>
> virshReconnect doesn’t pass a @freecb != NULL argument, does it? So the
> registered callback has not the responsibility to free the memory.
>
True virsh uses NULL so it's fine; however, I was thinking about more
generically - why would a Register routine with a callback to free
memory free the memory upon successful register.
I'm still not sure I understand why the API cannot return a failure, but
Daniel says it cannot.
I'm really not sure what to do - maybe Daniel will pipe it - I'll try to
ping him in the morning...
John
Polite ping.
[…snip]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:17 a.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
[...]
>
> True virsh uses NULL so it's fine; however, I was thinking about more
> generically - why would a Register routine with a callback to free
> memory free the memory upon successful register.
>
> I'm still not sure I understand why the API cannot return a failure, but
> Daniel says it cannot.
>
> I'm really not sure what to do - maybe Daniel will pipe it - I'll try to
> ping him in the morning...
>
> John
Polite ping.
Sorry - I totally forgot about that... I did ping him today and
hopefully with this bump we can come to a resolution.
John
11:25 a.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Thu, Apr 26, 2018 at 08:16:54PM -0400, John Ferlan wrote:
On 04/26/2018 12:09 PM, Marc Hartmayer wrote:
> On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan(a)redhat.com>
wrote:
>> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>>> The commit 'close callback: move it to driver' (88f09b75eb99) moved
>>> the responsibility for the close callback to the driver. But if the
>>> driver doesn't support the connectRegisterCloseCallback API this
>>> function does nothing, even no unsupported error report. This behavior
>>> may lead to problems, for example memory leaks, as the caller cannot
>>> differentiate whether the close callback was 'really' registered or
>>> not.
>>>
>>> Therefore, call directly @freecb if the connectRegisterCloseCallback
>>> API is not supported by the driver used by the connection.
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>>> ---
>>> src/libvirt-host.c | 10 +++++++---
>>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
>>> index 7ff7407a0874..cb2ace7d9778 100644
>>> --- a/src/libvirt-host.c
>>> +++ b/src/libvirt-host.c
>>> @@ -1221,9 +1221,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
>>> virCheckConnectReturn(conn, -1);
>>> virCheckNonNullArgGoto(cb, error);
>>>
>>> - if (conn->driver->connectRegisterCloseCallback &&
>>> - conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
freecb) < 0)
>>> - goto error;
>>> + if (conn->driver->connectRegisterCloseCallback) {
>>> + if (conn->driver->connectRegisterCloseCallback(conn, cb,
opaque, freecb) < 0)
>>> + goto error;
>>> + } else {
>>> + if (freecb)
>>> + freecb(opaque);
>>> + }
>>
>> I see this follows what Daniel suggests from v1:
>>
>> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>>
>> but I guess it still baffles me about calling @freecb w/ @opaque. If
>> there was a @freecb routine supplied it'd be called and free @opaque
>> which may actually be used afterwards - after all this is a
>> RegisterCloseCallback which would conceptually be called after open, but
>> before perhaps using the @opaque.
>
> I understand your concerns.
>
>> E.G., the virsh code uses this - with
>> virshReconnect being called from virshInit before entering the loop
>> processing commands. So if @ctl was free'd - things wouldn't be good!
>> Running in debug using '-c test:///default' dumps you into the else.
>
> virshReconnect doesn’t pass a @freecb != NULL argument, does it? So the
> registered callback has not the responsibility to free the memory.
>
True virsh uses NULL so it's fine; however, I was thinking about more
generically - why would a Register routine with a callback to free
memory free the memory upon successful register.
Well the memory passed in 'opaque' is only required to be kept alive for
as long as the callback is registered, and only documented for use by the
callback.
If the application wants to access 'opaque' outside the context of the
callback function, it must take steps to ensure it is still alive in
whatever thread it using it. This implies the data passed for 'opaque'
should be ref-counted and they must hold a reference for their own
usage, separately from the reference assoicated with the callback that
will be released by @freecb.
That all said, we could take a slightly different approach if we want
to be paranoid about this
eg move the
virConnectCloseCallbackDataPtr closeCallback;
out of the driver specific private structs, and put it in the main
struct _virConnect instead.
The main libvirt-host.c can add the callback to this itself. THe
driver code only needs to worry about actually invoking the callback
That would allow us to have freecb() called at the right time for
all drivers, even if they don't ever use the close callback.
I'm still not sure I understand why the API cannot return a failure, but
Daniel says it cannot.
It can break existing applications using hypervisors that don't
implement this API, becasue its a change in behaviour. In retrospect
I wouldn't have written the API in this way today, but we must live
with the design we have.
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 :|
3:22 a.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Mon, Jun 04, 2018 at 06:25 PM +0200, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
On Thu, Apr 26, 2018 at 08:16:54PM -0400, John Ferlan wrote:
>
>
> On 04/26/2018 12:09 PM, Marc Hartmayer wrote:
> > On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan(a)redhat.com>
wrote:
> >> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
> >>> The commit 'close callback: move it to driver' (88f09b75eb99)
moved
> >>> the responsibility for the close callback to the driver. But if the
> >>> driver doesn't support the connectRegisterCloseCallback API this
> >>> function does nothing, even no unsupported error report. This behavior
> >>> may lead to problems, for example memory leaks, as the caller cannot
> >>> differentiate whether the close callback was 'really' registered
or
> >>> not.
> >>>
> >>> Therefore, call directly @freecb if the connectRegisterCloseCallback
> >>> API is not supported by the driver used by the connection.
> >>>
> >>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> >>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> >>> ---
> >>> src/libvirt-host.c | 10 +++++++---
> >>> 1 file changed, 7 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
> >>> index 7ff7407a0874..cb2ace7d9778 100644
> >>> --- a/src/libvirt-host.c
> >>> +++ b/src/libvirt-host.c
> >>> @@ -1221,9 +1221,13 @@ virConnectRegisterCloseCallback(virConnectPtr
conn,
> >>> virCheckConnectReturn(conn, -1);
> >>> virCheckNonNullArgGoto(cb, error);
> >>>
> >>> - if (conn->driver->connectRegisterCloseCallback &&
> >>> - conn->driver->connectRegisterCloseCallback(conn, cb,
opaque, freecb) < 0)
> >>> - goto error;
> >>> + if (conn->driver->connectRegisterCloseCallback) {
> >>> + if (conn->driver->connectRegisterCloseCallback(conn, cb,
opaque, freecb) < 0)
> >>> + goto error;
> >>> + } else {
> >>> + if (freecb)
> >>> + freecb(opaque);
> >>> + }
> >>
> >> I see this follows what Daniel suggests from v1:
> >>
> >> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
> >>
> >> but I guess it still baffles me about calling @freecb w/ @opaque. If
> >> there was a @freecb routine supplied it'd be called and free @opaque
> >> which may actually be used afterwards - after all this is a
> >> RegisterCloseCallback which would conceptually be called after open, but
> >> before perhaps using the @opaque.
> >
> > I understand your concerns.
> >
> >> E.G., the virsh code uses this - with
> >> virshReconnect being called from virshInit before entering the loop
> >> processing commands. So if @ctl was free'd - things wouldn't be
good!
> >> Running in debug using '-c test:///default' dumps you into the
else.
> >
> > virshReconnect doesn’t pass a @freecb != NULL argument, does it? So the
> > registered callback has not the responsibility to free the memory.
> >
>
> True virsh uses NULL so it's fine; however, I was thinking about more
> generically - why would a Register routine with a callback to free
> memory free the memory upon successful register.
Well the memory passed in 'opaque' is only required to be kept alive for
as long as the callback is registered, and only documented for use by the
callback.
If the application wants to access 'opaque' outside the context of the
callback function, it must take steps to ensure it is still alive in
whatever thread it using it. This implies the data passed for 'opaque'
should be ref-counted and they must hold a reference for their own
usage, separately from the reference assoicated with the callback that
will be released by @freecb.
That all said, we could take a slightly different approach if we want
to be paranoid about this
eg move the
virConnectCloseCallbackDataPtr closeCallback;
out of the driver specific private structs, and put it in the main
struct _virConnect instead.
This sound like a revert of commit “close callback: move it to driver”
(88f09b75eb99415c). Shall we really do this?
The main libvirt-host.c can add the callback to this itself. THe
driver code only needs to worry about actually invoking the callback
That would allow us to have freecb() called at the right time for
all drivers, even if they don't ever use the close callback.
>
> I'm still not sure I understand why the API cannot return a failure, but
> Daniel says it cannot.
It can break existing applications using hypervisors that don't
implement this API, becasue its a change in behaviour. In retrospect
I wouldn't have written the API in this way today, but we must live
with the design we have.
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 :|
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:40 a.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Wed, Jun 13, 2018 at 10:22 AM +0200, Marc Hartmayer <mhartmay(a)linux.ibm.com>
wrote:
On Mon, Jun 04, 2018 at 06:25 PM +0200, "Daniel P.
Berrangé" <berrange(a)redhat.com> wrote:
> On Thu, Apr 26, 2018 at 08:16:54PM -0400, John Ferlan wrote:
[…snip…]
>
> If the application wants to access 'opaque' outside the context of the
> callback function, it must take steps to ensure it is still alive in
> whatever thread it using it. This implies the data passed for 'opaque'
> should be ref-counted and they must hold a reference for their own
> usage, separately from the reference assoicated with the callback that
> will be released by @freecb.
>
> That all said, we could take a slightly different approach if we want
> to be paranoid about this
>
> eg move the
>
> virConnectCloseCallbackDataPtr closeCallback;
>
> out of the driver specific private structs, and put it in the main
> struct _virConnect instead.
This sound like a revert of commit “close callback: move it to driver”
(88f09b75eb99415c). Shall we really do this?
Polite ping.
>
> The main libvirt-host.c can add the callback to this itself. THe
> driver code only needs to worry about actually invoking the callback
>
> That would allow us to have freecb() called at the right time for
> all drivers, even if they don't ever use the close callback.
>
>>
>> I'm still not sure I understand why the API cannot return a failure, but
>> Daniel says it cannot.
>
> It can break existing applications using hypervisors that don't
> implement this API, becasue its a change in behaviour. In retrospect
> I wouldn't have written the API in this way today, but we must live
> with the design we have.
>
>
> 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 :|
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:53 a.m.
New subject: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Tue, Aug 07, 2018 at 06:40:46PM +0200, Marc Hartmayer wrote:
On Wed, Jun 13, 2018 at 10:22 AM +0200, Marc Hartmayer
<mhartmay(a)linux.ibm.com> wrote:
> On Mon, Jun 04, 2018 at 06:25 PM +0200, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
>> On Thu, Apr 26, 2018 at 08:16:54PM -0400, John Ferlan wrote:
[…snip…]
>>
>> If the application wants to access 'opaque' outside the context of the
>> callback function, it must take steps to ensure it is still alive in
>> whatever thread it using it. This implies the data passed for 'opaque'
>> should be ref-counted and they must hold a reference for their own
>> usage, separately from the reference assoicated with the callback that
>> will be released by @freecb.
>>
>> That all said, we could take a slightly different approach if we want
>> to be paranoid about this
>>
>> eg move the
>>
>> virConnectCloseCallbackDataPtr closeCallback;
>>
>> out of the driver specific private structs, and put it in the main
>> struct _virConnect instead.
>
> This sound like a revert of commit “close callback: move it to driver”
> (88f09b75eb99415c). Shall we really do this?
Polite ping.
It is mostly a revert i think
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 :|
7:40 a.m.
New subject: [libvirt] [PATCH libvirt v2 2/9] test: Convert testDriver to virObjectLockable
The test driver state (@testDriver) uses it's own reference counting
and locking implementation. Instead of doing that, convert @testDriver
into a virObjectLockable and use the provided functionalities.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 200 ++++++++++++++++++++++---------------------------
1 file changed, 90 insertions(+), 110 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 856869c9d3a9..1a219316e7c5 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -94,7 +94,7 @@ typedef struct _testAuth testAuth;
typedef struct _testAuth *testAuthPtr;
struct _testDriver {
- virMutex lock;
+ virObjectLockable parent;
virNodeInfo nodeInfo;
virInterfaceObjListPtr ifaces;
@@ -126,9 +126,22 @@ typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
static testDriverPtr defaultPrivconn;
-static int defaultConnections;
static virMutex defaultLock = VIR_MUTEX_INITIALIZER;
+static virClassPtr testDriverClass;
+static void testDriverDispose(void *obj);
+static int testDriverOnceInit(void)
+{
+ if (!(testDriverClass = virClassNew(virClassForObjectLockable(),
+ "testDriver",
+ sizeof(testDriver),
+ testDriverDispose)))
+ return -1;
+
+ return 0;
+}
+VIR_ONCE_GLOBAL_INIT(testDriver)
+
#define TEST_MODEL "i686"
#define TEST_EMULATOR "/usr/bin/test-hv"
@@ -144,10 +157,9 @@ static const virNodeInfo defaultNodeInfo = {
};
static void
-testDriverFree(testDriverPtr driver)
+testDriverDispose(void *obj)
{
- if (!driver)
- return;
+ testDriverPtr driver = obj;
virObjectUnref(driver->caps);
virObjectUnref(driver->xmlopt);
@@ -157,23 +169,9 @@ testDriverFree(testDriverPtr driver)
virObjectUnref(driver->ifaces);
virObjectUnref(driver->pools);
virObjectUnref(driver->eventState);
- virMutexUnlock(&driver->lock);
- virMutexDestroy(&driver->lock);
-
- VIR_FREE(driver);
}
-static void testDriverLock(testDriverPtr driver)
-{
- virMutexLock(&driver->lock);
-}
-
-static void testDriverUnlock(testDriverPtr driver)
-{
- virMutexUnlock(&driver->lock);
-}
-
static void testObjectEventQueue(testDriverPtr driver,
virObjectEventPtr event)
{
@@ -405,14 +403,11 @@ testDriverNew(void)
};
testDriverPtr ret;
- if (VIR_ALLOC(ret) < 0)
+ if (testDriverInitialize() < 0)
return NULL;
- if (virMutexInit(&ret->lock) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("cannot initialize mutex"));
- goto error;
- }
+ if (!(ret = virObjectLockableNew(testDriverClass)))
+ return NULL;
if (!(ret->xmlopt = virDomainXMLOptionNew(NULL, NULL, &ns, NULL, NULL)) ||
!(ret->eventState = virObjectEventStateNew()) ||
@@ -428,7 +423,7 @@ testDriverNew(void)
return ret;
error:
- testDriverFree(ret);
+ virObjectUnref(ret);
return NULL;
}
@@ -1262,7 +1257,7 @@ testOpenFromFile(virConnectPtr conn, const char *file)
if (!(privconn = testDriverNew()))
return VIR_DRV_OPEN_ERROR;
- testDriverLock(privconn);
+ virObjectLock(privconn);
conn->privateData = privconn;
if (!(privconn->caps = testBuildCapabilities(conn)))
@@ -1279,14 +1274,14 @@ testOpenFromFile(virConnectPtr conn, const char *file)
xmlXPathFreeContext(ctxt);
xmlFreeDoc(doc);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return VIR_DRV_OPEN_SUCCESS;
error:
xmlXPathFreeContext(ctxt);
xmlFreeDoc(doc);
- testDriverFree(privconn);
+ virObjectUnref(privconn);
conn->privateData = NULL;
return VIR_DRV_OPEN_ERROR;
}
@@ -1304,8 +1299,8 @@ testOpenDefault(virConnectPtr conn)
size_t i;
virMutexLock(&defaultLock);
- if (defaultConnections++) {
- conn->privateData = defaultPrivconn;
+ if (defaultPrivconn) {
+ conn->privateData = virObjectRef(defaultPrivconn);
virMutexUnlock(&defaultLock);
return VIR_DRV_OPEN_SUCCESS;
}
@@ -1354,9 +1349,8 @@ testOpenDefault(virConnectPtr conn)
return ret;
error:
- testDriverFree(privconn);
+ virObjectUnref(privconn);
conn->privateData = NULL;
- defaultConnections--;
goto cleanup;
}
@@ -1369,9 +1363,9 @@ testConnectAuthenticate(virConnectPtr conn,
ssize_t i;
char *username = NULL, *password = NULL;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (privconn->numAuths == 0) {
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return 0;
}
@@ -1413,7 +1407,7 @@ testConnectAuthenticate(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
VIR_FREE(username);
VIR_FREE(password);
return ret;
@@ -1423,24 +1417,11 @@ testConnectAuthenticate(virConnectPtr conn,
static void
testDriverCloseInternal(testDriverPtr driver)
{
- bool dflt = false;
-
- if (driver == defaultPrivconn) {
- dflt = true;
- virMutexLock(&defaultLock);
- if (--defaultConnections) {
- virMutexUnlock(&defaultLock);
- return;
- }
- }
-
- testDriverLock(driver);
- testDriverFree(driver);
-
- if (dflt) {
+ virMutexLock(&defaultLock);
+ bool disposed = !virObjectUnref(driver);
+ if (disposed && driver == defaultPrivconn)
defaultPrivconn = NULL;
- virMutexUnlock(&defaultLock);
- }
+ virMutexUnlock(&defaultLock);
}
@@ -1571,9 +1552,9 @@ static int testNodeGetInfo(virConnectPtr conn,
virNodeInfoPtr info)
{
testDriverPtr privconn = conn->privateData;
- testDriverLock(privconn);
+ virObjectLock(privconn);
memcpy(info, &privconn->nodeInfo, sizeof(virNodeInfo));
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return 0;
}
@@ -1581,9 +1562,9 @@ static char *testConnectGetCapabilities(virConnectPtr conn)
{
testDriverPtr privconn = conn->privateData;
char *xml;
- testDriverLock(privconn);
+ virObjectLock(privconn);
xml = virCapabilitiesFormatXML(privconn->caps);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return xml;
}
@@ -1618,9 +1599,9 @@ static int testConnectNumOfDomains(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int count;
- testDriverLock(privconn);
+ virObjectLock(privconn);
count = virDomainObjListNumOfDomains(privconn->domains, true, NULL, NULL);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return count;
}
@@ -1673,7 +1654,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
if (flags & VIR_DOMAIN_START_VALIDATE)
parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if ((def = virDomainDefParseString(xml, privconn->caps, privconn->xmlopt,
NULL, parse_flags)) == NULL)
goto cleanup;
@@ -1708,7 +1689,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
virObjectUnlock(dom);
testObjectEventQueue(privconn, event);
virDomainDefFree(def);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -2891,7 +2872,7 @@ static int testNodeGetCellsFreeMemory(virConnectPtr conn,
size_t i;
int ret = -1;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (startCell >= privconn->numCells) {
virReportError(VIR_ERR_INVALID_ARG,
"%s", _("Range exceeds available cells"));
@@ -2906,7 +2887,7 @@ static int testNodeGetCellsFreeMemory(virConnectPtr conn,
ret = i;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -2964,12 +2945,12 @@ testNodeGetFreeMemory(virConnectPtr conn)
unsigned int freeMem = 0;
size_t i;
- testDriverLock(privconn);
+ virObjectLock(privconn);
for (i = 0; i < privconn->numCells; i++)
freeMem += privconn->cells[i].freeMem;
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return freeMem;
}
@@ -3006,7 +2987,7 @@ static int testDomainCreateWithFlags(virDomainPtr domain, unsigned
int flags)
virCheckFlags(0, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!(privdom = testDomObjFromDomain(domain)))
goto cleanup;
@@ -3030,7 +3011,7 @@ static int testDomainCreateWithFlags(virDomainPtr domain, unsigned
int flags)
cleanup:
virDomainObjEndAPI(&privdom);
testObjectEventQueue(privconn, event);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -3803,9 +3784,9 @@ testInterfaceObjFindByName(testDriverPtr privconn,
{
virInterfaceObjPtr obj;
- testDriverLock(privconn);
+ virObjectLock(privconn);
obj = virInterfaceObjListFindByName(privconn->ifaces, name);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!obj)
virReportError(VIR_ERR_NO_INTERFACE,
@@ -3822,9 +3803,9 @@ testConnectNumOfInterfaces(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int ninterfaces;
- testDriverLock(privconn);
+ virObjectLock(privconn);
ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, true);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ninterfaces;
}
@@ -3837,10 +3818,10 @@ testConnectListInterfaces(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
int nnames;
- testDriverLock(privconn);
+ virObjectLock(privconn);
nnames = virInterfaceObjListGetNames(privconn->ifaces, true,
names, maxnames);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return nnames;
}
@@ -3852,9 +3833,9 @@ testConnectNumOfDefinedInterfaces(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int ninterfaces;
- testDriverLock(privconn);
+ virObjectLock(privconn);
ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, false);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ninterfaces;
}
@@ -3867,10 +3848,10 @@ testConnectListDefinedInterfaces(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
int nnames;
- testDriverLock(privconn);
+ virObjectLock(privconn);
nnames = virInterfaceObjListGetNames(privconn->ifaces, false,
names, maxnames);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return nnames;
}
@@ -3905,10 +3886,10 @@ testInterfaceLookupByMACString(virConnectPtr conn,
char *ifacenames[] = { NULL, NULL };
virInterfacePtr ret = NULL;
- testDriverLock(privconn);
+ virObjectLock(privconn);
ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac,
ifacenames, 2);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (ifacect == 0) {
virReportError(VIR_ERR_NO_INTERFACE,
@@ -3956,7 +3937,7 @@ testInterfaceChangeBegin(virConnectPtr conn,
virCheckFlags(0, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (privconn->transaction_running) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("there is another transaction running."));
@@ -3970,7 +3951,7 @@ testInterfaceChangeBegin(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -3984,7 +3965,7 @@ testInterfaceChangeCommit(virConnectPtr conn,
virCheckFlags(0, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!privconn->transaction_running) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -3999,7 +3980,7 @@ testInterfaceChangeCommit(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -4014,7 +3995,7 @@ testInterfaceChangeRollback(virConnectPtr conn,
virCheckFlags(0, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!privconn->transaction_running) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -4032,7 +4013,7 @@ testInterfaceChangeRollback(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -4072,7 +4053,7 @@ testInterfaceDefineXML(virConnectPtr conn,
virCheckFlags(0, NULL);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if ((def = virInterfaceDefParseString(xmlStr)) == NULL)
goto cleanup;
@@ -4086,7 +4067,7 @@ testInterfaceDefineXML(virConnectPtr conn,
cleanup:
virInterfaceDefFree(def);
virInterfaceObjEndAPI(&obj);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -4190,9 +4171,9 @@ testStoragePoolObjFindByName(testDriverPtr privconn,
{
virStoragePoolObjPtr obj;
- testDriverLock(privconn);
+ virObjectLock(privconn);
obj = virStoragePoolObjFindByName(privconn->pools, name);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!obj)
virReportError(VIR_ERR_NO_STORAGE_POOL,
@@ -4250,9 +4231,9 @@ testStoragePoolObjFindByUUID(testDriverPtr privconn,
virStoragePoolObjPtr obj;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- testDriverLock(privconn);
+ virObjectLock(privconn);
obj = virStoragePoolObjFindByUUID(privconn->pools, uuid);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!obj) {
virUUIDFormat(uuid, uuidstr);
@@ -4318,10 +4299,10 @@ testConnectNumOfStoragePools(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int numActive = 0;
- testDriverLock(privconn);
+ virObjectLock(privconn);
numActive = virStoragePoolObjNumOfStoragePools(privconn->pools, conn,
true, NULL);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return numActive;
}
@@ -4335,10 +4316,10 @@ testConnectListStoragePools(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
int n = 0;
- testDriverLock(privconn);
+ virObjectLock(privconn);
n = virStoragePoolObjGetNames(privconn->pools, conn, true, NULL,
names, maxnames);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return n;
}
@@ -4350,10 +4331,10 @@ testConnectNumOfDefinedStoragePools(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int numInactive = 0;
- testDriverLock(privconn);
+ virObjectLock(privconn);
numInactive = virStoragePoolObjNumOfStoragePools(privconn->pools, conn,
false, NULL);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return numInactive;
}
@@ -4367,10 +4348,10 @@ testConnectListDefinedStoragePools(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
int n = 0;
- testDriverLock(privconn);
+ virObjectLock(privconn);
n = virStoragePoolObjGetNames(privconn->pools, conn, false, NULL,
names, maxnames);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return n;
}
@@ -4386,10 +4367,10 @@ testConnectListAllStoragePools(virConnectPtr conn,
virCheckFlags(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ALL, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
ret = virStoragePoolObjListExport(conn, privconn->pools, pools,
NULL, flags);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -4551,7 +4532,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
virCheckFlags(0, NULL);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!(newDef = virStoragePoolDefParseString(xml)))
goto cleanup;
@@ -4602,7 +4583,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
virStoragePoolDefFree(newDef);
testObjectEventQueue(privconn, event);
virStoragePoolObjEndAPI(&obj);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return pool;
}
@@ -4621,7 +4602,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
virCheckFlags(0, NULL);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!(newDef = virStoragePoolDefParseString(xml)))
goto cleanup;
@@ -4654,7 +4635,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
virStoragePoolDefFree(newDef);
testObjectEventQueue(privconn, event);
virStoragePoolObjEndAPI(&obj);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return pool;
}
@@ -5055,7 +5036,7 @@ testStorageVolLookupByKey(virConnectPtr conn,
.key = key, .voldef = NULL };
virStorageVolPtr vol = NULL;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if ((obj = virStoragePoolObjListSearch(privconn->pools,
testStorageVolLookupByKeyCallback,
&data)) && data.voldef) {
@@ -5065,7 +5046,7 @@ testStorageVolLookupByKey(virConnectPtr conn,
NULL, NULL);
virStoragePoolObjEndAPI(&obj);
}
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!vol)
virReportError(VIR_ERR_NO_STORAGE_VOL,
@@ -5099,7 +5080,7 @@ testStorageVolLookupByPath(virConnectPtr conn,
.path = path, .voldef = NULL };
virStorageVolPtr vol = NULL;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if ((obj = virStoragePoolObjListSearch(privconn->pools,
testStorageVolLookupByPathCallback,
&data)) && data.voldef) {
@@ -5109,7 +5090,7 @@ testStorageVolLookupByPath(virConnectPtr conn,
NULL, NULL);
virStoragePoolObjEndAPI(&obj);
}
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!vol)
virReportError(VIR_ERR_NO_STORAGE_VOL,
@@ -6859,7 +6840,6 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
-
static virHypervisorDriver testHypervisorDriver = {
.name = "Test",
.connectOpen = testConnectOpen, /* 0.1.1 */
--
2.13.4
10:07 a.m.
New subject: [libvirt] [PATCH libvirt v2 2/9] test: Convert testDriver to virObjectLockable
On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
The test driver state (@testDriver) uses it's own reference
counting
and locking implementation. Instead of doing that, convert @testDriver
into a virObjectLockable and use the provided functionalities.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 200 ++++++++++++++++++++++---------------------------
1 file changed, 90 insertions(+), 110 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 856869c9d3a9..1a219316e7c5 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -94,7 +94,7 @@ typedef struct _testAuth testAuth;
typedef struct _testAuth *testAuthPtr;
struct _testDriver {
- virMutex lock;
+ virObjectLockable parent;
virNodeInfo nodeInfo;
virInterfaceObjListPtr ifaces;
@@ -126,9 +126,22 @@ typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
static testDriverPtr defaultPrivconn;
-static int defaultConnections;
static virMutex defaultLock = VIR_MUTEX_INITIALIZER;
+static virClassPtr testDriverClass;
+static void testDriverDispose(void *obj);
+static int testDriverOnceInit(void)
+{
+ if (!(testDriverClass = virClassNew(virClassForObjectLockable(),
+ "testDriver",
+ sizeof(testDriver),
+ testDriverDispose)))
Probably posted/pushed after this posting, but commit id '10f94828'
added a new VIR_CLASS_NEW definition and commit id '825bb9b8' required
using it.
Simple to change:
if (!(VIR_CLASS_NEW(testDriver, virClassForObjectLockable())))
But figured you should know.
+ return -1;
+
+ return 0;
+}
+VIR_ONCE_GLOBAL_INIT(testDriver)
+
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
11:10 a.m.
New subject: [libvirt] [PATCH libvirt v2 2/9] test: Convert testDriver to virObjectLockable
On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
> The test driver state (@testDriver) uses it's own reference counting
> and locking implementation. Instead of doing that, convert @testDriver
> into a virObjectLockable and use the provided functionalities.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/test/test_driver.c | 200 ++++++++++++++++++++++---------------------------
> 1 file changed, 90 insertions(+), 110 deletions(-)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 856869c9d3a9..1a219316e7c5 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -94,7 +94,7 @@ typedef struct _testAuth testAuth;
> typedef struct _testAuth *testAuthPtr;
>
> struct _testDriver {
> - virMutex lock;
> + virObjectLockable parent;
>
> virNodeInfo nodeInfo;
> virInterfaceObjListPtr ifaces;
> @@ -126,9 +126,22 @@ typedef struct _testDriver testDriver;
> typedef testDriver *testDriverPtr;
>
> static testDriverPtr defaultPrivconn;
> -static int defaultConnections;
> static virMutex defaultLock = VIR_MUTEX_INITIALIZER;
>
> +static virClassPtr testDriverClass;
> +static void testDriverDispose(void *obj);
> +static int testDriverOnceInit(void)
> +{
> + if (!(testDriverClass = virClassNew(virClassForObjectLockable(),
> + "testDriver",
> + sizeof(testDriver),
> + testDriverDispose)))
Probably posted/pushed after this posting, but commit id '10f94828'
added a new VIR_CLASS_NEW definition and commit id '825bb9b8' required
using it.
Yep.
Simple to change:
if (!(VIR_CLASS_NEW(testDriver, virClassForObjectLockable())))
But figured you should know.
> + return -1;
> +
> + return 0;
> +}
> +VIR_ONCE_GLOBAL_INIT(testDriver)
> +
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks.
John
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
7:40 a.m.
New subject: [libvirt] [PATCH libvirt v2 3/9] remote: Add the information which program has to be used to daemonClientEventCallback
As a result, you can later determine at the callback which program has
to be used. This makes it easier to refactor the code in the future
and is less prone to error.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 108 ++++++++++++++++++++----------------
1 file changed, 59 insertions(+), 49 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 5b764bab48d5..b4c0e01b0832 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -78,6 +78,7 @@ VIR_LOG_INIT("daemon.remote");
struct daemonClientEventCallback {
virNetServerClientPtr client;
+ virNetServerProgramPtr program;
int eventID;
int callbackID;
bool legacy;
@@ -127,6 +128,7 @@ remoteEventCallbackFree(void *opaque)
daemonClientEventCallbackPtr callback = opaque;
if (!callback)
return;
+ virObjectUnref(callback->program);
virObjectUnref(callback->client);
VIR_FREE(callback);
}
@@ -318,7 +320,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
data.detail = detail;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_lifecycle_msg,
&data);
@@ -326,7 +328,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
remote_domain_event_callback_lifecycle_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_callback_lifecycle_msg,
&msg);
@@ -355,14 +357,14 @@ remoteRelayDomainEventReboot(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_REBOOT,
(xdrproc_t)xdr_remote_domain_event_reboot_msg,
&data);
} else {
remote_domain_event_callback_reboot_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_REBOOT,
(xdrproc_t)xdr_remote_domain_event_callback_reboot_msg, &msg);
}
@@ -394,14 +396,14 @@ remoteRelayDomainEventRTCChange(virConnectPtr conn,
data.offset = offset;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_rtc_change_msg,
&data);
} else {
remote_domain_event_callback_rtc_change_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_rtc_change_msg, &msg);
}
@@ -432,14 +434,14 @@ remoteRelayDomainEventWatchdog(virConnectPtr conn,
data.action = action;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_WATCHDOG,
(xdrproc_t)xdr_remote_domain_event_watchdog_msg,
&data);
} else {
remote_domain_event_callback_watchdog_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_WATCHDOG,
(xdrproc_t)xdr_remote_domain_event_callback_watchdog_msg, &msg);
}
@@ -476,14 +478,14 @@ remoteRelayDomainEventIOError(virConnectPtr conn,
data.action = action;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_IO_ERROR,
(xdrproc_t)xdr_remote_domain_event_io_error_msg,
&data);
} else {
remote_domain_event_callback_io_error_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_IO_ERROR,
(xdrproc_t)xdr_remote_domain_event_callback_io_error_msg, &msg);
}
@@ -527,14 +529,14 @@ remoteRelayDomainEventIOErrorReason(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_IO_ERROR_REASON,
(xdrproc_t)xdr_remote_domain_event_io_error_reason_msg, &data);
} else {
remote_domain_event_callback_io_error_reason_msg msg = {
callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_IO_ERROR_REASON,
(xdrproc_t)xdr_remote_domain_event_callback_io_error_reason_msg, &msg);
}
@@ -601,14 +603,14 @@ remoteRelayDomainEventGraphics(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_GRAPHICS,
(xdrproc_t)xdr_remote_domain_event_graphics_msg,
&data);
} else {
remote_domain_event_callback_graphics_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_GRAPHICS,
(xdrproc_t)xdr_remote_domain_event_callback_graphics_msg, &msg);
}
@@ -658,14 +660,14 @@ remoteRelayDomainEventBlockJob(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB,
(xdrproc_t)xdr_remote_domain_event_block_job_msg,
&data);
} else {
remote_domain_event_callback_block_job_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_BLOCK_JOB,
(xdrproc_t)xdr_remote_domain_event_callback_block_job_msg, &msg);
}
@@ -694,14 +696,14 @@ remoteRelayDomainEventControlError(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CONTROL_ERROR,
(xdrproc_t)xdr_remote_domain_event_control_error_msg, &data);
} else {
remote_domain_event_callback_control_error_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_CONTROL_ERROR,
(xdrproc_t)xdr_remote_domain_event_callback_control_error_msg, &msg);
}
@@ -752,14 +754,14 @@ remoteRelayDomainEventDiskChange(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE,
(xdrproc_t)xdr_remote_domain_event_disk_change_msg,
&data);
} else {
remote_domain_event_callback_disk_change_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DISK_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_disk_change_msg, &msg);
}
@@ -800,14 +802,14 @@ remoteRelayDomainEventTrayChange(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_TRAY_CHANGE,
(xdrproc_t)xdr_remote_domain_event_tray_change_msg,
&data);
} else {
remote_domain_event_callback_tray_change_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TRAY_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_tray_change_msg, &msg);
}
@@ -836,14 +838,14 @@ remoteRelayDomainEventPMWakeup(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_PMWAKEUP,
(xdrproc_t)xdr_remote_domain_event_pmwakeup_msg,
&data);
} else {
remote_domain_event_callback_pmwakeup_msg msg = { callback->callbackID,
reason, data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_PMWAKEUP,
(xdrproc_t)xdr_remote_domain_event_callback_pmwakeup_msg, &msg);
}
@@ -872,14 +874,14 @@ remoteRelayDomainEventPMSuspend(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND,
(xdrproc_t)xdr_remote_domain_event_pmsuspend_msg,
&data);
} else {
remote_domain_event_callback_pmsuspend_msg msg = { callback->callbackID,
reason, data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_PMSUSPEND,
(xdrproc_t)xdr_remote_domain_event_callback_pmsuspend_msg, &msg);
}
@@ -909,14 +911,14 @@ remoteRelayDomainEventBalloonChange(virConnectPtr conn,
data.actual = actual;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_BALLOON_CHANGE,
(xdrproc_t)xdr_remote_domain_event_balloon_change_msg, &data);
} else {
remote_domain_event_callback_balloon_change_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_BALLOON_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_balloon_change_msg, &msg);
}
@@ -946,14 +948,14 @@ remoteRelayDomainEventPMSuspendDisk(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK,
(xdrproc_t)xdr_remote_domain_event_pmsuspend_disk_msg, &data);
} else {
remote_domain_event_callback_pmsuspend_disk_msg msg = { callback->callbackID,
reason, data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_PMSUSPEND_DISK,
(xdrproc_t)xdr_remote_domain_event_callback_pmsuspend_disk_msg, &msg);
}
@@ -986,7 +988,7 @@ remoteRelayDomainEventDeviceRemoved(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED,
(xdrproc_t)xdr_remote_domain_event_device_removed_msg,
&data);
@@ -994,7 +996,7 @@ remoteRelayDomainEventDeviceRemoved(virConnectPtr conn,
remote_domain_event_callback_device_removed_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_REMOVED,
(xdrproc_t)xdr_remote_domain_event_callback_device_removed_msg,
&msg);
@@ -1031,7 +1033,7 @@ remoteRelayDomainEventBlockJob2(virConnectPtr conn,
data.status = status;
make_nonnull_domain(&data.dom, dom);
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_2,
(xdrproc_t)xdr_remote_domain_event_block_job_2_msg,
&data);
@@ -1069,7 +1071,7 @@ remoteRelayDomainEventTunable(virConnectPtr conn,
return -1;
}
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TUNABLE,
(xdrproc_t)xdr_remote_domain_event_callback_tunable_msg,
&data);
@@ -1104,7 +1106,7 @@ remoteRelayDomainEventAgentLifecycle(virConnectPtr conn,
data.state = state;
data.reason = reason;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_AGENT_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_callback_agent_lifecycle_msg,
&data);
@@ -1138,7 +1140,7 @@ remoteRelayDomainEventDeviceAdded(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_ADDED,
(xdrproc_t)xdr_remote_domain_event_callback_device_added_msg,
&data);
@@ -1171,7 +1173,7 @@ remoteRelayDomainEventMigrationIteration(virConnectPtr conn,
data.iteration = iteration;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_MIGRATION_ITERATION,
(xdrproc_t)xdr_remote_domain_event_callback_migration_iteration_msg,
&data);
@@ -1211,7 +1213,7 @@ remoteRelayDomainEventJobCompleted(virConnectPtr conn,
return -1;
}
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_JOB_COMPLETED,
(xdrproc_t)xdr_remote_domain_event_callback_job_completed_msg,
&data);
@@ -1244,7 +1246,7 @@ remoteRelayDomainEventDeviceRemovalFailed(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_REMOVAL_FAILED,
(xdrproc_t)xdr_remote_domain_event_callback_device_removal_failed_msg,
&data);
@@ -1288,7 +1290,7 @@ remoteRelayDomainEventMetadataChange(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_METADATA_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_metadata_change_msg,
&data);
@@ -1331,7 +1333,7 @@ remoteRelayDomainEventBlockThreshold(virConnectPtr conn,
data.excess = excess;
make_nonnull_domain(&data.dom, dom);
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_BLOCK_THRESHOLD,
(xdrproc_t)xdr_remote_domain_event_block_threshold_msg,
&data);
@@ -1396,7 +1398,7 @@ remoteRelayNetworkEventLifecycle(virConnectPtr conn,
data.event = event;
data.detail = detail;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_NETWORK_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_network_event_lifecycle_msg,
&data);
@@ -1433,7 +1435,7 @@ remoteRelayStoragePoolEventLifecycle(virConnectPtr conn,
data.event = event;
data.detail = detail;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_STORAGE_POOL_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_storage_pool_event_lifecycle_msg,
&data);
@@ -1461,7 +1463,7 @@ remoteRelayStoragePoolEventRefresh(virConnectPtr conn,
make_nonnull_storage_pool(&data.pool, pool);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_STORAGE_POOL_EVENT_REFRESH,
(xdrproc_t)xdr_remote_storage_pool_event_refresh_msg,
&data);
@@ -1500,7 +1502,7 @@ remoteRelayNodeDeviceEventLifecycle(virConnectPtr conn,
data.event = event;
data.detail = detail;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_NODE_DEVICE_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_node_device_event_lifecycle_msg,
&data);
@@ -1528,7 +1530,7 @@ remoteRelayNodeDeviceEventUpdate(virConnectPtr conn,
make_nonnull_node_device(&data.dev, dev);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE,
(xdrproc_t)xdr_remote_node_device_event_update_msg,
&data);
@@ -1567,7 +1569,7 @@ remoteRelaySecretEventLifecycle(virConnectPtr conn,
data.event = event;
data.detail = detail;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_SECRET_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_secret_event_lifecycle_msg,
&data);
@@ -1595,7 +1597,7 @@ remoteRelaySecretEventValueChanged(virConnectPtr conn,
make_nonnull_secret(&data.secret, secret);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_SECRET_EVENT_VALUE_CHANGED,
(xdrproc_t)xdr_remote_secret_event_value_changed_msg,
&data);
@@ -1645,7 +1647,7 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
data.details = details_p;
make_nonnull_domain(&data.dom, dom);
- remoteDispatchObjectEventSend(callback->client, qemuProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
QEMU_PROC_DOMAIN_MONITOR_EVENT,
(xdrproc_t)xdr_qemu_domain_monitor_event_msg,
&data);
@@ -3922,6 +3924,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
ATTRIBUTE_UNUSED
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
callback->callbackID = -1;
callback->legacy = true;
@@ -4158,6 +4161,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
callback->legacy = true;
@@ -4234,6 +4238,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server ATTRI
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5757,6 +5762,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UN
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5879,6 +5885,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server ATTRIBUT
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6000,6 +6007,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server ATTRIBUTE
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6121,6 +6129,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6237,6 +6246,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
ATTRIBUTE_U
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(qemuProgram);
callback->eventID = -1;
callback->callbackID = -1;
ref = callback;
--
2.13.4
10:07 a.m.
New subject: [libvirt] [PATCH libvirt v2 3/9] remote: Add the information which program has to be used to daemonClientEventCallback
On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
As a result, you can later determine at the callback which program
has
s/at/during
s/has to be/was
to be used. This makes it easier to refactor the code in the future
and is less prone to error.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 108 ++++++++++++++++++++----------------
1 file changed, 59 insertions(+), 49 deletions(-)
The $SUBJ is a bit long...
How about just:
remote: Save reference to program in daemonClientEventCallback
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
11:12 a.m.
New subject: [libvirt] [PATCH libvirt v2 3/9] remote: Add the information which program has to be used to daemonClientEventCallback
On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
> As a result, you can later determine at the callback which program has
s/at/during
s/has to be/was
> to be used. This makes it easier to refactor the code in the future
> and is less prone to error.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/remote/remote_daemon_dispatch.c | 108 ++++++++++++++++++++----------------
> 1 file changed, 59 insertions(+), 49 deletions(-)
>
The $SUBJ is a bit long...
How about just:
remote: Save reference to program in daemonClientEventCallback
Yep, that’s better.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks.
John
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
7:40 a.m.
New subject: [libvirt] [PATCH libvirt v2 4/9] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
This allows us to get rid of another usage of the global variable
remoteProgram.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index b4c0e01b0832..cf2cd0add7d6 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
static
void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason,
void *opaque)
{
- virNetServerClientPtr client = opaque;
+ daemonClientEventCallbackPtr callback = opaque;
VIR_DEBUG("Relaying connection closed event, reason %d", reason);
remote_connect_event_connection_closed_msg msg = { reason };
- remoteDispatchObjectEventSend(client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
&msg);
@@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
virNetMessageErrorPtr rerr)
{
int rv = -1;
+ daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
@@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
goto cleanup;
}
+ if (VIR_ALLOC(callback) < 0)
+ goto cleanup;
+ callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
+ /* eventID, callbackID, and legacy are not used */
+ callback->eventID = -1;
+ callback->callbackID = -1;
if (virConnectRegisterCloseCallback(priv->conn,
remoteRelayConnectionClosedEvent,
- client, NULL) < 0)
+ callback, remoteEventCallbackFree) < 0)
goto cleanup;
priv->closeRegistered = true;
@@ -3856,8 +3864,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
cleanup:
virMutexUnlock(&priv->lock);
- if (rv < 0)
+ if (rv < 0) {
+ remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
+ }
return rv;
}
--
2.13.4
10:07 a.m.
New subject: [libvirt] [PATCH libvirt v2 4/9] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
This allows us to get rid of another usage of the global variable
remoteProgram.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index b4c0e01b0832..cf2cd0add7d6 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
static
void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason,
void *opaque)
{
- virNetServerClientPtr client = opaque;
+ daemonClientEventCallbackPtr callback = opaque;
VIR_DEBUG("Relaying connection closed event, reason %d", reason);
remote_connect_event_connection_closed_msg msg = { reason };
- remoteDispatchObjectEventSend(client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
&msg);
@@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
virNetMessageErrorPtr rerr)
{
int rv = -1;
+ daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
@@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
goto cleanup;
}
+ if (VIR_ALLOC(callback) < 0)
+ goto cleanup;
+ callback->client = virObjectRef(client);
Oh and this would seem to fix a problem with the callback possibly using
@client after it had been freed!
+ callback->program = virObjectRef(remoteProgram);
+ /* eventID, callbackID, and legacy are not used */
+ callback->eventID = -1;
+ callback->callbackID = -1;
if (virConnectRegisterCloseCallback(priv->conn,
remoteRelayConnectionClosedEvent,
- client, NULL) < 0)
+ callback, remoteEventCallbackFree) < 0)
goto cleanup;
@callback would be leaked in the normal path... If you consider all the
other callbacks will load @callback onto some list that gets run during
remoteClientFreePrivateCallbacks via DEREG_CB, this code would need to
do something similar. Since there's only 1 it's perhaps easier at least.
priv->closeRegistered = true;
Perhaps change closeRegistered from bool to daemonClientEventCallbackPtr
and handle it that way similar to how other callback processing is handled.
John
@@ -3856,8 +3864,10 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
cleanup:
virMutexUnlock(&priv->lock);
- if (rv < 0)
+ if (rv < 0) {
+ remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
+ }
return rv;
}
11:27 a.m.
New subject: [libvirt] [PATCH libvirt v2 4/9] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
> This allows us to get rid of another usage of the global variable
> remoteProgram.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
> index b4c0e01b0832..cf2cd0add7d6 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
> static
> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int
reason, void *opaque)
> {
> - virNetServerClientPtr client = opaque;
> + daemonClientEventCallbackPtr callback = opaque;
>
> VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>
> remote_connect_event_connection_closed_msg msg = { reason };
> - remoteDispatchObjectEventSend(client, remoteProgram,
> + remoteDispatchObjectEventSend(callback->client, callback->program,
> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
> &msg);
> @@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
> virNetMessageErrorPtr rerr)
> {
> int rv = -1;
> + daemonClientEventCallbackPtr callback = NULL;
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> @@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
> goto cleanup;
> }
>
> + if (VIR_ALLOC(callback) < 0)
> + goto cleanup;
> + callback->client = virObjectRef(client);
Oh and this would seem to fix a problem with the callback possibly using
@client after it had been freed!
The problem is more of a theoretical nature as Nikolay had explained:
“Refcounting was here originally but then removed in [1] as it conflicts with
virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
is not implemented. This is safe though (at least nobody touch this place :).
[1] ce35122cfe: "daemon: fixup refcounting in close callback handling"”
> + callback->program = virObjectRef(remoteProgram);
> + /* eventID, callbackID, and legacy are not used */
> + callback->eventID = -1;
> + callback->callbackID = -1;
> if (virConnectRegisterCloseCallback(priv->conn,
> remoteRelayConnectionClosedEvent,
> - client, NULL) < 0)
> + callback, remoteEventCallbackFree) < 0)
> goto cleanup;
>
@callback would be leaked in the normal path...
By normal path, you mean without the first patch?
If you consider all the
other callbacks will load @callback onto some list that gets run during
remoteClientFreePrivateCallbacks via DEREG_CB, this code would need to
do something similar. Since there's only 1 it's perhaps easier at
least.
> priv->closeRegistered = true;
Perhaps change closeRegistered from bool to daemonClientEventCallbackPtr
and handle it that way similar to how other callback processing is
handled.
This would be one way how to deal with it (even without the first
patch). But a double free error must be prevented.
John
> @@ -3856,8 +3864,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
>
> cleanup:
> virMutexUnlock(&priv->lock);
> - if (rv < 0)
> + if (rv < 0) {
> + remoteEventCallbackFree(callback);
> virNetMessageSaveError(rerr);
> + }
> return rv;
> }
>
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
7:19 p.m.
New subject: [libvirt] [PATCH libvirt v2 4/9] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
On 04/26/2018 12:27 PM, Marc Hartmayer wrote:
On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan
<jferlan(a)redhat.com> wrote:
> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>> This allows us to get rid of another usage of the global variable
>> remoteProgram.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
>> ---
>> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
>> index b4c0e01b0832..cf2cd0add7d6 100644
>> --- a/src/remote/remote_daemon_dispatch.c
>> +++ b/src/remote/remote_daemon_dispatch.c
>> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
>> static
>> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int
reason, void *opaque)
>> {
>> - virNetServerClientPtr client = opaque;
>> + daemonClientEventCallbackPtr callback = opaque;
>>
>> VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>>
>> remote_connect_event_connection_closed_msg msg = { reason };
>> - remoteDispatchObjectEventSend(client, remoteProgram,
>> + remoteDispatchObjectEventSend(callback->client, callback->program,
>> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>>
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
>> &msg);
>> @@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
>> virNetMessageErrorPtr rerr)
>> {
>> int rv = -1;
>> + daemonClientEventCallbackPtr callback = NULL;
>> struct daemonClientPrivate *priv =
>> virNetServerClientGetPrivateData(client);
>>
>> @@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
>> goto cleanup;
>> }
>>
>> + if (VIR_ALLOC(callback) < 0)
>> + goto cleanup;
>> + callback->client = virObjectRef(client);
>
> Oh and this would seem to fix a problem with the callback possibly using
> @client after it had been freed!
The problem is more of a theoretical nature as Nikolay had explained:
“Refcounting was here originally but then removed in [1] as it conflicts with
virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
is not implemented. This is safe though (at least nobody touch this place :).
[1] ce35122cfe: "daemon: fixup refcounting in close callback handling"”
>
>> + callback->program = virObjectRef(remoteProgram);
>> + /* eventID, callbackID, and legacy are not used */
>> + callback->eventID = -1;
>> + callback->callbackID = -1;
>> if (virConnectRegisterCloseCallback(priv->conn,
>> remoteRelayConnectionClosedEvent,
>> - client, NULL) < 0)
>> + callback, remoteEventCallbackFree) <
0)
>> goto cleanup;
>>
>
> @callback would be leaked in the normal path...
By normal path, you mean without the first patch?
I was following how the other register functions proceeded and they
saved the callback in a list to be freed at unregister. So if Register
succeeds, rv == 0 and the removeEventCallbackFree(callback) isn't called
and we leak callback. At least that's how I read it
John
> If you consider all the
> other callbacks will load @callback onto some list that gets run during
> remoteClientFreePrivateCallbacks via DEREG_CB, this code would need to
> do something similar. Since there's only 1 it's perhaps easier at
> least.
>
>> priv->closeRegistered = true;
>
> Perhaps change closeRegistered from bool to daemonClientEventCallbackPtr
> and handle it that way similar to how other callback processing is
> handled.
This would be one way how to deal with it (even without the first
patch). But a double free error must be prevented.
>
> John
>
>> @@ -3856,8 +3864,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
>>
>> cleanup:
>> virMutexUnlock(&priv->lock);
>> - if (rv < 0)
>> + if (rv < 0) {
>> + remoteEventCallbackFree(callback);
>> virNetMessageSaveError(rerr);
>> + }
>> return rv;
>> }
>>
>>
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
9:54 a.m.
New subject: [libvirt] [PATCH libvirt v2 4/9] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
On Fri, Apr 27, 2018 at 02:19 AM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
[…snip…]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
On 04/26/2018 12:27 PM, Marc Hartmayer wrote:
> On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan(a)redhat.com>
wrote:
>> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>>> This allows us to get rid of another usage of the global variable
>>> remoteProgram.
>> >>> Signed-off-by: Marc
Hartmayer <mhartmay(a)linux.vnet.ibm.com
>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com >>> ---
>>> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>> >>> diff --git
a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>>> index b4c0e01b0832..cf2cd0add7d6 100644
>>> --- a/src/remote/remote_daemon_dispatch.c
>>> +++ b/src/remote/remote_daemon_dispatch.c
>>> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr
conn,
>>> static
>>> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED,
int reason, void *opaque)
>>> {
>>> - virNetServerClientPtr client = opaque;
>>> + daemonClientEventCallbackPtr callback = opaque;
>> >>>
VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>> >>>
remote_connect_event_connection_closed_msg msg = { reason };
>>> - remoteDispatchObjectEventSend(client, remoteProgram,
>>> + remoteDispatchObjectEventSend(callback->client,
callback->program,
>>>
REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>>>
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
>>> &msg);
>>> @@ -3836,6 +3836,7 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>> virNetMessageErrorPtr rerr)
>>> {
>>> int rv = -1;
>>> + daemonClientEventCallbackPtr callback = NULL;
>>> struct daemonClientPrivate *priv =
>>> virNetServerClientGetPrivateData(client);
>> >>> @@ -3846,9 +3847,16 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>> goto cleanup;
>>> }
>> >>> + if
(VIR_ALLOC(callback) < 0)
>>> + goto cleanup;
>>> + callback->client = virObjectRef(client);
> >> Oh and this would seem to fix a problem with the
callback possibly using
>> @client after it had been freed!
> The problem is more of a theoretical nature as Nikolay had
explained:
> “Refcounting was here originally but then removed in [1]
as it conflicts with
> virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
> is not implemented. This is safe though (at least nobody touch this place :).
> [1] ce35122cfe: "daemon: fixup refcounting in close
callback handling"”
> >>> +
callback->program = virObjectRef(remoteProgram);
>>> + /* eventID, callbackID, and legacy are not used */
>>> + callback->eventID = -1;
>>> + callback->callbackID = -1;
>>> if (virConnectRegisterCloseCallback(priv->conn,
>>> remoteRelayConnectionClosedEvent,
>>> - client, NULL) < 0)
>>> + callback, remoteEventCallbackFree)
< 0)
>>> goto cleanup;
>> > >>
@callback would be leaked in the normal path...
> By normal path, you mean without the first patch?
I was following how the other register functions proceeded and they
saved the callback in a list to be freed at unregister. So if Register
succeeds, rv == 0 and the removeEventCallbackFree(callback) isn't called
and we leak callback. At least that's how I read it
First - sorry for the late response!
The unregister/free’ing is either done within the
'remoteClientFreePrivateCallbacks' call or by an explicit call of
'remoteDispatchConnectUnregisterCloseCallback'. Do we agree on this? If
yes: the function 'remoteClientFreePrivateCallbacks' calls the
virConnectUnregisterCloseCallback -> remoteEventCallbackFree => no
memory leak.
There will be only a memory leak if the virConnectRegisterCloseCallback
call succeeds but the used driver had no support for registering a close
callback (conn->driver->connectRegisterCloseCallback was NULL) AND the
first patch of this series were not applied.
Did I miss something?
1:32 p.m.
New subject: [libvirt] [PATCH libvirt v2 4/9] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
[...]
>>>
>>>> + callback->program = virObjectRef(remoteProgram);
>>>> + /* eventID, callbackID, and legacy are not used */
>>>> + callback->eventID = -1;
>>>> + callback->callbackID = -1;
>>>> if (virConnectRegisterCloseCallback(priv->conn,
>>>>
remoteRelayConnectionClosedEvent,
>>>> - client, NULL) < 0)
>>>> + callback,
remoteEventCallbackFree) < 0)
>>>> goto cleanup;
>>>>
>>>
>>> @callback would be leaked in the normal path...
>>
>> By normal path, you mean without the first patch?
>>
>
> I was following how the other register functions proceeded and they
> saved the callback in a list to be freed at unregister. So if Register
> succeeds, rv == 0 and the removeEventCallbackFree(callback) isn't called
> and we leak callback. At least that's how I read it
First - sorry for the late response!
The unregister/free’ing is either done within the
'remoteClientFreePrivateCallbacks' call or by an explicit call of
'remoteDispatchConnectUnregisterCloseCallback'. Do we agree on this? If
yes: the function 'remoteClientFreePrivateCallbacks' calls the
virConnectUnregisterCloseCallback -> remoteEventCallbackFree => no
memory leak.
There will be only a memory leak if the virConnectRegisterCloseCallback
call succeeds but the used driver had no support for registering a close
callback (conn->driver->connectRegisterCloseCallback was NULL) AND the
first patch of this series were not applied.
Did I miss something?
Trying to recollect where I was.... and I cannot... I probably was
flipping between patched and unpatched code. I assume it had something
to do with adding the callback to a list, running DEREG_CB processing,
and perhaps seeing DELETE_ELEMENT that got me overthinking, but taking a
second look now I don't believe there's an issue.
So, to make it official then...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
7:41 a.m.
New subject: [libvirt] [PATCH libvirt v2 5/9] rpc: Introduce virNetServerGetProgramLocked helper function
This patch introduces virNetServerGetProgramLocked. It's a function to
determine which program has to be used for a given @msg. This function
will be reused in the next patch.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
---
src/rpc/virnetserver.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 3ce21a8f5345..ef214980b297 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -182,6 +182,29 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque)
VIR_FREE(job);
}
+
+/**
+ * virNetServerGetProgramLocked:
+ * @srv: server (must be locked by the caller)
+ * @msg: message
+ *
+ * Searches @srv for the right program for a given message @msg.
+ *
+ * Returns a pointer to the server program or NULL if not found.
+ */
+static virNetServerProgramPtr
+virNetServerGetProgramLocked(virNetServerPtr srv,
+ virNetMessagePtr msg)
+{
+ size_t i;
+ for (i = 0; i < srv->nprograms; i++) {
+ if (virNetServerProgramMatches(srv->programs[i], msg))
+ return srv->programs[i];
+ }
+ return NULL;
+}
+
+
static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
virNetMessagePtr msg,
void *opaque)
@@ -189,18 +212,12 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
virNetServerPtr srv = opaque;
virNetServerProgramPtr prog = NULL;
unsigned int priority = 0;
- size_t i;
VIR_DEBUG("server=%p client=%p message=%p",
srv, client, msg);
virObjectLock(srv);
- for (i = 0; i < srv->nprograms; i++) {
- if (virNetServerProgramMatches(srv->programs[i], msg)) {
- prog = srv->programs[i];
- break;
- }
- }
+ prog = virNetServerGetProgramLocked(srv, msg);
if (srv->workers) {
virNetServerJobPtr job;
--
2.13.4
7:41 a.m.
New subject: [libvirt] [PATCH libvirt v2 6/9] remote/rpc: Use virNetServerGetProgram() to determine the program
Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
Note: I'm not 100% sure that there is no case where the lock for
@client is already held by the thread which is calling
virNetServerGetProgram and thus the lock order would be violated (the
lock order has to be @server -> @client in the violating case it would
be @client -> @server and therefore a deadlock might occur).
---
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 3 -
src/remote/remote_daemon_dispatch.c | 116 +++++++++++++++++++++++++++---------
src/rpc/gendispatch.pl | 6 ++
src/rpc/virnetserver.c | 23 +++++++
src/rpc/virnetserver.h | 2 +
7 files changed, 122 insertions(+), 33 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 97e22275b980..c31b16cd5909 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -120,6 +120,7 @@ virNetServerGetCurrentUnauthClients;
virNetServerGetMaxClients;
virNetServerGetMaxUnauthClients;
virNetServerGetName;
+virNetServerGetProgram;
virNetServerGetThreadPoolParameters;
virNetServerHasClients;
virNetServerNew;
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 31c6ce1b6179..f854a1a6981e 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -71,8 +71,6 @@ VIR_LOG_INIT("daemon.libvirtd");
#if WITH_SASL
virNetSASLContextPtr saslCtxt = NULL;
#endif
-virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr qemuProgram = NULL;
volatile bool driversInitialized = false;
@@ -1049,6 +1047,8 @@ int main(int argc, char **argv) {
virNetServerPtr srv = NULL;
virNetServerPtr srvAdm = NULL;
virNetServerProgramPtr adminProgram = NULL;
+ virNetServerProgramPtr qemuProgram = NULL;
+ virNetServerProgramPtr remoteProgram = NULL;
virNetServerProgramPtr lxcProgram = NULL;
char *remote_config_file = NULL;
int statuswrite = -1;
diff --git a/src/remote/remote_daemon.h b/src/remote/remote_daemon.h
index 2834da04a9ae..a2eda209683b 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -88,7 +88,4 @@ struct daemonClientPrivate {
# if WITH_SASL
extern virNetSASLContextPtr saslCtxt;
# endif
-extern virNetServerProgramPtr remoteProgram;
-extern virNetServerProgramPtr qemuProgram;
-
#endif /* __REMOTE_DAEMON_H__ */
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index cf2cd0add7d6..94b9cc3377d8 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3830,15 +3830,16 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server
ATTRIBUTE_UNUSED,
}
static int
-remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr)
{
int rv = -1;
daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
+ virNetServerProgramPtr program;
virMutexLock(&priv->lock);
@@ -3847,10 +3848,15 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
goto cleanup;
}
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
/* eventID, callbackID, and legacy are not used */
callback->eventID = -1;
callback->callbackID = -1;
@@ -3903,9 +3909,9 @@ remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UN
}
static int
-remoteDispatchConnectDomainEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
remote_connect_domain_event_register_ret *ret
ATTRIBUTE_UNUSED)
{
@@ -3915,12 +3921,18 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
ATTRIBUTE_UNUSED
daemonClientEventCallbackPtr ref;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
+ virNetServerProgramPtr program;
if (!priv->conn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
virMutexLock(&priv->lock);
/* If we call register first, we could append a complete callback
@@ -3934,7 +3946,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
ATTRIBUTE_UNUSED
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
callback->callbackID = -1;
callback->legacy = true;
@@ -4132,9 +4144,9 @@ remoteDispatchDomainGetState(virNetServerPtr server
ATTRIBUTE_UNUSED,
* VIR_DRV_SUPPORTS_FEATURE(VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK),
* and must not mix the two styles. */
static int
-remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
remote_connect_domain_event_register_any_args
*args)
{
@@ -4144,12 +4156,18 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
daemonClientEventCallbackPtr ref;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
+ virNetServerProgramPtr program;
if (!priv->conn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
virMutexLock(&priv->lock);
/* We intentionally do not use VIR_DOMAIN_EVENT_ID_LAST here; any
@@ -4171,7 +4189,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
callback->legacy = true;
@@ -4207,9 +4225,9 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
static int
-remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg
ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_domain_event_callback_register_any_args *args,
remote_connect_domain_event_callback_register_any_ret *ret)
@@ -4221,12 +4239,18 @@
remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server ATTRI
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virDomainPtr dom = NULL;
+ virNetServerProgramPtr program;
if (!priv->conn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
virMutexLock(&priv->lock);
if (args->dom &&
@@ -4248,7 +4272,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server ATTRI
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5355,6 +5379,7 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr
server ATTRIBUTE
virNetServerClientGetPrivateData(client);
virStreamPtr st = NULL;
daemonClientStreamPtr stream = NULL;
+ virNetServerProgramPtr program;
if (!priv->conn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
@@ -5373,8 +5398,13 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr
server ATTRIBUTE
0, ¶ms, &nparams) < 0)
goto cleanup;
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
if (!(st = virStreamNew(priv->conn, VIR_STREAM_NONBLOCK)) ||
- !(stream = daemonCreateClientStream(client, st, remoteProgram,
+ !(stream = daemonCreateClientStream(client, st, program,
&msg->header, false)))
goto cleanup;
@@ -5731,9 +5761,9 @@ static int remoteDispatchDomainCreateWithFiles(virNetServerPtr
server ATTRIBUTE_
static int
-remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_network_event_register_any_args *args,
remote_connect_network_event_register_any_ret *ret)
@@ -5745,12 +5775,18 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr
server ATTRIBUTE_UN
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virNetworkPtr net = NULL;
+ virNetServerProgramPtr program;
if (!priv->networkConn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
virMutexLock(&priv->lock);
if (args->net &&
@@ -5772,7 +5808,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UN
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5854,9 +5890,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr
server ATTRIBUTE_
}
static int
-remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
+remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_storage_pool_event_register_any_args *args,
remote_connect_storage_pool_event_register_any_ret *ret)
@@ -5868,12 +5904,18 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server ATTRIBUT
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virStoragePoolPtr pool = NULL;
+ virNetServerProgramPtr program;
if (!priv->storageConn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
virMutexLock(&priv->lock);
if (args->pool &&
@@ -5895,7 +5937,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server ATTRIBUT
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5976,9 +6018,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr
server ATTRIB
}
static int
-remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_node_device_event_register_any_args *args,
remote_connect_node_device_event_register_any_ret *ret)
@@ -5990,12 +6032,18 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server ATTRIBUTE
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virNodeDevicePtr dev = NULL;
+ virNetServerProgramPtr program;
if (!priv->nodedevConn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
virMutexLock(&priv->lock);
if (args->dev &&
@@ -6017,7 +6065,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server ATTRIBUTE
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6098,9 +6146,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr
server ATTRIBU
}
static int
-remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
remote_connect_secret_event_register_any_args
*args,
remote_connect_secret_event_register_any_ret
*ret)
@@ -6112,12 +6160,18 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virSecretPtr secret = NULL;
+ virNetServerProgramPtr program;
if (!priv->secretConn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
virMutexLock(&priv->lock);
if (args->secret &&
@@ -6139,7 +6193,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6220,9 +6274,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server
ATTRIBUTE_U
}
static int
-qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED,
+qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
qemu_connect_domain_monitor_event_register_args *args,
qemu_connect_domain_monitor_event_register_ret *ret)
@@ -6235,12 +6289,18 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr
server ATTRIBUTE_U
virNetServerClientGetPrivateData(client);
virDomainPtr dom = NULL;
const char *event = args->event ? *args->event : NULL;
+ virNetServerProgramPtr program;
if (!priv->conn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
+
virMutexLock(&priv->lock);
if (args->dom &&
@@ -6256,7 +6316,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
ATTRIBUTE_U
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(qemuProgram);
+ callback->program = virObjectRef(program);
callback->eventID = -1;
callback->callbackID = -1;
ref = callback;
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index b8b83b6b40d3..ad442182d3c8 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -1039,6 +1039,7 @@ elsif ($mode eq "server") {
if ($call->{streamflag} ne "none") {
print " virStreamPtr st = NULL;\n";
print " daemonClientStreamPtr stream = NULL;\n";
+ print " virNetServerProgramPtr remoteProgram;\n";
if ($call->{sparseflag} ne "none") {
print " const bool sparse = args->flags &
$call->{sparseflag};\n"
} else {
@@ -1081,6 +1082,11 @@ elsif ($mode eq "server") {
print " if (!(st = virStreamNew($conn,
VIR_STREAM_NONBLOCK)))\n";
print " goto cleanup;\n";
print "\n";
+ print " if (!(remoteProgram = virNetServerGetProgram(server, msg)))
{\n";
+ print " virReportError(VIR_ERR_INTERNAL_ERROR, \"%s\",
_(\"no matching program found\"));\n";
+ print " goto cleanup;\n";
+ print " }\n";
+ print "\n";
print " if (!(stream = daemonCreateClientStream(client, st,
remoteProgram, &msg->header, sparse)))\n";
print " goto cleanup;\n";
print "\n";
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index ef214980b297..47ce88392b24 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -205,6 +205,29 @@ virNetServerGetProgramLocked(virNetServerPtr srv,
}
+/**
+ * virNetServerGetProgram:
+ * @srv: server (must NOT be locked by the caller)
+ * @msg: message
+ *
+ * Searches @srv for the right program for a given message @msg.
+ *
+ * Returns a pointer to the server program or NULL if not found.
+ */
+virNetServerProgramPtr
+virNetServerGetProgram(virNetServerPtr srv,
+ virNetMessagePtr msg)
+{
+ virNetServerProgramPtr ret;
+
+ virObjectLock(srv);
+ ret = virNetServerGetProgramLocked(srv, msg);
+ virObjectUnlock(srv);
+
+ return ret;
+}
+
+
static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
virNetMessagePtr msg,
void *opaque)
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index a79c39fdb2e7..1867e46664ba 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -76,6 +76,8 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
virNetTLSContextPtr tls);
# endif
+virNetServerProgramPtr virNetServerGetProgram(virNetServerPtr srv,
+ virNetMessagePtr msg);
int virNetServerAddClient(virNetServerPtr srv,
virNetServerClientPtr client);
--
2.13.4
10:08 a.m.
New subject: [libvirt] [PATCH libvirt v2 6/9] remote/rpc: Use virNetServerGetProgram() to determine the program
On 04/12/2018 08:41 AM, Marc Hartmayer wrote:
Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
Note: I'm not 100% sure that there is no case where the lock for
@client is already held by the thread which is calling
virNetServerGetProgram and thus the lock order would be violated (the
lock order has to be @server -> @client in the violating case it would
be @client -> @server and therefore a deadlock might occur).
Well the way I read virNetServerProgramDispatchCall it would seem both
@server and @client must be unlocked:
/*
* When the RPC handler is called:
*
* - Server object is unlocked
* - Client object is unlocked
*
* Without locking, it is safe to use:
*
* 'args and 'ret'
*/
rv = (dispatcher->func)(server, client, msg, &rerr, arg, ret);
---
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 3 -
src/remote/remote_daemon_dispatch.c | 116 +++++++++++++++++++++++++++---------
src/rpc/gendispatch.pl | 6 ++
src/rpc/virnetserver.c | 23 +++++++
src/rpc/virnetserver.h | 2 +
7 files changed, 122 insertions(+), 33 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
10 a.m.
New subject: [libvirt] [PATCH libvirt v2 6/9] remote/rpc: Use virNetServerGetProgram() to determine the program
On Thu, Apr 26, 2018 at 05:08 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 04/12/2018 08:41 AM, Marc Hartmayer wrote:
> Use virNetServerGetProgram() to determine the virNetServerProgram
> instead of using hard coded global variables. This allows us to remove
> the global variables @remoteProgram and @qemuProgram as they're now no
> longer necessary.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> ---
>
> Note: I'm not 100% sure that there is no case where the lock for
> @client is already held by the thread which is calling
> virNetServerGetProgram and thus the lock order would be violated (the
> lock order has to be @server -> @client in the violating case it would
> be @client -> @server and therefore a deadlock might occur).
>
Well the way I read virNetServerProgramDispatchCall it would seem both
@server and @client must be unlocked:
/*
* When the RPC handler is called:
*
* - Server object is unlocked
* - Client object is unlocked
*
* Without locking, it is safe to use:
*
* 'args and 'ret'
*/
rv = (dispatcher->func)(server, client, msg, &rerr, arg, ret);
Thanks for digging into that.
> ---
> src/libvirt_remote.syms | 1 +
> src/remote/remote_daemon.c | 4 +-
> src/remote/remote_daemon.h | 3 -
> src/remote/remote_daemon_dispatch.c | 116 +++++++++++++++++++++++++++---------
> src/rpc/gendispatch.pl | 6 ++
> src/rpc/virnetserver.c | 23 +++++++
> src/rpc/virnetserver.h | 2 +
> 7 files changed, 122 insertions(+), 33 deletions(-)
>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
…and thanks for the review.
John
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
7:41 a.m.
New subject: [libvirt] [PATCH libvirt v2 7/9] rpc: use the return value of virObjectRef directly
Use the return value of virObjectRef directly. This way, it's easier
for another reader to identify the reason why the additional reference
is required.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/rpc/virnetserver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 47ce88392b24..ec84c1f3d2b5 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -248,7 +248,7 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
if (VIR_ALLOC(job) < 0)
goto error;
- job->client = client;
+ job->client = virObjectRef(client);
job->msg = msg;
if (prog) {
@@ -256,7 +256,6 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
priority = virNetServerProgramGetPriority(prog, msg->header.proc);
}
- virObjectRef(client);
if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
virObjectUnref(client);
VIR_FREE(job);
--
2.13.4
10:08 a.m.
New subject: [libvirt] [PATCH libvirt v2 7/9] rpc: use the return value of virObjectRef directly
On 04/12/2018 08:41 AM, Marc Hartmayer wrote:
Use the return value of virObjectRef directly. This way, it's
easier
for another reader to identify the reason why the additional reference
is required.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/rpc/virnetserver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
7:41 a.m.
New subject: [libvirt] [PATCH libvirt v2 8/9] remote: remove ATTRIBUTE_UNUSED when attribute is actually used
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 94b9cc3377d8..041e7c7440bf 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3912,7 +3912,7 @@ static int
remoteDispatchConnectDomainEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
- virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_domain_event_register_ret *ret
ATTRIBUTE_UNUSED)
{
int callbackID;
@@ -3984,7 +3984,7 @@ static int
remoteDispatchConnectDomainEventDeregister(virNetServerPtr server ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_domain_event_deregister_ret
*ret ATTRIBUTE_UNUSED)
{
int callbackID = -1;
@@ -4147,7 +4147,7 @@ static int
remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
- virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_domain_event_register_any_args
*args)
{
int callbackID;
@@ -4228,7 +4228,7 @@ static int
remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_domain_event_callback_register_any_args *args,
remote_connect_domain_event_callback_register_any_ret *ret)
{
@@ -4312,7 +4312,7 @@ static int
remoteDispatchConnectDomainEventDeregisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_domain_event_deregister_any_args *args)
{
int callbackID = -1;
@@ -4370,7 +4370,7 @@ static int
remoteDispatchConnectDomainEventCallbackDeregisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
virNetMessagePtr msg
ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_domain_event_callback_deregister_any_args *args)
{
int rv = -1;
@@ -5764,7 +5764,7 @@ static int
remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_network_event_register_any_args *args,
remote_connect_network_event_register_any_ret *ret)
{
@@ -5848,7 +5848,7 @@ static int
remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_network_event_deregister_any_args *args)
{
int rv = -1;
@@ -5893,7 +5893,7 @@ static int
remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_storage_pool_event_register_any_args *args,
remote_connect_storage_pool_event_register_any_ret *ret)
{
@@ -5976,7 +5976,7 @@ static int
remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_storage_pool_event_deregister_any_args *args)
{
int rv = -1;
@@ -6021,7 +6021,7 @@ static int
remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_node_device_event_register_any_args *args,
remote_connect_node_device_event_register_any_ret *ret)
{
@@ -6104,7 +6104,7 @@ static int
remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_node_device_event_deregister_any_args *args)
{
int rv = -1;
@@ -6149,7 +6149,7 @@ static int
remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
- virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_secret_event_register_any_args
*args,
remote_connect_secret_event_register_any_ret
*ret)
{
@@ -6232,7 +6232,7 @@ static int
remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
remote_connect_secret_event_deregister_any_args *args)
{
int rv = -1;
@@ -6277,7 +6277,7 @@ static int
qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
qemu_connect_domain_monitor_event_register_args *args,
qemu_connect_domain_monitor_event_register_ret *ret)
{
@@ -6357,7 +6357,7 @@ static int
qemuDispatchConnectDomainMonitorEventDeregister(virNetServerPtr server ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
virNetMessagePtr msg ATTRIBUTE_UNUSED,
- virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
qemu_connect_domain_monitor_event_deregister_args *args)
{
int rv = -1;
--
2.13.4
10:08 a.m.
New subject: [libvirt] [PATCH libvirt v2 8/9] remote: remove ATTRIBUTE_UNUSED when attribute is actually used
On 04/12/2018 08:41 AM, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
Looks like commit id 'dd6b3318' didn't update @rerr for these APIs when
changing the CHECK_CONN macro and commit ids 'aaa6d7eb' and '158ba873'
didn't catch it either.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
7:41 a.m.
New subject: [libvirt] [PATCH libvirt v2 9/9] remote: shrink the critical sections
No lock is required to access 'priv->conn' therefore we can shrink the
critical sections.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 041e7c7440bf..d6699418392e 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3841,8 +3841,6 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientGetPrivateData(client);
virNetServerProgramPtr program;
- virMutexLock(&priv->lock);
-
if (!priv->conn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
@@ -3853,6 +3851,8 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
goto cleanup;
}
+ virMutexLock(&priv->lock);
+
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
@@ -3887,13 +3887,13 @@ remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UN
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
- virMutexLock(&priv->lock);
-
if (!priv->conn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ virMutexLock(&priv->lock);
+
if (virConnectUnregisterCloseCallback(priv->conn,
remoteRelayConnectionClosedEvent) < 0)
goto cleanup;
--
2.13.4
10:08 a.m.
New subject: [libvirt] [PATCH libvirt v2 9/9] remote: shrink the critical sections
On 04/12/2018 08:41 AM, Marc Hartmayer wrote:
No lock is required to access 'priv->conn' therefore we
can shrink the
critical sections.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
While this follows other usages, that means virMutexUnlock in cleanup:
may be called without holding the lock.
Perhaps this and other *Register/*Deregister API's should take the lock
around anything that could end up in cleanup - perhaps even the same
history as the previous patch
John
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 041e7c7440bf..d6699418392e 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3841,8 +3841,6 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientGetPrivateData(client);
virNetServerProgramPtr program;
- virMutexLock(&priv->lock);
-
if (!priv->conn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
@@ -3853,6 +3851,8 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
goto cleanup;
}
+ virMutexLock(&priv->lock);
+
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
@@ -3887,13 +3887,13 @@ remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UN
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
- virMutexLock(&priv->lock);
-
if (!priv->conn) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
goto cleanup;
}
+ virMutexLock(&priv->lock);
+
if (virConnectUnregisterCloseCallback(priv->conn,
remoteRelayConnectionClosedEvent) < 0)
goto cleanup;
11:40 a.m.
New subject: [libvirt] [PATCH libvirt v2 9/9] remote: shrink the critical sections
On Thu, Apr 26, 2018 at 05:08 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 04/12/2018 08:41 AM, Marc Hartmayer wrote:
> No lock is required to access 'priv->conn' therefore we can shrink the
> critical sections.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> ---
> src/remote/remote_daemon_dispatch.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
While this follows other usages, that means virMutexUnlock in cleanup:
may be called without holding the lock.
Yep. I saw it… another patch was in planning :)
Perhaps this and other *Register/*Deregister API's should take the lock
around anything that could end up in cleanup - perhaps even the same
history as the previous patch
This may lead to locking issues and it would increase the critical
sections for “no reason”.
What’s about:
Add another goto-label? Not beautiful but it would work.
John
> diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
> index 041e7c7440bf..d6699418392e 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -3841,8 +3841,6 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server,
> virNetServerClientGetPrivateData(client);
> virNetServerProgramPtr program;
>
> - virMutexLock(&priv->lock);
> -
> if (!priv->conn) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection
not open"));
> goto cleanup;
> @@ -3853,6 +3851,8 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server,
> goto cleanup;
> }
>
> + virMutexLock(&priv->lock);
> +
> if (VIR_ALLOC(callback) < 0)
> goto cleanup;
> callback->client = virObjectRef(client);
> @@ -3887,13 +3887,13 @@ remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UN
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> - virMutexLock(&priv->lock);
> -
> if (!priv->conn) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection
not open"));
> goto cleanup;
> }
>
> + virMutexLock(&priv->lock);
> +
> if (virConnectUnregisterCloseCallback(priv->conn,
> remoteRelayConnectionClosedEvent) < 0)
> goto cleanup;
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
7:20 p.m.
New subject: [libvirt] [PATCH libvirt v2 9/9] remote: shrink the critical sections
On 04/26/2018 12:40 PM, Marc Hartmayer wrote:
On Thu, Apr 26, 2018 at 05:08 PM +0200, John Ferlan
<jferlan(a)redhat.com> wrote:
> On 04/12/2018 08:41 AM, Marc Hartmayer wrote:
>> No lock is required to access 'priv->conn' therefore we can shrink
the
>> critical sections.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>> ---
>> src/remote/remote_daemon_dispatch.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>
> While this follows other usages, that means virMutexUnlock in cleanup:
> may be called without holding the lock.
Yep. I saw it… another patch was in planning :)
>
> Perhaps this and other *Register/*Deregister API's should take the lock
> around anything that could end up in cleanup - perhaps even the same
> history as the previous patch
This may lead to locking issues and it would increase the critical
sections for “no reason”.
What’s about:
Add another goto-label? Not beautiful but it would work.
That option could work too... I was thinking more about how to avoid the
call Unlock when we don't have the lock. Probably doesn't matter how
it's done.
John
>
> John
>
>> diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
>> index 041e7c7440bf..d6699418392e 100644
>> --- a/src/remote/remote_daemon_dispatch.c
>> +++ b/src/remote/remote_daemon_dispatch.c
>> @@ -3841,8 +3841,6 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server,
>> virNetServerClientGetPrivateData(client);
>> virNetServerProgramPtr program;
>>
>> - virMutexLock(&priv->lock);
>> -
>> if (!priv->conn) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("connection not open"));
>> goto cleanup;
>> @@ -3853,6 +3851,8 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server,
>> goto cleanup;
>> }
>>
>> + virMutexLock(&priv->lock);
>> +
>> if (VIR_ALLOC(callback) < 0)
>> goto cleanup;
>> callback->client = virObjectRef(client);
>> @@ -3887,13 +3887,13 @@
remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server ATTRIBUTE_UN
>> struct daemonClientPrivate *priv =
>> virNetServerClientGetPrivateData(client);
>>
>> - virMutexLock(&priv->lock);
>> -
>> if (!priv->conn) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("connection not open"));
>> goto cleanup;
>> }
>>
>> + virMutexLock(&priv->lock);
>> +
>> if (virConnectUnregisterCloseCallback(priv->conn,
>> remoteRelayConnectionClosedEvent) <
0)
>> goto cleanup;
>>
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
2112
days inactive
2229
days old
34 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
John Ferlan -
Marc Hartmayer -
Marc Hartmayer