[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/4] Convert libvirtd over to the new RPC handling APIs



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]