On Tue, Jan 20, 2009 at 10:12:04AM +0100, Jim Meyering wrote:
} 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;
Yes, really not sure why I change it like this. Clearly wrong
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.
Actually there are two options sasl_errstring / sasl_errdetail both giving suitable output.
- 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.
SOmething odd here - I don't think I should have been changing it to signed in the first place. The original code used unsigned int and this is on the wire. Oddly the original client code used a signed int, so we had a mis-match of client & server. I think it is better to fix the client though. So I'll re-visit this whole chunk
- 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).
Bug, it should be '&' and not '=='.
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.
Checking for 2^32/64 is not really helpful in this context, because both are totally inappropriate for nrequests - we should check for a more reasonable lower limit on nrequests, perhaps in range 1 -> 100. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|