Comment 28 for bug 2059272

Revision history for this message
Mauricio Faria de Oliveira (mfo) wrote :

Hi Sergio,

Thanks for taking the time to read and review the patch and test cases!

> this new code path could be reached even after the initialization, couldn't it?

No, this code path should only be reached during initialization, AFAICT.

That is because qemuProcessReconnect() can only be reached from function
calls in the initialization path when the daemon initializes the drivers.

For reference, this is the only call path reaching qemuProcessReconnect().
(I've started with it, and gone back into callers.)

@ src/remote/remote_daemon.c
- main()
- daemonStateInit()
- daemonRunStateInit()
@ src/libvirt.c
- virStateInitialize()
- virStateDriverTab[i]->stateInitialize()
@ src/qemu/qemu_driver.c
- virStateDriver qemuStateDriver.stateInitialize = qemuStateInitialize
- qemuStateInitialize()
@ src/qemu/qemu_process.c
- qemuProcessReconnectAll()
- qemuProcessReconnectHelper()
- qemuProcessReconnect()

> Worst case scenario (i.e., if we fail to consider a code path), we will
> have a "memory leak" during shutdown, which is not the end of the world.

Right, that's comforting. :)

And just to clarify on the code path consideration (so as to provide more
reassurance for the patch, regarding a code path possibly not considered):

There should be only one code path leading to qemuProcessReconnect() (above),
and fortunately the points to inc/dec references are straightforward there:

The thread is created (1) either successfully or fails (2), and has a single
return point (3).

So, if the reference count is incremented right before thread creation (1),
there is only 2 code sites to decrement it: on thread creation failure (2)
(since the function doesn't run, its return point "dec" doesn't run either),
and thread creation success: the function runs, so "dec" in return point (3).

> Otherwise, the patch LGTM and I'm satisfied with the testing you did.
> Feel free to go ahead and upload it.

Ok, cool. I think the clarifications above should address the two points
you brought up, so I'll continue and rebase and upload it.

Thanks again!