assuan_read_from_server() and return value

[Marcus Brinkmann]

At Sat, 8 Dec 2007 12:44:21 -0500,
Ben Kibbey <bjk at luxsci.net> wrote:
> 
> On Mon, Dec 03, 2007 at 03:56:53PM +0100, Marcus Brinkmann wrote:
> > Hi Ben,
> > 
> > there seem to be separate issues here:
> > 
> > 1) It seems to me that the case you describe can only happen if you
> > have no inquire callback set, right?  In this case, the error code is
> > going to be overwritten by assuan_transact anyway.
> 
> After the inquire callback is finished, assuan_transact() sends END to
> terminate the inquire then calls _assuan_read_from_server() again.

Are you sure?  That's not my reading of the code:

assuan-client.c::assuan_transact()
[...]

      if (!inquire_cb)
        {
          assuan_write_line (ctx, "END"); /* get out of inquire mode */
          _assuan_read_from_server (ctx, &okay, &off); /* dummy read */
          rc = _assuan_error (ASSUAN_No_Inquire_Callback);
        }
      else
        {
          rc = inquire_cb (inquire_cb_arg, line);
          if (!rc)
            rc = assuan_send_data (ctx, NULL, 0); /* flush and send END */
          if (!rc)
            goto again;
        }

The code path "END" followed by _assuan_read_from_server is only
invoked if "!inquire_cb" that means "inquire_cb == NULL".  Otherwise,
the inquire_cb is invoked.

Are you sure you have a callback function set up?

> On the server end, an error may have occured while processing the inquired
> data which may return an error code which is not guaranteed to be an
> ASSUAN_ error code.

Right.

> Without the patch, _assuan_read_from_server()
> returns the value returned from _assuan_read_line() (zero). I use
> GPG_ERR codes on the server. Pinentry uses ASSUAN_ errors and there is
> no way to tell if the error is an GPG_ERR_ or an ASSUAN_ error.

Not quite correct.  We have a heuristic to differentiate: Values that
have the high bits set indicate a gpg error with a source set.  All
our servers set a gpg error source.  On the other hand, low values
indicate an assuan based error code.  This is not failure proof, but
covers current and expected usage.  By the way: It is currently
undefined how assuan error codes can be extended by user defined error
codes.  In gpg-error, there are reserved values for that.

> So I
> guess pinentry has to migrate to use GPG_ERR_ codes, which may break
> clients, or I have to wait until libassuan removes ASSUAN_ codes
> completely forcing libassuan users to use GPG_ERR_ codes, which seems
> unlikely to happen any time soon?

We can map assuan error codes to gpg error codes (see the function
_assuan_error in mkerrors).

However, I want to clear up the following issues: Why you actually
encounter this code path (it seems you should have an inquire cb but
don't), and if sending END and reading from the server is the right
thing to do anyway.

> > Frankly, I do not understand why there is a dummy read in that place
> > anyway.  Comparing this with the code in assuan-buffer.c, where there
> 
> Maybe to flush the input?

Nah, that function doesn't flush.  I think it is simply to receive the
status code of the command which caused the inquire, to try to push
the server into a state where it will receive a new command.  But that
is not a failsafe way to do that.

> > So, I think I would still like to apply your patch, although it should
> > not make any difference in the scenario you describe.  Plus, I would
> > like to replace the dummy read and possibly (if it works) the sending
> > of END by a sending of CANCEL.
> 
> If CANCEL is used, wouldn't the INQUIRE on the server end not succeed
> (OK response) if all went well?

At that point in the code path, the client already will get an error
because of a missing inquire callback function.  It seems to me that
canceling the pending operation is then the right response.

Thanks,
Marcus