1:16 p.m.
Hi Richard,
None of the comment blocks added in this patch match the style of our
other interface documentation. Please see libcmpiutil.h and make sure
your comments are doxygen compliant.
RM> +/*
RM> + * This type defines the signature of the association handler function. The
RM> + * handler function receives the reference of the source class of the
RM> + * association and must map it to a list of CMPIInstance objects (targets
RM> + * of the association).
RM> + *
RM> + * In parameters:
RM> + * CMPIObjectPath * - Path to the source class
RM> + * struct std_assoc_info * - See std_assoc_info comment
RM> + *
RM> + * Out parameters:
RM> + * struct inst_list * - Instances associated to ref
RM> + */
RM> typedef CMPIStatus (*assoc_handler_t)(const CMPIObjectPath *ref,
RM> struct std_assoc_info *info,
RM> struct inst_list *list);
Personally, I think it's a little bizarre to document the behavior of
a function at the definition of the type. This type could be re-used
for functions that perform different tasks. I think the explanation
should be at the place where a specific instance is used, which would
be in std_assoc.
Further, I don't think that the documentation of the parameters adds
any clarity over the code itself. All the other CMPI functions take a
reference, so the meaning of that is well-known. Further, the meaning
of the assoc and assoc_info structures should be easily implied by the
documentation of the structures themselves.
RM> +/*
RM> + * This type defines the signature of the handler which creates the
RM> + * reference. The handler function receives the source object path,
RM> + * and the target instance, so it can create the reference which is returned
RM> + * by the function.
RM> + *
RM> + * In parameters:
RM> + * CMPIObjectPath * - Path to the source class
RM> + * CMPIInstance * - Target instance
RM> + * std_assoc_info - Information from the query that called this function.
RM> + * See std_assoc_info comment for more information.
RM> + * std_assoc - This structure keeps the following necessary info to
RM> + * the creation of the association class instance:
RM> + * association class name and the names of the attributes
RM> + * which will be set into the association class.
RM> + * See the std_assoc comment for more info
RM> + */
RM> typedef CMPIInstance *(*make_ref_t)(const CMPIObjectPath *,
RM> const CMPIInstance *,
RM> struct std_assoc_info *info,
RM> struct std_assoc *);
I think the same arguments apply to this as well. I'd rather add
parameter names to the typedef to both remain consistent with the
above, and to convey meaning.
RM> +/*
RM> + * std_assoc is the definition that the developer puts in their source file.
RM> + * It must be registered using the macro STDA_AssocMIStub.
RM> + *
RM> + * source_class - Defines the list of possible classes that can be passed to
RM> + * the association for this case
RM> + * source_prop - Defines the property of the association class that refers
RM> + * to the input (source class) of this case. This must match
RM> + * that of the schema, and is used for automatic generation of
RM> + * the reference object in the References() or ReferenceNames()
RM> + * operation
RM> + * target_class - Same as source_class, applied for target
RM> + * target_prop - Same as source_prop, applied for target
RM> + * assoc_class - Defines the list of association classes which implement this
RM> + * association
I think the wording here is a bit off. Association classes don't
implement an association. Perhaps this should be something like "A
list of association class names that this handler is able to perform".
In general, I think that you could drop a lot of the text out of the
explanation of the individual members of this structure and add a few
sentences about the semantic meaning of this structure at the top.
Specifically, this structure defines an association relationship,
between a set of source and target classes, through a named (set of)
association classes, and the function handler to do the work.
RM> +/*
RM> + * The std_assoc_info is used to keep information related to the query done
RM> + *
RM> + * assoc_class - Name of the association class
RM> + * result_class - Name of the class of the target
RM> + * role - Name of the property of the source class
RM> + * result_role - Name of the property of the target class
RM> + * properties -
RM> + * context -
RM> + * provider_name - Calling provider name
RM> + */
These structure members are named after the formal CIM association
query components, so I don't think the comments are adding any value
here.
Thanks!
--
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms(a)us.ibm.com