On Thu, Nov 14, 2019 at 09:20 AM +0100, Pavel Hrdina <phrd...@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 <phrd...@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 <mhart...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
