3:49 p.m.
On 06/27/2011 02:08 PM, Eric Blake wrote:
Aargh, I hit send too soon.
> + case trans_ext: {
> + char const *cmd_argv[] = { command, NULL };
> + if (!(priv->client = virNetClientNewExternal(cmd_argv)))
This appears to be the only call to virNetClientNewExternal. And it
only passes a single argument, command. Should we simplify the
signature to be const char *command instead of const char **cmdargv?
But that can be a separate patch (or you can prove me wrong, if later
patches also start using this with more than just the command name).
> @@ -874,26 +646,14 @@ doRemoteOpen (virConnectPtr conn,
> /* Now try and find out what URI the daemon used */
> if (conn->uri == NULL) {
> remote_get_uri_ret uriret;
> - int urierr;
>
> + VIR_DEBUG("Trying to query remote URI");
> memset (&uriret, 0, sizeof uriret);
> - urierr = call (conn, priv,
> - REMOTE_CALL_IN_OPEN | REMOTE_CALL_QUIET_MISSING_RPC,
> - REMOTE_PROC_GET_URI,
> - (xdrproc_t) xdr_void, (char *) NULL,
> - (xdrproc_t) xdr_remote_get_uri_ret, (char *) &uriret);
> - if (urierr == -2) {
> - /* Should not really happen, since we only probe local libvirtd's,
> - & the library should always match the daemon. Only case is post
> - RPM upgrade where an old daemon instance is still running with
> - new client. Too bad. It is not worth the hassle to fix this */
> - remoteError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("unable to auto-detect URI"));
> - goto failed;
> - }
> - if (urierr == -1) {
> + if (call (conn, priv, 0,
> + REMOTE_PROC_GET_URI,
Is the loss of REMOTE_CALL_QUIET_MISSING_RPC going to make this new code
noisy when talking to an (extremely) old remote client? Or is the
assumption that the deleted comment still applies, and the only reason
we were using the QUIET_MISSING_RPC call would be to silence a one-time
failure upon a libvirtd upgrade, but where we can assume that it is no
longer a practical concern.
>
>
> static int
> -check_cert_file(const char *type, const char *file)
> +doRemoteClose (virConnectPtr conn, struct private_data *priv)
> {
> + virNetClientProgramFree(priv->remoteProgram);
> + virNetClientProgramFree(priv->qemuProgram);
> + priv->remoteProgram = priv->qemuProgram = NULL;
> +
> + /* Free hostname copy */
> + VIR_FREE(priv->hostname);
> +
> + /* See comment for remoteType. */
> + VIR_FREE(priv->type);
> +
> + virDomainEventStateFree(priv->domainEventState);
You had several instances of cleanup followed by NULL-ing the argument,
to avoid double cleanup; does priv->domainEventState need this same
protection? But it does make sense that doRemoteClose is used to sever
connections while keeping the object still alive, so that it does part,
but not all, of the work of the final cleanup function and must protect
against double cleanup.
> - memset (&secprops, 0, sizeof secprops);
> /* If we've got a secure channel (TLS or UNIX sock), we don't care about
SSF */
> - secprops.min_ssf = priv->is_secure ? 0 : 56; /* Equiv to DES supported by all
Kerberos */
> - secprops.max_ssf = priv->is_secure ? 0 : 100000; /* Very strong ! AES == 256
*/
> - secprops.maxbufsize = 100000;
> /* If we're not secure, then forbid any anonymous or trivially crackable
auth */
> - secprops.security_flags = priv->is_secure ? 0 :
> - SASL_SEC_NOANONYMOUS | SASL_SEC_NOPLAINTEXT;
> -
> - err = sasl_setprop(saslconn, SASL_SEC_PROPS, &secprops);
> - if (err != SASL_OK) {
> - remoteError(VIR_ERR_INTERNAL_ERROR,
> - _("cannot set security props %d (%s)"),
> - err, sasl_errstring(err, NULL, NULL));
> + if (virNetSASLSessionSecProps(sasl,
> + priv->is_secure ? 0 : 56, /* Equiv to DES
supported by all Kerberos */
> + priv->is_secure ? 0 : 100000, /* Very strong !
AES == 256 */
> + priv->is_secure ? true : false) < 0)
Those look like magic numbers, but they were there before the
refactoring, so not fatal to this patch.
> @@ -3410,7 +2632,7 @@ remoteAuthSASL (virConnectPtr conn, struct
private_data *priv, int in_open,
> if (clientoutlen > REMOTE_AUTH_SASL_DATA_MAX) {
> remoteError(VIR_ERR_AUTH_FAILED,
> _("SASL negotiation data too long: %d bytes"),
> - clientoutlen);
> + (int)clientoutlen);
Rather than adding a cast to cope with the change of clientoutlen from
int to size_t, shouldn't you instead change %d to %zu?
> goto cleanup;
> }
> /* NB, distinction of NULL vs "" is *critical* in SASL */
> @@ -3419,11 +2641,12 @@ remoteAuthSASL (virConnectPtr conn, struct private_data
*priv, int in_open,
> sargs.data.data_val = (char*)clientout;
> sargs.data.data_len = clientoutlen;
> sargs.mech = (char*)mech;
> - VIR_DEBUG("Server start negotiation with mech %s. Data %d bytes %p",
mech, clientoutlen, clientout);
> + VIR_DEBUG("Server start negotiation with mech %s. Data %d bytes %p",
> + mech, (int)clientoutlen, clientout);
Likewise.
>
> /* Now send the initial auth data to the server */
> memset (&sret, 0, sizeof sret);
> - if (call (conn, priv, in_open, REMOTE_PROC_AUTH_SASL_START,
> + if (call (conn, priv, 0, REMOTE_PROC_AUTH_SASL_START,
> (xdrproc_t) xdr_remote_auth_sasl_start_args, (char *) &sargs,
> (xdrproc_t) xdr_remote_auth_sasl_start_ret, (char *) &sret) != 0)
> goto cleanup;
> @@ -3433,27 +2656,23 @@ remoteAuthSASL (virConnectPtr conn, struct private_data
*priv, int in_open,
> serverin = sret.nil ? NULL : sret.data.data_val;
> serverinlen = sret.data.data_len;
> VIR_DEBUG("Client step result complete: %d. Data %d bytes %p",
> - complete, serverinlen, serverin);
> + complete, (int)serverinlen, serverin);
Likewise.
> @@ -3479,10 +2698,11 @@ remoteAuthSASL (virConnectPtr conn,
struct private_data *priv, int in_open,
> }
>
> VIR_FREE(serverin);
> - VIR_DEBUG("Client step result %d. Data %d bytes %p", err,
clientoutlen, clientout);
> + VIR_DEBUG("Client step result %d. Data %d bytes %p",
> + err, (int)clientoutlen, clientout);
and so on.
> +static void remoteStreamEventCallback(virNetClientStreamPtr
stream ATTRIBUTE_UNUSED,
> + int events,
> + void *opaque)
> {
> + struct remoteStreamCallbackData *cbdata = opaque;
> + struct private_data *priv = cbdata->st->conn->privateData;
>
> remoteDriverUnlock(priv);
> + (cbdata->cb)(cbdata->st, events, cbdata->opaque);
> + remoteDriverLock(priv);
> }
>
>
> -static void
> -remoteStreamEventTimerFree(void *opaque)
> +static void remoteStreamCallbackFree(void *opaque)
> {
> - virStreamPtr st = opaque;
> - virUnrefStream(st);
> + struct remoteStreamCallbackData *cbdata = opaque;
> +
> + if (!cbdata->cb && cbdata->ff)
> + (cbdata->ff)(cbdata->opaque);
Given that remoteStreamEventCallback unlocked the driver for the
duration of the callback, should remoteStreamCallbackFree be doing likewise?
> /*
> * Serial a set of arguments into a method call message,
> * send that to the server and wait for reply
> */
> static int
> -call (virConnectPtr conn, struct private_data *priv,
> +call (virConnectPtr conn ATTRIBUTE_UNUSED,
> + struct private_data *priv,
> int flags,
> int proc_nr,
> xdrproc_t args_filter, char *args,
> xdrproc_t ret_filter, char *ret)
> {
> - struct remote_thread_call *thiscall;
> int rv;
> + virNetClientProgramPtr prog = flags & REMOTE_CALL_QEMU ?
priv->qemuProgram : priv->remoteProgram;
> + int counter = priv->counter++;
> + priv->localUses++;
This shares a single counter among two programs, rather than giving each
program its own counter. Is that ever going to cause a problem, where a
single program sees a skipped counter because it was interleaved with
other programs?
>
> - thiscall = prepareCall(priv, flags, proc_nr, args_filter, args,
> - ret_filter, ret);
> -
> - if (!thiscall) {
> - return -1;
> - }
> + /* Unlock, so that if we get any async events/stream data
> + * while processing the RPC, we don't deadlock when our
> + * callbacks for those are invoked
> + */
> + remoteDriverUnlock(priv);
> + rv = virNetClientProgramCall(prog,
> + priv->client,
Is it safe to grab priv->client while priv is unlocked, or do you need a
local variable cache?
Conditional ACK. Most of my comments can either be trivially addressed
or rebutted, or were ideas for future cleanups that don't impact this
patch. Squash this in to compile:
diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c
index 87879e3..17c27f8 100644
--- i/src/remote/remote_driver.c
+++ w/src/remote/remote_driver.c
@@ -43,7 +43,6 @@
#include "qemu_protocol.h"
#include "memory.h"
#include "util.h"
-#include "ignore-value.h"
#include "files.h"
#include "command.h"
#include "intprops.h"
@@ -220,10 +219,6 @@
remoteDomainBuildEventGraphics(virNetClientProgramPtr prog,
virNetClientPtr client,
void *evdata, void *opaque);
static void
-remoteDomainBuildEventBlockPull(virNetClientProgramPtr prog,
- virNetClientPtr client,
- void *evdata, void *opaque);
-static void
remoteDomainBuildEventControlError(virNetClientProgramPtr prog,
virNetClientPtr client,
void *evdata, void *opaque);
@@ -261,10 +256,6 @@ static virNetClientProgramEvent
remoteDomainEvents[] = {
remoteDomainBuildEventControlError,
sizeof(remote_domain_event_control_error_msg),
(xdrproc_t)xdr_remote_domain_event_control_error_msg },
- { REMOTE_PROC_DOMAIN_EVENT_BLOCK_PULL,
- remoteDomainBuildEventBlockPull,
- sizeof(remote_domain_event_block_pull_msg),
- (xdrproc_t)xdr_remote_domain_event_block_pull_msg },
};
enum virDrvOpenRemoteFlags {
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org