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]
The error messages printed when you fail to set one of these terms
should be similarly changed.
svc_fmri and svc_aux can be normal local variables (i.e. there is
no need for them to be static).
ctstat.c:
"svc_fmri ctid" -> "service fmri ctid"
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.
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.
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:".
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.
startd/fork.c:
I recommend putting the definition of SVC_SULOGIN_FMRI in either
libscf.h or 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.
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.
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.
ct_tmpl_get_cookie:
param.ctpm_size should be sizeof (uint64_t), not 0.
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.
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.
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.
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()?
Assuming you are using the standard means for passing in a string
parameter, str_value will never be NULL.
Dave
