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

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.