Quoth Sean Wilcox on Fri, Apr 10, 2009 at 10:02:53AM -0600:
> Seems like my mailer may have stomped the link in the original email...
>
> http://cr.opensolaris.org/~swilcox/webrev/
Here are comments for most of the changes for 6215238:
cmd/cmd-inet/usr.lib/inetd/configd.c
678: Is there a reason this string is duplicated from 663?
lib/librestart/common/librestart.c
110: Please add a comment explaining this static variable.
111: Please use 'sizeof ("Out of memory") - 1' instead of 13.
Although, can you explain when it will be used?
restarter_mc_error_create():
- Is there a reason you chose to start the name of this function
with "restarter_"? I think it might be useful to future
developers to reserve the restarter_ prefix for functions that are
exported from the library.
- This is an interesting approach to memory errors. Did you
consider including a static string buffer and vsnprintf()ing into
it? There's a chance that the error message will be too long, but
it eliminates the possibility that the error would be lost for
lack of memory.
- I don't think this is a good place for this function. Before
get_astring_val() or even free_restarter_event_handle() seem more
logical to me.
2001: I don't see how this function can return an mc_error_t with
destroy == 1.
restarter_mc_error_destroy(): I don't think this is a good place for
this function. After restarter_get_method_context() seems more
logical to me, since that's the order of the prototypes in
librestart.h. Though near restarter_mc_error_create() seems
logical, too.
2018: Function definition braces need to be on their own lines.
2054: Why did you choose ENOENT? If the property is missing, then it
seems that the method context is inconsistent.
2055: When you integrate, please file a bug to improve this error
message.
2073: Is there a reason you didn't include cmdp in this error message?
2082:
- Please add double quotes around %s.
- Did you consider using "... euid value \"%s\"." instead of
a colon? And I suspect it would be clearer to some users if you
explained that the value seems to be invalid. Though get_uid()
returns an error code explaining what went wrong. Is there
a reason you're not using it, or did you just decide to defer it?
lib/librestart/common/librestart.h
219: I presume you're increasing the version number because you're
changing the return type of restarter_get_method_context()?
I suspect the reason should be documented somewhere. Perhaps the
bug report?
struct mc_error: Please add comments for the type, destroy, and size
fields.
David
