On Mon, Jun 27, 2011 at 02:49:37PM -0600, Eric Blake wrote:
> 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).
This in turns calls to virNetSocketNewExternal which has the
same signature, and I thought it was more useful to have it
take a full argv, rather than restrict it to just server the
needs of our remote driver code.
> >> @@ -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.
Yes, this is a mistake. There were two places using REMOTE_CALL_QUIET_MISSING_RPC,
this and the authentication routines. In the latter I added
err = call (conn, priv, 0,
REMOTE_PROC_AUTH_LIST,
(xdrproc_t) xdr_void, (char *) NULL,
(xdrproc_t) xdr_remote_auth_list_ret, (char *) &ret);
if (err < 0) {
virErrorPtr verr = virGetLastError();
if (verr && verr->code == VIR_ERR_NO_SUPPORT) {
/* Missing RPC - old server - ignore */
virResetLastError();
return 0;
}
return -1;
}
And I should have done the same with the URI call too. I'll add a note
to explicitly test compatibility against an old server.
> >> 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.
Yes, it is probably worth nulling domainEventState.
>
> >> - 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?
Yes, I got most of those converted, but missed a few.
> >> +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?
There are a few bugs in this streams callback code which I'll
send a followup patch to fix soon.
> >> /*
> >> * 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?
Nothing requires that the counter variable be monotonically incrementing.
It merely needs to be unique for the duration of the connection, so we
can reliably match up calls to returns, and streams to calls. The previous
code also had one counter for both 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?
For sanity, I can cache the variable
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list