12:35 p.m.
After discussions with Peter and Pavel at the KVM forum, I am now
sending a v3 of this series after more than a year... sorry for that
long delay!
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:
+ 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 (6):
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/rpc: Use virNetServerGetProgram() to determine the program
src/libvirt-host.c | 12 +-
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 2 -
src/remote/remote_daemon_dispatch.c | 227 +++++++++++++++++++---------
src/rpc/gendispatch.pl | 6 +
src/rpc/virnetserver.c | 53 +++++--
src/rpc/virnetserver.h | 2 +
8 files changed, 217 insertions(+), 90 deletions(-)
--
2.21.0
12:35 p.m.
New subject: [libvirt] [PATCH v3 1/6] 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>
---
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 042661ffa5ea..54d0e4f31489 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -204,7 +204,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
if (VIR_ALLOC(job) < 0)
goto error;
- job->client = client;
+ job->client = virObjectRef(client);
job->msg = msg;
if (prog) {
@@ -212,7 +212,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
9:50 a.m.
New subject: [libvirt] [PATCH v3 1/6] rpc: use the return value of virObjectRef directly
On Fri, Nov 01, 2019 at 06:35:43PM +0100, Marc Hartmayer wrote:
Use the return value of virObjectRef directly. This way, it's
easier
for another reader to identify the reason why the additional reference
is required.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
---
src/rpc/virnetserver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
12:35 p.m.
New subject: [libvirt] [PATCH v3 2/6] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
The commit 'close callback: move it to driver' (88f09b75eb99) moved
the responsibility for the close callback to the driver. But if the
driver doesn't support the connectRegisterCloseCallback API this
function does nothing, even no unsupported error report. This behavior
may lead to problems, for example memory leaks, as the caller cannot
differentiate whether the close callback was 'really' registered or
not.
Therefore call directly @freecb if the connectRegisterCloseCallback
API is not supported by the driver used by the connection.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
---
src/libvirt-host.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 221a1b7a4353..9c3a19f33b12 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
@@ -1428,9 +1428,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
9:52 a.m.
New subject: [libvirt] [PATCH v3 2/6] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Fri, Nov 01, 2019 at 06:35:44PM +0100, Marc Hartmayer wrote:
The commit 'close callback: move it to driver' (88f09b75eb99)
moved
the responsibility for the close callback to the driver. But if the
driver doesn't support the connectRegisterCloseCallback API this
function does nothing, even no unsupported error report. This behavior
may lead to problems, for example memory leaks, as the caller cannot
differentiate whether the close callback was 'really' registered or
not.
Therefore call directly @freecb if the connectRegisterCloseCallback
API is not supported by the driver used by the connection.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
---
src/libvirt-host.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 221a1b7a4353..9c3a19f33b12 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
@@ -1428,9 +1428,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);
+ }
Looks good but I think we should improve the documentation of this API
to explicitly state that the @freecb is called only on success and if
the API fails the caller is responsible to free the opaque data.
Pavel
12:07 p.m.
New subject: [libvirt] [PATCH v3 2/6] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
On Fri, Nov 08, 2019 at 04:52 PM +0100, Pavel Hrdina <phrdina(a)redhat.com> wrote:
On Fri, Nov 01, 2019 at 06:35:44PM +0100, Marc Hartmayer wrote:
> The commit 'close callback: move it to driver' (88f09b75eb99) moved
> the responsibility for the close callback to the driver. But if the
> driver doesn't support the connectRegisterCloseCallback API this
> function does nothing, even no unsupported error report. This behavior
> may lead to problems, for example memory leaks, as the caller cannot
> differentiate whether the close callback was 'really' registered or
> not.
>
> Therefore call directly @freecb if the connectRegisterCloseCallback
> API is not supported by the driver used by the connection.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> ---
> src/libvirt-host.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
> index 221a1b7a4353..9c3a19f33b12 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
> @@ -1428,9 +1428,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);
> + }
Looks good but I think we should improve the documentation of this API
to explicitly state that the @freecb is called only on success and if
the API fails the caller is responsible to free the opaque data.
Will change it in v4.
Pavel
--
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
12:35 p.m.
New subject: [libvirt] [PATCH v3 3/6] 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>
---
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 be20556128ae..e7f6d4c9f138 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);
@@ -4259,6 +4261,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;
@@ -4487,6 +4490,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;
@@ -4562,6 +4566,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;
@@ -6044,6 +6049,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;
@@ -6164,6 +6170,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;
@@ -6283,6 +6290,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;
@@ -6402,6 +6410,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;
@@ -6516,6 +6525,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
2:25 a.m.
New subject: [libvirt] [PATCH v3 3/6] remote: Save reference to program in daemonClientEventCallback
On Fri, Nov 01, 2019 at 06:35:45PM +0100, Marc Hartmayer wrote:
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>
---
src/remote/remote_daemon_dispatch.c | 108 +++++++++++++++-------------
1 file changed, 59 insertions(+), 49 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
12:35 p.m.
New subject: [libvirt] [PATCH v3 4/6] 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>
---
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 e7f6d4c9f138..70f1f7d815e8 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);
@@ -4176,6 +4176,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSED,
virNetMessageErrorPtr rerr)
{
int rv = -1;
+ daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
@@ -4185,9 +4186,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;
@@ -4195,8 +4204,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
2:35 a.m.
New subject: [libvirt] [PATCH v3 4/6] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
On Fri, Nov 01, 2019 at 06:35:46PM +0100, Marc Hartmayer wrote:
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>
---
src/remote/remote_daemon_dispatch.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
12:35 p.m.
New subject: [libvirt] [PATCH v3 5/6] rpc: Introduce virNetServerGetProgramLocked helper function
This patch introduces virNetServerGetProgramLocked. It's a function to
determine which program has to be used for a given @msg. This function
will be reused in the next patch.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
---
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 54d0e4f31489..154747a1a097 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -171,6 +171,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,
@@ -180,18 +200,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
2:46 a.m.
New subject: [libvirt] [PATCH v3 5/6] rpc: Introduce virNetServerGetProgramLocked helper function
On Fri, Nov 01, 2019 at 06:35:47PM +0100, Marc Hartmayer wrote:
This patch introduces virNetServerGetProgramLocked. It's a
function to
determine which program has to be used for a given @msg. This function
will be reused in the next patch.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
---
src/rpc/virnetserver.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
12:35 p.m.
New subject: [libvirt] [PATCH v3 6/6] 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 | 118 +++++++++++++++++++++-------
src/rpc/gendispatch.pl | 6 ++
src/rpc/virnetserver.c | 22 ++++++
src/rpc/virnetserver.h | 2 +
7 files changed, 122 insertions(+), 33 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 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 7e63e180344d..c8ac224d52e9 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;
@@ -1007,6 +1005,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 70f1f7d815e8..8756bd1a222d 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4170,9 +4170,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;
@@ -4180,6 +4180,12 @@ 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);
@@ -4190,7 +4196,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSED,
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
/* eventID, callbackID, and legacy are not used */
callback->eventID = -1;
callback->callbackID = -1;
@@ -4242,9 +4248,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)
{
@@ -4255,6 +4261,12 @@ 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);
@@ -4272,7 +4284,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
G_GNUC_UNUSED,
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
callback->callbackID = -1;
callback->legacy = true;
@@ -4463,9 +4475,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)
{
@@ -4476,6 +4488,12 @@ 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);
@@ -4501,7 +4519,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
callback->legacy = true;
@@ -4537,9 +4555,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)
@@ -4552,6 +4570,12 @@ 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);
@@ -4577,7 +4601,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server G_GNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5647,7 +5671,7 @@ remoteDispatchDomainMigratePrepare3Params(virNetServerPtr server
G_GNUC_UNUSED,
}
static int
-remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr server,
virNetServerClientPtr client,
virNetMessagePtr msg,
virNetMessageErrorPtr rerr,
@@ -5662,6 +5686,7 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr
server G_GNUC_UN
virStreamPtr st = NULL;
daemonClientStreamPtr stream = NULL;
virConnectPtr conn = remoteGetHypervisorConn(client);
+ virNetServerProgramPtr program;
if (!conn)
goto cleanup;
@@ -5678,8 +5703,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;
@@ -6020,9 +6050,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)
@@ -6035,6 +6065,12 @@ 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);
@@ -6060,7 +6096,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSE
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6141,9 +6177,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)
@@ -6156,6 +6192,12 @@ 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);
@@ -6181,7 +6223,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server G_GNUC_U
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6261,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)
@@ -6276,6 +6318,12 @@ 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);
@@ -6301,7 +6349,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server G_GNUC_UN
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6381,9 +6429,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)
@@ -6396,6 +6444,12 @@ 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);
@@ -6421,7 +6475,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
G_GNUC_UNUSED
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(program);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6501,9 +6555,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)
@@ -6517,6 +6571,12 @@ 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);
@@ -6536,7 +6596,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
G_GNUC_UNUS
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(qemuProgram);
+ callback->program = virObjectRef(program);
callback->eventID = -1;
callback->callbackID = -1;
ref = callback;
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 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 154747a1a097..80a28123c536 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -192,6 +192,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
2:52 a.m.
New subject: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
On Fri, Nov 01, 2019 at 06:35:48PM +0100, Marc Hartmayer wrote:
Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.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 | 118 +++++++++++++++++++++-------
src/rpc/gendispatch.pl | 6 ++
src/rpc/virnetserver.c | 22 ++++++
src/rpc/virnetserver.h | 2 +
7 files changed, 122 insertions(+), 33 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 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 7e63e180344d..c8ac224d52e9 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;
@@ -1007,6 +1005,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 70f1f7d815e8..8756bd1a222d 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4170,9 +4170,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;
@@ -4180,6 +4180,12 @@ 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;
+ }
This doesn't look right. If the function fails we will jump to cleanup
where we will try to unlock &priv->lock. This has to happen after we
acquire that lock.
Pavel
12:12 p.m.
New subject: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina <phrdina(a)redhat.com> wrote:
On Fri, Nov 01, 2019 at 06:35:48PM +0100, Marc Hartmayer wrote:
> Use virNetServerGetProgram() to determine the virNetServerProgram
> instead of using hard coded global variables. This allows us to remove
> the global variables @remoteProgram and @qemuProgram as they're now no
> longer necessary.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
[…snip…]
> virNetMessageErrorPtr
rerr)
> {
> int rv = -1;
> @@ -4180,6 +4180,12 @@ 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;
> + }
This doesn't look right. If the function fails we will jump to cleanup
where we will try to unlock &priv->lock. This has to happen after we
acquire that lock.
Pavel
Yep, will fix that as well. Shall I directly return in the error case or
jump to another label (e.g. 'cleanup_unlock')?
Or do see any reason why we should hold the priv->lock during the
virNetServerGetProgram call?
--
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:20 a.m.
New subject: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
On Wed, Nov 13, 2019 at 07:12:34PM +0100, Marc Hartmayer wrote:
On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina
<phrdina(a)redhat.com> wrote:
> On Fri, Nov 01, 2019 at 06:35:48PM +0100, Marc Hartmayer wrote:
>> Use virNetServerGetProgram() to determine the virNetServerProgram
>> instead of using hard coded global variables. This allows us to remove
>> the global variables @remoteProgram and @qemuProgram as they're now no
>> longer necessary.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
[…snip…]
>> virNetMessageErrorPtr rerr)
>> {
>> int rv = -1;
>> @@ -4180,6 +4180,12 @@
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;
>> + }
>
> This doesn't look right. If the function fails we will jump to cleanup
> where we will try to unlock &priv->lock. This has to happen after we
> acquire that lock.
>
> Pavel
Yep, will fix that as well. Shall I directly return in the error case or
jump to another label (e.g. 'cleanup_unlock')?
Returning directly would not work properly as well, we call
virNetMessageSaveError() in case of error in the cleanup section.
Another label would work.
Or do see any reason why we should hold the priv->lock during the
virNetServerGetProgram call?
We don't have to hold the lock for virNetServerGetProgram(), it just
makes the function easier to follow as there will be only one cleanup
and I don't see any harm of holding the lock for that function call as
well if the function will later wait for the same lock.
Pavel
9:44 a.m.
New subject: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
On Thu, Nov 14, 2019 at 09:20 AM +0100, Pavel Hrdina <phrdina(a)redhat.com> wrote:
On Wed, Nov 13, 2019 at 07:12:34PM +0100, Marc Hartmayer wrote:
> On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina <phrdina(a)redhat.com>
wrote:
> > On Fri, Nov 01, 2019 at 06:35:48PM +0100, Marc Hartmayer wrote:
> >> Use virNetServerGetProgram() to determine the virNetServerProgram
> >> instead of using hard coded global variables. This allows us to remove
> >> the global variables @remoteProgram and @qemuProgram as they're now no
> >> longer necessary.
> >>
> >> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
>
> […snip…]
>
> >> virNetMessageErrorPtr rerr)
> >> {
> >> int rv = -1;
> >> @@ -4180,6 +4180,12 @@
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;
> >> + }
> >
> > This doesn't look right. If the function fails we will jump to cleanup
> > where we will try to unlock &priv->lock. This has to happen after we
> > acquire that lock.
> >
> > Pavel
>
> Yep, will fix that as well. Shall I directly return in the error case or
> jump to another label (e.g. 'cleanup_unlock')?
Returning directly would not work properly as well, we call
virNetMessageSaveError() in case of error in the cleanup section.
Another label would work.
> Or do see any reason why we should hold the priv->lock during the
> virNetServerGetProgram call?
We don't have to hold the lock for virNetServerGetProgram(), it just
makes the function easier to follow as there will be only one cleanup
and I don't see any harm of holding the lock for that function call as
well if the function will later wait for the same lock.
This would enforce the lock order 'server -> priv->lock' (not sure if
this is already the case) + it will become harder to identify what we’re
trying to protect with the lock. So if you have no strong opinion about
that I will introduce a 'cleanup_unlock' label. Fine with you?
Thanks.
Pavel
--
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
1865
days inactive
1878
days old
16 comments
2 participants
participants (2)
-
Marc Hartmayer -
Pavel Hrdina