Flux-core: libflux: change flux_future_error_string() to return flux_strerror() if textual error was not set

[chu11]

Was just talking to @garlick about what errno to pick for a strange corner case in #2071. He suggested the errno didn't matter that much, but perhaps pass along an error string to the caller for output.

So I thought, perhaps we could pick an errno as the general "check the error string associated with this future".

[garlick]

Just for background, the following may be used to return a string error from a request handler along with an error number:

/* Create an error response to the provided request message with optional
 * printf-style error string payload if 'fmt' is non-NULL.
 */
int flux_respond_error (flux_t *h, const flux_msg_t *request,
                        int errnum, const char *fmt, ...)
                        __attribute__ ((format (printf, 4, 5)));

On client side, the future will be fulfilled with error. If an error string was supplied, then the following returns non-NULL.

const char *flux_future_error_string (flux_future_t *f);

So I think the current idiom on the client side is something like

flux_future_t *f = flux_rpc (...)
if (flux_rpc_get (f, NULL) < 0) {
    char *errstr = flux_future_error_string (f);
    if (errstr)
        log_msg ("thing: %s", errstr);
    else
        log_err ("thing");
}

I think what you are proposing is something like:

flux_future_t *f = flux_rpc (...)
if (flux_rpc_get (f, NULL) < 0) {
    if (errno == EFLUXERRSTR)
        log_msg ("thing: %s", flux_future_error_string (f));
    else
        log_err ("thing");
}

where EFLUXERRSTR is some error we define that does not intersect wtih zeromq's or POSIX's errno space? Alternatively, we could choose a POSIX errno, but a problem with that is if it occurs in nature and is passed back in an RPC without interpretation, it could signal the wrong thing to the caller.

I'm not sure the second idiom really achieves much over the first.

Does anybody remember the reason we didn't just substitute flux_strerror (errnum) when flux_future_error_string (f) is called but no textual error is available?

Then you could just do:

flux_future_t *f = flux_rpc (...)
if (flux_rpc_get (f, NULL) < 0)
    log_msg ("thing: %s", flux_future_error_string (f));

[grondo]

Oh nice I like that last use!

Not sure why we didn't think of that before.

[chu11]

I think what you are proposing is something like:

Yup, that's basically what I was thinking. I think the main reason I thought about this was because there have been a number of circumstances where we don't really have the "right" errno, and we just end up picking something that is "ok". But since you now mention it ...

Does anybody remember the reason we didn't just substitute flux_strerror (errnum) when flux_future_error_string (f) is called but no textual error is available?

I think I didn't do that just so the user knows if an error string was set or not (vs. an error string being auto-created via the errno). But, I'm not sure how often that matters?

[garlick]

What do you guys think, should we just say that fixing up flux_future_error_string(f) as suggested above solves this? If so we should change the issue title.

[grondo]

Yeah let's try that out

[SteVwonder]

:+1: That last use-case would also simplify the python bindings RPC error handling, which currently uses a decorator to do the first idiom outlined above.