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

[Marc Hartmayer]

On Thu, Nov 14, 2019 at 09:20 AM +0100, Pavel Hrdina <phrdina at redhat.com> wrote:
> 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 at redhat.com> 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 at linux.ibm.com>
>> 
>> […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.

This would enforce the lock order 'server -> priv->lock' (not sure if
this is already the case) + it will become harder to identify what we’re
trying to protect with the lock. So if you have no strong opinion about
that I will introduce a 'cleanup_unlock' label. Fine with you?

Thanks.

>
> Pavel
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294