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@us.ibm.com