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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >  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.

I see I forgot to reply on few things here.

You're right, that's a leftover.

> > + */
> > +virNetServerPtr
> > +virNetDaemonGetServer(virNetDaemonPtr dmn,
> > +                      int subServerID)
> > +{
> > +    virNetServerPtr srv = NULL;
> > +
> > +    virObjectLock(dmn);
> > +    srv = virNetDaemonGetServerInternal(dmn, subServerID);
>
> Did you forget virObjectLock(srv) here?

Nope, the beauty of this is that just refs up the server and returns
it.  Similarly to virQEMUDriverGetCapabilities().

> > +    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.

What you describe is still possible, you can call
virNetServerUpdateServices() on one server, nobody is saying you
cannot.  This function just abstracts starting all servers together
into one function.

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list