Review Request 351: Indication manager [1/1] Indication manager

Jan Safranek reviewboard at gallagherhome.com
Wed May 15 16:36:08 UTC 2013


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/351/#review411
-----------------------------------------------------------



src/indmanager/ind_manager.h
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment248>

    I would appreciate some text what these callbacks should do, i.e. what an user of ind. manager is supposed to implement. The demo in "Testing Done" doesn't really help.



src/indmanager/ind_manager.h
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment249>

    Please rename those to clearly indicate which one is which.



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment250>

    Minor speedup: you can use snprintf return value and you don't need to count strlen.



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment252>

    CMPIValue is *union*, you can't directly compare any its member before knowing its type. You probably want:
    
    if (type & CMPI_ARRAY) {
          if (!v1->array && !v2->array) {
            return true;
        }
        if (!v1->array || !v2->array) {
            return false;
        }
    }



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment253>

    If I am reading right, for c=2 this will compare items at indexes 0 and -1 -> error.



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment256>

    probably CMGetPropertyCount(i2, NULL);



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment258>

    Please check malloc() result.



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment259>

    Either the comment or the function is wrong, it actually compares namespace and class name.



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment260>

    Missing check for malloc return value.



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment261>

    shouldn't you check CMToArray and CMClone return value?



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment262>

    CMSetProperty has CMPIValue* as the third argument, not CMPIInstance **.
    
    This applies to all CMSetProperty calls below.



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment264>

    This is really clumsy detection of SELECT * FROM... What if there are more whitespaces between SELECT and FROM? Or is the CQL expression guaranteed to be in some canonical state? What about strtok_r()?



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment263>

    Missing malloc check



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment265>

    missing check for NULL?



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment267>

    I am afraid most of CMGet* function calls in this thread will leak memory as they do not run in context of any CIM operation (which would free it upon completion). Double check with valgrind!



src/indmanager/ind_manager.c
<http://reviewboard-openlmi.rhcloud.com/r/351/#comment266>

    You should probably lock the mutex before updating 'running', but bool update should be atomic anyway, so I won't create an issue for it.


Overall, lot of functions defined in the patch return bool, but the return value is never checked. At least something meaningful should be logged.

In addition, the log messages in the patch should be actually helpful, we will get them from people as the only information what went wrong in our providers.

- Jan Safranek


On May 14, 2013, 4:43 p.m., Roman Rakus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard-openlmi.rhcloud.com/r/351/
> -----------------------------------------------------------
> 
> (Updated May 14, 2013, 4:43 p.m.)
> 
> 
> Review request for OpenLMI Developers.
> 
> 
> Repository: openlmi-providers
> 
> 
> Description
> -------
> 
> Indication manager
> 
> Signed-off-by: Roman Rakus <rrakus at redhat.com>
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt f25850e1640f52aed9f18c3bd343d41f7f5f21a4 
>   src/CMakeLists.txt 0cd11d911fb2cf8c26376790c2c9f86cc30d8358 
>   src/indmanager/CMakeLists.txt PRE-CREATION 
>   src/indmanager/ind_manager.h PRE-CREATION 
>   src/indmanager/ind_manager.c PRE-CREATION 
>   src/indmanager/openlmiindmanager.pc.in PRE-CREATION 
> 
> Diff: http://reviewboard-openlmi.rhcloud.com/r/351/diff/
> 
> 
> Testing
> -------
> 
> For testing purposes I used the following provider:
> #include <konkret/konkret.h>
> #include "LMI_AccountInstCreationIndication.h"
> 
> #include "ind_manager.h"
> #include <stdbool.h>
> 
> static const CMPIBroker* _cb = NULL;
> 
> static IMManager *im = NULL;
> IMError im_err = IM_ERR_OK;
> 
> static bool filter_checker(const CMPISelectExp *filter)
> {
>     //TODO - true checking
>     return true;
> }
> 
> // TODO - remove !
> #include <unistd.h>
> static bool spawner(void *data)
> {
>     //TODO - true checking
>     return true;
> }
> 
> // TODO - remove !
> #include <unistd.h>
> static bool spawner(void *data)
> {
>     //TODO - inotify
>     sleep(10);
>     return true;
> }
> 
> static void LMI_AccountInstCreationIndicationInitialize()
> {
>     // startup indication manager
>     im = im_create_manager(NULL, filter_checker, true, spawner,
>                            IM_IND_CREATION, (CMPIBroker *)_cb, &im_err);
> }
> 
> static CMPIStatus LMI_AccountInstCreationIndicationIndicationCleanup(
>     CMPIIndicationMI* mi,
>     const CMPIContext* cc,
>     CMPIBoolean term)
> {
>     // destroy indication manager
>     im_destroy_manager(im, cc, &im_err);
>     CMReturn(CMPI_RC_OK);
> }
> 
> static CMPIStatus LMI_AccountInstCreationIndicationAuthorizeFilter(
>     CMPIIndicationMI* mi,
>     const CMPIContext* cc,
>     const CMPISelectExp* se,
>     const char* ns,
>     const CMPIObjectPath* op,
>     const char* user)
> {
>     // verify filter
>     im_verify_filter(im, se, cc, &im_err);
>     CMReturn(CMPI_RC_OK);
> }
> 
> static CMPIStatus LMI_AccountInstCreationIndicationMustPoll(
>     CMPIIndicationMI* mi,
>     const CMPIContext* cc,
>     const CMPISelectExp* se,
>     const char* ns, 
>     const CMPIObjectPath* op)
> {
>   // yes, this is not supported :(
>     CMReturn(CMPI_RC_ERR_NOT_SUPPORTED);
> }
> 
> static CMPIStatus LMI_AccountInstCreationIndicationActivateFilter(
>     CMPIIndicationMI* mi,
>     const CMPIContext* cc,
>     const CMPISelectExp* se,
>     const char* ns,
>     const CMPIObjectPath* op,
>     CMPIBoolean firstActivation)
> {
>     // verify filter and add it via indication manager
>     im_verify_filter(im, se, cc, &im_err);
>     im_add_filter(im, (CMPISelectExp*)se, cc, &im_err);
>     CMReturn(CMPI_RC_OK);
> }
> 
> static CMPIStatus LMI_AccountInstCreationIndicationDeActivateFilter(
>     CMPIIndicationMI* mi,
>     const CMPIContext* cc,
>     const CMPISelectExp* se,
>     const char* ns,
>     const CMPIObjectPath* op,
>     CMPIBoolean lastActivation)
> {
>     // remove filter from indication manager
>     im_remove_filter(im, se, cc, &im_err);
>     CMReturn(CMPI_RC_OK);
> }
> 
> static CMPIStatus LMI_AccountInstCreationIndicationEnableIndications(
>     CMPIIndicationMI* mi, 
>     const CMPIContext* cc)
> {
>     // start indications
>     im_start_ind(im, cc, &im_err);
>     CMReturn(CMPI_RC_OK);
> }
> 
> static CMPIStatus LMI_AccountInstCreationIndicationDisableIndications(
>     CMPIIndicationMI* mi, 
>     const CMPIContext* cc)
> {
>     // stop indications
>     im_stop_ind(im, cc, &im_err);
>     CMReturn(CMPI_RC_OK);
> }
> 
> CMIndicationMIStub(
>     LMI_AccountInstCreationIndication,
>     LMI_AccountInstCreationIndication, 
>     _cb, 
>     LMI_AccountInstCreationIndicationInitialize())
> 
> KONKRET_REGISTRATION(
>     "root/cimv2",
>     "LMI_AccountInstCreationIndication",
>     "LMI_AccountInstCreationIndication",
>     "indication")
> 
> 
> Thanks,
> 
> Roman Rakus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/openlmi-devel/attachments/20130515/6655e303/attachment-0001.html>


More information about the openlmi-devel mailing list