[Libvirt-cim] [PATCH] [CU] Commented some structures in the std_association.h file

[Dan Smith]

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