Re: [libvirt] [PATCH v2] daemon: Unlink unix socket paths on shutdown

On 08/01/2011 09:34 AM, Osier Yang wrote: > This patch introduces a internal RPC API "virNetServerClose", which > is standalone with "virNetServerFree". it closes all the socket fds, > and unlinks the unix socket paths, regardless of whether the socket > is still referenced or not. > > This is to address regression bug: > https://bugzilla.redhat.com/show_bug.cgi?id=725702 > +++ b/src/rpc/virnetserver.c > @@ -794,3 +794,15 @@ void virNetServerFree(virNetServerPtr srv) > virMutexDestroy(&srv->lock); > VIR_FREE(srv); > } > + > +void virNetServerClose(virNetServerPtr srv) > +{ > + int i; > + > + if (!srv) > + return; > + > + for (i = 0; i< srv->nservices; i++) { > + virNetServerServiceClose(srv->services[i]); > + } Don't we need locking at every level of the chain? That is, srv->nservices involves reading srv, so srv must be locked, just as much as:

> + > +void virNetServerServiceClose(virNetServerServicePtr svc) > +{ > + int i; > + > + if (!svc) > + return; > + > + for (i = 0; i< svc->nsocks; i++) { > + virNetSocketClose(svc->socks[i]); svc->nsocks, as well as... > +void virNetSocketClose(virNetSocketPtr sock) > +{ > + if (!sock) > + return; > + > + virMutexLock(&sock->lock); sock, but you only locked sock. > + > + VIR_FORCE_CLOSE(sock->fd); > + > +#ifdef HAVE_SYS_UN_H > + /* If a server socket, then unlink UNIX path */ > + if (!sock->client&& > + sock->localAddr.data.sa.sa_family == AF_UNIX&& > + sock->localAddr.data.un.sun_path[0] != '\0') { > + if (unlink(sock->localAddr.data.un.sun_path) == 0) > + sock->localAddr.data.un.sun_path[0] = '\0'; > + } But I think this change is right.