[libvirt] PATCH: Pass 'remote_error' object to RPC handlers

[Daniel Veillard]

On Fri, Oct 17, 2008 at 12:27:52PM +0100, Daniel P. Berrange wrote:
> In the libvirtd daemon, remote.c file the current RPC handlers have 
> a return value contract saying
> 
>    0 - success
>   -1 - failure in libvirt call, no error returned
>   -2 - failure in dispatch process, error already serialized & sent
> 
> While this isn't a problem for the code as it stands today, for the thread
> support I want to be able to avoid the dispatch handlers having to touch
> the 'struct qemud_client' object in normal usage.  Allowing the RPC 
> handlers to directly serialize & send the error makes this impossible
> because it requires access to the client object.
> 
> So this patch changes the way the RPC handlers deal with errors. The
> RPC handler API changes from
> 
>   typedef int (*dispatch_fn) (struct qemud_server *server,
>                               struct qemud_client *client,
>                               dispatch_args *args,
>                               dispatch_ret *ret);
> 
> To
> 
>   typedef int (*dispatch_fn) (struct qemud_server *server,
>                               struct qemud_client *client,
>                               remote_error *err,
>                               dispatch_args *args,
>                               dispatch_ret *ret);
> 
> Note, the addition of a 'remote_error *err' argument. Whenever an error
> occurs during the dispatch process, the RPC handler must populate this
> 'remote_error *err' object with the error details. This rule applies
> whether its a libvirt error, or a dispatch process error, so there is
> no longer any need to have separate -1 or -2 return values, a simple
> -1 or 0 return value suffices.

  Sounds fine,

> @@ -1381,7 +1380,14 @@ static void qemudDispatchClientRead(stru
>          if (client->bufferOffset < client->bufferLength)
>              return; /* Not read enough */
>  
> -        remoteDispatchClientRequest (server, client);
> +        if ((len = remoteDispatchClientRequest (server, client)) == 0)
> +            qemudDispatchClientFailure(server, client);

  Shouldn't you return here instead of continuing ?

> +
> +        /* Set up the output buffer. */
> +        client->mode = QEMUD_MODE_TX_PACKET;
> +        client->bufferLength = len;
> +        client->bufferOffset = 0;
> +
>          if (qemudRegisterClientEvent(server, client, 1) < 0)
>              qemudDispatchClientFailure(server, client);
>  
[...]
> +static void
> +remoteDispatchFormatError (remote_error *rerr,
> +                           const char *fmt, ...)
> +{
> +    va_list args;
> +    char msgbuf[1024];
> +    char *msg = msgbuf;
> +
> +    va_start (args, fmt);
> +    vsnprintf (msgbuf, sizeof msgbuf, fmt, args);
> +    va_end (args);
> +
> +    remoteDispatchStringError (rerr, VIR_ERR_RPC, msg);
> +}

  Really no way we would get more than 1024 bytes ? The problem is that
if this happens it's usually a stack being printed and the useful info
can be at the end.

[...]
> -        remoteDispatchError (client, &req,
> -                             _("program mismatch (actual %x, expected %x)"),
> -                             req.prog, REMOTE_PROGRAM);
> -        xdr_destroy (&xdr);
> -        return;
> +        remoteDispatchFormatError (&rerr,
> +                                   _("program mismatch (actual %x, expected %x)"),
> +                                   req.prog, REMOTE_PROGRAM);

  stylistic issue, indenting to the opening ( is nice but can exceed the
  80 columns rule, I usually prefer dropping the ( alignment to fit in.


> @@ -1395,6 +1368,7 @@ remoteDispatchDomainMigratePrepare (stru
>                                     args->flags, dname, args->resource);
>      if (r == -1) {

  maybe if (r < 0)  { as a mor generic error catching instruction

>          VIR_FREE(uri_out);
> +        remoteDispatchConnError(rerr, client->conn);
>          return -1;
>      }
>  
> @@ -1439,7 +1413,10 @@ remoteDispatchDomainMigratePerform (stru
>                                     args->uri,
>                                     args->flags, dname, args->resource);
>      virDomainFree (dom);
> -    if (r == -1) return -1;
> +    if (r == -1) {

  Same thing, we didn't defined virDrvDomainMigratePerform error code
so maybe it's better to be generic

[...]
> @@ -482,8 +482,12 @@ void virDomainRemoveInactive(virDomainOb
>                  memmove(doms->objs + i, doms->objs + i + 1,
>                          sizeof(*(doms->objs)) * (doms->count - (i + 1)));
>  
> -            if (VIR_REALLOC_N(doms->objs, doms->count - 1) < 0) {
> -                ; /* Failure to reduce memory allocation isn't fatal */
> +            if (doms->count > 1) {
> +                if (VIR_REALLOC_N(doms->objs, doms->count - 1) < 0) {
> +                    ; /* Failure to reduce memory allocation isn't fatal */
> +                }
> +            } else {
> +                VIR_FREE(doms->objs);
>              }
>              doms->count--;

  Like this there is a couple case where new error handling blocks have
been added, but after having reviewed the full patch I really could not
find anything suspicious. Since arguments of functions have been changed
I don't think one could miss one function lacking the change.
  The only suggestion I would have is that there is a number of cases
where we test the error by checking against a -1 return value after
calling the API, maybe it's safer to just check for < 0 . But really as
if this looks okay (but that was long !)

  thanks, there is clearly many cases where propagating the real error
will really improve debugging problems !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/