Comment 27 for bug 2059272

Hi Mauricio,

Thanks for the detailed explanations, as usual.

I took some time here to read your patch. It took me down the rabbit hole and I did some archaeology to find out what these "inhibit*" pointers are about. It was fun; I learned that they were introduced back in 2012 as a means to allow drivers to signal libvirt when to inhibit shutdown because there are still VMs around. But I digress.

I liked your approach here and indeed, it seems simpler than the other one (although the initial approach was also simple to grasp and elegant; too bad it didn't work for all cases).

My only comment/question here would be this: the changes to qemuProcessReconnect mention that they're being done in order to prevent the XML "corruption" when there's a shutdown detected during initialization, but (and I may be wrong here) it seems to me that this new code path could be reached even after the initialization, couldn't it? Either way, this is not really important and doesn't affect the patch (aside from the possible amends to the comments being added), and it's OK if you don't know the answer, too.

My other source of "concern" here was the reference handling for the new pointer, but I couldn't find any potential problems with what you did. 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. The lock/unlock dance is also simple and trivially verifiable.

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

Thanks again.