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. > > Sorry about the wait. Here are my comments: > > ctrun.c: > > The addition to the usage message should read: > > [-F fmri] [-A aux] Done > The error messages printed when you fail to set one of these terms > should be similarly changed. Done > svc_fmri and svc_aux can be normal local variables (i.e. there is > no need for them to be static). Done > ctstat.c: > > "svc_fmri ctid" -> "service fmri ctid" Done > > init.c: > > INITTAB_ENTRY_ID_STR_FORMAT should be "%.4s". > > The errors you emit using console() in startd_run should end with > newlines. For consistency, they should also emit the cause. Done > > lsvcrun.c: > > Instead of defining your own LSVC_FMRI_PREFIX, define > SCF_FMRI_LEGACY_PREFIX in libscf_priv.h and update svcs.c to use > the new constant. Done > prepare contract should either use the same path_to_svc_name() > error handling logic as set_legacy_service() or should fail as it > does in the case of other memory allocation failure. As it stands > you can end up with a service that has an fmri of "lrc:". Done. If path_to_svc_name() fails, the Service FMRI for that contract will be lrc:/script > You store the result of the ct_* functions in err. This itself is S> 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. Is there an specific reason to explicit use errno to store the value returned but the library calls? > startd/fork.c: > > I recommend putting the definition of SVC_SULOGIN_FMRI in either > libscf.h or libscf_priv.h. Done. Placed in libscf_priv.h > libcontract.c: > > ct_tmpl_set_internal_string: > > If the size is too large, the kernel will catch it. Don't > double-check it here. Done > ct_tmpl_get_internal: > > This code is wrong in many ways. value points to a 32 bit piece > of memory, yet you are instructing the kernel to write a 64 bit > value to it. And in the end, you store to *value from... *value. > > Instead, pass a pointer to a local variable of type uint64_t to > the kernel, and store its contents to *value at the conclusion of > the function. Done > ct_tmpl_get_internal_string: > > This function doesn't do what it says it does. The contents need > to change, not the name. It should have the prototype: > > int ct_tmpl_get_internal_string(int fd, uint32_t id, > char *buf, size_t size); > > and should behave like ct_pr_tmpl_get_svc_fmri and > ct_pr_tmpl_get_svc_aux currently do. Those functions become > one-liners, and ct_dev_tmpl_get_minor becomes almost as short. Done > ct_tmpl_get_cookie: > > param.ctpm_size should be sizeof (uint64_t), not 0. Done > The kernel already stores the cookie value in *cookie for you. > The line: > > *cookie = *(uint64_t *)param.ctpm_value > > is a no-op and should be removed. Done > 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. > The buffer we copy out in CT_TGET needs to be the MIN of > local_ctpm_size and param.ctpm_size, otherwise we run the risk of > copying out undefined kernel memory. Done > contract/device.c: > > Your string handling assumes ct_dev_tmpl_set_minor() has provided > the string as a buffer and length to be copied in by the common > code, when in fact it uses ct_tmpl_set_internal() and passes in > only a pointer to the string. I'm guessing you intended to update > ct_dev_tmpl_set_minor()? Embarrassing... specially because I tested setting and getting minor. I probably have to clean the buffer between setting and getting the parameter in my test program. Fixed > Assuming you are using the standard means for passing in a string > parameter, str_value will never be NULL. If you are talking about the null check in ctmpl_device_set(), you are right. A null pointer would fail with EFAULT at the copyin of the ctfs_tmpl_ioctl() way before reaching ctmpl_device_set() Test removed Updated webrev at http://cr.opensolaris.org/~acruz/5079353/
