Webrev updated and changes made per comments inline below : http://cr.opensolaris.org/~swilcox/webrev.2
Begin forwarded message: > On Apr 16, 2009, at 8:17 AM, Sean Wilcox wrote: >> Quoth Sean Wilcox on Wed, Apr 15, 2009 at 03:11:18PM -0600: >>> 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: >>> >>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? >> >> You could call gettext() on the string once at the top of the >> function, >> or you could just add a comment noting that the two should be kept in >> sync. > Looks a little funky but I'm going with the gettext() at the beginning. > >> >>> >lib/librestart/common/librestart.c >>> > 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 >> >> No I meant the size field. The string is statically allocated, so >> nobody should be using umem_free() on it, or trying to modify it. >> And >> if some code needed to know the size of the string, couldn't it >> just use >> strlen()? >> > The size is calculated at the time of message creation, so I figure why not store it for use in whatever capacity so as not to make additional calls to caclulate the size. >>> > 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. >> >> I see. I suspect the only reason a client would want to allocate its >> own error structure is if it needed to call a function which doesn't >> allocate memory otherwise. But apparently you didn't encounter such >> a case, so I think we should defer exporting this function until a >> need >> becomes clear. (And therefore we should drop the restarter_ prefix.) > dropped. >> >>> > - 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. >> >> Yes, that's true. What if restarter_get_method_context() allocated >> an >> error message buffer as it begins, so that we know that if an error >> occurs, we'll be able to report it? The danger is that the buffer >> won't >> be big enough, but I think that's not as bad as dropping the error >> altogether. We could even have a flag which indicates when a message >> was truncated, which instructs the client to instruct the user to >> report >> the bug to Sun! > I've mulled this over a bit, and truncating a message seems to me to be a bad thing as well as dropping the message in the corner case of message A being preempted by a memory failure when message A itself was not a memory failure. And pre-allocation of buffers and such has it's drawbacks in locking a global static buffer in this case with a multi-threaded application usage. As well as pre-allocation of the buffer at the function level failing, or on success having to have the buffer passed up the stack potentially. But taking these things into consideration and Dave's concerns here, I've come up with a compromise in the code in such that I hope will address most of the concerns. 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. If the structure is allocated then it will be passed up to the get_id/ get_profile functions for use by these functions to return the mc_error_t as well as any future additions will need to pass the pre- allocated buffer around. When an error is detected and the structure is passed up the mc_error_create() the structure's msg buffer will be used to take on the message. If the message is greater than the static message buffer, then the structure will be reallocated to add "overflow" space to the end of the structure to take the rest of the message, eliminating message truncation due to a static size. If this re- allocation fails though, the original buffer will still be in tact for use, and a truncated message can be stored. Barring all this and we still can't get allocations then we fall back to the static nomem error structure. This has the added benefit that the mc_error_t structure will just be freed and there are no gyrations needed to free the msg pointer in the error structure. >> >>> > 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? >> >> I think EINVAL is better than ENOENT, though even better would be to >> have get_astring_val() return more a more precise error. >> > updated. >>> > 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. >> >> First of all, profile should be in double quotes to show that it is >> the >> name of the property we tried to access. Of course, with printf() >> capability, it should technically become %s with an >> SCF_PROPERTY_PROFILE >> argument. The message could explain that the method context pg or >> element says to use a profile, but the profile property couldn't be >> read. I'm not sure whether it should say what property group was >> tried. >> It could also explain exactly why the property couldn't be read, >> though >> I presume that would require modifying get_astring_val(). >> > Updated the message and included the scf_error that should be set by get_astring_val() calls to provide a bit more information as to failure reason. >>> > 2073: Is there a reason you didn't include cmdp in this error >>> message? >>> >>> Added. >> >> You should probably use the term "execution profile" like >> getexecattr()'s manpage does. Something like "Could not retrieve >> execution profile \"%s\" for command \"%s\".". I'm not sure >> whether it >> would be a good idea to include "getexecprof()" explicitly. >> > Updated the message > >>> > - 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. >> >> That's better. Did you consider switch()ing on get_uid()'s return >> value >> to explain each error independently? That's probably a substantial >> amount of work that could be deferred to another bug. >> > Did not consider this and will defer to later work. > >>> >lib/librestart/common/librestart.h >>> > struct mc_error: Please add comments for the type, destroy, and >>> size >>> > fields. >>> >>> Added comments. >> >> Thanks. I wonder if for the type field you should say that it's >> usually >> an errno code. The destroy field should be private to the error >> functions, right? I wonder if you should reorganize the variables to >> 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. >> >> David >> Sean Wilcox x79711 303.272.9711 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/smf-discuss/attachments/20090416/04aa00bb/attachment.html>
