[libvirt] [PATCH 1/9] libvirtd: Alter refcnt processing for domain server objects

John Ferlan posted 9 patches 7 years, 3 months ago
There is a newer version of this series
[libvirt] [PATCH 1/9] libvirtd: Alter refcnt processing for domain server objects
Posted by John Ferlan 7 years, 3 months ago
Once virNetDaemonAddServer returns, the @srv or @srvAdm have
either been added to the @dmn list increasing the refcnt or
there was an error. In either case, we can lower the refcnt
from virNetServerNew, but not set to NULL. Thus "using" the
hash table reference when adding programs later or allowing
the immediate free of the object on error.

The @dmn dispose function (virNetDaemonDispose) will handle
the Unref of each object during the virHashFree when the
object is removed from the hash table.

Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 daemon/libvirtd.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 6d3b83355..0afd1dd82 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
     char *remote_config_file = NULL;
     int statuswrite = -1;
     int ret = 1;
+    int rc;
     int pid_file_fd = -1;
     char *pid_file = NULL;
     char *sock_file = NULL;
@@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
         goto cleanup;
     }
 
-    if (virNetDaemonAddServer(dmn, srv) < 0) {
+    /* Add @srv to @dmn servers hash table and Unref to allow removal from
+     * hash table at @dmn disposal to decide when to free @srv */
+    rc = virNetDaemonAddServer(dmn, srv);
+    virObjectUnref(srv);
+    if (rc < 0) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
@@ -1393,7 +1398,11 @@ int main(int argc, char **argv) {
         goto cleanup;
     }
 
-    if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
+    /* Add @srvAdm to @dmn servers hash table and Unref to allow removal from
+     * hash table at @dmn disposal to decide when to free @srvAdm */
+    rc = virNetDaemonAddServer(dmn, srvAdm);
+    virObjectUnref(srvAdm);
+    if (rc < 0) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
@@ -1516,11 +1525,9 @@ int main(int argc, char **argv) {
     }
 
     virObjectUnref(adminProgram);
-    virObjectUnref(srvAdm);
     virObjectUnref(qemuProgram);
     virObjectUnref(lxcProgram);
     virObjectUnref(remoteProgram);
-    virObjectUnref(srv);
     virObjectUnref(dmn);
 
     virNetlinkShutdown();
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/9] libvirtd: Alter refcnt processing for domain server objects
Posted by Marc Hartmayer 7 years, 3 months ago
On Fri, Jan 19, 2018 at 06:23 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> Once virNetDaemonAddServer returns, the @srv or @srvAdm have
> either been added to the @dmn list increasing the refcnt or
> there was an error. In either case, we can lower the refcnt
> from virNetServerNew, but not set to NULL. Thus "using" the
> hash table reference when adding programs later or allowing
> the immediate free of the object on error.
>
> The @dmn dispose function (virNetDaemonDispose) will handle
> the Unref of each object during the virHashFree when the
> object is removed from the hash table.
>
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  daemon/libvirtd.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 6d3b83355..0afd1dd82 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
>      char *remote_config_file = NULL;
>      int statuswrite = -1;
>      int ret = 1;
> +    int rc;
>      int pid_file_fd = -1;
>      char *pid_file = NULL;
>      char *sock_file = NULL;
> @@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
>          goto cleanup;
>      }
>
> -    if (virNetDaemonAddServer(dmn, srv) < 0) {
> +    /* Add @srv to @dmn servers hash table and Unref to allow removal from
> +     * hash table at @dmn disposal to decide when to free @srv */
> +    rc = virNetDaemonAddServer(dmn, srv);

Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>

Wouldn’t it be cleaner to initialize the server and then add it to the
daemon? (disadvantage: it makes the correct unreferencing a little more
complicated).

<pseudo_code>
create_and_initialize_daemon
create_and_add_programs_server
virNetDaemonAddServer()
…
</pseudo_code>

> +    virObjectUnref(srv);
> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1393,7 +1398,11 @@ int main(int argc, char **argv) {
>          goto cleanup;
>      }
>
> -    if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
> +    /* Add @srvAdm to @dmn servers hash table and Unref to allow removal from
> +     * hash table at @dmn disposal to decide when to free @srvAdm */
> +    rc = virNetDaemonAddServer(dmn, srvAdm);
> +    virObjectUnref(srvAdm);
> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1516,11 +1525,9 @@ int main(int argc, char **argv) {
>      }
>
>      virObjectUnref(adminProgram);
> -    virObjectUnref(srvAdm);
>      virObjectUnref(qemuProgram);
>      virObjectUnref(lxcProgram);
>      virObjectUnref(remoteProgram);
> -    virObjectUnref(srv);
>      virObjectUnref(dmn);
>
>      virNetlinkShutdown();
> --
> 2.13.6
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
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