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. >> 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. Dave
