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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list