[libvirt] [PATCH 07/12] Introduce generic RPC server objects

[Eric Blake]

On 03/18/2011 12:54 PM, Daniel P. Berrange wrote:
> To facilitate creation of new daemons providing XDR RPC services,
> pull alot of the libvirtd daemon code into a set of reusable

s/alot/a lot/

> objects.
> 
>  * virNetServer: A server contains one or more services which
>    accept incoming clients. It maintains the list of active
>    clients. It has a list of RPC programs which can be used
>    by clients. When clients produce a complete RPC message,
>    the server passes this onto the corresponding program for
>    handling, and queues any response back with the client.
> 
>  * virNetServerClient: Encapsulates a single client connection.
>    All I/O for the client is handled, reading & writing RPC
>    messages.
> 
>  * virNetServerProgram: Handles processing and dispatch of
>    RPC method calls for a single RPC (program,version).
>    Multiple programs can be registered with the server.
> 
>  * virNetServerService: Encapsulates socket(s) listening for
>    new connections. Each service listens on a single host/port,
>    but may have multiple sockets if on a dual IPv4/6 host.
> 
> Each new daemon now merely has to define the list of RPC procedures
> & their handlers. It does not need to deal with any network related
> functionality at all.

We're now into the realm of this series where I've never before provided
review comments, so it may take me a bit longer...

> +++ b/src/rpc/virnetserver.c
> +
> +static void virNetServerHandleJob(void *jobOpaque, void *opaque)
> +{

Several control flow bugs.  I'm prefixing them with numbers (all lines
prefixed with 1 are in the same flow):

> +    virNetServerPtr srv = opaque;
> +    virNetServerJobPtr job = jobOpaque;
> +    virNetServerProgramPtr prog = NULL;
> +    size_t i;
> +
> +    virNetServerClientRef(job->client);
> +
> +    virNetServerLock(srv);
> +    VIR_DEBUG("server=%p client=%p message=%p",
> +              srv, job->client, job->msg);
> +
> +    for (i = 0 ; i < srv->nprograms ; i++) {
> +        if (virNetServerProgramMatches(srv->programs[i], job->msg)) {
> +            prog = srv->programs[i];
> +            break;
> +        }
> +    }
> +
> +    if (!prog) {
> +        VIR_DEBUG("Cannot find program %d version %d",
> +                  job->msg->header.prog,
> +                  job->msg->header.vers);
> +        goto error;
> +    }
> +
> +    virNetServerProgramRef(prog);
> +    virNetServerUnlock(srv);

1. unlock...

> +
> +    if (virNetServerProgramDispatch(prog,
> +                                    srv,
> +                                    job->client,
> +                                    job->msg) < 0)
> +        goto error;

1. error...

> +
> +    virNetServerLock(srv);
> +    virNetServerProgramFree(prog);

2. prog freed while server lock held

> +    virNetServerUnlock(srv);
> +    virNetServerClientFree(job->client);
> +
> +    VIR_FREE(job);
> +    return;
> +
> +error:
> +    virNetServerUnlock(srv);

1. unlock.  Oops - double-unlock is a recipe for disaster.

> +    virNetServerProgramFree(prog);

2. prog freed while server lock released.  Which should it be?

> +    virNetMessageFree(job->msg);
> +    virNetServerClientClose(job->client);
> +    virNetServerClientFree(job->client);
> +    VIR_FREE(job);
> +}
> +
> +

> +
> +static void virNetServerFatalSignal(int sig, siginfo_t * siginfo ATTRIBUTE_UNUSED,
> +                                    void* context ATTRIBUTE_UNUSED)
> +{
> +    struct sigaction sig_action;
> +    int origerrno;
> +
> +    origerrno = errno;
> +    virLogEmergencyDumpAll(sig);
> +
> +    /*
> +     * If the signal is fatal, avoid looping over this handler
> +     * by desactivating it

s/desactivating/deactivating/

> +     */
> +#ifdef SIGUSR2
> +    if (sig != SIGUSR2) {
> +#endif
> +        sig_action.sa_handler = SIG_IGN;
> +        sigaction(sig, &sig_action, NULL);

Also need to sigemptyset(&sig_action.sa_mask), in order to keep valgrind
happy (see commit fd21ecfd).

> +virNetServerPtr virNetServerNew(size_t min_workers,
> +                                size_t max_workers,
> +                                size_t max_clients,
> +                                virNetServerClientInitHook clientInitHook)

Ran out of time to finish this review today.

-- 
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/20110321/fdc77fac/attachment-0001.sig>