Danek Duvall wrote:
> Evan Layton wrote:
>
>> Error String definitions:
>> The entry points into the library would be changed to pass back
>> a structure containing strings with information about the failure.
>> The idea here is that the caller will then be able to build their
>> own error message based on this available information instead of
>> being locked into the error message that we construct.
>> - for example:
>>
>> typedef struct err_info_str{
>> char *cmd_str; /* who called into the library (beadm, pkg(5))
>> */
>> char *op_str; /* operation such as BE activate */
>> char *failed_at; /* where we failed */
>> char *fail_msg_str; /* error string from failure */
>> char *fail_fixit_str; /* Context for error, instructions on
>> what to check or link to html content
>> if instructions can't be determined.
>> */
>> } err_info_str_t;
>
> I think strings are a poor choice for this sort of interface, particularly
> informational strings, which will need to be localized anyway.
You're correct and based on johansen's (and others) comments I'm looking at
this
again. I agree with this and others statements that we should really be using a
defined string related to a enum which can be localized etc.
>
>> char *cmd_str - This is the name of the command or function that has
>> called into the library. It is expected that the
>> caller will provide this information before calling
>> into the library if they which this to be part of the
>> error strings. At some later point it is expected
>> that
>> this would be used for logging purposes.
>
> I don't understand the purpose of this member at all. The caller knows
> what it is, and so has no use for this information. And I don't see what
> use the library internals would have for this, either. You talk about
> logging, but you don't say whether it's the caller or the library that
> would be doing the logging. I'm dubious that the library should do any
> logging, but if it did, I don't know why this information would need to be
> passed *back* -- it would need to be passed *in*, and this isn't that
> interface (which would more likely be getexecname() or some thread-local
> storage).
This was originally something we where thinking would be used eventually
for logging. However you are correct that this is really something that
we can figure out for ourselves and not something that should be passed
in let alone passed back to the consumer. This will be removed.
>
>> char *op_str - This is the higher level operation being performed,
>> such
>> as activating a BE.
>> char *failed_at - This is the functional area or call that failed.
>> This
>> would be things like zfs_mount or installgrub
>
> These two are great candidates for #defines or enums.
agreed and that's what we will do.
>
>> char *fail_msg_str - This is any error string returned form the
>> failed
>> call. This would be things like the contents of the
>> string returned by libzfs_error_description() or the
>> captured error string from installgrub.
>
> This one should be derivable from the previous two, and thus simply a
> library call. In the case of command output, the data would need to be
> stored by the library for later retrieval, rather than being a static
> string or a simple wrapper around another library function.
Yes it's derivable in fact that's that point of this. We have to have a place
to put this information which
>
>> char *fail_fixit_str - This string will be generated by the code
>> calling
>> into the code that will fill in the structure.
>> This
>> generated string will contain information on what
>> the user can do to attempt to fix the cause of the
>> failure. If this can not be determined then
>> instead
>> of this information a link to more information
>> with
>> possible fixes for errors in this area will be
>> used
>> for this string.
>
> This also seems derivable from the operation and failure codes, but also
> has the property that different clients will likely want to suggest
> different ways forward. A commandline app won't talk about "click this,
> select that menu item", while a GUI app won't talk about what to type next.
> Given that, this may be a good candidate for a code rather than a string.
agreed this definitely doesn't really work for something that may need
to tell the user to click something.
I think maybe what's needed here is a set of enums for things we know
are issues we can tell a consumer to fix, a set of URL's if we can't
determine what the consumer needs to fix but know the area to point
them at. These would be chosen based on the operation and failure codes
as you suggested. This is kind of what I had in mind but a much cleaner
way to do it.
>
> I think johansen may have mentioned it, but you don't have any form of
> multiple-error encapsulation, either a heirarchy or a list. Given that
> your operations are complex, you may run into several errors along the way,
> some of which you managed to recover from, and some of which you didn't.
> Although a normal user might not want to see everything, having that
> debugging information available may prove invaluable. A list is probably
> sufficient here.
Yes he did mention this and I'm looking doing as was suggested there so that
we can have different "types" of errors. I'm also going to change the
interfaces so we have a handle passed back from the library that will
contain the error information encapsulated in it.
>
> You'd also need some way of distinguishing classes of errors -- an error
> which was safely handled, an error which could not be handled, an error
> which occurred trying to handle a previous error, etc.
This is something I hadn't really thought about. I was thinking that
if we handled something we wouldn't pass back an error. However it
does make more sense to pass back that we hit an error and fixed it.
I'll have to add something to cover this. Definitely a helpful
suggestion!
Thanks!
-evan
