More comments...
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.
Anything that happens after this failure it not something we necessarily want to
pass back back through to a user. We don't want to give them more information
than they really need and should stop at the first error that caused us to fail.
This was discussed with Frank and it was his suggestion that we don't provide
too much information and confuse the user.
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.
Error code are separate from this error structure and are returned by the libbe
functions themselves. Changing this would cause a major interface change and
require all the consumers of the current library and python bridge to change.
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.
I still don't see why you would need more than one type since you will be
puuting the error information you want into the available strings and passing
backe the error code separately from the structure. You can put what ever you
want into the strings within the structure.
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.
That's a good point, these should definitely be a fixed length if possible.
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.
I agree, I think it makes more sense that have this paired down to these two
functions. I'll change that...
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.
That's one of the ways of doing this that Ethan and I had been talking
about yesterday. The idea of creating a handle to contain the structure
and we'd be able to pass the information back inside the handle and free
up the memory when the handle is destroyed. The only problem we have
with this right now is that all of the interfaces use nvlists so passing
a handle from the library and back into it would also require an
interface change that would brake anything already using the library.
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.
agreed we should be either using a handle or some type of global to contain the
structure and allocating the storage when we set the strings and requring the
memory to already be allocated for the get function.
This looks like a good start, but there's a lot more work to do.
I think we'll be a lot closer once we get some of these misunderstandings
out of the way. :-)
-evan
-j
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
