Webrev updated : http://cr.opensolaris.org/~swilcox/webrev.3
David Bustos wrote: > 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 >= ? > > Fixed > 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. > > you are correct updated the calculation > 2095: I suspect you should include the profile's name, but I suppose > it's not a big deal. > > Added the profile name > 2415: Shouldn't you update this comment? > > Comment updated > 2424: Shouldn't we list the possible error codes? > > Comment updated > 2449: Did you consider including the two version numbers in the > message? > > Added... > 2494: This message seems a little bare. What would it return if the > pg was missing? > > This is a case where the the method_exec pg (start, stop, etc.) that was holds the mc data is now not present. Most likely this is a case of SCF_DELETED or some connection issue that come to light at the moment of attempt to find this pg. Simply put, if the start method is being used to the the start pg better be there, or bail out. > 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 > > > -- Sean Wilcox 303.272.9711 x79711
