See responses inline : New webrev is : http://cr.opensolaris.org/~swilcox/webrev.1/
David Bustos wrote: > 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? > > This is the error message "leader", that was presented originally by the caller of read_method_context(), in one case the error message returned from restarter_get_method_context() is presented, while in the later case the basic message is reported. What is the suggested practice in this case? > lib/librestart/common/librestart.c > 110: Please add a comment explaining this static variable. > > Done > 111: Please use 'sizeof ("Out of memory") - 1' instead of 13. > Although, can you explain when it will be used? > > Done... This structure is used when there are memory errors, or problems attempting to allocate errors as a static message to take handle the message allocation. > 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. > At the onset, I thought both the restarter_mc_error_create() and restarter_mc_error_destroy() functions would be exported from the library. But as development moved forward there was no need for the create message to be exported. With that being said and to respond to the next comment as well... it could be that the _error_create() function is exported, and the mc dropped so that this restarter error message can be used throughout for restarter error events. And in that case, as I originally designed this, I would not want to limit the error messages to a static length, which might lose valuable information. That is also the reason for using the static mc_nomem_err structure to report memory problems. But in the case of problem A occuring then a Memory problem occuring while attempting to report the problem at which point I could use a static string buffer to report what can be reported of the message. This would eliminate the issue of potential message data loss due to size, and allow for collection of errors when memory is an issue. Would this address the comments above, and below? > - 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. > See above note... although, there would have to be some locking around the message buffer in case more threads were attempting to use the message buffer at the same time. Or a per-thread string buffer that would then have to be tracked. > - 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. > > moved > 2001: I don't see how this function can return an mc_error_t with > destroy == 1. > > You are correct, cleared this up and re-worked both functions. > 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. > > used the latter. > 2018: Function definition braces need to be on their own lines. > > ARG.... I will break this habit. > 2054: Why did you choose ENOENT? If the property is missing, then it > seems that the method context is inconsistent. > > Seemed logical at the time. I agree this is an inconsistent method_context, what is the suggested error type in this case. Would it be more appropriate for the value to be EINVAL? > 2055: When you integrate, please file a bug to improve this error > message. > > Do you have any suggestions on what the improved message should be, and I'll just make the change here. > 2073: Is there a reason you didn't include cmdp in this error message? > > Added. > 2082: > - Please add double quotes around %s. > > Done throughout the messages I changed/added. > - 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? > > Using the return value for get_uid() as the type and reporting in the message. Changed the colon to value (or something more appropriate) throughout. > 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? > > Added a note in the evaluation. > struct mc_error: Please add comments for the type, destroy, and size > fields. > > Added comments. > David > > > -- Sean Wilcox 303.272.9711 x79711
