Re: [libvirt] [PATCH 02/13] Move daemon-related parts of virNetServer to virNetDaemon

On Wed, May 20, 2015 at 06:11:14PM +0200, Michal Privoznik wrote: > On 20.05.2015 07:19, Martin Kletzander wrote: > > This allows to have more servers in one daemon which helps isolating > > some resources. > > > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > > --- > > daemon/libvirtd.c | 101 ++--- > > docs/internals.html.in | 4 +- > > docs/internals/rpc.html.in | 7 + > > po/POTFILES.in | 1 + > > src/Makefile.am | 1 + > > src/libvirt_remote.syms | 28 +- > > src/locking/lock_daemon.c | 63 ++-- > > src/locking/lock_daemon_config.c | 2 +- > > src/locking/lock_daemon_dispatch.c | 4 +- > > src/lxc/lxc_controller.c | 65 ++-- > > src/rpc/virnetdaemon.c | 746 +++++++++++++++++++++++++++++++++++++ > > src/rpc/virnetdaemon.h | 82 ++++ > > src/rpc/virnetserver.c | 526 ++++---------------------- > > src/rpc/virnetserver.h | 46 +-- > > src/rpc/virnetserverprogram.h | 3 + > > 15 files changed, 1074 insertions(+), 605 deletions(-) > > create mode 100644 src/rpc/virnetdaemon.c > > create mode 100644 src/rpc/virnetdaemon.h > > > > > diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c > > new file mode 100644 > > index 0000000..8d42638 > > --- /dev/null > > +++ b/src/rpc/virnetdaemon.c > > > + > > + > > +static virClassPtr virNetDaemonClass; > > + > > +void > > static > > > +virNetDaemonDispose(void *obj) > > +{ > > + virNetDaemonPtr dmn = obj; > > + size_t i; > > + > > + VIR_FORCE_CLOSE(dmn->autoShutdownInhibitFd); > > + > > + for (i = 0; i < dmn->nsignals; i++) { > > + sigaction(dmn->signals[i]->signum, &dmn->signals[i]->oldaction, NULL); > > + VIR_FREE(dmn->signals[i]); > > + } > > + VIR_FREE(dmn->signals); > > + VIR_FORCE_CLOSE(dmn->sigread); > > + VIR_FORCE_CLOSE(dmn->sigwrite); > > + if (dmn->sigwatch > 0) > > + virEventRemoveHandle(dmn->sigwatch); > > + > > + for (i = 0; i < dmn->nservers; i++) > > + virObjectUnref(dmn->servers[i]); > > + VIR_FREE(dmn->servers); > > + > > + virJSONValueFree(dmn->srvObject); > > +} > > + > > +static int > > +virNetDaemonOnceInit(void) > > +{ > > + if (!(virNetDaemonClass = virClassNew(virClassForObjectLockable(), > > + "virNetDaemon", > > + sizeof(virNetDaemon), > > + virNetDaemonDispose))) > > + return -1; > > + > > + return 0; > > +} > > > > +/* > > + * Separate function merely for the purpose of unified error > > + * reporting. > > + */ > > +static virNetServerPtr > > +virNetDaemonGetServerInternal(virNetDaemonPtr dmn, > > + int subServerID) > > +{ > > + if (subServerID < 0 || subServerID >= dmn->nservers) { > > + virReportError(VIR_ERR_INVALID_ARG, > > + _("Invalid server ID: %d"), > > + subServerID); > > + return NULL; > > + } > > + > > + return virObjectRef(dmn->servers[subServerID]); > > +} > > + > > +/* > > + * The server is locked after this function. > > No, it's not. > > > + */ > > +virNetServerPtr > > +virNetDaemonGetServer(virNetDaemonPtr dmn, > > + int subServerID) > > +{ > > + virNetServerPtr srv = NULL; > > + > > + virObjectLock(dmn); > > + srv = virNetDaemonGetServerInternal(dmn, subServerID); > > Did you forget virObjectLock(srv) here? > > > + virObjectUnlock(dmn); > > + > > + return srv; > > +} > > + > > > +void > > +virNetDaemonUpdateServices(virNetDaemonPtr dmn, > > + bool enabled) > > +{ > > + size_t i; > > + > > + virObjectLock(dmn); > > + for (i = 0; i < dmn->nservers; i++) > > + virNetServerUpdateServices(dmn->servers[i], enabled); > > + virObjectUnlock(dmn); > > Hm... I guess we need something slightly different. While we may want > one service to stop accepting new clients, we may want the other one to > still accept new ones. But since this is a big split, and so far a > daemon will have only one server, it can be saved for a follow up patch. > > > +} > > + > > Besides Dan's finding, looking okay. ACK. Just to be clear, not having back-compat for the exec-restart is a NACK. So we need to confirm upgrades definitely work before we can merge this refactoring.