Quoth Sean Wilcox on Thu, Apr 16, 2009 at 04:56:35PM -0600:
> Webrev updated and changes made per comments inline below :
>
> http://cr.opensolaris.org/~swilcox/webrev.2
...
> >>>>lib/librestart/common/librestart.c
...
> In short and theory, the mc_error_t structure will have a static
> message field that will/must be at the end of the structure. This
> structure will be allocated at the beginning of the call of
> restarter_get_method_context(). Given failure of this the static
> nomem error structure will be returned and
> restarter_get_method_context() will bail out.
This approach looks good. Incomplete review:
lib/librestart/common/librestart.c
152: Doesn't this need to be >= ?
153: This will be much larger than necessary because
sizeof (mc_error_t) already includes RESTARTER_ERRMSGSZ. If that's
ok with you, please add a comment.
2095: I suspect you should include the profile's name, but I suppose
it's not a big deal.
2415: Shouldn't you update this comment?
2424: Shouldn't we list the possible error codes?
2449: Did you consider including the two version numbers in the
message?
2494: This message seems a little bare. What would it return if the
pg was missing?
2971: This is new code, right? Can you send out a full webrev?
> >>>>lib/librestart/common/librestart.h
...
> >>show that better. How do you envision clients using the size field?
> >>Should it always be the same as strlen(msg)?
>
> Yes the size should always be the same as strlen(msg), but I figured
> since it's been calculated once on allocation, it might as well be
> stored for later use saving the expense of making calls and such to
> recalculate the size for what ever use later. If it's a sticking
> point I can throw it out.
Since these aren't performance-critical paths, I would be inclined to
remove the possibility of bugs that set it to the wrong value. But that
should be rare enough that you don't need to change it.
David
