Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id

2020-07-08 Thread Floris Bruynooghe
On Sun 05 Jul 2020 at 08:17 -0300, David Bremner wrote:

> David Bremner  writes:
>
>> Floris Bruynooghe  writes:
>>
>>> notmuch_database_get_version currently returns and unsigned int and
>>> segfaults on use with a closed db.
>>
>> Yes, the ones without a proper status value are going to be a bit work.
>>
>> In the next series I just posted [1], I started providing status value
>> returning version (see notmuch_message_get_flag_st). We've been through
>> a few of these migrations and it has not been too painful.
>>
>
> I thought of another variation for the boolean valued functions. We
> could embed the boolean values in the notmuch_status_t value by adding
> one or more new status values corresponding to TRUE and FALSE. I'm not
> sure if that would be much simpler, but it would avoid the use of output
> parameters.

This also seems very reasonable.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id

2020-07-08 Thread Floris Bruynooghe
On Sat 04 Jul 2020 at 14:17 -0300, David Bremner wrote:

> Floris Bruynooghe  writes:
>
>
>>> - * This function will not return NULL since Notmuch ensures that every
>>> - * message has a unique message ID, (Notmuch will generate an ID for a
>>> - * message if the original file does not contain one).
>>> + * This function will return NULL if triggers an unhandled Xapian
>>> + * exception.
>
>> How much of a departure from the existing API is this?  Will this be
>> possible with all functions?  I had a quick look and tried some other
>> functions that don't return notmuch_status_t:
>
> It's upward compatible in that any code which crashes because it was not
> expecting a NULL pointer, will already be crashing in the same
> circumstances because of an uncaught exception / call to abort.

Oh yes, that is a very good point.  This choice seems very reason then.


Cheers,
Floris
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id

2020-07-05 Thread David Bremner
David Bremner  writes:

> Floris Bruynooghe  writes:
>
>> notmuch_database_get_version currently returns and unsigned int and
>> segfaults on use with a closed db.
>
> Yes, the ones without a proper status value are going to be a bit work.
>
> In the next series I just posted [1], I started providing status value
> returning version (see notmuch_message_get_flag_st). We've been through
> a few of these migrations and it has not been too painful.
>

I thought of another variation for the boolean valued functions. We
could embed the boolean values in the notmuch_status_t value by adding
one or more new status values corresponding to TRUE and FALSE. I'm not
sure if that would be much simpler, but it would avoid the use of output
parameters.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id

2020-07-04 Thread David Bremner
Floris Bruynooghe  writes:


>> - * This function will not return NULL since Notmuch ensures that every
>> - * message has a unique message ID, (Notmuch will generate an ID for a
>> - * message if the original file does not contain one).
>> + * This function will return NULL if triggers an unhandled Xapian
>> + * exception.

> How much of a departure from the existing API is this?  Will this be
> possible with all functions?  I had a quick look and tried some other
> functions that don't return notmuch_status_t:

It's upward compatible in that any code which crashes because it was not
expecting a NULL pointer, will already be crashing in the same
circumstances because of an uncaught exception / call to abort.

> notmuch_database_get_version currently returns and unsigned int and
> segfaults on use with a closed db.

Yes, the ones without a proper status value are going to be a bit work.

In the next series I just posted [1], I started providing status value
returning version (see notmuch_message_get_flag_st). We've been through
a few of these migrations and it has not been too painful.

> I wonder if a backwards-compatible errno-style API could work,
> notmuch_last_status(notmuch_database_t* database) or so.  This kind of
> thing is probably easy to adopt in bindings but harder for direct users
> of the API.  It's also an extra API call for everything that doesn't
> return notmuch_status_t.  But I'll leave the judgement to you, I'm not
> as experienced with the API.

I think my main objection to this is that there is no out-of-band value
to tell the caller they need to check errno. So basically every call to
to one of the relevant functions would need be followed by a call to
checking the error number. I don't think that's less work than switching
to a new API. Of course it's less work for me, and we already sort of
made that choice with notmuch_database_status_string. In that case it
was a matter of changing the entire API.  Here we're talking about 10
functions, and I'm not sure if they all need to be changed. For example
several of the notmuch_foo_valid functions just check pointers for being
NULL and can't generate I/O or exceptions.

d

[1]: id:20200704151805.3717715-1-da...@tethera.net
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org


Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id

2020-07-04 Thread Floris Bruynooghe
Nice.

On Mon 29 Jun 2020 at 22:14 -0300, David Bremner wrote:
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index ceb5a018..0dc89547 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -1363,9 +1363,8 @@ notmuch_message_get_database (const notmuch_message_t 
> *message);
>   * message is valid, (which is until the query from which it derived
>   * is destroyed).
>   *
> - * This function will not return NULL since Notmuch ensures that every
> - * message has a unique message ID, (Notmuch will generate an ID for a
> - * message if the original file does not contain one).
> + * This function will return NULL if triggers an unhandled Xapian
> + * exception.
>   */
>  const char *
>  notmuch_message_get_message_id (notmuch_message_t *message);

How much of a departure from the existing API is this?  Will this be
possible with all functions?  I had a quick look and tried some other
functions that don't return notmuch_status_t:

notmuch_database_get_version currently returns and unsigned int and
segfaults on use with a closed db.

notmuch_needs_upgrade returns notmuch_bool_t and seems to return a valid
bool with a closed db.  I'm not sure if this is always the right answer
though.

I wonder if a backwards-compatible errno-style API could work,
notmuch_last_status(notmuch_database_t* database) or so.  This kind of
thing is probably easy to adopt in bindings but harder for direct users
of the API.  It's also an extra API call for everything that doesn't
return notmuch_status_t.  But I'll leave the judgement to you, I'm not
as experienced with the API.
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org