Dave, Please see comments below.
David Powell wrote: > Antonello Cruz wrote: > > Dave, > > > > Thansk for the review. Please see my comments below. > > > > David Powell wrote: > >> Antonello Cruz wrote: > >> >> Antonello Cruz writes: > >> >>> Please review the code for RFE: 5079353 "Contract 'decoration' with > >> >>> service FMRI" > >> >>> > >> >>> webrev: http://cr.opensolaris.org/~acruz/5079353/ > >> >>> workspace: /net/coupe.sfbay/builds/acruz/fmri/ > >> >>> RFE: http://monaco.sfbay.sun.com/detail.jsf?cr=5079353 > >> > > >> > The webrev has been updated to reflect the above changes. > >> > >> lsvcrun.c: > >> > >> You store the result of the ct_* functions in err. This itself is > >> fine, but the subsequent calls to uu_warn and the assert after > >> ct_tmpl_activate are all using an undefined errno value. > > > > errno is set in the library calls and I don't see a reason to set it a > > second time. err is used only to verify if an error occurred. Besides, > > using errno to store the value returned by library calls is generally a > > bad idea. Also, nowhere in the man page for those functions, > > ct_tmpl_activate included, is clear that in case of error the value > > returned is errno. > > If the ct_* library calls set errno, it is an undocumented side > effect that can change at any time. They are defined to return an > error number. > > So no, you can't use errno unless you set it yourself, and yes, all > the documented non-zero return values are error numbers. > > > Is there an specific reason to explicit use errno to > > store the value returned but the library calls? > > The only reason here is because uu_warn, when passed a string that > isn't terminated with a newline, will automatically append a colon > and the output of strerror(errno). You can either assign the return > value of these functions to errno or explicitly add a ": %s\n" and a > strerror(err) to your calls to uu_warn(). > > However, doing that is kinda clunky. Assigning to errno ends up > being a lot cleaner. OK, fixed > >> ctfs_tmpl.c: > >> > >> In CT_TGET, it isn't an error if the user's buffer is larger than > >> CT_PARAM_MAX_SIZE. Instead, since parameters can't be larger than > >> that and we want to prevent arbitrarily large allocations, we > >> should be silently clamping the size. > > > > Agreed, it is better idea than failing. > > You need to set local_ctpm_size after you clamp param.ctpm_size. Done, thanks I also fixed the error message in init.c that we discussed offline updated webrev at: http://cr.opensolaris.org/~acruz/5079353/
