[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]