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

[Daniel P. Berrange]

On Mon, Jun 27, 2011 at 04:01:53PM -0600, Eric Blake wrote:
> 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.

Basically all the code from those 5 previous files can
now be consolidated into the one file, because the code
restructuring means we generate the callback tables in
a simpler way.

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

Rebase mistake :-)

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

I'll loook into that, probably another rebase mistake.

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

That's because when we later override these variables with data from
the config file it will also be in string format. We can't use int
format in the config file, because that's always decimal and we want
to have octal.

> >  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?

Actually it isn't new code. In the old code, this particular check
was hardcoded elsewhere in the daemon codebase, at the point where
we accept() the client socket. With the new code structure, we can't
do that, so I moved the check to here instead.

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

Yeah, we could do that with an annotation these days.

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 :|