Hi Goldwyn,
Thanks for review.
Goldwyn Rodrigues wrote:
>>
>> + set_dlm_hookers(); /* get dlm funcs from libdlm_lt.so.3 */
>> +
>
> Universal Comment: Let call it set_dlm_hooks as opposed to hookers.
>
Not sure I understood correctly, the second patch used set_hookers to
call set_dlm_hookers.
>>
>> #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>>
>> +static struct dlm_hookers *dlm_hookers = NULL;
>> +static int is_dlm_hookers_ready = 0;
>
> This should not be required, just checking for dlm_hooks == NULL
> should be enough. This needs to be set accordingly in set_dlm_hooks.
>
is_dlm_hookers_ready is introduced to check the dlm_* functions is
appeared in libdlm_lt.so.3
or not, in case there is problem within the dlm lib.
>> +static struct dlm_lock_resource *dlm_lock_res = NULL;
>> +static int ast_called = 0;
>> +
>> +struct dlm_lock_resource {
>> + dlm_lshandle_t *ls;
>> + struct dlm_lksb lksb;
>> +};
>> +
>> +int is_clustered(struct supertype *st)
>> +{
>> + /* is it a cluster md or not */
>> + if (is_dlm_hookers_ready && st->cluster_name)
>> + return 1;
>> + else
>> + return 0;
>> +}
>> +
>> +/* Using poll(2) to wait for and dispatch ASTs */
>> +static int poll_for_ast(dlm_lshandle_t ls)
>> +{
>> + struct pollfd pfd;
>
> Shouldn't you check dlm_hooks is NULL here? and starting of every
> function which requires dlm_hooks.
>
> Also, a return value from these functions do not mean an error, it
> means the library is not present.
>
I don't think so, because is_dlm_hookers_ready not only ensures
libdlm_lt.so.3 existed and it also make sure
all the needed dlm hookers are set. And
cluster_get_dlmlock/cluster_release_dlmlock/dlm_ast/poll_for_ast
could only be execute while is_dlm_hookers_ready is set to 1.
Thanks,
Guoqing
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
