On Thu, Aug 20, 2009 at 04:08:20PM -0600, Evan Layton wrote:
> johansen at sun.com wrote:
>>
>> - The error structure defined below is not sufficiently extensible. It
>> may work for today's errors, but it's not flexible enough to grow with
>> any further changes to libbe. Consumers aren't going to want to
>> change their code every 6 months, when new functionality gets added.
>
> Can you describe what you are referring to here? I'm not sure I see
> what new functionality within the the consumer would cause this to
> need to be more extensible. The consumer decides what goes into the
> strings described.
>
> Is there something specific you have in mind here?
That's part of the point. It's hard to see exactly what will be coming
down the road, but the existing structure limits you to 5 character
pointers, and that's it. I described more details about my concerns
further on in my e-mail, with relevant portions of your proposal inline.
You either didn't read those comments, or have chosen to ignore them.
>> - If you're going to encapsulate the error information in an object, and
>> only allow access through the accessor functions, they need to be
>> refactored. The error data should stay in the err_info_str. These
>> functions should also be able to expand, should more errors get added
>> later.
>
> These strings are generated outside the encapsulation and are not
> static strings that are looked up. The error information is not
> encapsulated only the structure is. The error codes will continue to
> be passed back as an be_errno_t as it is today. Any current consumer
> will continue to work without any changes.
>
> Since the errors themselves are not part of the encapsulation and
> the related strings are generated outside the accessor functions there
> will be nothing to add when new errors are created. The errors live
> outside this structure and only make use of the structure as a way to
> return the strings but not the error itself.
This doesn't make any sense. The point of encapsulating your errors in
a well known structure is that libbe maintains control of the error
information and callers have to arrange to obtain that information in
resources that they control. I also explained this point in more detail
further on my previous e-mail. Did you read the rest of it?
Read the parts below. I'm in >> you're in >>>.
>>
>>> 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 have a couple of issues with this approach.
>>
>> In my high-level comments, I observed that this structure doesn't allow
>> libbe to grow or add any other error information beyond what's provided
>> here. At a minimum, there should be a field, or linked list, that allow
>> callers in the error handling chain to add ancillary information as an
>> operation progresses. The initial failure may be that a device was in
>> use, but as the error is propagated up the call chain, there may be
>> other failures that occur too. How do you plan to handle this case?
>> Augmenting the error object with other information is one approach, but
>> there are certainly others.
>>
>> There are other extensibility issues to consider. In particular, this
>> format is fixed to contain 5 pointers to char *. I think it would make
>> more sense to define a generic err_info structure that contains a code
>> that describes the type of error that follows. The sockaddr structure
>> is a good example of something similar. Depending upon what socket
>> method is being used, the pointer to sockaddr is copied in and then read
>> as a particular structure. I can imagine that we might want to have
>> multiple different types of err_info structures, but the current
>> approach precludes that option.
>>
>> As a practical matter, enforcing encapsulation when you have a series of
>> pointers in an object is difficult. I would consider using fixed length
>> fields, if practical. That way, it's less likely that the error strings
>> will get leaked, or used after they've been freed. I have some more
>> comments on this, but I'll save them for the section on the accessor
>> functions.
>>
>>> Accessor Functions:
>>> These functions are used to access the fields in the data structure as the
>>> structure itself will be encapsulated.
>>> - err_set_cmd_str(err_info_str_t *err_info, char *cmd_str)
>>> - This is used by the caller before calling into the library to set
>>> the
>>> name of the command or function calling into the library.
>>> - err_set_op_str(err_info_str_t *err_info, char *op_str)
>>> - This function adds the operation to the error string structure.
>>> - err_get_op_str(err_info_str_t *err_info)
>>> - this function will return the contents of the option string from
>>> err_info_str structure.
>>> - err_set_fail_strs(err_info_str_t *err_info, char *failed_at,
>>> char *fail_msg_str, char *fail_fixit_str)
>>> - This function fills in the failed_at, fail_str and fail_info into
>>> the
>>> err_info_str_t structure
>>> - This functions can be used to fill in any or all of these fields
>>> in
>>> the structure.
>>> - err_get_failed_at_str(err_info_str_t *err_info)
>>> - retrieves the failed_at string from the structure
>>> - err_get_fail_msg_str(err_info_str_t *err_info)
>>> - retrieves the fail_msg_str string from the structure
>>> - err_get_fixit_str(err_info_str_t *err_info)
>>> - retrieves the fail_fixit_str string from the structure
>>
>> I think you could pare this down to to two functions:
>>
>> int err_get_str(err_info_str_t *ei, err_option_t eo, char *outstr,
>> size_t len);
>>
>> int err_set_str(err_info_t *ei, err_option_t eo, restrict char *instr);
>>
>> Introducing a function for each member of the structure means that
>> callers of the library get stuck using the various calls for all time.
>> If you add another field, you have to add another method. If you remove
>> a field, then you either have to change all of the consumers, or leave a
>> useless routine around. You can always add more error option values,
>> and enum or #define them to something sensible. With this approach, the
>> call interface doesn't change, but you can add or remove details from
>> the encapsulated structure.
>>
>> In addition to refining the interface, I would make sure that you have a
>> good way of constructing and destroying these objects. If they're tied
>> to a boot-environment specifc handle that's great. Otherwise, you'll
>> have to make sure that the caller calls some destroy_my_error_object()
>> routine to get these cleaned up. It's not optimal, especially since
>> some applications are haphazardly constructed.
>>
>> The previous set of accessor functions appear to just return a pointer
>> to the data that's contained in the err_info_str_t. I'd like to advise
>> against that approach since it violates encapsulation, and makes keeping
>> track of these strings more difficult. I would propose that in the case
>> of set_str() the instr argument is copied and set_str allocates the
>> storage, if it's available, for the string. For get_str, the caller
>> must provide the storage for get_str to copy the string into. This
>> makes it a lot easier to keep track of what routines are responsible for
>> allocating and freeing memory.
>>
>> This looks like a good start, but there's a lot more work to do.
>>
>> -j
