• Subject: [libvirt] [PATCH 00/20] Fix virConnect(Un)RegisterCloseCallback and get rid of global variables
  • Author: Marc Hartmayer
  • Date: March 8, 2018, 12:20 p.m.
  • Patches: 20 / 20
Changeset
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(-)
Git apply log
Switched to a new branch '20180308122043.15608-1-mhartmay@linux.vnet.ibm.com'
Applying: driver: Add typedef for the anonymous enum used for driver features
Applying: remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Applying: virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Applying: test: Implement virConnectSupportsFeature
Applying: test: Implement virConnect(Un)RegisterCloseCallback
Applying: test: testOpenDefault: introduce cleanup path
Applying: test: testOpenFromFile: return VIR_DRV_OPEN_SUCCESS in case of success
Applying: test: testConnectAuthenticate: Take the lock when accessing mutable values
Applying: test: testConnectClose: Set privateData to NULL in all cases
Applying: test: rename defaultConn to defaultPrivconn
Applying: test: introduce testDriverCloseInternal
Applying: test: fix error path in testConnectOpen
Applying: test: Convert testDriver to virObjectLockable
Applying: remote: remove unneeded global variables
Applying: stream: Access stream->prog instead of a hard-coded global variable
Applying: remote: Set eventID explicitly to an invalid value
Applying: remote: Add the information which program has to be used to daemonClientEventCallback
Applying: remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
Applying: rpc: Introduce virNetServerGetProgramLocked helper function
Using index info to reconstruct a base tree...
M	src/rpc/virnetserver.c
Falling back to patching base and 3-way merge...
Auto-merging src/rpc/virnetserver.c
CONFLICT (content): Merge conflict in src/rpc/virnetserver.c
error: Failed to merge in the changes.
Patch failed at 0001 rpc: Introduce virNetServerGetProgramLocked helper function
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Failed to apply patch:
[libvirt] [PATCH 19/20] rpc: Introduce virNetServerGetProgramLocked helper function
Test passed: syntax-check

loading

[libvirt] [PATCH 00/20] Fix virConnect(Un)RegisterCloseCallback and get rid of global variables
Posted by Marc Hartmayer, 15 weeks ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/20] Fix virConnect(Un)RegisterCloseCallback and get rid of global variables
Posted by John Ferlan, 14 weeks ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 01/20] driver: Add typedef for the anonymous enum used for driver features
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/20] driver: Add typedef for the anonymous enum used for driver features
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@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;
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@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@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;
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by Marc Hartmayer, 14 weeks ago
On Thu, Mar 15, 2018 at 12:02 AM +0100, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@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@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by Nikolay Shirokovskiy, 14 weeks ago

On 15.03.2018 21:52, Marc Hartmayer wrote:
> On Thu, Mar 15, 2018 at 12:02 AM +0100, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>>>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy@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@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
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by Daniel P. Berrangé, 13 weeks ago
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by Marc Hartmayer, 13 weeks ago
On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé" <berrange@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by Marc Hartmayer, 11 weeks ago
On Wed, Mar 21, 2018 at 07:57 AM +0100, Marc Hartmayer <mhartmay@linux.vnet.ibm.com> wrote:
> On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé" <berrange@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by Daniel P. Berrangé, 11 weeks ago
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@linux.vnet.ibm.com> wrote:
> > On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé" <berrange@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/20] remote: Don't hard code the feature VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK as available
Posted by Marc Hartmayer, 11 weeks ago
On Tue, Apr 03, 2018 at 02:05 PM +0200, "Daniel P. Berrangé" <berrange@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@linux.vnet.ibm.com> wrote:
>> > On Mon, Mar 19, 2018 at 04:06 PM +0100, "Daniel P. Berrangé" <berrange@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by John Ferlan, 14 weeks ago

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@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by Nikolay Shirokovskiy, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by Nikolay Shirokovskiy, 14 weeks ago

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@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@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@virtuozzo.com>
Date:   Tue Mar 1 14:17:38 2016 +0000

    close callback: move it to driver
    
    Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by Daniel P. Berrangé, 13 weeks ago
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 04/20] test: Implement virConnectSupportsFeature
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/20] test: Implement virConnectSupportsFeature
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@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 */
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/20] test: Implement virConnectSupportsFeature
Posted by Daniel P. Berrangé, 13 weeks ago
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@linux.vnet.ibm.com>
> > Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> > Reviewed-by: Boris Fiuczynski <fiuczy@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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 */
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback
Posted by Daniel P. Berrangé, 13 weeks ago
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 06/20] test: testOpenDefault: introduce cleanup path
Posted by Marc Hartmayer, 15 weeks ago
The two code paths have some cleanup in common so lets refactor it.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/20] test: testOpenDefault: introduce cleanup path
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@redhat.com>

John

[I'll continue with the rest tomorrow]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 07/20] test: testOpenFromFile: return VIR_DRV_OPEN_SUCCESS in case of success
Posted by Marc Hartmayer, 15 weeks ago
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/20] test: testOpenFromFile: return VIR_DRV_OPEN_SUCCESS in case of success
Posted by John Ferlan, 14 weeks ago

On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/test/test_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 08/20] test: testConnectAuthenticate: Take the lock when accessing mutable values
Posted by Marc Hartmayer, 15 weeks ago
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/20] test: testConnectAuthenticate: Take the lock when accessing mutable values
Posted by John Ferlan, 14 weeks ago

On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/test/test_driver.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 09/20] test: testConnectClose: Set privateData to NULL in all cases
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/20] test: testConnectClose: Set privateData to NULL in all cases
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/test/test_driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 10/20] test: rename defaultConn to defaultPrivconn
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/20] test: rename defaultConn to defaultPrivconn
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/20] test: rename defaultConn to defaultPrivconn
Posted by Marc Hartmayer, 14 weeks ago
On Thu, Mar 15, 2018 at 03:38 PM +0100, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@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@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 11/20] test: introduce testDriverCloseInternal
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/20] test: introduce testDriverCloseInternal
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/test/test_driver.c | 61 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 26 deletions(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 12/20] test: fix error path in testConnectOpen
Posted by Marc Hartmayer, 15 weeks ago
In case of an error do the cleanup of the private data of the
connection.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/20] test: fix error path in testConnectOpen
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/test/test_driver.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 13/20] test: Convert testDriver to virObjectLockable
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/20] test: Convert testDriver to virObjectLockable
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@redhat.com>

John

(another one of those things I can do)

> +    if (disposed && driver == defaultPrivconn)
>          defaultPrivconn = NULL;
> -        virMutexUnlock(&defaultLock);
> -    }
> +    virMutexUnlock(&defaultLock);
>  }
>  
>  
[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/20] test: Convert testDriver to virObjectLockable
Posted by Marc Hartmayer, 14 weeks ago
On Thu, Mar 15, 2018 at 04:05 PM +0100, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@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@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 14/20] remote: remove unneeded global variables
Posted by Marc Hartmayer, 15 weeks ago
Remove unneeded global variables and convert them into local variables
where they're needed.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 14/20] remote: remove unneeded global variables
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 15/20] stream: Access stream->prog instead of a hard-coded global variable
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 15/20] stream: Access stream->prog instead of a hard-coded global variable
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/remote/remote_daemon_stream.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 15/20] stream: Access stream->prog instead of a hard-coded global variable
Posted by Marc Hartmayer, 14 weeks ago
On Thu, Mar 15, 2018 at 04:22 PM +0100, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>> ---
>>  src/remote/remote_daemon_stream.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>
> Reviewed-by: John Ferlan <jferlan@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 16/20] remote: Set eventID explicitly to an invalid value
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 16/20] remote: Set eventID explicitly to an invalid value
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/remote/remote_daemon_dispatch.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

(always a bit "nervous" changing anything in these APIs...)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 16/20] remote: Set eventID explicitly to an invalid value
Posted by Marc Hartmayer, 14 weeks ago
On Thu, Mar 15, 2018 at 04:39 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> 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@linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>> ---
>>  src/remote/remote_daemon_dispatch.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
>
> (always a bit "nervous" changing anything in these APIs...)

Yep, I know that feeling :)

>
--
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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 17/20] remote: Add the information which program has to be used to daemonClientEventCallback
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 17/20] remote: Add the information which program has to be used to daemonClientEventCallback
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
Posted by Marc Hartmayer, 15 weeks ago
This allows us to get rid of another usage of the global variable
remoteProgram.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 

Reviewed-by: John Ferlan <jferlan@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;
>  }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
Posted by Marc Hartmayer, 14 weeks ago
On Thu, Mar 15, 2018 at 07:56 PM +0100, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>> ---
>>  src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>
> Reviewed-by: John Ferlan <jferlan@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
Posted by Marc Hartmayer, 14 weeks ago
On Thu, Mar 15, 2018 at 07:56 PM +0100, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>> ---
>>  src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>
> Reviewed-by: John Ferlan <jferlan@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/20] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
Posted by Nikolay Shirokovskiy, 14 weeks ago

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@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>> ---
>>  src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
> 
> Reviewed-by: John Ferlan <jferlan@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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 19/20] rpc: Introduce virNetServerGetProgramLocked helper function
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 19/20] rpc: Introduce virNetServerGetProgramLocked helper function
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@redhat.com>

John

(but I'll need an updated version before being able to push)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 20/20] remote/rpc: Use virNetServerGetProgram() to determine the program
Posted by Marc Hartmayer, 15 weeks ago
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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 20/20] remote/rpc: Use virNetServerGetProgram() to determine the program
Posted by Marc Hartmayer, 14 weeks ago
On Thu, Mar 08, 2018 at 01:20 PM +0100, Marc Hartmayer <mhartmay@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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 20/20] remote/rpc: Use virNetServerGetProgram() to determine the program
Posted by John Ferlan, 14 weeks ago

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list