Re: [PATCH V2] Introducing multipath C API <libdmmp/libdmmp.h>

[Gris Ge <fge@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2942 bytes --]

On Fri, Mar 04, 2016 at 10:06:24AM -0600, Benjamin Marzinski wrote:
> On Fri, Feb 12, 2016 at 04:10:23PM +0800, Gris Ge wrote:
>
> This looks good to me. Personally, I would have loved to see
> multipathd actually passing structured data across the IPC
> connection, and have the multipath client code responsible for
> making it pretty, but making that happen is a lot more invasive.
Hi Benjamin,

Thanks for the review.

I believe Todd is trying to patch multipathd IPC to provide JSON
output like:
    multipathd -k'show topology json'
>
> A couple of thoughts:
>
> I'm wrote a library interface for the multipath IPC code that wasn't
> included the upstream code, although nobody ever voiced a
> disagreement with it, and Hannes sounded supportive of it the last
> time I posted it.
>
> https://www.redhat.com/archives/dm-devel/2015-June/msg00033.html
> https://www.redhat.com/archives/dm-devel/2015-October/msg00062.html
>
> I plan on resending it with my next batch of patches, unless someone
> wants to tell me why it hasn't been accepted before.  It doesn't
> effect your code at all, but assuming that it does get in this time,
> we may want to make some of your IPC functions wrappers around it
> (possibly modifying my library code to work with it better), so that
> we aren't duplicating work.
It's good to remove duplicate code between daemon and client on the
IPC communication.
I will update my patch to unitize libmpath_cmd once it has been
committed.
>
> Also, why use the assert, instead of returning an error? I know some
> library's don't protect you from passing in NULL pointers, and just let
> you segfault.  I didn't find any cases where you would return junk if
> the asserts were disabled, but I didn't follow through all of the logic
> to verify that you wouldn't ever return junk if you passed in junk and
> the asserts were disabled.
Without assert, I have to provide a return code
(for example: DMMP_ERR_INVALID_ARGUMENT) and clean up all possible
messes.

Example:

    int dmmp_mpath_array_get(struct dmmp_context *ctx,
                             struct dmmp_mpath ***dmmp_mps,
                             uint32_t *dmmp_mp_count);

If any of three argument is NULL, I have to set output pointer as
NULL(if not NULL) just in case user use them without checking return
code.

Meanwhile, I have to set error message to indicate so when ctx is not
NULL.

IMHO, using error handling on this simple programmer fault is waste of
time. It seems assert is enough for this.
>
> Lastly, and this is even more nit-picky, in _dmmp_all_get_func_gen,
> why to you need to pass in a specific name for the array and the
> item_count?
It's my bad habit of keeping function prototype identical(including
argument name) to implementation.
I will purge them in V3 patch.
>
> But regardless of these nit-picks, ACK.
>
> -Ben
Best regards.

-- 
Gris Ge

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]