On 01/24/2018 05:12 AM, Marc Hartmayer wrote:
> On Fri, Jan 19, 2018 at 06:23 PM +0100, John Ferlan <jfer...@redhat.com>
> wrote:
>> Once virNetServerProgramNew returns, the program objects have
>> either been added to the @srv or @srvAdm list increasing the
>> refcnt or there was an error. In either case, we can lower the
>> refcnt from virNetServerProgramNew and set the object to NULL
>> since it won't be used any more.
>>
>> The @srv or @srvAdm dispose function (virNetServerDispose) will
>> handle the Unref of each object during the virHashFree when the
>> object is removed from the hash table.
>>
>> Reviewed-by: Erik Skultety <eskul...@redhat.com>
>> Signed-off-by: John Ferlan <jfer...@redhat.com>
>> ---
>> daemon/libvirtd.c | 28 ++++++++++++++++++++--------
>> 1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
>> index 0afd1dd82..b47f875d9 100644
>> --- a/daemon/libvirtd.c
>> +++ b/daemon/libvirtd.c
>> @@ -1352,7 +1352,11 @@ int main(int argc, char **argv) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> - if (virNetServerAddProgram(srv, remoteProgram) < 0) {
>> +
>> + rc = virNetServerAddProgram(srv, remoteProgram);
>> + virObjectUnref(remoteProgram);
>> + remoteProgram = NULL;
>
>
> I’ve just looked at the code and all those variables are a global
> variables…
>
> libvirtd.c:
> virNetServerProgramPtr remoteProgram = NULL;
> virNetServerProgramPtr adminProgram = NULL;
> virNetServerProgramPtr qemuProgram = NULL;
> virNetServerProgramPtr lxcProgram = NULL;
>
> So this is not a good idea to set them to NULL… As this results in a
> segmentation fault
Oh right... There really was no reason to set them to NULL since the
only time they're used is in the context they are used. If the
AddProgram was successful, then there would be 2 refs anyway. They're
not used later in the code, so I'll remove the = NULL after the Unref.
John
>
> [Current thread is 1 (Thread 0x3ff908d7910 (LWP 195236))]
> (gdb) bt
> #0 virNetServerProgramGetID (prog=prog@entry=0x0) at
> ../../src/rpc/virnetserverprogram.c:93
> #1 0x000000011b726ac2 in remoteDispatchObjectEventSend (client=<optimized
> out>, program=0x0, procnr=procnr@entry=319, proc=0x11b767120
> <xdr_remote_domain_event_callback_reboot_msg>, data=data@entry=0x3fffee7d948)
> at ../../daemon/remote.c:3997
> #2 0x000000011b7577d2 in remoteRelayDomainEventReboot (conn=<optimized out>,
> dom=0x143663320, opaque=0x3ff700330c0) at ../../daemon/remote.c:365
> #3 0x000003ff904173a8 in virDomainEventDispatchDefaultFunc
> (conn=0x3fef42ca860, event=0x1436a37b0, cb=0x11b7576d0
> <remoteRelayDomainEventReboot>, cbopaque=0x3ff700330c0) at
> ../../src/conf/domain_event.c:1787
> #4 0x000003ff90414e4e in virObjectEventStateDispatchCallbacks
> (state=state@entry=0x3ff2c0facf0, event=0x1436a37b0,
> callbacks=callbacks@entry=0x3ff2c106370) at ../../src/conf/object_event.c:715
> #5 0x000003ff90414eb4 in virObjectEventStateQueueDispatch
> (state=state@entry=0x3ff2c0facf0, queue=queue@entry=0x3fffee7dc10,
> callbacks=0x3ff2c106370) at ../../src/conf/object_event.c:729
> #6 0x000003ff904156d4 in virObjectEventStateFlush (state=0x3ff2c0facf0) at
> ../../src/conf/object_event.c:830
> #7 0x000003ff9041574e in virObjectEventTimer (timer=<optimized out>,
> opaque=<optimized out>) at ../../src/conf/object_event.c:560
> #8 0x000003ff90343cc4 in virEventPollDispatchTimeouts () at
> ../../src/util/vireventpoll.c:457
> #9 0x000003ff9034577c in virEventPollRunOnce () at
> ../../src/util/vireventpoll.c:653
> #10 0x000003ff90343852 in virEventRunDefaultImpl () at
> ../../src/util/virevent.c:327
> #11 0x000003ff904e5e78 in virNetDaemonRun (dmn=0x1436303b0) at
> ../../src/rpc/virnetdaemon.c:880
> #12 0x000000011b7256ca in main (argc=<optimized out>, argv=<optimized out>)
> at ../../daemon/libvirtd.c:1523
>
> Therefore, I’ll withdraw my Reviewed-by: Marc Hartmayer
> <mhart...@linux.vnet.ibm.com>.
>
>
>> + if (rc < 0) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> @@ -1364,7 +1368,11 @@ int main(int argc, char **argv) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> - if (virNetServerAddProgram(srv, lxcProgram) < 0) {
>> +
>> + rc = virNetServerAddProgram(srv, lxcProgram);
>> + virObjectUnref(lxcProgram);
>> + lxcProgram = NULL;
>> + if (rc < 0) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> @@ -1376,7 +1384,11 @@ int main(int argc, char **argv) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> - if (virNetServerAddProgram(srv, qemuProgram) < 0) {
>> +
>> + rc = virNetServerAddProgram(srv, qemuProgram);
>> + virObjectUnref(qemuProgram);
>> + qemuProgram = NULL;
>> + if (rc < 0) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> @@ -1414,7 +1426,11 @@ int main(int argc, char **argv) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> - if (virNetServerAddProgram(srvAdm, adminProgram) < 0) {
>> +
>> + rc = virNetServerAddProgram(srvAdm, adminProgram);
>> + virObjectUnref(adminProgram);
>> + adminProgram = NULL;
>> + if (rc < 0) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> @@ -1524,10 +1540,6 @@ int main(int argc, char **argv) {
>> virStateCleanup();
>> }
>>
>> - virObjectUnref(adminProgram);
>> - virObjectUnref(qemuProgram);
>> - virObjectUnref(lxcProgram);
>> - virObjectUnref(remoteProgram);
>> 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
