8:20 p.m.
The first part of this patch series fixes the behavior of
virConnectSupportsFeatures, virConnect(Un)RegisterCloseCallback, and
implements these features in the test driver. This results in a better
code coverage of our test suite.
The subsequent patches remove the need to have the global variables
'qemuProgram', 'adminProgram', 'lxcProgram, and
'remoteProgram' in
remote_daemon.[ch]. They only work in combination with the fixed
behavior of virConnectSupportsFeatures and
virConnect(Un)RegisterCloseCallback.
Marc Hartmayer (20):
driver: Add typedef for the anonymous enum used for driver features
remote: Don't hard code the feature
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
virConnect(Un)RegisterCloseCallback: Throw an error in case the API is
not supported
test: Implement virConnectSupportsFeature
test: Implement virConnect(Un)RegisterCloseCallback
test: testOpenDefault: introduce cleanup path
test: testOpenFromFile: return VIR_DRV_OPEN_SUCCESS in case of success
test: testConnectAuthenticate: Take the lock when accessing mutable
values
test: testConnectClose: Set privateData to NULL in all cases
test: rename defaultConn to defaultPrivconn
test: introduce testDriverCloseInternal
test: fix error path in testConnectOpen
test: Convert testDriver to virObjectLockable
remote: remove unneeded global variables
stream: Access stream->prog instead of a hard-coded global variable
remote: Set eventID explicitly to an invalid value
remote: Add the information which program has to be used to
daemonClientEventCallback
remote: Use domainClientEventCallbacks for
remoteReplayConnectionClosedEvent
rpc: Introduce virNetServerGetProgramLocked helper function
remote/rpc: Use virNetServerGetProgram() to determine the program
src/esx/esx_driver.c | 18 +-
src/libvirt-host.c | 24 +--
src/libvirt_internal.h | 4 +-
src/libvirt_remote.syms | 1 +
src/libxl/libxl_driver.c | 13 +-
src/lxc/lxc_driver.c | 24 ++-
src/openvz/openvz_driver.c | 15 +-
src/qemu/qemu_driver.c | 8 +-
src/remote/remote_daemon.c | 8 +-
src/remote/remote_daemon.h | 3 -
src/remote/remote_daemon_dispatch.c | 182 +++++++++++--------
src/remote/remote_daemon_stream.c | 14 +-
src/rpc/gendispatch.pl | 2 +
src/rpc/virnetserver.c | 54 +++++-
src/rpc/virnetserver.h | 2 +
src/test/test_driver.c | 339 ++++++++++++++++++++++--------------
src/vz/vz_driver.c | 15 +-
src/xen/xen_driver.c | 15 +-
tools/virsh.c | 11 +-
19 files changed, 504 insertions(+), 248 deletions(-)
--
2.13.4
8:20 p.m.
New subject: [libvirt] [PATCH 01/20] driver: Add typedef for the anonymous enum used for driver features
Add typedef for the anonymous enum used for the driver features. This
allows the usage of the type in a switch statement and taking
advantage of the compilers feature to detect uncovered cases.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/esx/esx_driver.c | 18 ++++++++++++++++--
src/libvirt_internal.h | 4 ++--
src/libxl/libxl_driver.c | 13 ++++++++++++-
src/lxc/lxc_driver.c | 24 +++++++++++++++++++-----
src/openvz/openvz_driver.c | 15 ++++++++++++++-
src/qemu/qemu_driver.c | 8 +++++++-
src/remote/remote_daemon_dispatch.c | 19 ++++++++++++++++---
src/vz/vz_driver.c | 15 ++++++++++++++-
src/xen/xen_driver.c | 15 ++++++++++++++-
9 files changed, 114 insertions(+), 17 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index b065cdc51323..927267f1cc98 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -1122,7 +1122,7 @@ esxConnectSupportsFeature(virConnectPtr conn, int feature)
esxPrivate *priv = conn->privateData;
esxVI_Boolean supportsVMotion = esxVI_Boolean_Undefined;
- switch (feature) {
+ switch ((virDrvFeature) feature) {
case VIR_DRV_FEATURE_MIGRATION_V1:
supportsVMotion = esxSupportsVMotion(priv);
@@ -1133,7 +1133,21 @@ esxConnectSupportsFeature(virConnectPtr conn, int feature)
return priv->vCenter &&
supportsVMotion == esxVI_Boolean_True ? 1 : 0;
- default:
+ case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
+ case VIR_DRV_FEATURE_MIGRATION_DIRECT:
+ case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
+ case VIR_DRV_FEATURE_MIGRATION_P2P:
+ case VIR_DRV_FEATURE_MIGRATION_PARAMS:
+ case VIR_DRV_FEATURE_MIGRATION_V2:
+ case VIR_DRV_FEATURE_MIGRATION_V3:
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
+ case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
+ case VIR_DRV_FEATURE_XML_MIGRATABLE:
+ default:
return 0;
}
}
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 62f490a7dfd5..0e008a65518d 100644
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -45,7 +45,7 @@ int virStateStop(void);
* feature. Queries for VIR_DRV_FEATURE_PROGRAM* features are answered
* directly by the RPC layer and not by the real driver.
*/
-enum {
+typedef enum {
/* Driver supports V1-style virDomainMigrate, ie. domainMigratePrepare/
* domainMigratePerform/domainMigrateFinish.
*/
@@ -123,7 +123,7 @@ enum {
* Support for driver close callback rpc
*/
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK = 15,
-};
+} virDrvFeature;
int virConnectSupportsFeature(virConnectPtr conn, int feature);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index c3616a86d7f4..562966cc2062 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5684,12 +5684,23 @@ libxlConnectSupportsFeature(virConnectPtr conn, int feature)
if (virConnectSupportsFeatureEnsureACL(conn) < 0)
return -1;
- switch (feature) {
+ switch ((virDrvFeature) feature) {
case VIR_DRV_FEATURE_MIGRATION_V3:
case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
case VIR_DRV_FEATURE_MIGRATION_PARAMS:
case VIR_DRV_FEATURE_MIGRATION_P2P:
return 1;
+ case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
+ case VIR_DRV_FEATURE_MIGRATION_DIRECT:
+ case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
+ case VIR_DRV_FEATURE_MIGRATION_V1:
+ case VIR_DRV_FEATURE_MIGRATION_V2:
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
+ case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
+ case VIR_DRV_FEATURE_XML_MIGRATABLE:
default:
return 0;
}
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index fa6fc4643ee2..4f6b93bdbd30 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1793,11 +1793,25 @@ lxcConnectSupportsFeature(virConnectPtr conn, int feature)
if (virConnectSupportsFeatureEnsureACL(conn) < 0)
return -1;
- switch (feature) {
- case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
- return 1;
- default:
- return 0;
+ switch ((virDrvFeature) feature) {
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
+ return 1;
+ case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
+ case VIR_DRV_FEATURE_MIGRATION_DIRECT:
+ case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
+ case VIR_DRV_FEATURE_MIGRATION_P2P:
+ case VIR_DRV_FEATURE_MIGRATION_PARAMS:
+ case VIR_DRV_FEATURE_MIGRATION_V1:
+ case VIR_DRV_FEATURE_MIGRATION_V2:
+ case VIR_DRV_FEATURE_MIGRATION_V3:
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
+ case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
+ case VIR_DRV_FEATURE_XML_MIGRATABLE:
+ default:
+ return 0;
}
}
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 9bd73d85c49f..312cac35a3c3 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -2204,10 +2204,23 @@ openvzNodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED,
static int
openvzConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED, int feature)
{
- switch (feature) {
+ switch ((virDrvFeature) feature) {
case VIR_DRV_FEATURE_MIGRATION_PARAMS:
case VIR_DRV_FEATURE_MIGRATION_V3:
return 1;
+ case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
+ case VIR_DRV_FEATURE_MIGRATION_DIRECT:
+ case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
+ case VIR_DRV_FEATURE_MIGRATION_P2P:
+ case VIR_DRV_FEATURE_MIGRATION_V1:
+ case VIR_DRV_FEATURE_MIGRATION_V2:
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
+ case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
+ case VIR_DRV_FEATURE_XML_MIGRATABLE:
default:
return 0;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9e715e7a00d4..874cb46981a3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1206,7 +1206,7 @@ qemuConnectSupportsFeature(virConnectPtr conn, int feature)
if (virConnectSupportsFeatureEnsureACL(conn) < 0)
return -1;
- switch (feature) {
+ switch ((virDrvFeature) feature) {
case VIR_DRV_FEATURE_MIGRATION_V2:
case VIR_DRV_FEATURE_MIGRATION_V3:
case VIR_DRV_FEATURE_MIGRATION_P2P:
@@ -1217,6 +1217,12 @@ qemuConnectSupportsFeature(virConnectPtr conn, int feature)
case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
case VIR_DRV_FEATURE_MIGRATION_PARAMS:
return 1;
+ case VIR_DRV_FEATURE_MIGRATION_DIRECT:
+ case VIR_DRV_FEATURE_MIGRATION_V1:
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
+ case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
default:
return 0;
}
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index ea67cb7bc018..82f6400ca49d 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4645,7 +4645,7 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr
server ATTRIBUTE
remote_connect_supports_feature_ret
*ret)
{
int rv = -1;
- int supported;
+ int supported = -1;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
@@ -4664,17 +4664,30 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr
server ATTRIBUTE
goto cleanup;
}
- switch (args->feature) {
+ switch ((virDrvFeature) args->feature) {
case VIR_DRV_FEATURE_FD_PASSING:
case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
supported = 1;
break;
-
+ case VIR_DRV_FEATURE_MIGRATION_V1:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_MIGRATION_V2:
+ case VIR_DRV_FEATURE_MIGRATION_P2P:
+ case VIR_DRV_FEATURE_MIGRATION_DIRECT:
+ case VIR_DRV_FEATURE_MIGRATION_V3:
+ case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
+ case VIR_DRV_FEATURE_XML_MIGRATABLE:
+ case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
+ case VIR_DRV_FEATURE_MIGRATION_PARAMS:
default:
if ((supported = virConnectSupportsFeature(priv->conn, args->feature)) <
0)
goto cleanup;
break;
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ /* should not be possible! */
+ goto cleanup;
}
done:
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 6d02ef274fec..a425bc85529d 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -3079,10 +3079,23 @@ vzConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED, int
feature)
if (virConnectSupportsFeatureEnsureACL(conn) < 0)
return -1;
- switch (feature) {
+ switch ((virDrvFeature) feature) {
case VIR_DRV_FEATURE_MIGRATION_PARAMS:
case VIR_DRV_FEATURE_MIGRATION_P2P:
return 1;
+ case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
+ case VIR_DRV_FEATURE_MIGRATION_DIRECT:
+ case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
+ case VIR_DRV_FEATURE_MIGRATION_V1:
+ case VIR_DRV_FEATURE_MIGRATION_V2:
+ case VIR_DRV_FEATURE_MIGRATION_V3:
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
+ case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
+ case VIR_DRV_FEATURE_XML_MIGRATABLE:
default:
return 0;
}
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index f521fd1f2c03..85c3424b11ed 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -619,10 +619,23 @@ xenUnifiedConnectSupportsFeature(virConnectPtr conn, int feature)
if (virConnectSupportsFeatureEnsureACL(conn) < 0)
return -1;
- switch (feature) {
+ switch ((virDrvFeature) feature) {
case VIR_DRV_FEATURE_MIGRATION_V1:
case VIR_DRV_FEATURE_MIGRATION_DIRECT:
return 1;
+ case VIR_DRV_FEATURE_MIGRATION_V2:
+ case VIR_DRV_FEATURE_MIGRATION_V3:
+ case VIR_DRV_FEATURE_MIGRATION_P2P:
+ case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
+ case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
+ case VIR_DRV_FEATURE_XML_MIGRATABLE:
+ case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
+ case VIR_DRV_FEATURE_MIGRATION_PARAMS:
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
+ case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
default:
return 0;
}
--
2.13.4
5:26 a.m.
New subject: [libvirt] [PATCH 01/20] driver: Add typedef for the anonymous enum used for driver features
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Add typedef for the anonymous enum used for the driver features.
This
allows the usage of the type in a switch statement and taking
advantage of the compilers feature to detect uncovered cases.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/esx/esx_driver.c | 18 ++++++++++++++++--
src/libvirt_internal.h | 4 ++--
src/libxl/libxl_driver.c | 13 ++++++++++++-
src/lxc/lxc_driver.c | 24 +++++++++++++++++++-----
src/openvz/openvz_driver.c | 15 ++++++++++++++-
src/qemu/qemu_driver.c | 8 +++++++-
src/remote/remote_daemon_dispatch.c | 19 ++++++++++++++++---
src/vz/vz_driver.c | 15 ++++++++++++++-
src/xen/xen_driver.c | 15 ++++++++++++++-
9 files changed, 114 insertions(+), 17 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
(I forgot that I'd turned off deadcode detection in coverity; otherwise,
it surely would have complained in remoteDispatchConnectSupportsFeature
that it's not possible to get to the case since the code already checked
for VIR_DRV_FEATURE_PROGRAM_KEEPALIVE
8:20 p.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
available for every driver used for the connection.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 82f6400ca49d..bf6c00348a5e 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4667,7 +4667,6 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr
server ATTRIBUTE
switch ((virDrvFeature) args->feature) {
case VIR_DRV_FEATURE_FD_PASSING:
case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
- case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
supported = 1;
break;
case VIR_DRV_FEATURE_MIGRATION_V1:
@@ -4681,6 +4680,7 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr
server ATTRIBUTE
case VIR_DRV_FEATURE_XML_MIGRATABLE:
case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
case VIR_DRV_FEATURE_MIGRATION_PARAMS:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
default:
if ((supported = virConnectSupportsFeature(priv->conn, args->feature)) <
0)
goto cleanup;
--
2.13.4
5:30 a.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Don't assume that the feature
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
available for every driver used for the connection.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Something that is not clear about this one - since this was added for
'vz' driver by commit id 'f484310a', then shouldn't
vzConnectSupportsFeature be updated to indicate support?
If I'm right and you add the feature to the vz routine along with a
reference to the commit id that forgot to in your commit message, then
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
If I'm wrong - then help me understand!
John
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 82f6400ca49d..bf6c00348a5e 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4667,7 +4667,6 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr
server ATTRIBUTE
switch ((virDrvFeature) args->feature) {
case VIR_DRV_FEATURE_FD_PASSING:
case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
- case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
supported = 1;
break;
case VIR_DRV_FEATURE_MIGRATION_V1:
@@ -4681,6 +4680,7 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr
server ATTRIBUTE
case VIR_DRV_FEATURE_XML_MIGRATABLE:
case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
case VIR_DRV_FEATURE_MIGRATION_PARAMS:
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
default:
if ((supported = virConnectSupportsFeature(priv->conn, args->feature))
< 0)
goto cleanup;
7:02 a.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
On 03/14/2018 05:30 PM, John Ferlan wrote:
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
> available for every driver used for the connection.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/remote/remote_daemon_dispatch.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Something that is not clear about this one - since this was added for
'vz' driver by commit id 'f484310a', then shouldn't
vzConnectSupportsFeature be updated to indicate support?
If I'm right and you add the feature to the vz routine along with a
reference to the commit id that forgot to in your commit message, then
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
If I'm wrong - then help me understand!
John
Once I got to patch 5 I started questioning my (limited) understanding
of what's going on here.
Still if we move the REMOTE_CLOSE_CALLBACK into the "check with the
connection" to see if it's supported, then how does patch 3/virsh
actually utilize the virConnectRegisterCloseCallback unless the vz
driver is enabled?
Won't the check with the connection driver for everyone else return 0?
Maybe I just need to understand this code a bit more <sigh>
John
> diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
> index 82f6400ca49d..bf6c00348a5e 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -4667,7 +4667,6 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr
server ATTRIBUTE
> switch ((virDrvFeature) args->feature) {
> case VIR_DRV_FEATURE_FD_PASSING:
> case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
> - case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
> supported = 1;
> break;
> case VIR_DRV_FEATURE_MIGRATION_V1:
> @@ -4681,6 +4680,7 @@ static int remoteDispatchConnectSupportsFeature(virNetServerPtr
server ATTRIBUTE
> case VIR_DRV_FEATURE_XML_MIGRATABLE:
> case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
> case VIR_DRV_FEATURE_MIGRATION_PARAMS:
> + case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
> default:
> if ((supported = virConnectSupportsFeature(priv->conn, args->feature))
< 0)
> goto cleanup;
>
2:52 a.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
On Thu, Mar 15, 2018 at 12:02 AM +0100, John Ferlan <jferlan(a)redhat.com> wrote:
On 03/14/2018 05:30 PM, John Ferlan wrote:
>
>
> On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
>> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
>> available for every driver used for the connection.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
>> ---
>> src/remote/remote_daemon_dispatch.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>
> Something that is not clear about this one - since this was added for
> 'vz' driver by commit id 'f484310a', then shouldn't
> vzConnectSupportsFeature be updated to indicate support?
>
> If I'm right and you add the feature to the vz routine along with a
> reference to the commit id that forgot to in your commit message,
> then
You’re right.
>
> Reviewed-by: John Ferlan <jferlan(a)redhat.com>
>
> If I'm wrong - then help me understand!
>
> John
>
Once I got to patch 5 I started questioning my (limited) understanding
of what's going on here.
Still if we move the REMOTE_CLOSE_CALLBACK into the "check with the
connection" to see if it's supported, then how does patch 3/virsh
actually utilize the virConnectRegisterCloseCallback unless the vz
driver is enabled?
For the vz driver we’ve to add this feature to the
'vzConnectSupportsFeature' function (and for all other internal drivers,
which supports the register/unregister close callback interface).
Won't the check with the connection driver for everyone else return 0?
So lets start from the beginning…
'doRemoteOpen' checks if the server has the feature to register a close
callback for the connection between the “remote daemon driver” and the
used internal driver (e.g. QEMU, bhyve, etc.) (this is cached in
priv->serverCloseCallback (bool) on the client side). In my opinion this
depends (also) on the internal driver and therefore there is the need to
check this by use of 'virConnectSupportsFeature'.
The cover letter '[libvirt] [PATCH v5 0/10] add close callback for
drivers with persistent connection' says to the original change:
“Currently close callback API can only inform us of closing the
connection between remote driver and daemon. But what if a driver
running in the daemon itself can have another persistent connection?
In this case we want to be informed of that connection changes state
too.”
'doRemoteOpen' does the following RPC call in remote_driver.c for the
check:
remoteConnectSupportsFeatureUnlocked(conn, priv, VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK);
Before this patch, 'remoteDispatchConnectSupportsFeature' always
returned that the feature is supported (regardless of the capability of
the used internal driver) => priv->serverCloseCallback is always true on
the client side.
If now 'remoteConnectRegisterCloseCallback' is called on the client side
(virsh calls it in virshReconnect) the client tests for
'priv->serverCloseCallback' and as this is always true, it will call the
RPC 'REMOTE_PROC_CONNECT_REGISTER_CLOSE_CALLBACK' =>
'remoteDispatchConnectRegisterCloseCallback' is called on the server
side.
So lets have a look at 'remoteDispatchConnectRegisterCloseCallback'. It
registers internally that a close callback has been registered
('priv->closeRegistered = true' (daemon/remote.c)) and calls:
if (virConnectRegisterCloseCallback(priv->conn,
remoteRelayConnectionClosedEvent,
client, NULL) < 0)
priv->conn is the connection to the internal driver (e.g. QEMU, vz,
etc.). virConnectRegisterCloseCallback is a _public_ API and it returns
_only_ an error if the used internal driver implements the
'connectRegisterCloseCallback' function and an error happens during the
'connectRegisterCloseCallback(…)' call (src/libvirt-host.c).
Important: virConnect(Un)RegisterCloseCallback throws _no_ unsupported
error even if the internal driver doesn’t support this API (this is
changed in patch 3 of this series).
This works as long as you don't rely on the return value of
virConnect(Un)RegisterCloseCallback. I stumbled across the problem as I
tried to use 'domainClientEventCallbacks' for
'remoteReplayConnectionClosedEvent' (patch 18 of this series) - actually
I produced a memory leak as the callback object was never freed as it
was never registered (and I thought it is).
So at least that’s my understanding of the code. But for safeness, I
added Nikolay to CC.
[…snip]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
4:46 p.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
On 15.03.2018 21:52, Marc Hartmayer wrote:
On Thu, Mar 15, 2018 at 12:02 AM +0100, John Ferlan
<jferlan(a)redhat.com> wrote:
> On 03/14/2018 05:30 PM, John Ferlan wrote:
>>
>>
>> On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
>>> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
>>> available for every driver used for the connection.
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>>> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
>>> ---
>>> src/remote/remote_daemon_dispatch.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>
>> Something that is not clear about this one - since this was added for
>> 'vz' driver by commit id 'f484310a', then shouldn't
>> vzConnectSupportsFeature be updated to indicate support?
>>
>> If I'm right and you add the feature to the vz routine along with a
>> reference to the commit id that forgot to in your commit message,
>> then
You’re right.
>>
>> Reviewed-by: John Ferlan <jferlan(a)redhat.com>
>>
>> If I'm wrong - then help me understand!
>>
>> John
>>
>
> Once I got to patch 5 I started questioning my (limited) understanding
> of what's going on here.
>
> Still if we move the REMOTE_CLOSE_CALLBACK into the "check with the
> connection" to see if it's supported, then how does patch 3/virsh
> actually utilize the virConnectRegisterCloseCallback unless the vz
> driver is enabled?
For the vz driver we’ve to add this feature to the
'vzConnectSupportsFeature' function (and for all other internal drivers,
which supports the register/unregister close callback interface).
>
> Won't the check with the connection driver for everyone else return 0?
So lets start from the beginning…
'doRemoteOpen' checks if the server has the feature to register a close
callback for the connection between the “remote daemon driver” and the
used internal driver (e.g. QEMU, bhyve, etc.) (this is cached in
priv->serverCloseCallback (bool) on the client side). In my opinion this
depends (also) on the internal driver and therefore there is the need to
check this by use of 'virConnectSupportsFeature'.
Hi, Marc.
I agree with the suggested change. There is no need to register/unregister
close callback in remote driver if internal driver does not implement appropriate feature
(now this is only vz driver). Current code works because if internal driver
does not implement connect(Un)RegisterCloseCallback then virConnectRegisterCloseCallback
returns success as you mentioned below.
As to changing virConnectRegisterCloseCallback to return "not supported"
- I don't think this possible as this breaks backward compat.
Nikolay
The cover letter '[libvirt] [PATCH v5 0/10] add close callback for
drivers with persistent connection' says to the original change:
“Currently close callback API can only inform us of closing the
connection between remote driver and daemon. But what if a driver
running in the daemon itself can have another persistent connection?
In this case we want to be informed of that connection changes state
too.”
'doRemoteOpen' does the following RPC call in remote_driver.c for the
check:
remoteConnectSupportsFeatureUnlocked(conn, priv, VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK);
Before this patch, 'remoteDispatchConnectSupportsFeature' always
returned that the feature is supported (regardless of the capability of
the used internal driver) => priv->serverCloseCallback is always true on
the client side.
If now 'remoteConnectRegisterCloseCallback' is called on the client side
(virsh calls it in virshReconnect) the client tests for
'priv->serverCloseCallback' and as this is always true, it will call the
RPC 'REMOTE_PROC_CONNECT_REGISTER_CLOSE_CALLBACK' =>
'remoteDispatchConnectRegisterCloseCallback' is called on the server
side.
So lets have a look at 'remoteDispatchConnectRegisterCloseCallback'. It
registers internally that a close callback has been registered
('priv->closeRegistered = true' (daemon/remote.c)) and calls:
if (virConnectRegisterCloseCallback(priv->conn,
remoteRelayConnectionClosedEvent,
client, NULL) < 0)
priv->conn is the connection to the internal driver (e.g. QEMU, vz,
etc.). virConnectRegisterCloseCallback is a _public_ API and it returns
_only_ an error if the used internal driver implements the
'connectRegisterCloseCallback' function and an error happens during the
'connectRegisterCloseCallback(…)' call (src/libvirt-host.c).
Important: virConnect(Un)RegisterCloseCallback throws _no_ unsupported
error even if the internal driver doesn’t support this API (this is
changed in patch 3 of this series).
This works as long as you don't rely on the return value of
virConnect(Un)RegisterCloseCallback. I stumbled across the problem as I
tried to use 'domainClientEventCallbacks' for
'remoteReplayConnectionClosedEvent' (patch 18 of this series) - actually
I produced a memory leak as the callback object was never freed as it
was never registered (and I thought it is).
So at least that’s my understanding of the code. But for safeness, I
added Nikolay to CC.
[…snip]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:06 p.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote:
Don't assume that the feature
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
available for every driver used for the connection.
The very definition of the virConnectRegisterCloseCallback() API is that
it will always succeed. What varies is that some driver connections
may never fail and so the close callback will never be invoked. The actual
registration of the callback will always succeed regardless of which driver
is in use. So it is correct to report the VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK
as always supported regardless of driver.
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 :|
2:57 p.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote:
> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
> available for every driver used for the connection.
The very definition of the virConnectRegisterCloseCallback() API is that
it will always succeed. What varies is that some driver connections
may never fail and so the close callback will never be invoked. The actual
registration of the callback will always succeed regardless of which driver
is in use. So it is correct to report the VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK
as always supported regardless of driver.
Okay. So how can we deal with the situation in patch 18 if we cannot
differentiate between 'callback was “really” registered' and only the
call was 'successful'? If it’s not really registered nobody will free
the callback data. This was also the cause for the fix: ce35122cfe:
"daemon: fixup refcounting in close callback handling".
Thanks in advance.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
7:46 p.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
On Wed, Mar 21, 2018 at 07:57 AM +0100, Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
wrote:
On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P.
Berrangé" <berrange(a)redhat.com> wrote:
> On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote:
>> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
>> available for every driver used for the connection.
>
> The very definition of the virConnectRegisterCloseCallback() API is that
> it will always succeed. What varies is that some driver connections
> may never fail and so the close callback will never be invoked. The actual
> registration of the callback will always succeed regardless of which driver
> is in use. So it is correct to report the VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK
> as always supported regardless of driver.
Okay. So how can we deal with the situation in patch 18 if we cannot
differentiate between 'callback was “really” registered' and only the
call was 'successful'? If it’s not really registered nobody will free
the callback data. This was also the cause for the fix: ce35122cfe:
"daemon: fixup refcounting in close callback handling".
Thanks in advance.
Polite ping.
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
8:05 p.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
On Tue, Apr 03, 2018 at 01:46:13PM +0200, Marc Hartmayer wrote:
On Wed, Mar 21, 2018 at 07:57 AM +0100, Marc Hartmayer
<mhartmay(a)linux.vnet.ibm.com> wrote:
> On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
>> On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote:
>>> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK is
>>> available for every driver used for the connection.
>>
>> The very definition of the virConnectRegisterCloseCallback() API is that
>> it will always succeed. What varies is that some driver connections
>> may never fail and so the close callback will never be invoked. The actual
>> registration of the callback will always succeed regardless of which driver
>> is in use. So it is correct to report the VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK
>> as always supported regardless of driver.
>
> Okay. So how can we deal with the situation in patch 18 if we cannot
> differentiate between 'callback was “really” registered' and only the
> call was 'successful'? If it’s not really registered nobody will free
> the callback data. This was also the cause for the fix: ce35122cfe:
> "daemon: fixup refcounting in close callback handling".
>
> Thanks in advance.
Polite ping.
If conn->driver->connectRegisterCloseCallback is NULL, then the
virConnectRegisterCloseCallback() method should immediately
call freecb(opaque).
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 :|
9:41 p.m.
New subject: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
On Tue, Apr 03, 2018 at 02:05 PM +0200, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
On Tue, Apr 03, 2018 at 01:46:13PM +0200, Marc Hartmayer wrote:
> On Wed, Mar 21, 2018 at 07:57 AM +0100, Marc Hartmayer
<mhartmay(a)linux.vnet.ibm.com> wrote:
> > On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé"
<berrange(a)redhat.com> wrote:
> >> On Thu, Mar 08, 2018 at 01:20:25PM +0100, Marc Hartmayer wrote:
> >>> Don't assume that the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK
is
> >>> available for every driver used for the connection.
> >>
> >> The very definition of the virConnectRegisterCloseCallback() API is that
> >> it will always succeed. What varies is that some driver connections
> >> may never fail and so the close callback will never be invoked. The actual
> >> registration of the callback will always succeed regardless of which driver
> >> is in use. So it is correct to report the
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK
> >> as always supported regardless of driver.
> >
> > Okay. So how can we deal with the situation in patch 18 if we cannot
> > differentiate between 'callback was “really” registered' and only the
> > call was 'successful'? If it’s not really registered nobody will free
> > the callback data. This was also the cause for the fix: ce35122cfe:
> > "daemon: fixup refcounting in close callback handling".
> >
> > Thanks in advance.
>
> Polite ping.
If conn->driver->connectRegisterCloseCallback is NULL, then the
virConnectRegisterCloseCallback() method should immediately
call freecb(opaque).
Thanks! I’ll send a v2.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
8:20 p.m.
New subject: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Report an error in case the driver does not support
connect(Un)registerCloseCallback. 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. The only case where an error is reported is
when the API is supported but the call fails. The same behavior
applies to virConnectUnregisterCloseCallback.
This behavior is not intended as there are many use cases of this API
where the state of for example allocations depends on the result of
these functions.
To keep the behavior of virsh as before it must silently ignore
unsupported error for virConnectRegisterCloseCallback. For the remote
driver this change wouldn't be needed, but for the byhve driver, for
example. Otherwise the user would see the error message that virsh was
unable to register a disconnect callback.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/libvirt-host.c | 24 ++++++++++++++----------
tools/virsh.c | 11 +++++++++--
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 7ff7407a0874..bfa104b39918 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1221,12 +1221,14 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
virCheckNonNullArgGoto(cb, error);
- if (conn->driver->connectRegisterCloseCallback &&
- conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) <
0)
- goto error;
-
- return 0;
+ if (conn->driver->connectRegisterCloseCallback) {
+ int ret = conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
freecb);
+ if (ret < 0)
+ goto error;
+ return ret;
+ }
+ virReportUnsupportedError();
error:
virDispatchError(conn);
return -1;
@@ -1256,12 +1258,14 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
virCheckNonNullArgGoto(cb, error);
- if (conn->driver->connectUnregisterCloseCallback &&
- conn->driver->connectUnregisterCloseCallback(conn, cb) < 0)
- goto error;
-
- return 0;
+ if (conn->driver->connectUnregisterCloseCallback) {
+ int ret = conn->driver->connectUnregisterCloseCallback(conn, cb);
+ if (ret < 0)
+ goto error;
+ return ret;
+ }
+ virReportUnsupportedError();
error:
virDispatchError(conn);
return -1;
diff --git a/tools/virsh.c b/tools/virsh.c
index 5f8352e861d3..2df1197252b3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -256,8 +256,15 @@ virshReconnect(vshControl *ctl, const char *name, bool readonly, bool
force)
priv->readonly = readonly;
if (virConnectRegisterCloseCallback(priv->conn, virshCatchDisconnect,
- ctl, NULL) < 0)
- vshError(ctl, "%s", _("Unable to register disconnect
callback"));
+ ctl, NULL) < 0) {
+ virErrorPtr error = virGetLastError();
+ if (error && error->code == VIR_ERR_NO_SUPPORT) {
+ /* silently ignore the unsupported error */
+ virResetLastError();
+ } else {
+ vshError(ctl, "%s", _("Unable to register disconnect
callback"));
+ }
+ }
if (connected && !force)
vshError(ctl, "%s", _("Reconnected to the hypervisor"));
}
--
2.13.4
6:12 a.m.
New subject: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Report an error in case the driver does not support
connect(Un)registerCloseCallback. 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. The only case where an error is reported is
when the API is supported but the call fails. The same behavior
applies to virConnectUnregisterCloseCallback.
This behavior is not intended as there are many use cases of this API
where the state of for example allocations depends on the result of
these functions.
To keep the behavior of virsh as before it must silently ignore
unsupported error for virConnectRegisterCloseCallback. For the remote
driver this change wouldn't be needed, but for the byhve driver, for
example. Otherwise the user would see the error message that virsh was
^^^^^^^
I think you forgot to end your sentence for example what about bhyve?
unable to register a disconnect callback.
So the reason to keep the virsh behavior is because failure to do so
could be problematic if connecting to an older server that doesn't
support virConnectRegisterCloseCallback? I almost wonder if we should
blat a vshDebug type message to "inform" the consumer that it's not
possible to catch disconnect...
Since the code path doesn't return -1 anyway (even if "Unable to
register..." is printed), it may not hurt. Your call though - it's not
that important other than the oh, OK so there's something newer in the
newer server that might be useful.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
5:39 p.m.
New subject: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
On 08.03.2018 15:20, Marc Hartmayer wrote:
Report an error in case the driver does not support
connect(Un)registerCloseCallback. 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. The only case where an error is reported is
when the API is supported but the call fails. The same behavior
applies to virConnectUnregisterCloseCallback.
This behavior is not intended as there are many use cases of this API
where the state of for example allocations depends on the result of
these functions.
To keep the behavior of virsh as before it must silently ignore
unsupported error for virConnectRegisterCloseCallback. For the remote
driver this change wouldn't be needed, but for the byhve driver, for
example. Otherwise the user would see the error message that virsh was
unable to register a disconnect callback.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/libvirt-host.c | 24 ++++++++++++++----------
tools/virsh.c | 11 +++++++++--
2 files changed, 23 insertions(+), 12 deletions(-)
We can't change public API. As to patch 18 I suggest either to:
- don't refcount client object, this works though it is dangerous. Or
- check if driver supports VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK and throw
an error if it does not (looks better to me)
Nikolay
6:23 p.m.
New subject: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
On 16.03.2018 12:39, Nikolay Shirokovskiy wrote:
On 08.03.2018 15:20, Marc Hartmayer wrote:
> Report an error in case the driver does not support
> connect(Un)registerCloseCallback. 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. The only case where an error is reported is
> when the API is supported but the call fails. The same behavior
> applies to virConnectUnregisterCloseCallback.
>
> This behavior is not intended as there are many use cases of this API
> where the state of for example allocations depends on the result of
> these functions.
>
> To keep the behavior of virsh as before it must silently ignore
> unsupported error for virConnectRegisterCloseCallback. For the remote
> driver this change wouldn't be needed, but for the byhve driver, for
> example. Otherwise the user would see the error message that virsh was
> unable to register a disconnect callback.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/libvirt-host.c | 24 ++++++++++++++----------
> tools/virsh.c | 11 +++++++++--
> 2 files changed, 23 insertions(+), 12 deletions(-)
>
We can't change public API. As to patch 18 I suggest either to:
- don't refcount client object, this works though it is dangerous. Or
- check if driver supports VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK and throw
an error if it does not (looks better to me)
Still other clients (other than daemon) can have opaque leaks as they
don't check for VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK before call to
virConnectRegisterCloseCallback. This is introduced in [1]
[1] 88f09b75e
Author: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
Date: Tue Mar 1 14:17:38 2016 +0000
close callback: move it to driver
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
To fix this we can fallback to previous scheme (using closeCallback in
virConnectPtr) if connectRegisterCloseCallback is not defined for
the driver. Then we can refcount client object in patch 18 without
any additional checks.
Nikolay
11:03 p.m.
New subject: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
On Thu, Mar 08, 2018 at 01:20:26PM +0100, Marc Hartmayer wrote:
Report an error in case the driver does not support
connect(Un)registerCloseCallback. 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. The only case where an error is reported is
when the API is supported but the call fails. The same behavior
applies to virConnectUnregisterCloseCallback.
This behavior is not intended as there are many use cases of this API
where the state of for example allocations depends on the result of
these functions.
To keep the behavior of virsh as before it must silently ignore
unsupported error for virConnectRegisterCloseCallback. For the remote
driver this change wouldn't be needed, but for the byhve driver, for
example. Otherwise the user would see the error message that virsh was
unable to register a disconnect callback.
NACK, you cann't change the API semantics in this way.
Fixing virsh is no where near sufficient, becasue this change will
still potentially break every other application using this libvirt
API that expect it to always succeeed.
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 :|
8:20 p.m.
New subject: [libvirt] [PATCH 04/20] test: Implement virConnectSupportsFeature
Implement virConnectSupportsFeature for the test driver as this API is
used by various API functions (the functions usually use the macro
VIR_DRV_SUPPORTS_FEATURE for calling virConnectSupportsFeature).
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 043caa976274..203358c7017f 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6829,6 +6829,32 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
+static int
+testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
+ int feature)
+{
+ switch ((virDrvFeature) feature) {
+ case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
+ return 1;
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
+ case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
+ case VIR_DRV_FEATURE_MIGRATION_DIRECT:
+ case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
+ case VIR_DRV_FEATURE_MIGRATION_P2P:
+ case VIR_DRV_FEATURE_MIGRATION_PARAMS:
+ case VIR_DRV_FEATURE_MIGRATION_V1:
+ case VIR_DRV_FEATURE_MIGRATION_V2:
+ case VIR_DRV_FEATURE_MIGRATION_V3:
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
+ case VIR_DRV_FEATURE_XML_MIGRATABLE:
+ default:
+ return 0;
+ }
+}
+
static virHypervisorDriver testHypervisorDriver = {
.name = "Test",
@@ -6837,6 +6863,7 @@ static virHypervisorDriver testHypervisorDriver = {
.connectGetVersion = testConnectGetVersion, /* 0.1.1 */
.connectGetHostname = testConnectGetHostname, /* 0.6.3 */
.connectGetMaxVcpus = testConnectGetMaxVcpus, /* 0.3.2 */
+ .connectSupportsFeature = testConnectSupportsFeature, /* 4.2.0 */
.nodeGetInfo = testNodeGetInfo, /* 0.1.1 */
.nodeGetCPUStats = testNodeGetCPUStats, /* 2.3.0 */
.nodeGetFreeMemory = testNodeGetFreeMemory, /* 2.3.0 */
--
2.13.4
6:35 a.m.
New subject: [libvirt] [PATCH 04/20] test: Implement virConnectSupportsFeature
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Implement virConnectSupportsFeature for the test driver as this API
is
used by various API functions (the functions usually use the macro
VIR_DRV_SUPPORTS_FEATURE for calling virConnectSupportsFeature).
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 043caa976274..203358c7017f 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6829,6 +6829,32 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
+static int
+testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
+ int feature)
+{
+ switch ((virDrvFeature) feature) {
+ case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
+ return 1;
Not that it makes a difference, but since
src/remote/remote_daemon_dispatch.c returns 1 for
VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK always like
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK used to before patch 2, then does
returning 1 here really matter?
Perhaps why Nikolay added to remoteDispatchConnectSupportsFeature since
commit id '03722957' was the last to add some new feature bit...
[yes, I'm still a bit mystified about how all this works - so I'm
learning a bit as I go... Still not clear why the same API returns 1 for
VIR_DRV_FEATURE_FD_PASSING always].
So while it seems reasonable, if you return 0, then what happens? Mostly
curious, what's here looks fine to me otherwise.
A weak,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
+ case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
+ case VIR_DRV_FEATURE_MIGRATION_DIRECT:
+ case VIR_DRV_FEATURE_MIGRATION_OFFLINE:
+ case VIR_DRV_FEATURE_MIGRATION_P2P:
+ case VIR_DRV_FEATURE_MIGRATION_PARAMS:
+ case VIR_DRV_FEATURE_MIGRATION_V1:
+ case VIR_DRV_FEATURE_MIGRATION_V2:
+ case VIR_DRV_FEATURE_MIGRATION_V3:
+ case VIR_DRV_FEATURE_PROGRAM_KEEPALIVE:
+ case VIR_DRV_FEATURE_REMOTE:
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
+ case VIR_DRV_FEATURE_XML_MIGRATABLE:
+ default:
+ return 0;
+ }
+}
+
static virHypervisorDriver testHypervisorDriver = {
.name = "Test",
@@ -6837,6 +6863,7 @@ static virHypervisorDriver testHypervisorDriver = {
.connectGetVersion = testConnectGetVersion, /* 0.1.1 */
.connectGetHostname = testConnectGetHostname, /* 0.6.3 */
.connectGetMaxVcpus = testConnectGetMaxVcpus, /* 0.3.2 */
+ .connectSupportsFeature = testConnectSupportsFeature, /* 4.2.0 */
.nodeGetInfo = testNodeGetInfo, /* 0.1.1 */
.nodeGetCPUStats = testNodeGetCPUStats, /* 2.3.0 */
.nodeGetFreeMemory = testNodeGetFreeMemory, /* 2.3.0 */
11:11 p.m.
New subject: [libvirt] [PATCH 04/20] test: Implement virConnectSupportsFeature
On Wed, Mar 14, 2018 at 06:35:59PM -0400, John Ferlan wrote:
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> Implement virConnectSupportsFeature for the test driver as this API is
> used by various API functions (the functions usually use the macro
> VIR_DRV_SUPPORTS_FEATURE for calling virConnectSupportsFeature).
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/test/test_driver.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 043caa976274..203358c7017f 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -6829,6 +6829,32 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> }
>
>
> +static int
> +testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
> + int feature)
> +{
> + switch ((virDrvFeature) feature) {
> + case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
> + return 1;
Not that it makes a difference, but since
src/remote/remote_daemon_dispatch.c returns 1 for
VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK always like
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK used to before patch 2, then does
returning 1 here really matter?
Perhaps why Nikolay added to remoteDispatchConnectSupportsFeature since
commit id '03722957' was the last to add some new feature bit...
[yes, I'm still a bit mystified about how all this works - so I'm
learning a bit as I go... Still not clear why the same API returns 1 for
VIR_DRV_FEATURE_FD_PASSING always].
The VIR_DRV_FEATURES don't all correspond to driver specific features.
A bunch of the bits correspond to aspects of the remote protocol, so
do not require involvement of driver specific code. The EVENT_CALLBACK
bit is one such feature - the way we supported events at the RPC level
originally turned out to be flawed, so we introduced a second way to
represent them. The remote driver & libvirtd negotiate which way to
use. The virt drivers have no involvement in this, as the actual libvirt
API implemented is unchanged.
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 :|
8:20 p.m.
New subject: [libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback
Implement the hypervisor APIs virConnectRegisterCloseCallback and
virConnectUnregisterCloseCallbacks and mark this feature as supported
in testConnectSupportsFeature. This increases test test coverage of
the test suite as then the testConnectRegisterCloseCallback and
testConnectUnregisterCloseCallback (which are almost identical to the
implementations of the remote and vz driver) will be called by the
virsh tests.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 62 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 203358c7017f..2773f5c758c8 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -121,6 +121,7 @@ struct _testDriver {
virDomainObjListPtr domains;
virNetworkObjListPtr networks;
virObjectEventStatePtr eventState;
+ virConnectCloseCallbackDataPtr closeCallback;
};
typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
@@ -157,6 +158,7 @@ testDriverFree(testDriverPtr driver)
virObjectUnref(driver->ifaces);
virObjectUnref(driver->pools);
virObjectUnref(driver->eventState);
+ virObjectUnref(driver->closeCallback);
virMutexUnlock(&driver->lock);
virMutexDestroy(&driver->lock);
@@ -404,6 +406,7 @@ testDriverNew(void)
.free = testDomainDefNamespaceFree,
};
testDriverPtr ret;
+ virConnectCloseCallbackDataPtr closeCallback;
if (VIR_ALLOC(ret) < 0)
return NULL;
@@ -423,6 +426,12 @@ testDriverNew(void)
!(ret->pools = virStoragePoolObjListNew()))
goto error;
+ closeCallback = virNewConnectCloseCallbackData();
+ if (!closeCallback)
+ goto error;
+
+ ret->closeCallback = closeCallback;
+
virAtomicIntSet(&ret->nextDomID, 1);
return ret;
@@ -6834,9 +6843,9 @@ testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
int feature)
{
switch ((virDrvFeature) feature) {
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
return 1;
- case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
case VIR_DRV_FEATURE_FD_PASSING:
case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
case VIR_DRV_FEATURE_MIGRATION_DIRECT:
@@ -6855,6 +6864,56 @@ testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
}
}
+static int
+testConnectRegisterCloseCallback(virConnectPtr conn,
+ virConnectCloseFunc cb,
+ void *opaque,
+ virFreeCallback freecb)
+{
+ testDriverPtr priv = conn->privateData;
+ int ret = -1;
+
+ testDriverLock(priv);
+
+ if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != NULL) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("A close callback is already registered"));
+ goto cleanup;
+ }
+
+ virConnectCloseCallbackDataRegister(priv->closeCallback, conn, cb,
+ opaque, freecb);
+ ret = 0;
+
+ cleanup:
+ testDriverUnlock(priv);
+ return ret;
+}
+
+
+static int
+testConnectUnregisterCloseCallback(virConnectPtr conn,
+ virConnectCloseFunc cb)
+{
+ testDriverPtr priv = conn->privateData;
+ int ret = -1;
+
+ testDriverLock(priv);
+
+ if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != cb) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("A different callback was requested"));
+ goto cleanup;
+ }
+
+ virConnectCloseCallbackDataUnregister(priv->closeCallback, cb);
+ ret = 0;
+
+ cleanup:
+ testDriverUnlock(priv);
+ return ret;
+}
+
static virHypervisorDriver testHypervisorDriver = {
.name = "Test",
@@ -6863,7 +6922,9 @@ static virHypervisorDriver testHypervisorDriver = {
.connectGetVersion = testConnectGetVersion, /* 0.1.1 */
.connectGetHostname = testConnectGetHostname, /* 0.6.3 */
.connectGetMaxVcpus = testConnectGetMaxVcpus, /* 0.3.2 */
+ .connectRegisterCloseCallback = testConnectRegisterCloseCallback, /* 4.2.0 */
.connectSupportsFeature = testConnectSupportsFeature, /* 4.2.0 */
+ .connectUnregisterCloseCallback = testConnectUnregisterCloseCallback, /* 4.2.0 */
.nodeGetInfo = testNodeGetInfo, /* 0.1.1 */
.nodeGetCPUStats = testNodeGetCPUStats, /* 2.3.0 */
.nodeGetFreeMemory = testNodeGetFreeMemory, /* 2.3.0 */
--
2.13.4
6:57 a.m.
New subject: [libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Implement the hypervisor APIs virConnectRegisterCloseCallback and
virConnectUnregisterCloseCallbacks and mark this feature as supported
in testConnectSupportsFeature. This increases test test coverage of
s/test test//
the test suite as then the testConnectRegisterCloseCallback and
testConnectUnregisterCloseCallback (which are almost identical to the
implementations of the remote and vz driver) will be called by the
virsh tests.
Oh, um, hmmm... Now I'm confused again. It would seem that the
REMOTE_CLOSE_CALLBACK would need to be somehow marked as supported for
the remote driver (e.g. patch 2); otherwise, how would virsh really be
notified if say for example vz and test weren't in the target environment.
The rest seems fine, but I'm now mystified by the need for patch 2.
John
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 62 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 203358c7017f..2773f5c758c8 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -121,6 +121,7 @@ struct _testDriver {
virDomainObjListPtr domains;
virNetworkObjListPtr networks;
virObjectEventStatePtr eventState;
+ virConnectCloseCallbackDataPtr closeCallback;
};
typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
@@ -157,6 +158,7 @@ testDriverFree(testDriverPtr driver)
virObjectUnref(driver->ifaces);
virObjectUnref(driver->pools);
virObjectUnref(driver->eventState);
+ virObjectUnref(driver->closeCallback);
virMutexUnlock(&driver->lock);
virMutexDestroy(&driver->lock);
@@ -404,6 +406,7 @@ testDriverNew(void)
.free = testDomainDefNamespaceFree,
};
testDriverPtr ret;
+ virConnectCloseCallbackDataPtr closeCallback;
if (VIR_ALLOC(ret) < 0)
return NULL;
@@ -423,6 +426,12 @@ testDriverNew(void)
!(ret->pools = virStoragePoolObjListNew()))
goto error;
+ closeCallback = virNewConnectCloseCallbackData();
+ if (!closeCallback)
+ goto error;
+
+ ret->closeCallback = closeCallback;
+
virAtomicIntSet(&ret->nextDomID, 1);
return ret;
@@ -6834,9 +6843,9 @@ testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
int feature)
{
switch ((virDrvFeature) feature) {
+ case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
return 1;
- case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
case VIR_DRV_FEATURE_FD_PASSING:
case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
case VIR_DRV_FEATURE_MIGRATION_DIRECT:
@@ -6855,6 +6864,56 @@ testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
}
}
+static int
+testConnectRegisterCloseCallback(virConnectPtr conn,
+ virConnectCloseFunc cb,
+ void *opaque,
+ virFreeCallback freecb)
+{
+ testDriverPtr priv = conn->privateData;
+ int ret = -1;
+
+ testDriverLock(priv);
+
+ if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != NULL) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("A close callback is already registered"));
+ goto cleanup;
+ }
+
+ virConnectCloseCallbackDataRegister(priv->closeCallback, conn, cb,
+ opaque, freecb);
+ ret = 0;
+
+ cleanup:
+ testDriverUnlock(priv);
+ return ret;
+}
+
+
+static int
+testConnectUnregisterCloseCallback(virConnectPtr conn,
+ virConnectCloseFunc cb)
+{
+ testDriverPtr priv = conn->privateData;
+ int ret = -1;
+
+ testDriverLock(priv);
+
+ if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != cb) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("A different callback was requested"));
+ goto cleanup;
+ }
+
+ virConnectCloseCallbackDataUnregister(priv->closeCallback, cb);
+ ret = 0;
+
+ cleanup:
+ testDriverUnlock(priv);
+ return ret;
+}
+
static virHypervisorDriver testHypervisorDriver = {
.name = "Test",
@@ -6863,7 +6922,9 @@ static virHypervisorDriver testHypervisorDriver = {
.connectGetVersion = testConnectGetVersion, /* 0.1.1 */
.connectGetHostname = testConnectGetHostname, /* 0.6.3 */
.connectGetMaxVcpus = testConnectGetMaxVcpus, /* 0.3.2 */
+ .connectRegisterCloseCallback = testConnectRegisterCloseCallback, /* 4.2.0 */
.connectSupportsFeature = testConnectSupportsFeature, /* 4.2.0 */
+ .connectUnregisterCloseCallback = testConnectUnregisterCloseCallback, /* 4.2.0 */
.nodeGetInfo = testNodeGetInfo, /* 0.1.1 */
.nodeGetCPUStats = testNodeGetCPUStats, /* 2.3.0 */
.nodeGetFreeMemory = testNodeGetFreeMemory, /* 2.3.0 */
11:02 p.m.
New subject: [libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback
On Thu, Mar 08, 2018 at 01:20:28PM +0100, Marc Hartmayer wrote:
Implement the hypervisor APIs virConnectRegisterCloseCallback and
virConnectUnregisterCloseCallbacks and mark this feature as supported
in testConnectSupportsFeature. This increases test test coverage of
the test suite as then the testConnectRegisterCloseCallback and
testConnectUnregisterCloseCallback (which are almost identical to the
implementations of the remote and vz driver) will be called by the
virsh tests.
I'm not really understanding why this is needed at all.
For most drivers, the close callback stuff does not need any interaction
with the driver code. It is handled entirely at the RPC protocol level
by the remote driver & libvirtd.
We only needed to wire it up in the VZ driver, because VZ had a further
internal connection that could be lost, and we want to propagate that
back out from the driver as a close event, despite the main reomte
driver <-> libvirtd connection being intact.
So I don't see any test for the test driver to require special handling
for close callbacks - if you run the test driver inside libvirtd, it
would work normally. If the test driver is run outside libvirtd, there
is no network connection that could fail and thus no close callbacks
ever need to be run.
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 :|
8:20 p.m.
New subject: [libvirt] [PATCH 06/20] test: testOpenDefault: introduce cleanup path
The two code paths have some cleanup in common so lets refactor it.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2773f5c758c8..e7307fddad4a 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1306,6 +1306,7 @@ testOpenFromFile(virConnectPtr conn, const char *file)
static int
testOpenDefault(virConnectPtr conn)
{
+ int ret;
testDriverPtr privconn = NULL;
xmlDocPtr doc = NULL;
xmlXPathContextPtr ctxt = NULL;
@@ -1354,21 +1355,19 @@ testOpenDefault(virConnectPtr conn)
goto error;
defaultConn = privconn;
-
+ ret = VIR_DRV_OPEN_SUCCESS;
+ cleanup:
+ virMutexUnlock(&defaultLock);
xmlXPathFreeContext(ctxt);
xmlFreeDoc(doc);
- virMutexUnlock(&defaultLock);
-
- return VIR_DRV_OPEN_SUCCESS;
+ return ret;
error:
testDriverFree(privconn);
- xmlXPathFreeContext(ctxt);
- xmlFreeDoc(doc);
conn->privateData = NULL;
defaultConnections--;
- virMutexUnlock(&defaultLock);
- return VIR_DRV_OPEN_ERROR;
+ ret = VIR_DRV_OPEN_ERROR;
+ goto cleanup;
}
static int
--
2.13.4
6:57 a.m.
New subject: [libvirt] [PATCH 06/20] test: testOpenDefault: introduce cleanup path
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
The two code paths have some cleanup in common so lets refactor it.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2773f5c758c8..e7307fddad4a 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1306,6 +1306,7 @@ testOpenFromFile(virConnectPtr conn, const char *file)
static int
testOpenDefault(virConnectPtr conn)
{
+ int ret;
So that no one ever adds a goto cleanup some day and @ret isn't set,
let's initialize to VIR_DRV_OPEN_ERROR (and remove that setting in the
error: label).
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[I'll continue with the rest tomorrow]
8:20 p.m.
New subject: [libvirt] [PATCH 07/20] test: testOpenFromFile: return VIR_DRV_OPEN_SUCCESS in case of success
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index e7307fddad4a..388407d4371f 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1290,7 +1290,7 @@ testOpenFromFile(virConnectPtr conn, const char *file)
xmlFreeDoc(doc);
testDriverUnlock(privconn);
- return 0;
+ return VIR_DRV_OPEN_SUCCESS;
error:
xmlXPathFreeContext(ctxt);
--
2.13.4
10:20 p.m.
New subject: [libvirt] [PATCH 07/20] test: testOpenFromFile: return VIR_DRV_OPEN_SUCCESS in case of success
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
8:20 p.m.
New subject: [libvirt] [PATCH 08/20] test: testConnectAuthenticate: Take the lock when accessing mutable values
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 388407d4371f..b0ce7d0eea0a 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1379,8 +1379,11 @@ testConnectAuthenticate(virConnectPtr conn,
ssize_t i;
char *username = NULL, *password = NULL;
- if (privconn->numAuths == 0)
+ testDriverLock(privconn);
+ if (privconn->numAuths == 0) {
+ testDriverUnlock(privconn);
return 0;
+ }
/* Authentication is required because the test XML contains a
* non-empty <auth/> section. First we must ask for a username.
@@ -1420,6 +1423,7 @@ testConnectAuthenticate(virConnectPtr conn,
ret = 0;
cleanup:
+ testDriverUnlock(privconn);
VIR_FREE(username);
VIR_FREE(password);
return ret;
--
2.13.4
10:22 p.m.
New subject: [libvirt] [PATCH 08/20] test: testConnectAuthenticate: Take the lock when accessing mutable values
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
8:20 p.m.
New subject: [libvirt] [PATCH 09/20] test: testConnectClose: Set privateData to NULL in all cases
Set privateData to NULL also for a connection that uses @defaultConn
as privateData regardless of whether @defaultConn was freed or
not. @defaultConn is shared between multiple connections and it's
ensured that there will be no memory leak by counting references.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index b0ce7d0eea0a..5561d0c2ae70 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1482,6 +1482,7 @@ static int testConnectClose(virConnectPtr conn)
dflt = true;
virMutexLock(&defaultLock);
if (--defaultConnections) {
+ conn->privateData = NULL;
virMutexUnlock(&defaultLock);
return 0;
}
--
2.13.4
10:33 p.m.
New subject: [libvirt] [PATCH 09/20] test: testConnectClose: Set privateData to NULL in all cases
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Set privateData to NULL also for a connection that uses @defaultConn
as privateData regardless of whether @defaultConn was freed or
not. @defaultConn is shared between multiple connections and it's
ensured that there will be no memory leak by counting references.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
8:20 p.m.
New subject: [libvirt] [PATCH 10/20] test: rename defaultConn to defaultPrivconn
Rename the variable @defaultConn to @defaultPrivconn as it doesn't
point to a default connection but to the private data used for the
shared default connection of the test driver.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 5561d0c2ae70..d450be21704e 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -126,7 +126,7 @@ struct _testDriver {
typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
-static testDriverPtr defaultConn;
+static testDriverPtr defaultPrivconn;
static int defaultConnections;
static virMutex defaultLock = VIR_MUTEX_INITIALIZER;
@@ -1314,7 +1314,7 @@ testOpenDefault(virConnectPtr conn)
virMutexLock(&defaultLock);
if (defaultConnections++) {
- conn->privateData = defaultConn;
+ conn->privateData = defaultPrivconn;
virMutexUnlock(&defaultLock);
return VIR_DRV_OPEN_SUCCESS;
}
@@ -1354,7 +1354,7 @@ testOpenDefault(virConnectPtr conn)
if (testOpenParse(privconn, NULL, ctxt) < 0)
goto error;
- defaultConn = privconn;
+ defaultPrivconn = privconn;
ret = VIR_DRV_OPEN_SUCCESS;
cleanup:
virMutexUnlock(&defaultLock);
@@ -1478,7 +1478,7 @@ static int testConnectClose(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
bool dflt = false;
- if (privconn == defaultConn) {
+ if (privconn == defaultPrivconn) {
dflt = true;
virMutexLock(&defaultLock);
if (--defaultConnections) {
@@ -1492,7 +1492,7 @@ static int testConnectClose(virConnectPtr conn)
testDriverFree(privconn);
if (dflt) {
- defaultConn = NULL;
+ defaultPrivconn = NULL;
virMutexUnlock(&defaultLock);
}
--
2.13.4
10:38 p.m.
New subject: [libvirt] [PATCH 10/20] test: rename defaultConn to defaultPrivconn
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Rename the variable @defaultConn to @defaultPrivconn as it
doesn't
point to a default connection but to the private data used for the
shared default connection of the test driver.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 5561d0c2ae70..d450be21704e 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -126,7 +126,7 @@ struct _testDriver {
typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
-static testDriverPtr defaultConn;
+static testDriverPtr defaultPrivconn;
Probably should be defaultPrivateData then instead since it's assigned
to conn->privateData...
(I can change locally before doing any sort of push as long as you agree)...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
12:13 a.m.
New subject: [libvirt] [PATCH 10/20] test: rename defaultConn to defaultPrivconn
On Thu, Mar 15, 2018 at 03:38 PM +0100, John Ferlan <jferlan(a)redhat.com> wrote:
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> Rename the variable @defaultConn to @defaultPrivconn as it doesn't
> point to a default connection but to the private data used for the
> shared default connection of the test driver.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/test/test_driver.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 5561d0c2ae70..d450be21704e 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -126,7 +126,7 @@ struct _testDriver {
> typedef struct _testDriver testDriver;
> typedef testDriver *testDriverPtr;
>
> -static testDriverPtr defaultConn;
> +static testDriverPtr defaultPrivconn;
Probably should be defaultPrivateData then instead since it's assigned
to conn->privateData...
The naming is “inherited” by the variable 'privconn' :)
(I can change locally before doing any sort of push as long as you agree)...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
8:20 p.m.
New subject: [libvirt] [PATCH 11/20] test: introduce testDriverCloseInternal
Refactor testConnectClose as it's then obvious that conn->privateData
is set to NULL in all cases. In addition, 'testConnectCloseInternal'
can be better reused.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 61 +++++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 26 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index d450be21704e..fca607f57d51 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1429,10 +1429,36 @@ testConnectAuthenticate(virConnectPtr conn,
return ret;
}
-static virDrvOpenStatus testConnectOpen(virConnectPtr conn,
- virConnectAuthPtr auth,
- virConfPtr conf ATTRIBUTE_UNUSED,
- unsigned int flags)
+
+static void
+testDriverCloseInternal(testDriverPtr driver)
+{
+ bool dflt = false;
+
+ if (driver == defaultPrivconn) {
+ dflt = true;
+ virMutexLock(&defaultLock);
+ if (--defaultConnections) {
+ virMutexUnlock(&defaultLock);
+ return;
+ }
+ }
+
+ testDriverLock(driver);
+ testDriverFree(driver);
+
+ if (dflt) {
+ defaultPrivconn = NULL;
+ virMutexUnlock(&defaultLock);
+ }
+}
+
+
+static virDrvOpenStatus
+testConnectOpen(virConnectPtr conn,
+ virConnectAuthPtr auth,
+ virConfPtr conf ATTRIBUTE_UNUSED,
+ unsigned int flags)
{
int ret;
@@ -1473,33 +1499,16 @@ static virDrvOpenStatus testConnectOpen(virConnectPtr conn,
return VIR_DRV_OPEN_SUCCESS;
}
-static int testConnectClose(virConnectPtr conn)
+
+static int
+testConnectClose(virConnectPtr conn)
{
- testDriverPtr privconn = conn->privateData;
- bool dflt = false;
-
- if (privconn == defaultPrivconn) {
- dflt = true;
- virMutexLock(&defaultLock);
- if (--defaultConnections) {
- conn->privateData = NULL;
- virMutexUnlock(&defaultLock);
- return 0;
- }
- }
-
- testDriverLock(privconn);
- testDriverFree(privconn);
-
- if (dflt) {
- defaultPrivconn = NULL;
- virMutexUnlock(&defaultLock);
- }
-
+ testDriverCloseInternal(conn->privateData);
conn->privateData = NULL;
return 0;
}
+
static int testConnectGetVersion(virConnectPtr conn ATTRIBUTE_UNUSED,
unsigned long *hvVer)
{
--
2.13.4
10:43 p.m.
New subject: [libvirt] [PATCH 11/20] test: introduce testDriverCloseInternal
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Refactor testConnectClose as it's then obvious that
conn->privateData
is set to NULL in all cases. In addition, 'testConnectCloseInternal'
can be better reused.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 61 +++++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 26 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
8:20 p.m.
New subject: [libvirt] [PATCH 12/20] test: fix error path in testConnectOpen
In case of an error do the cleanup of the private data of the
connection.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index fca607f57d51..3b55453efe00 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1493,8 +1493,11 @@ testConnectOpen(virConnectPtr conn,
return ret;
/* Fake authentication. */
- if (testConnectAuthenticate(conn, auth) < 0)
+ if (testConnectAuthenticate(conn, auth) < 0) {
+ testDriverCloseInternal(conn->privateData);
+ conn->privateData = NULL;
return VIR_DRV_OPEN_ERROR;
+ }
return VIR_DRV_OPEN_SUCCESS;
}
--
2.13.4
10:49 p.m.
New subject: [libvirt] [PATCH 12/20] test: fix error path in testConnectOpen
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
In case of an error do the cleanup of the private data of the
connection.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
8:20 p.m.
New subject: [libvirt] [PATCH 13/20] test: Convert testDriver to virObjectLockable
The test driver state (@testDriver) uses it's own reference counting
and locking implementation. Instead of doing that, convert @testDriver
into a virObjectLockable and use the provided functionalities.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 207 ++++++++++++++++++++++---------------------------
1 file changed, 94 insertions(+), 113 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 3b55453efe00..f1dd11867143 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -94,7 +94,7 @@ typedef struct _testAuth testAuth;
typedef struct _testAuth *testAuthPtr;
struct _testDriver {
- virMutex lock;
+ virObjectLockable parent;
virNodeInfo nodeInfo;
virInterfaceObjListPtr ifaces;
@@ -127,9 +127,22 @@ typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
static testDriverPtr defaultPrivconn;
-static int defaultConnections;
static virMutex defaultLock = VIR_MUTEX_INITIALIZER;
+static virClassPtr testDriverClass;
+static void testDriverDispose(void *obj);
+static int testDriverOnceInit(void)
+{
+ if (!(testDriverClass = virClassNew(virClassForObjectLockable(),
+ "testDriver",
+ sizeof(testDriver),
+ testDriverDispose)))
+ return -1;
+
+ return 0;
+}
+VIR_ONCE_GLOBAL_INIT(testDriver)
+
#define TEST_MODEL "i686"
#define TEST_EMULATOR "/usr/bin/test-hv"
@@ -145,10 +158,9 @@ static const virNodeInfo defaultNodeInfo = {
};
static void
-testDriverFree(testDriverPtr driver)
+testDriverDispose(void *obj)
{
- if (!driver)
- return;
+ testDriverPtr driver = obj;
virObjectUnref(driver->caps);
virObjectUnref(driver->xmlopt);
@@ -159,23 +171,9 @@ testDriverFree(testDriverPtr driver)
virObjectUnref(driver->pools);
virObjectUnref(driver->eventState);
virObjectUnref(driver->closeCallback);
- virMutexUnlock(&driver->lock);
- virMutexDestroy(&driver->lock);
-
- VIR_FREE(driver);
}
-static void testDriverLock(testDriverPtr driver)
-{
- virMutexLock(&driver->lock);
-}
-
-static void testDriverUnlock(testDriverPtr driver)
-{
- virMutexUnlock(&driver->lock);
-}
-
static void testObjectEventQueue(testDriverPtr driver,
virObjectEventPtr event)
{
@@ -408,14 +406,11 @@ testDriverNew(void)
testDriverPtr ret;
virConnectCloseCallbackDataPtr closeCallback;
- if (VIR_ALLOC(ret) < 0)
+ if (testDriverInitialize() < 0)
return NULL;
- if (virMutexInit(&ret->lock) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("cannot initialize mutex"));
- goto error;
- }
+ if (!(ret = virObjectLockableNew(testDriverClass)))
+ return NULL;
if (!(ret->xmlopt = virDomainXMLOptionNew(NULL, NULL, &ns, NULL, NULL)) ||
!(ret->eventState = virObjectEventStateNew()) ||
@@ -437,7 +432,7 @@ testDriverNew(void)
return ret;
error:
- testDriverFree(ret);
+ virObjectUnref(ret);
return NULL;
}
@@ -1271,7 +1266,7 @@ testOpenFromFile(virConnectPtr conn, const char *file)
if (!(privconn = testDriverNew()))
return VIR_DRV_OPEN_ERROR;
- testDriverLock(privconn);
+ virObjectLock(privconn);
conn->privateData = privconn;
if (!(privconn->caps = testBuildCapabilities(conn)))
@@ -1288,14 +1283,14 @@ testOpenFromFile(virConnectPtr conn, const char *file)
xmlXPathFreeContext(ctxt);
xmlFreeDoc(doc);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return VIR_DRV_OPEN_SUCCESS;
error:
xmlXPathFreeContext(ctxt);
xmlFreeDoc(doc);
- testDriverFree(privconn);
+ virObjectUnref(privconn);
conn->privateData = NULL;
return VIR_DRV_OPEN_ERROR;
}
@@ -1313,8 +1308,8 @@ testOpenDefault(virConnectPtr conn)
size_t i;
virMutexLock(&defaultLock);
- if (defaultConnections++) {
- conn->privateData = defaultPrivconn;
+ if (defaultPrivconn) {
+ conn->privateData = virObjectRef(defaultPrivconn);
virMutexUnlock(&defaultLock);
return VIR_DRV_OPEN_SUCCESS;
}
@@ -1363,9 +1358,8 @@ testOpenDefault(virConnectPtr conn)
return ret;
error:
- testDriverFree(privconn);
+ virObjectUnref(privconn);
conn->privateData = NULL;
- defaultConnections--;
ret = VIR_DRV_OPEN_ERROR;
goto cleanup;
}
@@ -1379,9 +1373,9 @@ testConnectAuthenticate(virConnectPtr conn,
ssize_t i;
char *username = NULL, *password = NULL;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (privconn->numAuths == 0) {
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return 0;
}
@@ -1423,7 +1417,7 @@ testConnectAuthenticate(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
VIR_FREE(username);
VIR_FREE(password);
return ret;
@@ -1433,24 +1427,11 @@ testConnectAuthenticate(virConnectPtr conn,
static void
testDriverCloseInternal(testDriverPtr driver)
{
- bool dflt = false;
-
- if (driver == defaultPrivconn) {
- dflt = true;
- virMutexLock(&defaultLock);
- if (--defaultConnections) {
- virMutexUnlock(&defaultLock);
- return;
- }
- }
-
- testDriverLock(driver);
- testDriverFree(driver);
-
- if (dflt) {
+ virMutexLock(&defaultLock);
+ bool disposed = !virObjectUnref(driver);
+ if (disposed && driver == defaultPrivconn)
defaultPrivconn = NULL;
- virMutexUnlock(&defaultLock);
- }
+ virMutexUnlock(&defaultLock);
}
@@ -1581,9 +1562,9 @@ static int testNodeGetInfo(virConnectPtr conn,
virNodeInfoPtr info)
{
testDriverPtr privconn = conn->privateData;
- testDriverLock(privconn);
+ virObjectLock(privconn);
memcpy(info, &privconn->nodeInfo, sizeof(virNodeInfo));
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return 0;
}
@@ -1591,9 +1572,9 @@ static char *testConnectGetCapabilities(virConnectPtr conn)
{
testDriverPtr privconn = conn->privateData;
char *xml;
- testDriverLock(privconn);
+ virObjectLock(privconn);
xml = virCapabilitiesFormatXML(privconn->caps);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return xml;
}
@@ -1628,9 +1609,9 @@ static int testConnectNumOfDomains(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int count;
- testDriverLock(privconn);
+ virObjectLock(privconn);
count = virDomainObjListNumOfDomains(privconn->domains, true, NULL, NULL);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return count;
}
@@ -1683,7 +1664,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
if (flags & VIR_DOMAIN_START_VALIDATE)
parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if ((def = virDomainDefParseString(xml, privconn->caps, privconn->xmlopt,
NULL, parse_flags)) == NULL)
goto cleanup;
@@ -1718,7 +1699,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
virObjectUnlock(dom);
testObjectEventQueue(privconn, event);
virDomainDefFree(def);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -2887,7 +2868,7 @@ static int testNodeGetCellsFreeMemory(virConnectPtr conn,
size_t i;
int ret = -1;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (startCell >= privconn->numCells) {
virReportError(VIR_ERR_INVALID_ARG,
"%s", _("Range exceeds available cells"));
@@ -2902,7 +2883,7 @@ static int testNodeGetCellsFreeMemory(virConnectPtr conn,
ret = i;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -2960,12 +2941,12 @@ testNodeGetFreeMemory(virConnectPtr conn)
unsigned int freeMem = 0;
size_t i;
- testDriverLock(privconn);
+ virObjectLock(privconn);
for (i = 0; i < privconn->numCells; i++)
freeMem += privconn->cells[i].freeMem;
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return freeMem;
}
@@ -3002,7 +2983,7 @@ static int testDomainCreateWithFlags(virDomainPtr domain, unsigned
int flags)
virCheckFlags(0, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!(privdom = testDomObjFromDomain(domain)))
goto cleanup;
@@ -3026,7 +3007,7 @@ static int testDomainCreateWithFlags(virDomainPtr domain, unsigned
int flags)
cleanup:
virDomainObjEndAPI(&privdom);
testObjectEventQueue(privconn, event);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -3797,9 +3778,9 @@ testInterfaceObjFindByName(testDriverPtr privconn,
{
virInterfaceObjPtr obj;
- testDriverLock(privconn);
+ virObjectLock(privconn);
obj = virInterfaceObjListFindByName(privconn->ifaces, name);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!obj)
virReportError(VIR_ERR_NO_INTERFACE,
@@ -3816,9 +3797,9 @@ testConnectNumOfInterfaces(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int ninterfaces;
- testDriverLock(privconn);
+ virObjectLock(privconn);
ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, true);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ninterfaces;
}
@@ -3831,10 +3812,10 @@ testConnectListInterfaces(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
int nnames;
- testDriverLock(privconn);
+ virObjectLock(privconn);
nnames = virInterfaceObjListGetNames(privconn->ifaces, true,
names, maxnames);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return nnames;
}
@@ -3846,9 +3827,9 @@ testConnectNumOfDefinedInterfaces(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int ninterfaces;
- testDriverLock(privconn);
+ virObjectLock(privconn);
ninterfaces = virInterfaceObjListNumOfInterfaces(privconn->ifaces, false);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ninterfaces;
}
@@ -3861,10 +3842,10 @@ testConnectListDefinedInterfaces(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
int nnames;
- testDriverLock(privconn);
+ virObjectLock(privconn);
nnames = virInterfaceObjListGetNames(privconn->ifaces, false,
names, maxnames);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return nnames;
}
@@ -3899,10 +3880,10 @@ testInterfaceLookupByMACString(virConnectPtr conn,
char *ifacenames[] = { NULL, NULL };
virInterfacePtr ret = NULL;
- testDriverLock(privconn);
+ virObjectLock(privconn);
ifacect = virInterfaceObjListFindByMACString(privconn->ifaces, mac,
ifacenames, 2);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (ifacect == 0) {
virReportError(VIR_ERR_NO_INTERFACE,
@@ -3950,7 +3931,7 @@ testInterfaceChangeBegin(virConnectPtr conn,
virCheckFlags(0, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (privconn->transaction_running) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("there is another transaction running."));
@@ -3964,7 +3945,7 @@ testInterfaceChangeBegin(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -3978,7 +3959,7 @@ testInterfaceChangeCommit(virConnectPtr conn,
virCheckFlags(0, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!privconn->transaction_running) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -3993,7 +3974,7 @@ testInterfaceChangeCommit(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -4008,7 +3989,7 @@ testInterfaceChangeRollback(virConnectPtr conn,
virCheckFlags(0, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!privconn->transaction_running) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -4026,7 +4007,7 @@ testInterfaceChangeRollback(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -4066,7 +4047,7 @@ testInterfaceDefineXML(virConnectPtr conn,
virCheckFlags(0, NULL);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if ((def = virInterfaceDefParseString(xmlStr)) == NULL)
goto cleanup;
@@ -4080,7 +4061,7 @@ testInterfaceDefineXML(virConnectPtr conn,
cleanup:
virInterfaceDefFree(def);
virInterfaceObjEndAPI(&obj);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -4184,9 +4165,9 @@ testStoragePoolObjFindByName(testDriverPtr privconn,
{
virStoragePoolObjPtr obj;
- testDriverLock(privconn);
+ virObjectLock(privconn);
obj = virStoragePoolObjFindByName(privconn->pools, name);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!obj)
virReportError(VIR_ERR_NO_STORAGE_POOL,
@@ -4244,9 +4225,9 @@ testStoragePoolObjFindByUUID(testDriverPtr privconn,
virStoragePoolObjPtr obj;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- testDriverLock(privconn);
+ virObjectLock(privconn);
obj = virStoragePoolObjFindByUUID(privconn->pools, uuid);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!obj) {
virUUIDFormat(uuid, uuidstr);
@@ -4312,10 +4293,10 @@ testConnectNumOfStoragePools(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int numActive = 0;
- testDriverLock(privconn);
+ virObjectLock(privconn);
numActive = virStoragePoolObjNumOfStoragePools(privconn->pools, conn,
true, NULL);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return numActive;
}
@@ -4329,10 +4310,10 @@ testConnectListStoragePools(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
int n = 0;
- testDriverLock(privconn);
+ virObjectLock(privconn);
n = virStoragePoolObjGetNames(privconn->pools, conn, true, NULL,
names, maxnames);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return n;
}
@@ -4344,10 +4325,10 @@ testConnectNumOfDefinedStoragePools(virConnectPtr conn)
testDriverPtr privconn = conn->privateData;
int numInactive = 0;
- testDriverLock(privconn);
+ virObjectLock(privconn);
numInactive = virStoragePoolObjNumOfStoragePools(privconn->pools, conn,
false, NULL);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return numInactive;
}
@@ -4361,10 +4342,10 @@ testConnectListDefinedStoragePools(virConnectPtr conn,
testDriverPtr privconn = conn->privateData;
int n = 0;
- testDriverLock(privconn);
+ virObjectLock(privconn);
n = virStoragePoolObjGetNames(privconn->pools, conn, false, NULL,
names, maxnames);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return n;
}
@@ -4380,10 +4361,10 @@ testConnectListAllStoragePools(virConnectPtr conn,
virCheckFlags(VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_ALL, -1);
- testDriverLock(privconn);
+ virObjectLock(privconn);
ret = virStoragePoolObjListExport(conn, privconn->pools, pools,
NULL, flags);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return ret;
}
@@ -4545,7 +4526,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
virCheckFlags(0, NULL);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!(newDef = virStoragePoolDefParseString(xml)))
goto cleanup;
@@ -4596,7 +4577,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
virStoragePoolDefFree(newDef);
testObjectEventQueue(privconn, event);
virStoragePoolObjEndAPI(&obj);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return pool;
}
@@ -4615,7 +4596,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
virCheckFlags(0, NULL);
- testDriverLock(privconn);
+ virObjectLock(privconn);
if (!(newDef = virStoragePoolDefParseString(xml)))
goto cleanup;
@@ -4648,7 +4629,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
virStoragePoolDefFree(newDef);
testObjectEventQueue(privconn, event);
virStoragePoolObjEndAPI(&obj);
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
return pool;
}
@@ -5050,7 +5031,7 @@ testStorageVolLookupByKey(virConnectPtr conn,
.conn = conn, .key = key, .voldef = NULL };
virStorageVolPtr vol = NULL;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if ((obj = virStoragePoolObjListSearch(privconn->pools,
testStorageVolLookupByKeyCallback,
&data)) && data.voldef) {
@@ -5060,7 +5041,7 @@ testStorageVolLookupByKey(virConnectPtr conn,
NULL, NULL);
virStoragePoolObjEndAPI(&obj);
}
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!vol)
virReportError(VIR_ERR_NO_STORAGE_VOL,
@@ -5094,7 +5075,7 @@ testStorageVolLookupByPath(virConnectPtr conn,
.conn = conn, .path = path, .voldef = NULL };
virStorageVolPtr vol = NULL;
- testDriverLock(privconn);
+ virObjectLock(privconn);
if ((obj = virStoragePoolObjListSearch(privconn->pools,
testStorageVolLookupByPathCallback,
&data)) && data.voldef) {
@@ -5104,7 +5085,7 @@ testStorageVolLookupByPath(virConnectPtr conn,
NULL, NULL);
virStoragePoolObjEndAPI(&obj);
}
- testDriverUnlock(privconn);
+ virObjectUnlock(privconn);
if (!vol)
virReportError(VIR_ERR_NO_STORAGE_VOL,
@@ -6889,7 +6870,7 @@ testConnectRegisterCloseCallback(virConnectPtr conn,
testDriverPtr priv = conn->privateData;
int ret = -1;
- testDriverLock(priv);
+ virObjectLock(priv);
if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != NULL) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -6902,7 +6883,7 @@ testConnectRegisterCloseCallback(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(priv);
+ virObjectUnlock(priv);
return ret;
}
@@ -6914,7 +6895,7 @@ testConnectUnregisterCloseCallback(virConnectPtr conn,
testDriverPtr priv = conn->privateData;
int ret = -1;
- testDriverLock(priv);
+ virObjectLock(priv);
if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != cb) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -6926,7 +6907,7 @@ testConnectUnregisterCloseCallback(virConnectPtr conn,
ret = 0;
cleanup:
- testDriverUnlock(priv);
+ virObjectUnlock(priv);
return ret;
}
--
2.13.4
11:05 p.m.
New subject: [libvirt] [PATCH 13/20] test: Convert testDriver to virObjectLockable
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
The test driver state (@testDriver) uses it's own reference
counting
and locking implementation. Instead of doing that, convert @testDriver
into a virObjectLockable and use the provided functionalities.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/test/test_driver.c | 207 ++++++++++++++++++++++---------------------------
1 file changed, 94 insertions(+), 113 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 3b55453efe00..f1dd11867143 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -94,7 +94,7 @@ typedef struct _testAuth testAuth;
typedef struct _testAuth *testAuthPtr;
struct _testDriver {
- virMutex lock;
+ virObjectLockable parent;
virNodeInfo nodeInfo;
virInterfaceObjListPtr ifaces;
@@ -127,9 +127,22 @@ typedef struct _testDriver testDriver;
typedef testDriver *testDriverPtr;
static testDriverPtr defaultPrivconn;
Oh and of course I see the pain associated with changing the name (and
perhaps initializing to NULL just to be painfully obvious).
-static int defaultConnections;
static virMutex defaultLock = VIR_MUTEX_INITIALIZER;
[...]
@@ -1433,24 +1427,11 @@ testConnectAuthenticate(virConnectPtr conn,
static void
testDriverCloseInternal(testDriverPtr driver)
{
- bool dflt = false;
-
- if (driver == defaultPrivconn) {
- dflt = true;
- virMutexLock(&defaultLock);
- if (--defaultConnections) {
- virMutexUnlock(&defaultLock);
- return;
- }
- }
-
- testDriverLock(driver);
- testDriverFree(driver);
-
- if (dflt) {
+ virMutexLock(&defaultLock);
+ bool disposed = !virObjectUnref(driver);
I know it builds, but it's preferable to not intermingle defs inside
code, e.g. change to:
bool disposed = false;
virMutexLock(&defaultLock);
disposed = !virObjectUnref(driver);
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
(another one of those things I can do)
+ if (disposed && driver == defaultPrivconn)
defaultPrivconn = NULL;
- virMutexUnlock(&defaultLock);
- }
+ virMutexUnlock(&defaultLock);
}
[...]
12:15 a.m.
New subject: [libvirt] [PATCH 13/20] test: Convert testDriver to virObjectLockable
On Thu, Mar 15, 2018 at 04:05 PM +0100, John Ferlan <jferlan(a)redhat.com> wrote:
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> The test driver state (@testDriver) uses it's own reference counting
> and locking implementation. Instead of doing that, convert @testDriver
> into a virObjectLockable and use the provided functionalities.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/test/test_driver.c | 207 ++++++++++++++++++++++---------------------------
> 1 file changed, 94 insertions(+), 113 deletions(-)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 3b55453efe00..f1dd11867143 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -94,7 +94,7 @@ typedef struct _testAuth testAuth;
> typedef struct _testAuth *testAuthPtr;
>
> struct _testDriver {
> - virMutex lock;
> + virObjectLockable parent;
>
> virNodeInfo nodeInfo;
> virInterfaceObjListPtr ifaces;
> @@ -127,9 +127,22 @@ typedef struct _testDriver testDriver;
> typedef testDriver *testDriverPtr;
>
> static testDriverPtr defaultPrivconn;
Oh and of course I see the pain associated with changing the name (and
perhaps initializing to NULL just to be painfully obvious).
> -static int defaultConnections;
> static virMutex defaultLock = VIR_MUTEX_INITIALIZER;
>
[...]
> @@ -1433,24 +1427,11 @@ testConnectAuthenticate(virConnectPtr conn,
> static void
> testDriverCloseInternal(testDriverPtr driver)
> {
> - bool dflt = false;
> -
> - if (driver == defaultPrivconn) {
> - dflt = true;
> - virMutexLock(&defaultLock);
> - if (--defaultConnections) {
> - virMutexUnlock(&defaultLock);
> - return;
> - }
> - }
> -
> - testDriverLock(driver);
> - testDriverFree(driver);
> -
> - if (dflt) {
> + virMutexLock(&defaultLock);
> + bool disposed = !virObjectUnref(driver);
I know it builds, but it's preferable to not intermingle defs inside
code, e.g. change to:
bool disposed = false;
virMutexLock(&defaultLock);
disposed = !virObjectUnref(driver);
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
(another one of those things I can do)
Thanks!
> + if (disposed && driver == defaultPrivconn)
> defaultPrivconn = NULL;
> - virMutexUnlock(&defaultLock);
> - }
> + virMutexUnlock(&defaultLock);
> }
>
>
[...]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
8:20 p.m.
New subject: [libvirt] [PATCH 14/20] remote: remove unneeded global variables
Remove unneeded global variables and convert them into local variables
where they're needed.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index f8082f62f698..078430f91c97 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -72,9 +72,7 @@ VIR_LOG_INIT("daemon.libvirtd");
virNetSASLContextPtr saslCtxt = NULL;
#endif
virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr adminProgram = NULL;
virNetServerProgramPtr qemuProgram = NULL;
-virNetServerProgramPtr lxcProgram = NULL;
volatile bool driversInitialized = false;
@@ -1062,6 +1060,8 @@ int main(int argc, char **argv) {
virNetDaemonPtr dmn = NULL;
virNetServerPtr srv = NULL;
virNetServerPtr srvAdm = NULL;
+ virNetServerProgramPtr adminProgram = NULL;
+ virNetServerProgramPtr lxcProgram = NULL;
char *remote_config_file = NULL;
int statuswrite = -1;
int ret = 1;
--
2.13.4
11:06 p.m.
New subject: [libvirt] [PATCH 14/20] remote: remove unneeded global variables
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Remove unneeded global variables and convert them into local
variables
where they're needed.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
I had this lined up in one of my branches too, but just haven't had the
time/desire to go back to that other series yet...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
8:20 p.m.
New subject: [libvirt] [PATCH 15/20] stream: Access stream->prog instead of a hard-coded global variable
Use stream->prog instead of a hard-coded
virNetServerProgram. Especially since these functions are intended as
generic helpers for streams.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_stream.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/remote/remote_daemon_stream.c b/src/remote/remote_daemon_stream.c
index 4dd3af9e0d59..21f0ecd53844 100644
--- a/src/remote/remote_daemon_stream.c
+++ b/src/remote/remote_daemon_stream.c
@@ -213,7 +213,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
msg->cb = daemonStreamMessageFinished;
msg->opaque = stream;
stream->refs++;
- if (virNetServerProgramSendStreamData(remoteProgram,
+ if (virNetServerProgramSendStreamData(stream->prog,
client,
msg,
stream->procedure,
@@ -253,7 +253,7 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
if (!msg) {
ret = -1;
} else {
- ret = virNetServerProgramSendStreamError(remoteProgram,
+ ret = virNetServerProgramSendStreamError(stream->prog,
client,
msg,
&rerr,
@@ -644,7 +644,7 @@ daemonStreamHandleAbort(virNetServerClientPtr client,
if (raise_error) {
virNetMessageError rerr;
memset(&rerr, 0, sizeof(rerr));
- return virNetServerProgramSendReplyError(remoteProgram,
+ return virNetServerProgramSendReplyError(stream->prog,
client,
msg,
&rerr,
@@ -839,7 +839,7 @@ daemonStreamHandleRead(virNetServerClientPtr client,
VIR_DEBUG("rv=%d inData=%d length=%lld", rv, inData, length);
if (rv < 0) {
- if (virNetServerProgramSendStreamError(remoteProgram,
+ if (virNetServerProgramSendStreamError(stream->prog,
client,
msg,
&rerr,
@@ -856,7 +856,7 @@ daemonStreamHandleRead(virNetServerClientPtr client,
msg->cb = daemonStreamMessageFinished;
msg->opaque = stream;
stream->refs++;
- if (virNetServerProgramSendStreamHole(remoteProgram,
+ if (virNetServerProgramSendStreamHole(stream->prog,
client,
msg,
stream->procedure,
@@ -887,7 +887,7 @@ daemonStreamHandleRead(virNetServerClientPtr client,
/* Should never get this, since we're only called when we know
* we're readable, but hey things change... */
} else if (rv < 0) {
- if (virNetServerProgramSendStreamError(remoteProgram,
+ if (virNetServerProgramSendStreamError(stream->prog,
client,
msg,
&rerr,
@@ -906,7 +906,7 @@ daemonStreamHandleRead(virNetServerClientPtr client,
msg->cb = daemonStreamMessageFinished;
msg->opaque = stream;
stream->refs++;
- if (virNetServerProgramSendStreamData(remoteProgram,
+ if (virNetServerProgramSendStreamData(stream->prog,
client,
msg,
stream->procedure,
--
2.13.4
11:22 p.m.
New subject: [libvirt] [PATCH 15/20] stream: Access stream->prog instead of a hard-coded global variable
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Use stream->prog instead of a hard-coded
virNetServerProgram. Especially since these functions are intended as
generic helpers for streams.
Looks like you were editing your commit message and didn't reformat. In
essence though it seems you're making sure to use stream->prog instead
of the hard coded "remoteProgram" that "we know" is used when the
stream
was created in daemonCreateClientStream.
Is that about right?
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_stream.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
12:31 a.m.
New subject: [libvirt] [PATCH 15/20] stream: Access stream->prog instead of a hard-coded global variable
On Thu, Mar 15, 2018 at 04:22 PM +0100, John Ferlan <jferlan(a)redhat.com> wrote:
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> Use stream->prog instead of a hard-coded
> virNetServerProgram. Especially since these functions are intended as
> generic helpers for streams.
Looks like you were editing your commit message and didn't reformat. In
essence though it seems you're making sure to use stream->prog instead
of the hard coded "remoteProgram" that "we know" is used when the
stream
was created in daemonCreateClientStream.
Is that about right?
Yep. At least for all callers of 'daemonCreateClientStream' I’ve
found. It might be useful to document the “we know” somewhere… :)
With this change it’s definitely easier to understand the code and the
daemonStream* functions can now be reused (if they’re needed somewhere).
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/remote/remote_daemon_stream.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
8:20 p.m.
New subject: [libvirt] [PATCH 16/20] remote: Set eventID explicitly to an invalid value
Set the eventID for remoteRelayDomainQemuMonitorEvent explicitly to an
invalid value. Although the value is not used by
remoteRelayDomainQemuMonitorEvent, but it might be less prone to
errors for further refactorings.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index bf6c00348a5e..ff26574c9f6b 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -6215,6 +6215,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
ATTRIBUTE_U
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->eventID = -1;
callback->callbackID = -1;
ref = callback;
if (VIR_APPEND_ELEMENT(priv->qemuEventCallbacks,
--
2.13.4
11:39 p.m.
New subject: [libvirt] [PATCH 16/20] remote: Set eventID explicitly to an invalid value
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Set the eventID for remoteRelayDomainQemuMonitorEvent explicitly to
an
invalid value. Although the value is not used by
remoteRelayDomainQemuMonitorEvent, but it might be less prone to
errors for further refactorings.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
(always a bit "nervous" changing anything in these APIs...)
12:34 a.m.
New subject: [libvirt] [PATCH 16/20] remote: Set eventID explicitly to an invalid value
On Thu, Mar 15, 2018 at 04:39 PM +0100, John Ferlan <jferlan(a)redhat.com> wrote:
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> Set the eventID for remoteRelayDomainQemuMonitorEvent explicitly to an
> invalid value. Although the value is not used by
> remoteRelayDomainQemuMonitorEvent, but it might be less prone to
> errors for further refactorings.
> Signed-off-by: Marc Hartmayer
<mhartmay(a)linux.vnet.ibm.com > Reviewed-by: Bjoern Walk
<bwalk(a)linux.vnet.ibm.com > Reviewed-by: Boris Fiuczynski
<fiuczy(a)linux.vnet.ibm.com > ---
> src/remote/remote_daemon_dispatch.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: John Ferlan <jferlan(a)redhat.com
John
(always a bit "nervous" changing anything in these APIs...)
Yep, I know that feeling :)
8:20 p.m.
New subject: [libvirt] [PATCH 17/20] remote: Add the information which program has to be used to daemonClientEventCallback
As a result, you can later determine at the callback which program has
to be used. This makes it easier to refactor the code in the future
and is less prone to error.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 108 ++++++++++++++++++++----------------
1 file changed, 59 insertions(+), 49 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index ff26574c9f6b..9580e854efbe 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -78,6 +78,7 @@ VIR_LOG_INIT("daemon.remote");
struct daemonClientEventCallback {
virNetServerClientPtr client;
+ virNetServerProgramPtr program;
int eventID;
int callbackID;
bool legacy;
@@ -127,6 +128,7 @@ remoteEventCallbackFree(void *opaque)
daemonClientEventCallbackPtr callback = opaque;
if (!callback)
return;
+ virObjectUnref(callback->program);
virObjectUnref(callback->client);
VIR_FREE(callback);
}
@@ -318,7 +320,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
data.detail = detail;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_lifecycle_msg,
&data);
@@ -326,7 +328,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
remote_domain_event_callback_lifecycle_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_callback_lifecycle_msg,
&msg);
@@ -355,14 +357,14 @@ remoteRelayDomainEventReboot(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_REBOOT,
(xdrproc_t)xdr_remote_domain_event_reboot_msg,
&data);
} else {
remote_domain_event_callback_reboot_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_REBOOT,
(xdrproc_t)xdr_remote_domain_event_callback_reboot_msg, &msg);
}
@@ -394,14 +396,14 @@ remoteRelayDomainEventRTCChange(virConnectPtr conn,
data.offset = offset;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_rtc_change_msg,
&data);
} else {
remote_domain_event_callback_rtc_change_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_rtc_change_msg, &msg);
}
@@ -432,14 +434,14 @@ remoteRelayDomainEventWatchdog(virConnectPtr conn,
data.action = action;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_WATCHDOG,
(xdrproc_t)xdr_remote_domain_event_watchdog_msg,
&data);
} else {
remote_domain_event_callback_watchdog_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_WATCHDOG,
(xdrproc_t)xdr_remote_domain_event_callback_watchdog_msg, &msg);
}
@@ -476,14 +478,14 @@ remoteRelayDomainEventIOError(virConnectPtr conn,
data.action = action;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_IO_ERROR,
(xdrproc_t)xdr_remote_domain_event_io_error_msg,
&data);
} else {
remote_domain_event_callback_io_error_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_IO_ERROR,
(xdrproc_t)xdr_remote_domain_event_callback_io_error_msg, &msg);
}
@@ -527,14 +529,14 @@ remoteRelayDomainEventIOErrorReason(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_IO_ERROR_REASON,
(xdrproc_t)xdr_remote_domain_event_io_error_reason_msg, &data);
} else {
remote_domain_event_callback_io_error_reason_msg msg = {
callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_IO_ERROR_REASON,
(xdrproc_t)xdr_remote_domain_event_callback_io_error_reason_msg, &msg);
}
@@ -601,14 +603,14 @@ remoteRelayDomainEventGraphics(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_GRAPHICS,
(xdrproc_t)xdr_remote_domain_event_graphics_msg,
&data);
} else {
remote_domain_event_callback_graphics_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_GRAPHICS,
(xdrproc_t)xdr_remote_domain_event_callback_graphics_msg, &msg);
}
@@ -658,14 +660,14 @@ remoteRelayDomainEventBlockJob(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB,
(xdrproc_t)xdr_remote_domain_event_block_job_msg,
&data);
} else {
remote_domain_event_callback_block_job_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_BLOCK_JOB,
(xdrproc_t)xdr_remote_domain_event_callback_block_job_msg, &msg);
}
@@ -694,14 +696,14 @@ remoteRelayDomainEventControlError(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CONTROL_ERROR,
(xdrproc_t)xdr_remote_domain_event_control_error_msg, &data);
} else {
remote_domain_event_callback_control_error_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_CONTROL_ERROR,
(xdrproc_t)xdr_remote_domain_event_callback_control_error_msg, &msg);
}
@@ -752,14 +754,14 @@ remoteRelayDomainEventDiskChange(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE,
(xdrproc_t)xdr_remote_domain_event_disk_change_msg,
&data);
} else {
remote_domain_event_callback_disk_change_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DISK_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_disk_change_msg, &msg);
}
@@ -800,14 +802,14 @@ remoteRelayDomainEventTrayChange(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_TRAY_CHANGE,
(xdrproc_t)xdr_remote_domain_event_tray_change_msg,
&data);
} else {
remote_domain_event_callback_tray_change_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TRAY_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_tray_change_msg, &msg);
}
@@ -836,14 +838,14 @@ remoteRelayDomainEventPMWakeup(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_PMWAKEUP,
(xdrproc_t)xdr_remote_domain_event_pmwakeup_msg,
&data);
} else {
remote_domain_event_callback_pmwakeup_msg msg = { callback->callbackID,
reason, data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_PMWAKEUP,
(xdrproc_t)xdr_remote_domain_event_callback_pmwakeup_msg, &msg);
}
@@ -872,14 +874,14 @@ remoteRelayDomainEventPMSuspend(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND,
(xdrproc_t)xdr_remote_domain_event_pmsuspend_msg,
&data);
} else {
remote_domain_event_callback_pmsuspend_msg msg = { callback->callbackID,
reason, data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_PMSUSPEND,
(xdrproc_t)xdr_remote_domain_event_callback_pmsuspend_msg, &msg);
}
@@ -909,14 +911,14 @@ remoteRelayDomainEventBalloonChange(virConnectPtr conn,
data.actual = actual;
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_BALLOON_CHANGE,
(xdrproc_t)xdr_remote_domain_event_balloon_change_msg, &data);
} else {
remote_domain_event_callback_balloon_change_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_BALLOON_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_balloon_change_msg, &msg);
}
@@ -946,14 +948,14 @@ remoteRelayDomainEventPMSuspendDisk(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK,
(xdrproc_t)xdr_remote_domain_event_pmsuspend_disk_msg, &data);
} else {
remote_domain_event_callback_pmsuspend_disk_msg msg = { callback->callbackID,
reason, data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_PMSUSPEND_DISK,
(xdrproc_t)xdr_remote_domain_event_callback_pmsuspend_disk_msg, &msg);
}
@@ -986,7 +988,7 @@ remoteRelayDomainEventDeviceRemoved(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED,
(xdrproc_t)xdr_remote_domain_event_device_removed_msg,
&data);
@@ -994,7 +996,7 @@ remoteRelayDomainEventDeviceRemoved(virConnectPtr conn,
remote_domain_event_callback_device_removed_msg msg = { callback->callbackID,
data };
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_REMOVED,
(xdrproc_t)xdr_remote_domain_event_callback_device_removed_msg,
&msg);
@@ -1031,7 +1033,7 @@ remoteRelayDomainEventBlockJob2(virConnectPtr conn,
data.status = status;
make_nonnull_domain(&data.dom, dom);
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_BLOCK_JOB_2,
(xdrproc_t)xdr_remote_domain_event_block_job_2_msg,
&data);
@@ -1069,7 +1071,7 @@ remoteRelayDomainEventTunable(virConnectPtr conn,
return -1;
}
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_TUNABLE,
(xdrproc_t)xdr_remote_domain_event_callback_tunable_msg,
&data);
@@ -1104,7 +1106,7 @@ remoteRelayDomainEventAgentLifecycle(virConnectPtr conn,
data.state = state;
data.reason = reason;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_AGENT_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_callback_agent_lifecycle_msg,
&data);
@@ -1138,7 +1140,7 @@ remoteRelayDomainEventDeviceAdded(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_ADDED,
(xdrproc_t)xdr_remote_domain_event_callback_device_added_msg,
&data);
@@ -1171,7 +1173,7 @@ remoteRelayDomainEventMigrationIteration(virConnectPtr conn,
data.iteration = iteration;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_MIGRATION_ITERATION,
(xdrproc_t)xdr_remote_domain_event_callback_migration_iteration_msg,
&data);
@@ -1211,7 +1213,7 @@ remoteRelayDomainEventJobCompleted(virConnectPtr conn,
return -1;
}
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_JOB_COMPLETED,
(xdrproc_t)xdr_remote_domain_event_callback_job_completed_msg,
&data);
@@ -1244,7 +1246,7 @@ remoteRelayDomainEventDeviceRemovalFailed(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_REMOVAL_FAILED,
(xdrproc_t)xdr_remote_domain_event_callback_device_removal_failed_msg,
&data);
@@ -1288,7 +1290,7 @@ remoteRelayDomainEventMetadataChange(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_METADATA_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_metadata_change_msg,
&data);
@@ -1331,7 +1333,7 @@ remoteRelayDomainEventBlockThreshold(virConnectPtr conn,
data.excess = excess;
make_nonnull_domain(&data.dom, dom);
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_BLOCK_THRESHOLD,
(xdrproc_t)xdr_remote_domain_event_block_threshold_msg,
&data);
@@ -1396,7 +1398,7 @@ remoteRelayNetworkEventLifecycle(virConnectPtr conn,
data.event = event;
data.detail = detail;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_NETWORK_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_network_event_lifecycle_msg,
&data);
@@ -1433,7 +1435,7 @@ remoteRelayStoragePoolEventLifecycle(virConnectPtr conn,
data.event = event;
data.detail = detail;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_STORAGE_POOL_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_storage_pool_event_lifecycle_msg,
&data);
@@ -1461,7 +1463,7 @@ remoteRelayStoragePoolEventRefresh(virConnectPtr conn,
make_nonnull_storage_pool(&data.pool, pool);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_STORAGE_POOL_EVENT_REFRESH,
(xdrproc_t)xdr_remote_storage_pool_event_refresh_msg,
&data);
@@ -1500,7 +1502,7 @@ remoteRelayNodeDeviceEventLifecycle(virConnectPtr conn,
data.event = event;
data.detail = detail;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_NODE_DEVICE_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_node_device_event_lifecycle_msg,
&data);
@@ -1528,7 +1530,7 @@ remoteRelayNodeDeviceEventUpdate(virConnectPtr conn,
make_nonnull_node_device(&data.dev, dev);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE,
(xdrproc_t)xdr_remote_node_device_event_update_msg,
&data);
@@ -1567,7 +1569,7 @@ remoteRelaySecretEventLifecycle(virConnectPtr conn,
data.event = event;
data.detail = detail;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_SECRET_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_secret_event_lifecycle_msg,
&data);
@@ -1595,7 +1597,7 @@ remoteRelaySecretEventValueChanged(virConnectPtr conn,
make_nonnull_secret(&data.secret, secret);
data.callbackID = callback->callbackID;
- remoteDispatchObjectEventSend(callback->client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_SECRET_EVENT_VALUE_CHANGED,
(xdrproc_t)xdr_remote_secret_event_value_changed_msg,
&data);
@@ -1645,7 +1647,7 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
data.details = details_p;
make_nonnull_domain(&data.dom, dom);
- remoteDispatchObjectEventSend(callback->client, qemuProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
QEMU_PROC_DOMAIN_MONITOR_EVENT,
(xdrproc_t)xdr_qemu_domain_monitor_event_msg,
&data);
@@ -3900,6 +3902,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
ATTRIBUTE_UNUSED
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
callback->callbackID = -1;
callback->legacy = true;
@@ -4136,6 +4139,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
callback->legacy = true;
@@ -4212,6 +4216,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server ATTRI
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5735,6 +5740,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UN
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5857,6 +5863,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server ATTRIBUT
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5978,6 +5985,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server ATTRIBUTE
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6099,6 +6107,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6215,6 +6224,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
ATTRIBUTE_U
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
+ callback->program = virObjectRef(qemuProgram);
callback->eventID = -1;
callback->callbackID = -1;
ref = callback;
--
2.13.4
2:34 a.m.
New subject: [libvirt] [PATCH 17/20] remote: Add the information which program has to be used to daemonClientEventCallback
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
As a result, you can later determine at the callback which program
has
to be used. This makes it easier to refactor the code in the future
and is less prone to error.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 108 ++++++++++++++++++++----------------
1 file changed, 59 insertions(+), 49 deletions(-)
Well if the previous patch made me nervous, this one has me shaking even
more! Still my look through didn't find issues. Certainly an area that
I hope danpb may also take a loot. I can ping him when I'm done - for
at least the remote/libvirtd stuff.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
8:20 p.m.
New subject: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
This allows us to get rid of another usage of the global variable
remoteProgram.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 9580e854efbe..95940ffdeefe 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
static
void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason,
void *opaque)
{
- virNetServerClientPtr client = opaque;
+ daemonClientEventCallbackPtr callback = opaque;
VIR_DEBUG("Relaying connection closed event, reason %d", reason);
remote_connect_event_connection_closed_msg msg = { reason };
- remoteDispatchObjectEventSend(client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
&msg);
@@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
virNetMessageErrorPtr rerr)
{
int rv = -1;
+ daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
@@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
goto cleanup;
}
+ if (VIR_ALLOC(callback) < 0)
+ goto cleanup;
+ callback->client = virObjectRef(client);
+ callback->program = virObjectRef(remoteProgram);
+ /* eventID, callbackID, and legacy are not used */
+ callback->eventID = -1;
+ callback->callbackID = -1;
if (virConnectRegisterCloseCallback(priv->conn,
remoteRelayConnectionClosedEvent,
- client, NULL) < 0)
+ callback, remoteEventCallbackFree) < 0)
goto cleanup;
priv->closeRegistered = true;
@@ -3834,8 +3842,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
cleanup:
virMutexUnlock(&priv->lock);
- if (rv < 0)
+ if (rv < 0) {
+ remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
+ }
return rv;
}
--
2.13.4
2:56 a.m.
New subject: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
This allows us to get rid of another usage of the global variable
remoteProgram.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
(see my note below, I imagine you agree...)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 9580e854efbe..95940ffdeefe 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
static
void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason,
void *opaque)
{
- virNetServerClientPtr client = opaque;
+ daemonClientEventCallbackPtr callback = opaque;
VIR_DEBUG("Relaying connection closed event, reason %d", reason);
remote_connect_event_connection_closed_msg msg = { reason };
- remoteDispatchObjectEventSend(client, remoteProgram,
+ remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
&msg);
@@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
virNetMessageErrorPtr rerr)
{
int rv = -1;
+ daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
@@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
goto cleanup;
}
+ if (VIR_ALLOC(callback) < 0)
+ goto cleanup;
+ callback->client = virObjectRef(client);
This one's scary because currently that means we currently call
virConnectRegisterCloseCallback, but haven't been doing the
virObjectRef(client) prior to using it as an opaque... Meaning
remoteRelayConnectionClosedEvent could be called with client already free'd.
+ callback->program = virObjectRef(remoteProgram);
+ /* eventID, callbackID, and legacy are not used */
+ callback->eventID = -1;
+ callback->callbackID = -1;
if (virConnectRegisterCloseCallback(priv->conn,
remoteRelayConnectionClosedEvent,
- client, NULL) < 0)
+ callback, remoteEventCallbackFree) < 0)
goto cleanup;
priv->closeRegistered = true;
@@ -3834,8 +3842,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
cleanup:
virMutexUnlock(&priv->lock);
- if (rv < 0)
+ if (rv < 0) {
+ remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
+ }
return rv;
}
3:05 a.m.
New subject: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
On Thu, Mar 15, 2018 at 07:56 PM +0100, John Ferlan <jferlan(a)redhat.com> wrote:
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> This allows us to get rid of another usage of the global variable
> remoteProgram.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
Thanks.
(see my note below, I imagine you agree...)
> diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
> index 9580e854efbe..95940ffdeefe 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
> static
> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int
reason, void *opaque)
> {
> - virNetServerClientPtr client = opaque;
> + daemonClientEventCallbackPtr callback = opaque;
>
> VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>
> remote_connect_event_connection_closed_msg msg = { reason };
> - remoteDispatchObjectEventSend(client, remoteProgram,
> + remoteDispatchObjectEventSend(callback->client, callback->program,
> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
> &msg);
> @@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
> virNetMessageErrorPtr rerr)
> {
> int rv = -1;
> + daemonClientEventCallbackPtr callback = NULL;
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> @@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
> goto cleanup;
> }
>
> + if (VIR_ALLOC(callback) < 0)
> + goto cleanup;
> + callback->client = virObjectRef(client);
This one's scary because currently that means we currently call
virConnectRegisterCloseCallback, but haven't been doing the
virObjectRef(client) prior to using it as an opaque... Meaning
remoteRelayConnectionClosedEvent could be called with client already
free'd.
Yep. Or at least, I think so.
> + callback->program = virObjectRef(remoteProgram);
> + /* eventID, callbackID, and legacy are not used */
> + callback->eventID = -1;
> + callback->callbackID = -1;
> if (virConnectRegisterCloseCallback(priv->conn,
> remoteRelayConnectionClosedEvent,
> - client, NULL) < 0)
> + callback, remoteEventCallbackFree) < 0)
> goto cleanup;
>
> priv->closeRegistered = true;
> @@ -3834,8 +3842,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
>
> cleanup:
> virMutexUnlock(&priv->lock);
> - if (rv < 0)
> + if (rv < 0) {
> + remoteEventCallbackFree(callback);
> virNetMessageSaveError(rerr);
> + }
> return rv;
> }
>
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
3:08 a.m.
New subject: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
On Thu, Mar 15, 2018 at 07:56 PM +0100, John Ferlan <jferlan(a)redhat.com> wrote:
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> This allows us to get rid of another usage of the global variable
> remoteProgram.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
(see my note below, I imagine you agree...)
> diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
> index 9580e854efbe..95940ffdeefe 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
> static
> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int
reason, void *opaque)
> {
> - virNetServerClientPtr client = opaque;
> + daemonClientEventCallbackPtr callback = opaque;
>
> VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>
> remote_connect_event_connection_closed_msg msg = { reason };
> - remoteDispatchObjectEventSend(client, remoteProgram,
> + remoteDispatchObjectEventSend(callback->client, callback->program,
> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
> &msg);
> @@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
> virNetMessageErrorPtr rerr)
> {
> int rv = -1;
> + daemonClientEventCallbackPtr callback = NULL;
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> @@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
> goto cleanup;
> }
>
> + if (VIR_ALLOC(callback) < 0)
> + goto cleanup;
> + callback->client = virObjectRef(client);
This one's scary because currently that means we currently call
virConnectRegisterCloseCallback, but haven't been doing the
virObjectRef(client) prior to using it as an opaque... Meaning
remoteRelayConnectionClosedEvent could be called with client already free'd.
> + callback->program = virObjectRef(remoteProgram);
> + /* eventID, callbackID, and legacy are not used */
> + callback->eventID = -1;
> + callback->callbackID = -1;
> if (virConnectRegisterCloseCallback(priv->conn,
> remoteRelayConnectionClosedEvent,
> - client, NULL) < 0)
> + callback, remoteEventCallbackFree) < 0)
> goto cleanup;
This is where I produced a memory leak, relying on
virConnectRegisterCloseCallback to return < 0 if something failed.
>
> priv->closeRegistered = true;
> @@ -3834,8 +3842,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
>
> cleanup:
> virMutexUnlock(&priv->lock);
> - if (rv < 0)
> + if (rv < 0) {
> + remoteEventCallbackFree(callback);
> virNetMessageSaveError(rerr);
> + }
> return rv;
> }
>
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
5:29 p.m.
New subject: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
On 15.03.2018 21:56, John Ferlan wrote:
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> This allows us to get rid of another usage of the global variable
> remoteProgram.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
> ---
> src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
(see my note below, I imagine you agree...)
> diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
> index 9580e854efbe..95940ffdeefe 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
> static
> void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int
reason, void *opaque)
> {
> - virNetServerClientPtr client = opaque;
> + daemonClientEventCallbackPtr callback = opaque;
>
> VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>
> remote_connect_event_connection_closed_msg msg = { reason };
> - remoteDispatchObjectEventSend(client, remoteProgram,
> + remoteDispatchObjectEventSend(callback->client, callback->program,
> REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
> &msg);
> @@ -3814,6 +3814,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
> virNetMessageErrorPtr rerr)
> {
> int rv = -1;
> + daemonClientEventCallbackPtr callback = NULL;
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> @@ -3824,9 +3825,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
> goto cleanup;
> }
>
> + if (VIR_ALLOC(callback) < 0)
> + goto cleanup;
> + callback->client = virObjectRef(client);
This one's scary because currently that means we currently call
virConnectRegisterCloseCallback, but haven't been doing the
virObjectRef(client) prior to using it as an opaque... Meaning
remoteRelayConnectionClosedEvent could be called with client already free'd.
Refcounting was here originally but then removed in [1] as it conflicts with
virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
is not implemented. This is safe though (at least nobody touch this place :).
ce35122cfe: "daemon: fixup refcounting in close callback handling"
Nikolay
> + callback->program = virObjectRef(remoteProgram);
> + /* eventID, callbackID, and legacy are not used */
> + callback->eventID = -1;
> + callback->callbackID = -1;
> if (virConnectRegisterCloseCallback(priv->conn,
> remoteRelayConnectionClosedEvent,
> - client, NULL) < 0)
> + callback, remoteEventCallbackFree) < 0)
> goto cleanup;
>
> priv->closeRegistered = true;
> @@ -3834,8 +3842,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr
server ATTRIBUTE_UNUS
>
> cleanup:
> virMutexUnlock(&priv->lock);
> - if (rv < 0)
> + if (rv < 0) {
> + remoteEventCallbackFree(callback);
> virNetMessageSaveError(rerr);
> + }
> return rv;
> }
>
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:20 p.m.
New subject: [libvirt] [PATCH 19/20] rpc: Introduce virNetServerGetProgramLocked helper function
This patch introduces virNetServerGetProgramLocked. It's a function to
determine which program has to be used for a given @msg. This function
will be reused in the next patch.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/rpc/virnetserver.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 204f425264fa..4cd42ad7fd40 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -182,6 +182,29 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque)
VIR_FREE(job);
}
+
+/**
+ * virNetServerGetProgramLocked:
+ * @srv: server (must be locked by the caller)
+ * @msg: message
+ *
+ * Searches @srv for the right program for a given message @msg.
+ *
+ * Returns a pointer to the server program or NULL if not found.
+ */
+static virNetServerProgramPtr
+virNetServerGetProgramLocked(virNetServerPtr srv,
+ virNetMessagePtr msg)
+{
+ size_t i;
+ for (i = 0; i < srv->nprograms; i++) {
+ if (virNetServerProgramMatches(srv->programs[i], msg))
+ return srv->programs[i];
+ }
+ return NULL;
+}
+
+
static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
virNetMessagePtr msg,
void *opaque)
@@ -189,19 +212,13 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr
client,
virNetServerPtr srv = opaque;
virNetServerProgramPtr prog = NULL;
unsigned int priority = 0;
- size_t i;
int ret = -1;
VIR_DEBUG("server=%p client=%p message=%p",
srv, client, msg);
virObjectLock(srv);
- for (i = 0; i < srv->nprograms; i++) {
- if (virNetServerProgramMatches(srv->programs[i], msg)) {
- prog = srv->programs[i];
- break;
- }
- }
+ prog = virNetServerGetProgramLocked(srv, msg);
if (srv->workers) {
virNetServerJobPtr job;
--
2.13.4
2:59 a.m.
New subject: [libvirt] [PATCH 19/20] rpc: Introduce virNetServerGetProgramLocked helper function
On 03/08/2018 07:20 AM, 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>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
src/rpc/virnetserver.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
This and the followup patch I haven't been able to git am -3 apply due
to other changes... So no build, but from a purely visual look
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
(but I'll need an updated version before being able to push)
8:20 p.m.
New subject: [libvirt] [PATCH 20/20] remote/rpc: Use virNetServerGetProgram() to determine the program
Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
Note: I'm not 100% sure that there is no case where the lock for
@client is already held by the thread which is calling
virNetServerGetProgram and thus the lock order would be violated (the
lock order has to be @server -> @client in the violating case it would
be @client -> @server and therefore a deadlock might occur).
---
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +--
src/remote/remote_daemon.h | 3 ---
src/remote/remote_daemon_dispatch.c | 52 ++++++++++++++++++-------------------
src/rpc/gendispatch.pl | 2 ++
src/rpc/virnetserver.c | 23 ++++++++++++++++
src/rpc/virnetserver.h | 2 ++
7 files changed, 56 insertions(+), 31 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 97e22275b980..c31b16cd5909 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -120,6 +120,7 @@ virNetServerGetCurrentUnauthClients;
virNetServerGetMaxClients;
virNetServerGetMaxUnauthClients;
virNetServerGetName;
+virNetServerGetProgram;
virNetServerGetThreadPoolParameters;
virNetServerHasClients;
virNetServerNew;
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 078430f91c97..64f67ef2fefb 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -71,8 +71,6 @@ VIR_LOG_INIT("daemon.libvirtd");
#if WITH_SASL
virNetSASLContextPtr saslCtxt = NULL;
#endif
-virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr qemuProgram = NULL;
volatile bool driversInitialized = false;
@@ -1061,6 +1059,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 4467f71da910..93f2e1f531b0 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -82,7 +82,4 @@ struct daemonClientPrivate {
# if WITH_SASL
extern virNetSASLContextPtr saslCtxt;
# endif
-extern virNetServerProgramPtr remoteProgram;
-extern virNetServerProgramPtr qemuProgram;
-
#endif /* __REMOTE_DAEMON_H__ */
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 95940ffdeefe..3dce6195230e 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3808,9 +3808,9 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server
ATTRIBUTE_UNUSED,
}
static int
-remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr)
{
int rv = -1;
@@ -3828,7 +3828,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
/* eventID, callbackID, and legacy are not used */
callback->eventID = -1;
callback->callbackID = -1;
@@ -3912,7 +3912,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
ATTRIBUTE_UNUSED
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
callback->callbackID = -1;
callback->legacy = true;
@@ -4110,9 +4110,9 @@ remoteDispatchDomainGetState(virNetServerPtr server
ATTRIBUTE_UNUSED,
* VIR_DRV_SUPPORTS_FEATURE(VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK),
* and must not mix the two styles. */
static int
-remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
remote_connect_domain_event_register_any_args
*args)
{
@@ -4149,7 +4149,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
callback->legacy = true;
@@ -4185,9 +4185,9 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
static int
-remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg
ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_domain_event_callback_register_any_args *args,
remote_connect_domain_event_callback_register_any_ret *ret)
@@ -4226,7 +4226,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server ATTRI
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5352,7 +5352,7 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr
server ATTRIBUTE
goto cleanup;
if (!(st = virStreamNew(priv->conn, VIR_STREAM_NONBLOCK)) ||
- !(stream = daemonCreateClientStream(client, st, remoteProgram,
+ !(stream = daemonCreateClientStream(client, st, virNetServerGetProgram(server,
msg),
&msg->header, false)))
goto cleanup;
@@ -5709,9 +5709,9 @@ static int remoteDispatchDomainCreateWithFiles(virNetServerPtr
server ATTRIBUTE_
static int
-remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_network_event_register_any_args *args,
remote_connect_network_event_register_any_ret *ret)
@@ -5750,7 +5750,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UN
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5832,9 +5832,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr
server ATTRIBUTE_
}
static int
-remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
+remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_storage_pool_event_register_any_args *args,
remote_connect_storage_pool_event_register_any_ret *ret)
@@ -5873,7 +5873,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server ATTRIBUT
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5954,9 +5954,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr
server ATTRIB
}
static int
-remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_node_device_event_register_any_args *args,
remote_connect_node_device_event_register_any_ret *ret)
@@ -5995,7 +5995,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server ATTRIBUTE
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6076,9 +6076,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr
server ATTRIBU
}
static int
-remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
remote_connect_secret_event_register_any_args
*args,
remote_connect_secret_event_register_any_ret
*ret)
@@ -6117,7 +6117,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6198,9 +6198,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server
ATTRIBUTE_U
}
static int
-qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED,
+qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
qemu_connect_domain_monitor_event_register_args *args,
qemu_connect_domain_monitor_event_register_ret *ret)
@@ -6234,7 +6234,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
ATTRIBUTE_U
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(qemuProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = -1;
callback->callbackID = -1;
ref = callback;
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index fb15cc4849bc..725319bf8da4 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -990,6 +990,7 @@ elsif ($mode eq "server") {
if ($call->{streamflag} ne "none") {
print " virStreamPtr st = NULL;\n";
print " daemonClientStreamPtr stream = NULL;\n";
+ print " virNetServerProgramPtr remoteProgram = NULL;\n";
if ($call->{sparseflag} ne "none") {
print " const bool sparse = args->flags &
$call->{sparseflag};\n"
} else {
@@ -1037,6 +1038,7 @@ elsif ($mode eq "server") {
print " if (!(st = virStreamNew(priv->conn,
VIR_STREAM_NONBLOCK)))\n";
print " goto cleanup;\n";
print "\n";
+ print " remoteProgram = virNetServerGetProgram(server,
msg);\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 4cd42ad7fd40..b1ff4a0cd751 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -205,6 +205,29 @@ virNetServerGetProgramLocked(virNetServerPtr srv,
}
+/**
+ * virNetServerGetProgram:
+ * @srv: server (must NOT be locked by the caller)
+ * @msg: message
+ *
+ * Searches @srv for the right program for a given message @msg.
+ *
+ * Returns a pointer to the server program or NULL if not found.
+ */
+virNetServerProgramPtr
+virNetServerGetProgram(virNetServerPtr srv,
+ virNetMessagePtr msg)
+{
+ virNetServerProgramPtr ret = NULL;
+
+ virObjectLock(srv);
+ ret = virNetServerGetProgramLocked(srv, msg);
+ virObjectUnlock(srv);
+
+ return ret;
+}
+
+
static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
virNetMessagePtr msg,
void *opaque)
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index a79c39fdb2e7..1867e46664ba 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -76,6 +76,8 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
virNetTLSContextPtr tls);
# endif
+virNetServerProgramPtr virNetServerGetProgram(virNetServerPtr srv,
+ virNetMessagePtr msg);
int virNetServerAddClient(virNetServerPtr srv,
virNetServerClientPtr client);
--
2.13.4
3:12 a.m.
New subject: [libvirt] [PATCH 20/20] remote/rpc: Use virNetServerGetProgram() to determine the program
On Thu, Mar 08, 2018 at 01:20 PM +0100, Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
wrote:
Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
[…snip…]
+/**
+ * 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 = NULL;
The initialization to NULL is useless here… :) Will change it for v2.
+
+ virObjectLock(srv);
+ ret = virNetServerGetProgramLocked(srv, msg);
+ virObjectUnlock(srv);
+
+ return ret;
+}
+
+
static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
virNetMessagePtr msg,
void *opaque)
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index a79c39fdb2e7..1867e46664ba 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -76,6 +76,8 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
virNetTLSContextPtr tls);
# endif
+virNetServerProgramPtr virNetServerGetProgram(virNetServerPtr srv,
+ virNetMessagePtr msg);
int virNetServerAddClient(virNetServerPtr srv,
virNetServerClientPtr client);
--
2.13.4
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:57 a.m.
New subject: [libvirt] [PATCH 20/20] remote/rpc: Use virNetServerGetProgram() to determine the program
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
---
Note: I'm not 100% sure that there is no case where the lock for
@client is already held by the thread which is calling
virNetServerGetProgram and thus the lock order would be violated (the
lock order has to be @server -> @client in the violating case it would
be @client -> @server and therefore a deadlock might occur).
My quick look didn't see any, but this usually becomes a danpb type
question - that is "historically" has any entrance into
remote_daemon_dispatch.c API's already taken the client lock.
---
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +--
src/remote/remote_daemon.h | 3 ---
src/remote/remote_daemon_dispatch.c | 52 ++++++++++++++++++-------------------
src/rpc/gendispatch.pl | 2 ++
src/rpc/virnetserver.c | 23 ++++++++++++++++
src/rpc/virnetserver.h | 2 ++
7 files changed, 56 insertions(+), 31 deletions(-)
As noted in 19/20 - I wasn't able to git am -3 apply this, so I'll need
a v2... From visual inspection things look good - I do have one
observation below...
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 97e22275b980..c31b16cd5909 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -120,6 +120,7 @@ virNetServerGetCurrentUnauthClients;
virNetServerGetMaxClients;
virNetServerGetMaxUnauthClients;
virNetServerGetName;
+virNetServerGetProgram;
virNetServerGetThreadPoolParameters;
virNetServerHasClients;
virNetServerNew;
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 078430f91c97..64f67ef2fefb 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -71,8 +71,6 @@ VIR_LOG_INIT("daemon.libvirtd");
#if WITH_SASL
virNetSASLContextPtr saslCtxt = NULL;
#endif
-virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr qemuProgram = NULL;
volatile bool driversInitialized = false;
@@ -1061,6 +1059,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 4467f71da910..93f2e1f531b0 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -82,7 +82,4 @@ struct daemonClientPrivate {
# if WITH_SASL
extern virNetSASLContextPtr saslCtxt;
# endif
-extern virNetServerProgramPtr remoteProgram;
-extern virNetServerProgramPtr qemuProgram;
-
#endif /* __REMOTE_DAEMON_H__ */
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 95940ffdeefe..3dce6195230e 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -3808,9 +3808,9 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server
ATTRIBUTE_UNUSED,
}
static int
-remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr)
{
int rv = -1;
@@ -3828,7 +3828,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
ATTRIBUTE_UNUS
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
So while it "shouldn't happen", virNetServerGetProgramLocked could
return NULL causing chaos...
John
/* eventID, callbackID, and legacy are not used */
callback->eventID = -1;
callback->callbackID = -1;
@@ -3912,7 +3912,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
ATTRIBUTE_UNUSED
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
callback->callbackID = -1;
callback->legacy = true;
@@ -4110,9 +4110,9 @@ remoteDispatchDomainGetState(virNetServerPtr server
ATTRIBUTE_UNUSED,
* VIR_DRV_SUPPORTS_FEATURE(VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK),
* and must not mix the two styles. */
static int
-remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_domain_event_register_any_args *args)
{
@@ -4149,7 +4149,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
callback->legacy = true;
@@ -4185,9 +4185,9 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
static int
-remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
+remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg
ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_domain_event_callback_register_any_args *args,
remote_connect_domain_event_callback_register_any_ret *ret)
@@ -4226,7 +4226,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server ATTRI
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5352,7 +5352,7 @@ remoteDispatchDomainMigratePrepareTunnel3Params(virNetServerPtr
server ATTRIBUTE
goto cleanup;
if (!(st = virStreamNew(priv->conn, VIR_STREAM_NONBLOCK)) ||
- !(stream = daemonCreateClientStream(client, st, remoteProgram,
+ !(stream = daemonCreateClientStream(client, st, virNetServerGetProgram(server,
msg),
&msg->header, false)))
goto cleanup;
@@ -5709,9 +5709,9 @@ static int remoteDispatchDomainCreateWithFiles(virNetServerPtr
server ATTRIBUTE_
static int
-remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_network_event_register_any_args *args,
remote_connect_network_event_register_any_ret *ret)
@@ -5750,7 +5750,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UN
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5832,9 +5832,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr
server ATTRIBUTE_
}
static int
-remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
+remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_storage_pool_event_register_any_args *args,
remote_connect_storage_pool_event_register_any_ret *ret)
@@ -5873,7 +5873,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server ATTRIBUT
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5954,9 +5954,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr
server ATTRIB
}
static int
-remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNUSED,
+remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_node_device_event_register_any_args *args,
remote_connect_node_device_event_register_any_ret *ret)
@@ -5995,7 +5995,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server ATTRIBUTE
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6076,9 +6076,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr
server ATTRIBU
}
static int
-remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server ATTRIBUTE_UNUSED,
+remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
remote_connect_secret_event_register_any_args *args,
remote_connect_secret_event_register_any_ret
*ret)
@@ -6117,7 +6117,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(remoteProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6198,9 +6198,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr
server ATTRIBUTE_U
}
static int
-qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server ATTRIBUTE_UNUSED,
+qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr
ATTRIBUTE_UNUSED,
qemu_connect_domain_monitor_event_register_args *args,
qemu_connect_domain_monitor_event_register_ret *ret)
@@ -6234,7 +6234,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr
server ATTRIBUTE_U
if (VIR_ALLOC(callback) < 0)
goto cleanup;
callback->client = virObjectRef(client);
- callback->program = virObjectRef(qemuProgram);
+ callback->program = virObjectRef(virNetServerGetProgram(server, msg));
callback->eventID = -1;
callback->callbackID = -1;
ref = callback;
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index fb15cc4849bc..725319bf8da4 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -990,6 +990,7 @@ elsif ($mode eq "server") {
if ($call->{streamflag} ne "none") {
print " virStreamPtr st = NULL;\n";
print " daemonClientStreamPtr stream = NULL;\n";
+ print " virNetServerProgramPtr remoteProgram = NULL;\n";
if ($call->{sparseflag} ne "none") {
print " const bool sparse = args->flags &
$call->{sparseflag};\n"
} else {
@@ -1037,6 +1038,7 @@ elsif ($mode eq "server") {
print " if (!(st = virStreamNew(priv->conn,
VIR_STREAM_NONBLOCK)))\n";
print " goto cleanup;\n";
print "\n";
+ print " remoteProgram = virNetServerGetProgram(server,
msg);\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 4cd42ad7fd40..b1ff4a0cd751 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -205,6 +205,29 @@ virNetServerGetProgramLocked(virNetServerPtr srv,
}
+/**
+ * virNetServerGetProgram:
+ * @srv: server (must NOT be locked by the caller)
+ * @msg: message
+ *
+ * Searches @srv for the right program for a given message @msg.
+ *
+ * Returns a pointer to the server program or NULL if not found.
+ */
+virNetServerProgramPtr
+virNetServerGetProgram(virNetServerPtr srv,
+ virNetMessagePtr msg)
+{
+ virNetServerProgramPtr ret = NULL;
+
+ virObjectLock(srv);
+ ret = virNetServerGetProgramLocked(srv, msg);
+ virObjectUnlock(srv);
+
+ return ret;
+}
+
+
static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
virNetMessagePtr msg,
void *opaque)
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index a79c39fdb2e7..1867e46664ba 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -76,6 +76,8 @@ int virNetServerSetTLSContext(virNetServerPtr srv,
virNetTLSContextPtr tls);
# endif
+virNetServerProgramPtr virNetServerGetProgram(virNetServerPtr srv,
+ virNetMessagePtr msg);
int virNetServerAddClient(virNetServerPtr srv,
virNetServerClientPtr client);
5:07 a.m.
On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
The first part of this patch series fixes the behavior of
virConnectSupportsFeatures, virConnect(Un)RegisterCloseCallback, and
implements these features in the test driver. This results in a better
code coverage of our test suite.
The subsequent patches remove the need to have the global variables
'qemuProgram', 'adminProgram', 'lxcProgram, and
'remoteProgram' in
remote_daemon.[ch]. They only work in combination with the fixed
behavior of virConnectSupportsFeatures and
virConnect(Un)RegisterCloseCallback.
Marc Hartmayer (20):
driver: Add typedef for the anonymous enum used for driver features
remote: Don't hard code the feature
VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
virConnect(Un)RegisterCloseCallback: Throw an error in case the API is
not supported
test: Implement virConnectSupportsFeature
test: Implement virConnect(Un)RegisterCloseCallback
test: testOpenDefault: introduce cleanup path
test: testOpenFromFile: return VIR_DRV_OPEN_SUCCESS in case of success
test: testConnectAuthenticate: Take the lock when accessing mutable
values
test: testConnectClose: Set privateData to NULL in all cases
test: rename defaultConn to defaultPrivconn
test: introduce testDriverCloseInternal
test: fix error path in testConnectOpen
test: Convert testDriver to virObjectLockable
remote: remove unneeded global variables
stream: Access stream->prog instead of a hard-coded global variable
remote: Set eventID explicitly to an invalid value
remote: Add the information which program has to be used to
daemonClientEventCallback
remote: Use domainClientEventCallbacks for
remoteReplayConnectionClosedEvent
rpc: Introduce virNetServerGetProgramLocked helper function
remote/rpc: Use virNetServerGetProgram() to determine the program
src/esx/esx_driver.c | 18 +-
src/libvirt-host.c | 24 +--
src/libvirt_internal.h | 4 +-
src/libvirt_remote.syms | 1 +
src/libxl/libxl_driver.c | 13 +-
src/lxc/lxc_driver.c | 24 ++-
src/openvz/openvz_driver.c | 15 +-
src/qemu/qemu_driver.c | 8 +-
src/remote/remote_daemon.c | 8 +-
src/remote/remote_daemon.h | 3 -
src/remote/remote_daemon_dispatch.c | 182 +++++++++++--------
src/remote/remote_daemon_stream.c | 14 +-
src/rpc/gendispatch.pl | 2 +
src/rpc/virnetserver.c | 54 +++++-
src/rpc/virnetserver.h | 2 +
src/test/test_driver.c | 339 ++++++++++++++++++++++--------------
src/vz/vz_driver.c | 15 +-
src/xen/xen_driver.c | 15 +-
tools/virsh.c | 11 +-
19 files changed, 504 insertions(+), 248 deletions(-)
So based on what I R-B'd - I can grab patches 1, 6 -> 12, and 14 -> 16
alter the couple of minor things noted, then push...
FWIW: patch 13 depends on a couple of things changed in patch 5, so
while it is fine, I'd rather not get too many merge conflicts. There is
one minor one from
I'll also keep defaultPrivconn and not change to defaultPrivateData as
that just gets way too messy. I agree since we have privconn in general,
it'll be fine for the global too...
That should help reduce the v2 pile a bit!
John
2403
days inactive
2429
days old
62 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
John Ferlan -
Marc Hartmayer -
Nikolay Shirokovskiy