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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/