[libvirt] [PATCH 3/5] rpc: invoke the message dispatch callback with client unlocked

[Daniel P. Berrangé]

On Wed, Mar 07, 2018 at 06:48:25PM -0500, John Ferlan wrote:
> 
> 
> On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> > Currently if the virNetServer instance is created with max_workers==0 to
> > request a non-threaded dispatch process, we deadlock during dispatch
> > 
> >   #0  0x00007fb845f6f42d in __lll_lock_wait () from /lib64/libpthread.so.0
> >   #1  0x00007fb845f681d3 in pthread_mutex_lock () from /lib64/libpthread.so.0
> >   #2  0x000055a6628bb305 in virMutexLock (m=<optimized out>) at util/virthread.c:89
> >   #3  0x000055a6628a984b in virObjectLock (anyobj=<optimized out>) at util/virobject.c:435
> >   #4  0x000055a66286fcde in virNetServerClientIsAuthenticated (client=client at entry=0x55a663a7b960)
> >       at rpc/virnetserverclient.c:1565
> >   #5  0x000055a66286cc17 in virNetServerProgramDispatchCall (msg=0x55a663a7bc50, client=0x55a663a7b960,
> >       server=0x55a663a77550, prog=0x55a663a78020) at rpc/virnetserverprogram.c:407
> >   #6  virNetServerProgramDispatch (prog=prog at entry=0x55a663a78020, server=server at entry=0x55a663a77550,
> >       client=client at entry=0x55a663a7b960, msg=msg at entry=0x55a663a7bc50) at rpc/virnetserverprogram.c:307
> >   #7  0x000055a662871d56 in virNetServerProcessMsg (msg=0x55a663a7bc50, prog=0x55a663a78020, client=0x55a663a7b960,
> >       srv=0x55a663a77550) at rpc/virnetserver.c:148
> >   #8  virNetServerDispatchNewMessage (client=0x55a663a7b960, msg=0x55a663a7bc50, opaque=0x55a663a77550)
> >       at rpc/virnetserver.c:227
> >   #9  0x000055a66286e4c0 in virNetServerClientDispatchRead (client=client at entry=0x55a663a7b960)
> >       at rpc/virnetserverclient.c:1322
> >   #10 0x000055a66286e813 in virNetServerClientDispatchEvent (sock=<optimized out>, events=1, opaque=0x55a663a7b960)
> >       at rpc/virnetserverclient.c:1507
> >   #11 0x000055a662899be0 in virEventPollDispatchHandles (fds=0x55a663a7bdc0, nfds=<optimized out>)
> >       at util/vireventpoll.c:508
> >   #12 virEventPollRunOnce () at util/vireventpoll.c:657
> >   #13 0x000055a6628982f1 in virEventRunDefaultImpl () at util/virevent.c:327
> >   #14 0x000055a6628716d5 in virNetDaemonRun (dmn=0x55a663a771b0) at rpc/virnetdaemon.c:858
> >   #15 0x000055a662864c1d in main (argc=<optimized out>, argv=0x7ffd105b4838) at logging/log_daemon.c:1235
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> >  src/rpc/virnetserverclient.c | 79 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 55 insertions(+), 24 deletions(-)
> > 
> 
> I had the same syntax-check notes that Jim had.
> 
> Beyond that since both callers Unlock prior to calling
> virNetServerClientDispatchMessage and the code relocks right away, would
> calling with client lock'd still be prudent/possible?  Callers would
> then be able to
> 
>     if (msg)
>         virNetServerClientDispatchMessage(...)
>     else
>         Unlock(client)
> 
> Once we get to virNetServerProgramDispatch it expects an unlocked
> client. Maybe we should comment some of the other callers in the path to
> indicate whether server/client need to be locked. Not required though -
> only helpful for future spelunkers of this code.

I generally like to avoid situations where one method locks the object and
then calls another method which unlocks it. IMHO, it leads to harder to
understand code where the lock/unlock calls are spread out.

Since we own the reference on client, I think unlocking & locking again
is harmless here.

> > @@ -1313,16 +1347,6 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
> >              }
> >          }
> >  
> > -        /* Send off to for normal dispatch to workers */
> > -        if (msg) {
> > -            if (!client->dispatchFunc) {
> > -                virNetMessageFree(msg);
> > -                client->wantClose = true;
> > -            } else {
> > -                client->dispatchFunc(client, msg, client->dispatchOpaque);
> > -            }
> > -        }
> > -
> 
> I thought about the order change here w/r/t the following code being run
> before the dispatch options above; however, that would perhaps more of a
> concern for the path where we have no workers. If we have a worker, then
> it gets queued and the following would then occur first anyway, right?
> So, IOW, I think the reordering here is fine. It would happen before the
> worker got around to handling the client and moving it around to
> virNetServerClientDispatchMessage could get ugly w/r/t locks.

Yeah, that was my rationale too - since worker threads are asynchronous
we're effectively running the second block first regardless, so the
ordering should be harmless.

> 
> Reviewed-by: John Ferlan <jferlan at redhat.com>
> 
> John
> 
> 
> >          /* Possibly need to create another receive buffer */
> >          if (client->nrequests < client->nrequests_max) {
> >              if (!(client->rx = virNetMessageNew(true))) {
> > @@ -1338,6 +1362,8 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
> >              }
> >          }
> >          virNetServerClientUpdateEvent(client);
> > +
> > +        return msg;
> >      }
> >  }
> >  
> 
> [...]

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|