Re: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program

On Wed, Nov 13, 2019 at 07:12:34PM +0100, Marc Hartmayer wrote: > On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina <phrdina(a)redhat.com&gt; wrote: > > On Fri, Nov 01, 2019 at 06:35:48PM +0100, Marc Hartmayer wrote: > >> Use virNetServerGetProgram() to determine the virNetServerProgram > >> instead of using hard coded global variables. This allows us to remove > >> the global variables @remoteProgram and @qemuProgram as they're now no > >> longer necessary. > >> > >> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; > > […snip…] > > >> virNetMessageErrorPtr rerr) > >> { > >> int rv = -1; > >> @@ -4180,6 +4180,12 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED, > >> struct daemonClientPrivate *priv = > >> virNetServerClientGetPrivateData(client); > >> virConnectPtr conn = remoteGetHypervisorConn(client); > >> + virNetServerProgramPtr program; > >> + > >> + if (!(program = virNetServerGetProgram(server, msg))) { > >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program found")); > >> + goto cleanup; > >> + } > > > > This doesn't look right. If the function fails we will jump to cleanup > > where we will try to unlock &priv->lock. This has to happen after we > > acquire that lock. > > > > Pavel > > Yep, will fix that as well. Shall I directly return in the error case or > jump to another label (e.g. 'cleanup_unlock')? Returning directly would not work properly as well, we call virNetMessageSaveError() in case of error in the cleanup section. Another label would work. > Or do see any reason why we should hold the priv->lock during the > virNetServerGetProgram call? We don't have to hold the lock for virNetServerGetProgram(), it just makes the function easier to follow as there will be only one cleanup and I don't see any harm of holding the lock for that function call as well if the function will later wait for the same lock.

Pavel