11:44 a.m.
The second patch of this patch series fixes the behavior of
virConnectRegisterCloseCallback.
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:
+ v3->v4:
- Rebased to current master
- Added Pavel's r-bs
- Worked in Pavel's comments
- Added patch "remote: shrink the critical sections" in preparation
for patch "remote/rpc: Use virNetServerGetProgram() to determine
the program"
+ v2->v3:
- Rebased to current master
- Added Johns r-b to the first patch, all other r-b's I have
dropped as there were to many changes in the meantime
- Removed accepted patches
- Dropped patches 8 and 9
+ v1->v2:
- Removed accepted patches
- Removed NACKed patches
- Added r-b to patch 5
- Worked in comments
- Rebased
- Added patches 7-9
Marc Hartmayer (7):
rpc: use the return value of virObjectRef directly
virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no
connectRegisterCloseCallback
remote: Save reference to program in daemonClientEventCallback
remote: Use domainClientEventCallbacks for
remoteReplayConnectionClosedEvent
rpc: Introduce virNetServerGetProgramLocked helper function
remote: shrink the critical sections
remote/rpc: Use virNetServerGetProgram() to determine the program
src/libvirt-host.c | 16 +-
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 2 -
src/remote/remote_daemon_dispatch.c | 363 +++++++++++++++++-----------
src/rpc/gendispatch.pl | 6 +
src/rpc/virnetserver.c | 53 +++-
src/rpc/virnetserver.h | 2 +
8 files changed, 293 insertions(+), 154 deletions(-)
--
2.21.0
11:44 a.m.
New subject: [libvirt] [PATCH v4 1/7] 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.ibm.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.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 590e780b641e..673bb7c10c86 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -199,7 +199,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
if (VIR_ALLOC(job) < 0)
goto error;
- job->client = client;
+ job->client = virObjectRef(client);
job->msg = msg;
if (prog) {
@@ -207,7 +207,6 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
priority = virNetServerProgramGetPriority(prog, msg->header.proc);
}
- virObjectRef(client);
if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
virObjectUnref(client);
VIR_FREE(job);
--
2.21.0
11:44 a.m.
New subject: [libvirt] [PATCH v4 2/7] 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 and update
the documentation.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
---
src/libvirt-host.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 221a1b7a4353..94383ed449a9 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn)
* @conn: pointer to connection object
* @cb: callback to invoke upon close
* @opaque: user data to pass to @cb
- * @freecb: callback to free @opaque
+ * @freecb: callback to free @opaque when not used anymore
*
* Registers a callback to be invoked when the connection
* is closed. This callback is invoked when there is any
@@ -1412,7 +1412,9 @@ virConnectIsAlive(virConnectPtr conn)
*
* The @freecb must not invoke any other libvirt public
* APIs, since it is not called from a re-entrant safe
- * context.
+ * context. Only if virConnectRegisterCloseCallback is
+ * successful, @freecb will be called, otherwise the
+ * caller is responsible to free @opaque.
*
* Returns 0 on success, -1 on error
*/
@@ -1428,9 +1430,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.21.0
7:11 p.m.
New subject: [libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On 11/14/19 12:44 PM, 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.
Full context:
v1 subtrhead with jferlan and danpb:
https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
v2 subthread with john:
https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html
My first thought is, why not just make this API start raising an error
if it isn't supported?
But you tried that here:
https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html
I'm not really sure I buy the argument that we can't change the
semantics of the API here. This is the only callback API that seems to
not raise an explicit error. It's documented to raise an error. And
there's possible memory leak due the ambiguity.
Yeah I see that virsh needs to be updated to match. In practice virsh
shouldn't be a problem: this issue will only hit for local drivers, and
virsh and client library will be updated together for that case.
In theory if a python app is using this API and not expecting an
exception, it could cause a regression. But it's also informing them
that, hey, that connection callback you requested wasn't working in the
first place. So it's arguably a correctness issue.
So IMO we should be able to adjust this to return a proper error.
BUT, if we stick with this direction, then we need to extend the doc
text here to describe all of this:
* Returns -1 if the driver can support close callback, but registering
one failed. User must free opaque?
* Returns 0 if the driver does not support close callback. We will free
data for you
* Returns 0 if the driver successfully registered a close callback. When
that callback is triggered, opaque will be free'd
But that sounds pretty nutty IMO :/
Therefore call directly @freecb if the connectRegisterCloseCallback
API is not supported by the driver used by the connection and update
the documentation.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
---
src/libvirt-host.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 221a1b7a4353..94383ed449a9 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn)
* @conn: pointer to connection object
* @cb: callback to invoke upon close
* @opaque: user data to pass to @cb
- * @freecb: callback to free @opaque
+ * @freecb: callback to free @opaque when not used anymore
*
* Registers a callback to be invoked when the connection
* is closed. This callback is invoked when there is any
@@ -1412,7 +1412,9 @@ virConnectIsAlive(virConnectPtr conn)
*
* The @freecb must not invoke any other libvirt public
* APIs, since it is not called from a re-entrant safe
- * context.
+ * context. Only if virConnectRegisterCloseCallback is
+ * successful, @freecb will be called, otherwise the
+ * caller is responsible to free @opaque.
This reads wrong to me. If cb() is successfully registered, then
freecb() is invoked automatically after cb() is called, right? This
comment makes it sound like freecb() is invoked immediately when
virConnectRegisterCloseCallback returns 0
Hopefully I'm not confusing things more :)
- Cole
*
* Returns 0 on success, -1 on error
*/
@@ -1428,9 +1430,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;
7:46 a.m.
New subject: [libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson <crobinso(a)redhat.com> wrote:
On 11/14/19 12:44 PM, 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.
>
Full context:
v1 subtrhead with jferlan and danpb:
https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
v2 subthread with john:
https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html
My first thought is, why not just make this API start raising an error
if it isn't supported?
But you tried that here:
https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html
First, thanks for doing all the “history research”.
I'm not really sure I buy the argument that we can't change the
semantics of the API here. This is the only callback API that seems to
not raise an explicit error. It's documented to raise an error. And
there's possible memory leak due the ambiguity.
If we’re doing this so let’s fix the behavior of
'virConnectUnregisterCloseCallback' as well.
Yeah I see that virsh needs to be updated to match. In practice virsh
shouldn't be a problem: this issue will only hit for local drivers, and
virsh and client library will be updated together for that case.
In theory if a python app is using this API and not expecting an
exception, it could cause a regression. But it's also informing them
that, hey, that connection callback you requested wasn't working in the
first place. So it's arguably a correctness issue.
So IMO we should be able to adjust this to return a proper error.
BUT, if we stick with this direction, then we need to extend the doc
text here to describe all of this:
* Returns -1 if the driver can support close callback, but registering
one failed. User must free opaque?
* Returns 0 if the driver does not support close callback. We will free
data for you
* Returns 0 if the driver successfully registered a close callback. When
that callback is triggered, opaque will be free'd
But that sounds pretty nutty IMO :/
I know…
> Therefore call directly @freecb if the connectRegisterCloseCallback
> API is not supported by the driver used by the connection and update
> the documentation.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> ---
> src/libvirt-host.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
> index 221a1b7a4353..94383ed449a9 100644
> --- a/src/libvirt-host.c
> +++ b/src/libvirt-host.c
> @@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn)
> * @conn: pointer to connection object
> * @cb: callback to invoke upon close
> * @opaque: user data to pass to @cb
> - * @freecb: callback to free @opaque
> + * @freecb: callback to free @opaque when not used anymore
> *
> * Registers a callback to be invoked when the connection
> * is closed. This callback is invoked when there is any
> @@ -1412,7 +1412,9 @@ virConnectIsAlive(virConnectPtr conn)
> *
> * The @freecb must not invoke any other libvirt public
> * APIs, since it is not called from a re-entrant safe
> - * context.
> + * context. Only if virConnectRegisterCloseCallback is
> + * successful, @freecb will be called, otherwise the
> + * caller is responsible to free @opaque.
This reads wrong to me. If cb() is successfully registered, then
freecb() is invoked automatically after cb() is called, right?
Yep, or if the callback is unregistered.
This
comment makes it sound like freecb() is invoked immediately when
virConnectRegisterCloseCallback returns 0
I can reword it.
Hopefully I'm not confusing things more :)
No, I appreciate that you’re looking for it.
- Cole
> *
> * Returns 0 on success, -1 on error
> */
> @@ -1428,9 +1430,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;
>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
2:32 p.m.
New subject: [libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On 12/12/19 8:46 AM, Marc Hartmayer wrote:
On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson
<crobinso(a)redhat.com> wrote:
> On 11/14/19 12:44 PM, 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.
>>
>
>
> Full context:
> v1 subtrhead with jferlan and danpb:
> https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html
> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>
> v2 subthread with john:
> https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html
>
> My first thought is, why not just make this API start raising an error
> if it isn't supported?
>
> But you tried that here:
> https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html
First, thanks for doing all the “history research”.
>
> I'm not really sure I buy the argument that we can't change the
> semantics of the API here. This is the only callback API that seems to
> not raise an explicit error. It's documented to raise an error. And
> there's possible memory leak due the ambiguity.
If we’re doing this so let’s fix the behavior of
'virConnectUnregisterCloseCallback' as well.
>
> Yeah I see that virsh needs to be updated to match. In practice virsh
> shouldn't be a problem: this issue will only hit for local drivers, and
> virsh and client library will be updated together for that case.
>
> In theory if a python app is using this API and not expecting an
> exception, it could cause a regression. But it's also informing them
> that, hey, that connection callback you requested wasn't working in the
> first place. So it's arguably a correctness issue.
>
> So IMO we should be able to adjust this to return a proper error.
>
>
> BUT, if we stick with this direction, then we need to extend the doc
> text here to describe all of this:
>
> * Returns -1 if the driver can support close callback, but registering
> one failed. User must free opaque?
> * Returns 0 if the driver does not support close callback. We will free
> data for you
> * Returns 0 if the driver successfully registered a close callback. When
> that callback is triggered, opaque will be free'd
>
> But that sounds pretty nutty IMO :/
I know…
I did a bit more digging. Even the virsh case isn't the biggest deal
because CloseCallback failing is non-fatal. But like I mentioned before
it shouldn't realistically be hit in practice because we can expect
virsh and libvirt-client to be updated in lockstep.
virt-manager, libguestfs, vdsm, kubevirt don't use this API
nova does (nova/virt/libvirt/host.py) but it has code to catch the error
So IMO this should be changed to have semantics like all the other event
functions. Yes it's a semantic change, but I see it as explicitly
erroring in a case that never actually worked. We've made changes like
that many times, happens with XML validation semi regularly, and the
UNDEFINE flag changes are other notable examples.
danpb has your thinking changed on this? Previous discussion context is
in this thread:
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
Thanks,
Cole
3:34 a.m.
New subject: [libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Fri, Dec 13, 2019 at 03:32 PM -0500, Cole Robinson <crobinso(a)redhat.com> wrote:
On 12/12/19 8:46 AM, Marc Hartmayer wrote:
> On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson <crobinso(a)redhat.com>
wrote:
>> On 11/14/19 12:44 PM, 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.
>>>
>>
>>
>> Full context:
>> v1 subtrhead with jferlan and danpb:
>> https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html
>> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>>
>> v2 subthread with john:
>> https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html
>>
>> My first thought is, why not just make this API start raising an error
>> if it isn't supported?
>>
>> But you tried that here:
>> https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html
>
> First, thanks for doing all the “history research”.
>
>>
>> I'm not really sure I buy the argument that we can't change the
>> semantics of the API here. This is the only callback API that seems to
>> not raise an explicit error. It's documented to raise an error. And
>> there's possible memory leak due the ambiguity.
>
> If we’re doing this so let’s fix the behavior of
> 'virConnectUnregisterCloseCallback' as well.
>
>>
>> Yeah I see that virsh needs to be updated to match. In practice virsh
>> shouldn't be a problem: this issue will only hit for local drivers, and
>> virsh and client library will be updated together for that case.
>>
>> In theory if a python app is using this API and not expecting an
>> exception, it could cause a regression. But it's also informing them
>> that, hey, that connection callback you requested wasn't working in the
>> first place. So it's arguably a correctness issue.
>>
>> So IMO we should be able to adjust this to return a proper error.
>>
>>
>> BUT, if we stick with this direction, then we need to extend the doc
>> text here to describe all of this:
>>
>> * Returns -1 if the driver can support close callback, but registering
>> one failed. User must free opaque?
>> * Returns 0 if the driver does not support close callback. We will free
>> data for you
>> * Returns 0 if the driver successfully registered a close callback. When
>> that callback is triggered, opaque will be free'd
>>
>> But that sounds pretty nutty IMO :/
>
> I know…
I did a bit more digging. Even the virsh case isn't the biggest deal
because CloseCallback failing is non-fatal. But like I mentioned before
it shouldn't realistically be hit in practice because we can expect
virsh and libvirt-client to be updated in lockstep.
virt-manager, libguestfs, vdsm, kubevirt don't use this API
nova does (nova/virt/libvirt/host.py) but it has code to catch the error
So IMO this should be changed to have semantics like all the other event
functions. Yes it's a semantic change, but I see it as explicitly
erroring in a case that never actually worked. We've made changes like
that many times, happens with XML validation semi regularly, and the
UNDEFINE flag changes are other notable examples.
danpb has your thinking changed on this? Previous discussion context is
in this thread:
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
Thanks,
Cole
Polite ping.
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
5:17 a.m.
New subject: [libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Tue, Jan 14, 2020 at 10:34:21AM +0100, Marc Hartmayer wrote:
On Fri, Dec 13, 2019 at 03:32 PM -0500, Cole Robinson
<crobinso(a)redhat.com> wrote:
> On 12/12/19 8:46 AM, Marc Hartmayer wrote:
>> On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson
<crobinso(a)redhat.com> wrote:
>>> On 11/14/19 12:44 PM, Marc Hartmayer wrote:
>
> danpb has your thinking changed on this? Previous discussion context is
> in this thread:
> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>
> Thanks,
> Cole
>
Polite ping.
Sorry for the ridiculous delay in responding. This has been on my
todo list for ages and I didn't prioritize completing it well enough.
I've sent a mail with my thoughts now.
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 :|
5:14 a.m.
New subject: [libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Wed, Dec 11, 2019 at 08:11:38PM -0500, Cole Robinson wrote:
On 11/14/19 12:44 PM, 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.
>
Full context:
v1 subtrhead with jferlan and danpb:
https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
v2 subthread with john:
https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html
My first thought is, why not just make this API start raising an error
if it isn't supported?
But you tried that here:
https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html
I'm not really sure I buy the argument that we can't change the
semantics of the API here. This is the only callback API that seems to
not raise an explicit error. It's documented to raise an error. And
there's possible memory leak due the ambiguity.
It can raise an error because you are only permitted to register the
close callback once - a duplicated call reports an error. Also any
other invalid parameters result in an error. So this is not
inconsistent with the idea that registering a close callback is
supported for all drivers.
Yeah I see that virsh needs to be updated to match. In practice virsh
shouldn't be a problem: this issue will only hit for local drivers, and
virsh and client library will be updated together for that case.
The very fact that we need to update virsh shows that this would
be an API regression. That we only know of virsh having a problem
is not a valid reason. It is only by luck that virt-viewer doesn't
have the same problem, because for inexplicable reasons we didn't
handling the error as an error condition.
BUT, if we stick with this direction, then we need to extend the doc
text here to describe all of this:
* Returns -1 if the driver can support close callback, but registering
one failed. User must free opaque?
* Returns 0 if the driver does not support close callback. We will free
data for you
* Returns 0 if the driver successfully registered a close callback. When
that callback is triggered, opaque will be free'd
But that sounds pretty nutty IMO :/
This is giving apps an uncessary level of impl detail for their
needs.
* Returns -1: if a close callback was already registered, or
if one of the parameters was invalid.
* Returns 0: if the close callback was successfully registered
The driver specific caveat is described earlier in the docs, that
not all drivers will invoke the close callback, as some may not
ever be in a situation where there is a connection is closed. Not
ever invoking the callback is not a bug, as it simply means the
error condition the callback is designed to report has not
arisen.
To fix the leak of te opaque data, we need to partially revert
the change that caused this leak in the first place
88f09b75eb99415c7f2ce3d1b010600ba8e37580.
That commit introduces some per driver handling into the close
callbacks. This is conceptually fine. The mistake was that the
virConnectCloseCallbackDataPtr closeCallback;
field was moved out of virConnectPtr, and into the per-driver
private structs. This meant that we no longer kept track of
the callback in other drivers, and thus no longer free'd the
opaque data when calling Unregister.
So from the public API entry point I think we need approx
this change:
diff --git a/src/datatypes.h b/src/datatypes.h
index 2d0407f7ec..924ef8d8e9 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -546,6 +546,9 @@ struct _virConnect {
virError err; /* the last error */
virErrorFunc handler; /* associated handler */
void *userData; /* the user data */
+
+ /* Per-connection close callback */
+ virConnectCloseCallbackDataPtr closeCallback;
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virConnect, virObjectUnref);
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index bc3d1d2803..68517bae9c 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1444,9 +1444,20 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
virCheckNonNullArgGoto(cb, error);
+ if (virConnectCloseCallbackDataGetCallback(conn->closeCallback) != cb) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("A different callback was requested"));
+ goto error;
+ }
+
+ virConnectCloseCallbackDataRegister(conn->closeCallback, conn, cb,
+ opaque, freecb);
+
if (conn->driver->connectRegisterCloseCallback &&
- conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) <
0)
+ conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) <
0) {
+ virConnectCloseCallbackDataUnregister(conn->closeCallback, cb);
goto error;
+ }
return 0;
@@ -1483,6 +1494,8 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
conn->driver->connectUnregisterCloseCallback(conn, cb) < 0)
goto error;
+ virConnectCloseCallbackDataUnregister(conn->closeCallback, cb);
+
return 0;
error:
The remote & vz drivers then need to be updated to use the
conn->closeCallback and remove their own direct calls to
virConnectCloseCallbackData{Unregister,Register}
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 :|
11:44 a.m.
New subject: [libvirt] [PATCH v4 3/7] remote: Save reference to program in daemonClientEventCallback
As a result, you can later determine during the callback which program
was 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.ibm.com>
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.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 f369ffb02a65..7b53c2241c05 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;
@@ -149,6 +150,7 @@ remoteEventCallbackFree(void *opaque)
daemonClientEventCallbackPtr callback = opaque;
if (!callback)
return;
+ virObjectUnref(callback->program);
virObjectUnref(callback->client);
VIR_FREE(callback);
}
@@ -334,7 +336,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);
@@ -342,7 +344,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);
@@ -371,14 +373,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);
}
@@ -410,14 +412,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);
}
@@ -448,14 +450,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);
}
@@ -491,14 +493,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);
}
@@ -536,14 +538,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);
}
@@ -606,14 +608,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);
}
@@ -647,14 +649,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);
}
@@ -683,14 +685,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);
}
@@ -736,14 +738,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);
}
@@ -777,14 +779,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);
}
@@ -813,14 +815,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);
}
@@ -849,14 +851,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);
}
@@ -886,14 +888,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);
}
@@ -923,14 +925,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);
}
@@ -962,7 +964,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);
@@ -970,7 +972,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);
@@ -1006,7 +1008,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);
@@ -1045,7 +1047,7 @@ remoteRelayDomainEventTunable(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
- 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);
@@ -1079,7 +1081,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);
@@ -1111,7 +1113,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);
@@ -1144,7 +1146,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);
@@ -1184,7 +1186,7 @@ remoteRelayDomainEventJobCompleted(virConnectPtr conn,
data.callbackID = callback->callbackID;
make_nonnull_domain(&data.dom, dom);
- 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);
@@ -1216,7 +1218,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);
@@ -1254,7 +1256,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);
@@ -1294,7 +1296,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);
@@ -1356,7 +1358,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);
@@ -1393,7 +1395,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);
@@ -1421,7 +1423,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);
@@ -1460,7 +1462,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);
@@ -1488,7 +1490,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);
@@ -1527,7 +1529,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);
@@ -1555,7 +1557,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);
@@ -1603,7 +1605,7 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
}
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);
@@ -4253,6 +4255,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
G_GNUC_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;
@@ -4481,6 +4484,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
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;
@@ -4556,6 +4560,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server G_GNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6038,6 +6043,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSE
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6158,6 +6164,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server G_GNUC_U
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6277,6 +6284,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server G_GNUC_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;
@@ -6396,6 +6404,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6510,6 +6519,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
G_GNUC_UNUS
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(qemuProgram);
callback->eventID = -1;
callback->callbackID = -1;
ref = callback;
--
2.21.0
11:44 a.m.
New subject: [libvirt] [PATCH v4 4/7] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
This allows us later to get rid of another usage of the global
variable `remoteProgram`.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/remote/remote_daemon_dispatch.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 7b53c2241c05..9dc2083d715a 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1620,12 +1620,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
static
void remoteRelayConnectionClosedEvent(virConnectPtr conn G_GNUC_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);
@@ -4170,6 +4170,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSED,
virNetMessageErrorPtr rerr)
{
int rv = -1;
+ daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
@@ -4179,9 +4180,17 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSED,
if (!conn)
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(conn,
remoteRelayConnectionClosedEvent,
- client, NULL) < 0)
+ callback, remoteEventCallbackFree) < 0)
goto cleanup;
priv->closeRegistered = true;
@@ -4189,8 +4198,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSED,
cleanup:
virMutexUnlock(&priv->lock);
- if (rv < 0)
+ if (rv < 0) {
+ remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
+ }
return rv;
}
--
2.21.0
11:44 a.m.
New subject: [libvirt] [PATCH v4 5/7] 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.ibm.com>
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/rpc/virnetserver.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 673bb7c10c86..41226368058f 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -166,6 +166,26 @@ 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,
@@ -175,18 +195,12 @@ 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);
/* we can unlock @srv since @prog can only become invalid in case
* of disposing @srv, but let's grab a ref first to ensure nothing
* disposes of it before we use it. */
--
2.21.0
11:44 a.m.
New subject: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
To free the structs and save the error, it is not necessary to hold @priv->lock,
therefore move these parts after the mutex unlock.
Signed-off-by: Marc Hartmayer <mhartmay(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 9dc2083d715a..6ece51c2889d 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4293,10 +4293,10 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
G_GNUC_UNUSED,
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -4342,9 +4342,9 @@ remoteDispatchConnectDomainEventDeregister(virNetServerPtr server
G_GNUC_UNUSED,
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -4522,10 +4522,10 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -4598,11 +4598,11 @@
remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server G_GNU
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(dom);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -4657,9 +4657,9 @@ remoteDispatchConnectDomainEventDeregisterAny(virNetServerPtr server
G_GNUC_UNUS
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -4702,9 +4702,9 @@
remoteDispatchConnectDomainEventCallbackDeregisterAny(virNetServerPtr server G_G
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6081,11 +6081,11 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr
server G_GNUC_UNUSE
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(net);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6128,9 +6128,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr
server G_GNUC_UNU
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6202,11 +6202,11 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server G_GNUC_U
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(pool);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6248,9 +6248,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr
server G_GNUC
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6322,11 +6322,11 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server G_GNUC_UN
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(dev);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6368,9 +6368,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr
server G_GNUC_
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6442,11 +6442,11 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(secret);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6488,9 +6488,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server
G_GNUC_UNUS
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6558,11 +6558,11 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr
server G_GNUC_UNUS
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(dom);
- virMutexUnlock(&priv->lock);
return rv;
}
@@ -6606,9 +6606,9 @@ qemuDispatchConnectDomainMonitorEventDeregister(virNetServerPtr
server G_GNUC_UN
rv = 0;
cleanup:
+ virMutexUnlock(&priv->lock);
if (rv < 0)
virNetMessageSaveError(rerr);
- virMutexUnlock(&priv->lock);
return rv;
}
--
2.21.0
6:48 p.m.
New subject: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
On 11/14/19 12:44 PM, Marc Hartmayer wrote:
To free the structs and save the error, it is not necessary to hold
@priv->lock,
therefore move these parts after the mutex unlock.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
1 file changed, 16 insertions(+), 16 deletions(-)
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
Do I understand correctly that 1,3-5 are all independent and can be
pushed separately? If so I will do that tomorrow. I'm doing some
archaeology on patch #2
- Cole
8:13 a.m.
New subject: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
On Wed, Dec 11, 2019 at 07:48 PM -0500, Cole Robinson <crobinso(a)redhat.com> wrote:
On 11/14/19 12:44 PM, Marc Hartmayer wrote:
> To free the structs and save the error, it is not necessary to hold @priv->lock,
> therefore move these parts after the mutex unlock.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> ---
> src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
Do I understand correctly that 1,3-5 are all independent and can be
pushed separately? If so I will do that tomorrow. I'm doing some
archaeology on patch #2
1, 3, and 5 are all independent.
Patch 4 depends on the second patch as
remoteDispatchConnectRegisterCloseCallback uses
virConnectRegisterCloseCallback. Otherwise we would never do the unref
for @client and @program when conn->driver->connectRegisterCloseCallback
was NULL.
- Cole
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
2 p.m.
New subject: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
On 12/12/19 9:13 AM, Marc Hartmayer wrote:
On Wed, Dec 11, 2019 at 07:48 PM -0500, Cole Robinson
<crobinso(a)redhat.com> wrote:
> On 11/14/19 12:44 PM, Marc Hartmayer wrote:
>> To free the structs and save the error, it is not necessary to hold
@priv->lock,
>> therefore move these parts after the mutex unlock.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
>> ---
>> src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
>> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
>
> Do I understand correctly that 1,3-5 are all independent and can be
> pushed separately? If so I will do that tomorrow. I'm doing some
> archaeology on patch #2
1, 3, and 5 are all independent.
Patch 4 depends on the second patch as
remoteDispatchConnectRegisterCloseCallback uses
virConnectRegisterCloseCallback. Otherwise we would never do the unref
for @client and @program when conn->driver->connectRegisterCloseCallback
was NULL.
Thanks, I pushed 1, 3, 5, 6. I'll reply to your other message
- Cole
3:36 a.m.
New subject: [libvirt] [PATCH v4 6/7] remote: shrink the critical sections
On Fri, Dec 13, 2019 at 03:00 PM -0500, Cole Robinson <crobinso(a)redhat.com> wrote:
On 12/12/19 9:13 AM, Marc Hartmayer wrote:
> On Wed, Dec 11, 2019 at 07:48 PM -0500, Cole Robinson <crobinso(a)redhat.com>
wrote:
>> On 11/14/19 12:44 PM, Marc Hartmayer wrote:
>>> To free the structs and save the error, it is not necessary to hold
@priv->lock,
>>> therefore move these parts after the mutex unlock.
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
>>> ---
>>> src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++---------------
>>> 1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
>>
>> Do I understand correctly that 1,3-5 are all independent and can be
>> pushed separately? If so I will do that tomorrow. I'm doing some
>> archaeology on patch #2
>
> 1, 3, and 5 are all independent.
>
> Patch 4 depends on the second patch as
> remoteDispatchConnectRegisterCloseCallback uses
> virConnectRegisterCloseCallback. Otherwise we would never do the unref
> for @client and @program when conn->driver->connectRegisterCloseCallback
> was NULL.
Thanks, I pushed 1, 3, 5, 6. I'll reply to your other message
- Cole
Thanks.
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:44 a.m.
New subject: [libvirt] [PATCH v4 7/7] 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.ibm.com>
---
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 2 -
src/remote/remote_daemon_dispatch.c | 238 ++++++++++++++++++----------
src/rpc/gendispatch.pl | 6 +
src/rpc/virnetserver.c | 22 +++
src/rpc/virnetserver.h | 2 +
7 files changed, 187 insertions(+), 88 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0493467f4603..a6883f373608 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -124,6 +124,7 @@ virNetServerGetCurrentUnauthClients;
virNetServerGetMaxClients;
virNetServerGetMaxUnauthClients;
virNetServerGetName;
+virNetServerGetProgram;
virNetServerGetThreadPoolParameters;
virNetServerHasClients;
virNetServerNeedsAuth;
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index b400b1dd1059..a36b5449d5ae 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -73,8 +73,6 @@ VIR_LOG_INIT("daemon." DAEMON_NAME);
#if WITH_SASL
virNetSASLContextPtr saslCtxt = NULL;
#endif
-virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr qemuProgram = NULL;
volatile bool driversInitialized = false;
@@ -996,6 +994,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 a2d9af403619..a3d6a220f868 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -97,5 +97,3 @@ struct daemonClientPrivate {
#if WITH_SASL
extern virNetSASLContextPtr saslCtxt;
#endif
-extern virNetServerProgramPtr remoteProgram;
-extern virNetServerProgramPtr qemuProgram;
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 6ece51c2889d..7ebb97f49f3b 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4164,9 +4164,9 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server
G_GNUC_UNUSED,
}
static int
-remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr)
{
int rv = -1;
@@ -4174,30 +4174,37 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSED,
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
+ virNetServerProgramPtr program;
+
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
virMutexLock(&priv->lock);
if (!conn)
- goto cleanup;
+ goto cleanup_unlock;
if (VIR_ALLOC(callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
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;
if (virConnectRegisterCloseCallback(conn,
remoteRelayConnectionClosedEvent,
callback, remoteEventCallbackFree) < 0)
- goto cleanup;
+ goto cleanup_unlock;
priv->closeRegistered = true;
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(&priv->lock);
+ cleanup:
if (rv < 0) {
remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
@@ -4236,9 +4243,9 @@ remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSE
}
static int
-remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectDomainEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr G_GNUC_UNUSED,
remote_connect_domain_event_register_ret *ret
G_GNUC_UNUSED)
{
@@ -4249,11 +4256,17 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
G_GNUC_UNUSED,
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
+ virNetServerProgramPtr program;
+
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
virMutexLock(&priv->lock);
if (!conn)
- goto cleanup;
+ goto cleanup_unlock;
/* If we call register first, we could append a complete callback
* to our array, but on OOM append failure, we'd have to then hope
@@ -4264,9 +4277,9 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
G_GNUC_UNUSED,
* between 'ref' and 'callback'.
*/
if (VIR_ALLOC(callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
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;
@@ -4274,7 +4287,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
G_GNUC_UNUSED,
if (VIR_APPEND_ELEMENT(priv->domainEventCallbacks,
priv->ndomainEventCallbacks,
callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
if ((callbackID = virConnectDomainEventRegisterAny(conn,
NULL,
@@ -4285,15 +4298,16 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
G_GNUC_UNUSED,
VIR_SHRINK_N(priv->domainEventCallbacks,
priv->ndomainEventCallbacks, 1);
callback = ref;
- goto cleanup;
+ goto cleanup_unlock;
}
ref->callbackID = callbackID;
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(&priv->lock);
+ cleanup:
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
@@ -4457,9 +4471,9 @@ remoteDispatchDomainGetState(virNetServerPtr server G_GNUC_UNUSED,
* VIR_DRV_SUPPORTS_FEATURE(VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK),
* and must not mix the two styles. */
static int
-remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr G_GNUC_UNUSED,
remote_connect_domain_event_register_any_args
*args)
{
@@ -4470,11 +4484,17 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
+ virNetServerProgramPtr program;
+
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
virMutexLock(&priv->lock);
if (!conn)
- goto cleanup;
+ goto cleanup_unlock;
/* We intentionally do not use VIR_DOMAIN_EVENT_ID_LAST here; any
* new domain events added after this point should only use the
@@ -4483,7 +4503,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
args->eventID < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported event ID %d"),
args->eventID);
- goto cleanup;
+ goto cleanup_unlock;
}
/* If we call register first, we could append a complete callback
@@ -4493,9 +4513,9 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
* our callback; but since VIR_APPEND_ELEMENT clears 'callback' on
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
callback->legacy = true;
@@ -4503,7 +4523,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
if (VIR_APPEND_ELEMENT(priv->domainEventCallbacks,
priv->ndomainEventCallbacks,
callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
if ((callbackID = virConnectDomainEventRegisterAny(conn,
NULL,
@@ -4514,15 +4534,16 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
VIR_SHRINK_N(priv->domainEventCallbacks,
priv->ndomainEventCallbacks, 1);
callback = ref;
- goto cleanup;
+ goto cleanup_unlock;
}
ref->callbackID = callbackID;
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(&priv->lock);
+ cleanup:
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
@@ -4531,9 +4552,9 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
static int
-remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server
G_GNUC_UNUSED,
+remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
G_GNUC_UNUSED,
remote_connect_domain_event_callback_register_any_args *args,
remote_connect_domain_event_callback_register_any_ret *ret)
@@ -4546,20 +4567,26 @@
remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server G_GNU
virNetServerClientGetPrivateData(client);
virDomainPtr dom = NULL;
virConnectPtr conn = remoteGetHypervisorConn(client);
+ virNetServerProgramPtr program;
+
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
virMutexLock(&priv->lock);
if (!conn)
- goto cleanup;
+ goto cleanup_unlock;
if (args->dom &&
!(dom = get_nonnull_domain(conn, *args->dom)))
- goto cleanup;
+ goto cleanup_unlock;
if (args->eventID >= VIR_DOMAIN_EVENT_ID_LAST || args->eventID < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported event ID %d"),
args->eventID);
- goto cleanup;
+ goto cleanup_unlock;
}
/* If we call register first, we could append a complete callback
@@ -4569,16 +4596,16 @@
remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server G_GNU
* our callback; but since VIR_APPEND_ELEMENT clears 'callback' on
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
if (VIR_APPEND_ELEMENT(priv->domainEventCallbacks,
priv->ndomainEventCallbacks,
callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
if ((callbackID = virConnectDomainEventRegisterAny(conn,
dom,
@@ -4589,7 +4616,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server G_GNU
VIR_SHRINK_N(priv->domainEventCallbacks,
priv->ndomainEventCallbacks, 1);
callback = ref;
- goto cleanup;
+ goto cleanup_unlock;
}
ref->callbackID = callbackID;
@@ -4597,8 +4624,9 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server G_GNU
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(&priv->lock);
+ cleanup:
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
@@ -5641,7 +5669,7 @@ remoteDispatchDomainMigratePrepare3Params(virNetServerPtr server
G_GNUC_UNUSED,
}
static int
-remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
virNetMessageErrorPtr rerr,
@@ -5656,6 +5684,7 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr
server G_GNUC_UN
virStreamPtr st = NULL;
daemonClientStreamPtr stream = NULL;
virConnectPtr conn = remoteGetHypervisorConn(client);
+ virNetServerProgramPtr program;
if (!conn)
goto cleanup;
@@ -5672,8 +5701,13 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr
server G_GNUC_UN
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(conn, VIR_STREAM_NONBLOCK)) ||
- !(stream = daemonCreateClientStream(client, st, remoteProgram,
+ !(stream = daemonCreateClientStream(client, st, program,
&msg->header, false)))
goto cleanup;
@@ -6014,9 +6048,9 @@ static int remoteDispatchDomainCreateWithFiles(virNetServerPtr
server G_GNUC_UNU
static int
-remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr G_GNUC_UNUSED,
remote_connect_network_event_register_any_args *args,
remote_connect_network_event_register_any_ret *ret)
@@ -6029,20 +6063,26 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr
server G_GNUC_UNUSE
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetNetworkConn(client);
+ virNetServerProgramPtr program;
+
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
virMutexLock(&priv->lock);
if (!conn)
- goto cleanup;
+ goto cleanup_unlock;
if (args->net &&
!(net = get_nonnull_network(conn, *args->net)))
- goto cleanup;
+ goto cleanup_unlock;
if (args->eventID >= VIR_NETWORK_EVENT_ID_LAST || args->eventID < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unsupported network event ID %d"),
args->eventID);
- goto cleanup;
+ goto cleanup_unlock;
}
/* If we call register first, we could append a complete callback
@@ -6052,16 +6092,16 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr
server G_GNUC_UNUSE
* our callback; but since VIR_APPEND_ELEMENT clears 'callback' on
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
if (VIR_APPEND_ELEMENT(priv->networkEventCallbacks,
priv->nnetworkEventCallbacks,
callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
if ((callbackID = virConnectNetworkEventRegisterAny(conn,
net,
@@ -6072,7 +6112,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSE
VIR_SHRINK_N(priv->networkEventCallbacks,
priv->nnetworkEventCallbacks, 1);
callback = ref;
- goto cleanup;
+ goto cleanup_unlock;
}
ref->callbackID = callbackID;
@@ -6080,8 +6120,9 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSE
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(&priv->lock);
+ cleanup:
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
@@ -6135,9 +6176,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr
server G_GNUC_UNU
}
static int
-remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
G_GNUC_UNUSED,
remote_connect_storage_pool_event_register_any_args *args,
remote_connect_storage_pool_event_register_any_ret *ret)
@@ -6150,20 +6191,26 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server G_GNUC_U
virNetServerClientGetPrivateData(client);
virStoragePoolPtr pool = NULL;
virConnectPtr conn = remoteGetStorageConn(client);
+ virNetServerProgramPtr program;
+
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
virMutexLock(&priv->lock);
if (!conn)
- goto cleanup;
+ goto cleanup_unlock;
if (args->pool &&
!(pool = get_nonnull_storage_pool(conn, *args->pool)))
- goto cleanup;
+ goto cleanup_unlock;
if (args->eventID >= VIR_STORAGE_POOL_EVENT_ID_LAST || args->eventID < 0)
{
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unsupported storage pool event ID %d"),
args->eventID);
- goto cleanup;
+ goto cleanup_unlock;
}
/* If we call register first, we could append a complete callback
@@ -6173,16 +6220,16 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server G_GNUC_U
* our callback; but since VIR_APPEND_ELEMENT clears 'callback' on
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
if (VIR_APPEND_ELEMENT(priv->storageEventCallbacks,
priv->nstorageEventCallbacks,
callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
if ((callbackID = virConnectStoragePoolEventRegisterAny(conn,
pool,
@@ -6193,7 +6240,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server G_GNUC_U
VIR_SHRINK_N(priv->storageEventCallbacks,
priv->nstorageEventCallbacks, 1);
callback = ref;
- goto cleanup;
+ goto cleanup_unlock;
}
ref->callbackID = callbackID;
@@ -6201,8 +6248,9 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server G_GNUC_U
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(&priv->lock);
+ cleanup:
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
@@ -6255,9 +6303,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr
server G_GNUC
}
static int
-remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
G_GNUC_UNUSED,
remote_connect_node_device_event_register_any_args *args,
remote_connect_node_device_event_register_any_ret *ret)
@@ -6270,20 +6318,26 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server G_GNUC_UN
virNetServerClientGetPrivateData(client);
virNodeDevicePtr dev = NULL;
virConnectPtr conn = remoteGetNodeDevConn(client);
+ virNetServerProgramPtr program;
+
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
virMutexLock(&priv->lock);
if (!conn)
- goto cleanup;
+ goto cleanup_unlock;
if (args->dev &&
!(dev = get_nonnull_node_device(conn, *args->dev)))
- goto cleanup;
+ goto cleanup_unlock;
if (args->eventID >= VIR_NODE_DEVICE_EVENT_ID_LAST || args->eventID < 0)
{
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unsupported node device event ID %d"),
args->eventID);
- goto cleanup;
+ goto cleanup_unlock;
}
/* If we call register first, we could append a complete callback
@@ -6293,16 +6347,16 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server G_GNUC_UN
* our callback; but since VIR_APPEND_ELEMENT clears 'callback' on
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
if (VIR_APPEND_ELEMENT(priv->nodeDeviceEventCallbacks,
priv->nnodeDeviceEventCallbacks,
callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
if ((callbackID = virConnectNodeDeviceEventRegisterAny(conn,
dev,
@@ -6313,7 +6367,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server G_GNUC_UN
VIR_SHRINK_N(priv->nodeDeviceEventCallbacks,
priv->nnodeDeviceEventCallbacks, 1);
callback = ref;
- goto cleanup;
+ goto cleanup_unlock;
}
ref->callbackID = callbackID;
@@ -6321,8 +6375,9 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server G_GNUC_UN
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(&priv->lock);
+ cleanup:
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
@@ -6375,9 +6430,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr
server G_GNUC_
}
static int
-remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr G_GNUC_UNUSED,
remote_connect_secret_event_register_any_args
*args,
remote_connect_secret_event_register_any_ret
*ret)
@@ -6390,20 +6445,26 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
virNetServerClientGetPrivateData(client);
virSecretPtr secret = NULL;
virConnectPtr conn = remoteGetSecretConn(client);
+ virNetServerProgramPtr program;
+
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
virMutexLock(&priv->lock);
if (!conn)
- goto cleanup;
+ goto cleanup_unlock;
if (args->secret &&
!(secret = get_nonnull_secret(conn, *args->secret)))
- goto cleanup;
+ goto cleanup_unlock;
if (args->eventID >= VIR_SECRET_EVENT_ID_LAST || args->eventID < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unsupported secret event ID %d"), args->eventID);
- goto cleanup;
+ goto cleanup_unlock;
}
/* If we call register first, we could append a complete callback
@@ -6413,16 +6474,16 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
* our callback; but since VIR_APPEND_ELEMENT clears 'callback' on
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
if (VIR_APPEND_ELEMENT(priv->secretEventCallbacks,
priv->nsecretEventCallbacks,
callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
if ((callbackID = virConnectSecretEventRegisterAny(conn,
secret,
@@ -6433,7 +6494,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
VIR_SHRINK_N(priv->secretEventCallbacks,
priv->nsecretEventCallbacks, 1);
callback = ref;
- goto cleanup;
+ goto cleanup_unlock;
}
ref->callbackID = callbackID;
@@ -6441,8 +6502,9 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(&priv->lock);
+ cleanup:
remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
@@ -6495,9 +6557,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server
G_GNUC_UNUS
}
static int
-qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server G_GNUC_UNUSED,
+qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr G_GNUC_UNUSED,
qemu_connect_domain_monitor_event_register_args *args,
qemu_connect_domain_monitor_event_register_ret *ret)
@@ -6511,15 +6573,21 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr
server G_GNUC_UNUS
virDomainPtr dom = NULL;
const char *event = args->event ? *args->event : NULL;
virConnectPtr conn = remoteGetHypervisorConn(client);
+ virNetServerProgramPtr program;
+
+ if (!(program = virNetServerGetProgram(server, msg))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
program found"));
+ goto cleanup;
+ }
virMutexLock(&priv->lock);
if (!conn)
- goto cleanup;
+ goto cleanup_unlock;
if (args->dom &&
!(dom = get_nonnull_domain(conn, *args->dom)))
- goto cleanup;
+ goto cleanup_unlock;
/* If we call register first, we could append a complete callback
* to our array, but on OOM append failure, we'd have to then hope
@@ -6528,16 +6596,16 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr
server G_GNUC_UNUS
* our callback; but since VIR_APPEND_ELEMENT clears 'callback' on
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(qemuProgram);
+ callback->program = virObjectRef(program);
callback->eventID = -1;
callback->callbackID = -1;
ref = callback;
if (VIR_APPEND_ELEMENT(priv->qemuEventCallbacks,
priv->nqemuEventCallbacks,
callback) < 0)
- goto cleanup;
+ goto cleanup_unlock;
if ((callbackID = virConnectDomainQemuMonitorEventRegister(conn,
dom,
@@ -6549,7 +6617,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
G_GNUC_UNUS
VIR_SHRINK_N(priv->qemuEventCallbacks,
priv->nqemuEventCallbacks, 1);
callback = ref;
- goto cleanup;
+ goto cleanup_unlock;
}
ref->callbackID = callbackID;
@@ -6557,6 +6625,8 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
G_GNUC_UNUS
rv = 0;
+ cleanup_unlock:
+ virMutexUnlock(&priv->lock);
cleanup:
virMutexUnlock(&priv->lock);
remoteEventCallbackFree(callback);
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 7c868191d19c..2b8795beec16 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -1052,6 +1052,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 {
@@ -1093,6 +1094,11 @@ elsif ($mode eq "server") {
print " if (!(st = virStreamNew($conn_var,
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 41226368058f..787ace916c9d 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -187,6 +187,28 @@ virNetServerGetProgramLocked(virNetServerPtr srv,
return NULL;
}
+/**
+ * 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,
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 260c99b22d5e..46ecb0e91077 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -90,6 +90,8 @@ int virNetServerAddProgram(virNetServerPtr srv,
int virNetServerSetTLSContext(virNetServerPtr srv,
virNetTLSContextPtr tls);
+virNetServerProgramPtr virNetServerGetProgram(virNetServerPtr srv,
+ virNetMessagePtr msg);
int virNetServerAddClient(virNetServerPtr srv,
virNetServerClientPtr client);
--
2.21.0
3:45 a.m.
On Thu, Nov 14, 2019 at 06:44 PM +0100, Marc Hartmayer <mhartmay(a)linux.ibm.com>
wrote:
The second patch of this patch series fixes the behavior of
virConnectRegisterCloseCallback.
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:
+ v3->v4:
- Rebased to current master
- Added Pavel's r-bs
- Worked in Pavel's comments
- Added patch "remote: shrink the critical sections" in preparation
for patch "remote/rpc: Use virNetServerGetProgram() to determine
the program"
+ v2->v3:
- Rebased to current master
- Added Johns r-b to the first patch, all other r-b's I have
dropped as there were to many changes in the meantime
- Removed accepted patches
- Dropped patches 8 and 9
+ v1->v2:
- Removed accepted patches
- Removed NACKed patches
- Added r-b to patch 5
- Worked in comments
- Rebased
- Added patches 7-9
Marc Hartmayer (7):
rpc: use the return value of virObjectRef directly
virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no
connectRegisterCloseCallback
remote: Save reference to program in daemonClientEventCallback
remote: Use domainClientEventCallbacks for
remoteReplayConnectionClosedEvent
rpc: Introduce virNetServerGetProgramLocked helper function
remote: shrink the critical sections
remote/rpc: Use virNetServerGetProgram() to determine the program
src/libvirt-host.c | 16 +-
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 2 -
src/remote/remote_daemon_dispatch.c | 363 +++++++++++++++++-----------
src/rpc/gendispatch.pl | 6 +
src/rpc/virnetserver.c | 53 +++-
src/rpc/virnetserver.h | 2 +
8 files changed, 293 insertions(+), 154 deletions(-)
--
2.21.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Polite ping :)
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
1771
days inactive
1839
days old
18 comments
3 participants
participants (3)
-
Cole Robinson -
Daniel P. Berrangé -
Marc Hartmayer