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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list