Instead of holding onto the reference for each program added to
a NetServer, let's Unref the program reference as soon as the
attempt is made to add to the @srv->programs list. This then
allows the virNetServerDispose to handle the final Unref of
the object for actual deletion and cleans up the cleanup: path.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 daemon/libvirtd.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 5c47e49d48..87c5b22710 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1352,7 +1352,10 @@ int main(int argc, char **argv) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
-    if (virNetServerAddProgram(srv, remoteProgram) < 0) {
+
+    rc = virNetServerAddProgram(srv, remoteProgram);
+    virObjectUnref(remoteProgram);
+    if (rc < 0) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
@@ -1364,7 +1367,10 @@ int main(int argc, char **argv) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
-    if (virNetServerAddProgram(srv, lxcProgram) < 0) {
+
+    rc = virNetServerAddProgram(srv, lxcProgram);
+    virObjectUnref(lxcProgram);
+    if (rc < 0) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
@@ -1376,7 +1382,10 @@ int main(int argc, char **argv) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
-    if (virNetServerAddProgram(srv, qemuProgram) < 0) {
+
+    rc = virNetServerAddProgram(srv, qemuProgram);
+    virObjectUnref(qemuProgram);
+    if (rc < 0) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
@@ -1414,7 +1423,10 @@ int main(int argc, char **argv) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
-    if (virNetServerAddProgram(srvAdm, adminProgram) < 0) {
+
+    rc = virNetServerAddProgram(srvAdm, adminProgram);
+    virObjectUnref(adminProgram);
+    if (rc < 0) {
         ret = VIR_DAEMON_ERR_INIT;
         goto cleanup;
     }
@@ -1513,10 +1525,6 @@ int main(int argc, char **argv) {
 
  cleanup:
     virNetlinkEventServiceStopAll();
-    virObjectUnref(remoteProgram);
-    virObjectUnref(lxcProgram);
-    virObjectUnref(qemuProgram);
-    virObjectUnref(adminProgram);
     virNetDaemonClose(dmn);
     virNetlinkShutdown();
     if (statuswrite != -1) {
-- 
2.13.6

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

On Tue, Nov 07, 2017 at 09:39:55PM -0500, John Ferlan wrote:
> Instead of holding onto the reference for each program added to
> a NetServer, let's Unref the program reference as soon as the
> attempt is made to add to the @srv->programs list. This then
> allows the virNetServerDispose to handle the final Unref of
> the object for actual deletion and cleans up the cleanup: path.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  daemon/libvirtd.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 5c47e49d48..87c5b22710 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -1352,7 +1352,10 @@ int main(int argc, char **argv) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> -    if (virNetServerAddProgram(srv, remoteProgram) < 0) {
> +
> +    rc = virNetServerAddProgram(srv, remoteProgram);
> +    virObjectUnref(remoteProgram);

Again, why can't we drop the ref from the AddProgram, if we as the @srv mutex
owners got rescheduled and someone unreff'd @program before we inserted it to
the array (while holding mutex), we'd fail to ref a dangling pointer
consequently, so I don't see any added protection here and I think ^these hunks
are unnecessary, once we agree to drop virObjectRef from virNet<Foo>Add<Bar>
during the init phase.

> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1364,7 +1367,10 @@ int main(int argc, char **argv) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> -    if (virNetServerAddProgram(srv, lxcProgram) < 0) {
> +
> +    rc = virNetServerAddProgram(srv, lxcProgram);
> +    virObjectUnref(lxcProgram);
> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1376,7 +1382,10 @@ int main(int argc, char **argv) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> -    if (virNetServerAddProgram(srv, qemuProgram) < 0) {
> +
> +    rc = virNetServerAddProgram(srv, qemuProgram);
> +    virObjectUnref(qemuProgram);
> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1414,7 +1423,10 @@ int main(int argc, char **argv) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> -    if (virNetServerAddProgram(srvAdm, adminProgram) < 0) {
> +
> +    rc = virNetServerAddProgram(srvAdm, adminProgram);
> +    virObjectUnref(adminProgram);
> +    if (rc < 0) {
>          ret = VIR_DAEMON_ERR_INIT;
>          goto cleanup;
>      }
> @@ -1513,10 +1525,6 @@ int main(int argc, char **argv) {
>
>   cleanup:
>      virNetlinkEventServiceStopAll();
> -    virObjectUnref(remoteProgram);
> -    virObjectUnref(lxcProgram);
> -    virObjectUnref(qemuProgram);
> -    virObjectUnref(adminProgram);

This hunk would stay either way.

Erik

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