This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1657/ |
On May 5th, 2014, 9:26 a.m. CEST, Michal Minar wrote:
Damn! Too late...
I'll make a separate review request.
On May 5th, 2014, 9:25 a.m. CEST, Michal Minar wrote:
src/libs/libopenlmi/openlmi.c (Diff revision 2) 401 data = CMGetKey(o, "CSCreationClassName", &st);402 lmi_check_status(st);403 404 if (CMIsNullValue(data)) {405 CMReturnWithChars(b, CMPI_RC_ERR_FAILED, "CSCreationClassName is empty");406 }
407 prop = lmi_get_string_property_from_objectpath(o, "CSCreationClassName");408 if (strcmp(prop, lmi_get_system_creation_class_name()) != 0) {409 CMReturnWithChars(b, CMPI_RC_ERR_FAILED, "Wrong CSCreationClassName");410 }
411 412 /* check fqdn */
413 data = CMGetKey(o, "CSName", &st);414 lmi_check_status(st);415 if (CMIsNullValue(data)) {416 CMReturnWithChars(b, CMPI_RC_ERR_FAILED, "CSName is empty");417 }
418 prop = lmi_get_string_property_from_objectpath(o, "CSName");419 if (strcmp(prop, lmi_get_system_name()) != 0) {420 CMReturnWithChars(b, CMPI_RC_ERR_FAILED, "Wrong CSName");CSCreationClassName and CSName are specific property names for specific classes. They may be called SystemCreationClassName and SystemName in other classes (like LMI_SoftwareIdentityResource and LMI_AccountManagementService). Could you perhaps add optional csccn_prop_name and csn_prop_name parameters?
I really don't like the names, but didn't come up with anything better:) It would probably be best to make this function general, but... it would probably require more macros and ugliness along the way.
On May 5th, 2014, 9:25 a.m. CEST, Michal Minar wrote:
src/libs/libopenlmi/openlmi.c (Diff revision 2) 408 if (strcmp(prop, lmi_get_system_creation_class_name()) != 0) {What about subclasses or generic CIM_ComputerSystem? I would accept them as well.
I'll try to add that, although I don't see much use to that.
On May 5th, 2014, 9:25 a.m. CEST, Michal Minar wrote:
src/libs/libopenlmi/openlmi.h (Diff revision 2) 203 /* Shortcuts for functions and macros from openlmicommon library. */
openlmicommon -> openlmi
I'll remove that.
On May 5th, 2014, 9:25 a.m. CEST, Michal Minar wrote:
src/libs/libopenlmi/openlmi.h (Diff revision 2) 204 #define get_system_creation_class_name lmi_get_system_creation_class_name
205 #define get_system_name lmi_get_system_name
It disrupts consistency. Do we really need any shortcuts? "lmi_" is just 4 characters long.
I'll remove those.
On May 5th, 2014, 9:25 a.m. CEST, Michal Minar wrote:
src/libs/libopenlmi/openlmi.h (Diff revision 2) 208 #define lmi_check_status(st) \
I'd prefer to have return embedded in name of these functions. *check_something* doesn't look like a function that could terminate current procedure. What about: lmi_return_if_nok lmi_return_if_bad_class_or_nok ?
I've been meaning to do something like that, but couldn't come up with good enough names and then forgot about that... I'll fix those.
On May 5th, 2014, 9:25 a.m. CEST, Michal Minar wrote:
src/libs/libopenlmi/openlmi.h (Diff revision 2) 230 CMPIStatus lmi_check_required(const CMPIBroker *, const CMPIContext *, const CMPIObjectPath *);231 CMPIStatus lmi_check_assoc_class(const CMPIBroker *, const char *, const char *, const char *);Could you add some comments explaining what they do?
Yes.
- Jan
On May 5th, 2014, 8:25 a.m. CEST, Jan Synacek wrote:
Review request for OpenLMI Developers.
By Jan Synacek.
Updated May 5, 2014, 8:25 a.m.
Repository:
openlmi-providers
Description
Diffs
|