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.
> >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()?
> > 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.)
> > - 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!
> > 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.
> > 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().
> > 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.
> > - 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.
> >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)?
David
