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 > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
