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

[Daniel P. Berrange]

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 at redhat.com>
> > ---
> >  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.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|