Re: [libvirt] PATCH: 17/25: Concurrent client dispatch in libvirtd

> } else { > ret = gnutls_record_recv (client->tlssession, data, len); > - if (qemudRegisterClientEvent (server, client, 1) < 0) > - qemudDispatchClientFailure (server, client); > - else if (ret <= 0) { > - if (ret == 0 || (ret != GNUTLS_E_AGAIN && > - ret != GNUTLS_E_INTERRUPTED)) { > - if (ret != 0) > - VIR_ERROR(_("gnutls_record_recv: %s"), > - gnutls_strerror (ret)); > - qemudDispatchClientFailure (server, client); > - } > + > + if (ret == -1 && (ret == GNUTLS_E_AGAIN && > + ret == GNUTLS_E_INTERRUPTED)) > + return 0; The above is dead code, since the condition can never be true. It should be testing "ret < 0", not ret == -1. Also, the 2nd "&&" should be "||". if (ret < 0 && (ret == GNUTLS_E_AGAIN || ret == GNUTLS_E_INTERRUPTED)) return 0;

> if (encodedLen < 0) > return -1; > > - sasl_decode(client->saslconn, encoded, encodedLen, > - &client->saslDecoded, &client->saslDecodedLength); > + ret = sasl_decode(client->saslconn, encoded, encodedLen, > + &client->saslDecoded, &client->saslDecodedLength); > + if (ret != SASL_OK) > + return -1; If this ever fails, it's sure be nice to log why, but I don't see a strerror analog for SASL_* values.

> - if (qemudClientRead(server, client) < 0) > - return; /* Error, or blocking */ > + xdrmem_create(&x, client->rx->buffer, client->rx->bufferLength, XDR_DECODE); > > - if (client->bufferOffset < client->bufferLength) > - return; /* Not read enough */ > - > - xdrmem_create(&x, client->buffer, client->bufferLength, XDR_DECODE); > - > - if (!xdr_u_int(&x, &len)) { > + if (!xdr_int(&x, &len)) { > xdr_destroy (&x); > DEBUG0("Failed to decode packet length"); > - qemudDispatchClientFailure(server, client); > + qemudDispatchClientFailure(client); > return; > } > xdr_destroy (&x); > > + /* Length includes the size of the length word itself */ > + len -= REMOTE_MESSAGE_HEADER_XDR_LEN; > + > if (len > REMOTE_MESSAGE_MAX) { > DEBUG("Packet length %u too large", len); > - qemudDispatchClientFailure(server, client); > + qemudDispatchClientFailure(client); > return; > } > > /* Length include length of the length field itself, so > * check minimum size requirements */ > - if (len <= REMOTE_MESSAGE_HEADER_XDR_LEN) { > + if (len <= 0) { > DEBUG("Packet length %u too small", len); "len" may be negative here, so printing with %u will give a misleading diagnostic. Better use the original value, "len + REMOTE_MESSAGE_HEADER_XDR_LEN", which is more likely to be non-negative. Might as well use %d, in case even the original value is negative.

> > - if (client->fd != fd) > - return; > + if (events & (VIR_EVENT_HANDLE_WRITABLE | > + VIR_EVENT_HANDLE_READABLE)) { > + if (client->handshake) { > + qemudDispatchClientHandshake(server, client); > + } else { > + if (events & VIR_EVENT_HANDLE_WRITABLE) > + qemudDispatchClientWrite(server, client); > + if (events == VIR_EVENT_HANDLE_READABLE) > + qemudDispatchClientRead(server, client); Why test with & to write, and then with "==" to read? That makes it so we don't read when we've just written (i.e., if both read and write bits were set).

> diff --git a/qemud/qemud.h b/qemud/qemud.h ... > +struct qemud_client_message { ... > + int nrequests; Logically, this should be an unsigned type. But that means max_clients should be, too, since they're compared, but max_clients comes from the config file, which currently uses "int" (via GET_CONF_INT). Maybe we need GET_CONF_UINT? I wonder if a bogus config "int" value of 2^32 or 2^64 maps back to 0.