On Wed, May 20, 2015 at 11:38:16AM -0700, Martin Kletzander wrote:
> On Wed, May 20, 2015 at 05:16:04PM +0100, Daniel P. Berrange wrote:
> >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.
> >>
> >>> + */
> >>> +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.
> >
>
> The case you described now will fail, yes. I didn't want to pollute
> to code with that at first, but then forgot about it (you can se part
> of it is implemented. But as the first version had it, I can add it
> here as well, that's no problem. And looking at the code after the
> rewrite it won't be that big of a mess as it was before. The combined
> diff for both the virnetdaemon and the locking daemon (and one bug
> found plus the static keyword) looks like this and it works now:
>
> diff --git i/src/locking/lock_daemon.c w/src/locking/lock_daemon.c
> index f9afc8c..ecbe03a 100644
> --- i/src/locking/lock_daemon.c
> +++ w/src/locking/lock_daemon.c
> @@ -237,10 +237,18 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged)
> }
> }
>
> - if (!(child = virJSONValueObjectGet(object, "daemon"))) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Missing daemon data from JSON file"));
> - goto error;
> + if (virJSONValueObjectHasKey(object, "daemon")) {
> + if (!(child = virJSONValueObjectGet(object, "daemon"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Malformed daemon data from JSON file"));
> + goto error;
> + }
> + } else {
> + if (!(child = virJSONValueObjectGet(object, "server"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Missing server data from JSON file"));
> + goto error;
> + }
> }
>
> if (!(lockd->dmn = virNetDaemonNewPostExecRestart(child)))
> diff --git i/src/rpc/virnetdaemon.c w/src/rpc/virnetdaemon.c
> index 8d42638..ff06146 100644
> --- i/src/rpc/virnetdaemon.c
> +++ w/src/rpc/virnetdaemon.c
> @@ -84,7 +84,7 @@ struct _virNetDaemon {
>
> static virClassPtr virNetDaemonClass;
>
> -void
> +static void
> virNetDaemonDispose(void *obj)
> {
> virNetDaemonPtr dmn = obj;
> @@ -240,7 +240,7 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
> clientPrivFree,
> clientPrivOpaque);
>
> - if (!srv || VIR_APPEND_ELEMENT(dmn->servers, dmn->nservers, srv) < 0)
> + if (!srv || VIR_APPEND_ELEMENT_COPY(dmn->servers, dmn->nservers, srv) < 0)
> goto error;
>
> virJSONValueFree(object);
> @@ -260,17 +260,18 @@ virNetDaemonNewPostExecRestart(virJSONValuePtr object)
> {
> virNetDaemonPtr dmn = NULL;
> virJSONValuePtr servers = virJSONValueObjectGet(object, "servers");
> + bool new_version = virJSONValueObjectHasKey(object, "servers");
>
> if (!(dmn = virNetDaemonNew()))
> goto error;
>
> - if (!servers) {
> + if (new_version && !servers) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Malformed servers data in JSON document"));
> goto error;
> }
>
> - if (!(dmn->srvObject = virJSONValueCopy(servers)))
> + if (!(dmn->srvObject = virJSONValueCopy(new_version ? servers : object)))
> goto error;
>
> return dmn;
Ok, I've just sent a patch which adds tests for virNetServer load/save
JSON so we can verify your change now. You'll need to update the test
to deal with the changed API obviously and add new JOSN data files
for the new format.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list