[libvirt] [PATCH 2/4] Convert libvirtd over to the new RPC handling APIs
Eric Blake
eblake at redhat.com
Mon Jun 27 22:01:53 UTC 2011
On 06/27/2011 08:24 AM, Daniel P. Berrange wrote:
> This guts the libvirtd daemon, removing all its networking and
> RPC handling code. Instead it calls out to the new virServerPtr
> APIs for all its RPC & networking work
>
> As a fallout all libvirtd daemon error reporting now takes place
> via the normal internal error reporting APIs. There is no need
> to call separate error reporting APIs in RPC code, nor should
> code use VIR_WARN/VIR_ERROR for reporting fatal problems anymore.
>
> * daemon/qemu_dispatch_*.h, daemon/remote_dispatch_*.h: Remove
> old generated dispatcher code
> * daemon/qemu_dispatch.h, daemon/remote_dispatch.h: New dispatch
> code
> * daemon/dispatch.c, daemon/dispatch.h: Remove obsoleted code
> * daemon/remote.c, daemon/remote.h: Rewrite for new dispatch
> APIs
> * daemon/libvirtd.c, daemon/libvirtd.h: Remove all networking
> code
> * daemon/stream.c, daemon/stream.h: Update for new APIs
> * daemon/Makefile.am: Link to libvirt-net-rpc-server.la
> +++ b/daemon/Makefile.am
> @@ -3,33 +3,21 @@
> CLEANFILES =
>
> DAEMON_GENERATED = \
> - $(srcdir)/remote_dispatch_prototypes.h \
> - $(srcdir)/remote_dispatch_table.h \
> - $(srcdir)/remote_dispatch_args.h \
> - $(srcdir)/remote_dispatch_ret.h \
> - $(srcdir)/remote_dispatch_bodies.h \
> - $(srcdir)/qemu_dispatch_prototypes.h \
> - $(srcdir)/qemu_dispatch_table.h \
> - $(srcdir)/qemu_dispatch_args.h \
> - $(srcdir)/qemu_dispatch_ret.h \
> - $(srcdir)/qemu_dispatch_bodies.h
> + $(srcdir)/remote_dispatch.h \
> + $(srcdir)/qemu_dispatch.h
Should prove to be interesting, with fewer files.
> @@ -211,11 +162,7 @@ policyfile = libvirtd.policy-1
> endif
> endif
>
> -if HAVE_AVAHI
> -libvirtd_SOURCES += $(AVAHI_SOURCES)
> -libvirtd_CFLAGS += $(AVAHI_CFLAGS)
> -libvirtd_LDADD += $(AVAHI_LIBS)
> -endif
> +EXTRA_DIST += probes.d libvirtd.stp
Spurious addition; this line is already present elsewhere in Makefile.am.
> +++ b/daemon/libvirtd.c
> @@ -23,31 +23,13 @@
>
> #include <grp.h>
> -#include <signal.h>
> -#include <netdb.h>
> -#include <locale.h>
A bit too prune-happy; compilation failed. And even then, I'm getting a
test failure:
31) corrupted config audit_logging ... OK
./daemon-conf: line 98: kill: (22788) - No such process
32) valid config file (sleeping 2 seconds) ... FAILED
FAIL: daemon-conf
> #include "memory.h"
> -#include "stream.h"
> +#include "conf.h"
> +#include "virnetserver.h"
> +#include "threads.h"
> +#include "remote.h"
> +#include "remote_driver.h"
> +//#include "stream.h"
Delete this line, rather than commenting it.
> +
> + if (data->auth_unix_rw == REMOTE_AUTH_POLKIT)
> + data->unix_sock_rw_perms = strdup("0777"); /* Allow world */
> + else
> + data->unix_sock_rw_perms = strdup("0700"); /* Allow user only */
> + data->unix_sock_ro_perms = strdup("0777"); /* Always allow world */
Why are we passing this as formatted strings, instead of as raw octal
mode_t numbers?
> @@ -3218,69 +1325,91 @@ int main(int argc, char **argv) {
> break;
>
> case 'p':
> - pid_file = optarg;
> + VIR_FREE(pid_file);
> + if (!(pid_file = strdup(optarg)))
> + exit(EXIT_FAILURE);
> break;
>
> case 'f':
> - remote_config_file = optarg;
> + if (!(remote_config_file = strdup(optarg)))
> + exit(EXIT_FAILURE);
Mem leak if -f is used more than once (you need the same VIR_FREE to
nuke prior use of -f as you had for prior use of -p).
>
> +/*
> + * You must hold lock for at least the client
> + * We don't free stuff here, merely disconnect the client's
> + * network socket & resources.
> + * We keep the libvirt connection open until any async
> + * jobs have finished, then clean it up elsehwere
> + */
> +static void remoteClientFreeFunc(void *data)
s/elsehwere/elsewhere/
> static int
> -remoteDispatchDomainGetVcpupinInfo(struct qemud_server *server ATTRIBUTE_UNUSED,
> - struct qemud_client *client ATTRIBUTE_UNUSED,
> - virConnectPtr conn,
> - remote_message_header *hdr ATTRIBUTE_UNUSED,
> - remote_error *rerr,
> +remoteDispatchDomainGetVcpupinInfo(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client ATTRIBUTE_UNUSED,
> + virNetMessageHeaderPtr hdr ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr,
> remote_domain_get_vcpupin_info_args *args,
> remote_domain_get_vcpupin_info_ret *ret)
Simple merge conflict due to the Vcpupin->VcpuPin rename.
> static int
> -remoteDispatchAuthList(struct qemud_server *server,
> - struct qemud_client *client,
> - virConnectPtr conn ATTRIBUTE_UNUSED,
> - remote_message_header *hdr ATTRIBUTE_UNUSED,
> - remote_error *rerr,
> - void *args ATTRIBUTE_UNUSED,
> +remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client,
> + virNetMessageHeaderPtr hdr ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr,
> remote_auth_list_ret *ret)
> {
> int rv = -1;
> + int auth = virNetServerClientGetAuth(client);
> + uid_t callerUid;
> + pid_t callerPid;
> +
> + /* If the client is root then we want to bypass the
> + * policykit auth to avoid root being denied if
> + * some piece of polkit isn't present/running
> + */
> + if (auth == VIR_NET_SERVER_SERVICE_AUTH_POLKIT) {
> + if (virNetServerClientGetLocalIdentity(client, &callerUid, &callerPid) < 0) {
> + /* Don't do anything on error - it'll be validated at next
> + * phase of auth anyway */
This is new code; should it be split into a separate patch?
> - ssf *= 8; /* tls key size is bytes, sasl wants bits */
> -
> - err = sasl_setprop(client->saslconn, SASL_SSF_EXTERNAL, &ssf);
> - if (err != SASL_OK) {
> - VIR_ERROR(_("cannot set SASL external SSF %d (%s)"),
> - err, sasl_errstring(err, NULL, NULL));
> - sasl_dispose(&client->saslconn);
> - client->saslconn = NULL;
> +
> + ssf *= 8; /* key size is bytes, sasl wants bits */
Pre-existing, but I like CHAR_BIT (from <limits.h>) better than the
magic number 8.
> + virNetSASLSessionSecProps(sasl,
> + 56, /* Good enough to require kerberos */
> + 100000, /* Arbitrary big number */
> + false); /* No anonymous */
Same magic numbers as in patch 1/4.
>
> - VIR_DEBUG("Queue event %d %d", procnr, msg->bufferLength);
> - qemudClientMessageQueuePush(&client->tx, msg);
> - qemudUpdateClientEvent(client);
> + VIR_DEBUG("Queue event %d %Zu", procnr, msg->bufferLength);
%Zu is not portable. Use %zu instead.
>
> /*
> * @conn: a connection object to associate the stream with
> - * @hdr: the method call to associate with the stram
> + * @header: the method call to associate with the stram
s/stram/stream/
> + # Finally we write out the huge dispatch table which lists
> + # the dispatch helper method. the XDR proc for processing
> + # args and return values, and the size of the args and
> + # return value structs. All methods are marked as requiring
> + # authentication. Methods are selectively relaxed in the
> + # daemon code which registers the program.
Should that instead be an annotation in the .x file? But we can change
that later.
Conditional ACK - fix the testsuite failure, address the above problems,
and squash this in:
diff --git i/.gitignore w/.gitignore
index 4fbecfa..be4193d 100644
--- i/.gitignore
+++ w/.gitignore
@@ -34,7 +34,7 @@
/config.sub
/configure
/configure.lineno
-/daemon/*_dispatch_*.h
+/daemon/*_dispatch.h
/docs/hvsupport.html.in
/gnulib/
/libtool
diff --git i/daemon/libvirtd.c w/daemon/libvirtd.c
index 214199b..83ea094 100644
--- i/daemon/libvirtd.c
+++ w/daemon/libvirtd.c
@@ -30,6 +30,7 @@
#include <getopt.h>
#include <stdlib.h>
#include <grp.h>
+#include <locale.h>
#include "libvirt_internal.h"
#include "virterror_internal.h"
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110627/b1217bd8/attachment-0001.sig>
More information about the libvir-list
mailing list