[libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest*/nla_put* to make code more readable

[Shi Lei]

On 2018-09-06 at 21:27, Erik Skultety wrote:
>...
>
>> >
>> >> +    if (dummy && nla_put(msg, attrtype, datalen, data) < 0) { \
>> >> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \
>> >> +                       _("nla_put error [" #attrtype "]")); \
>> >> +        return -1; \
>> >> +    } \
>> >> +} while(0)
>> >> +
>> >> +# define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \
>> >> +do { \
>> >> +    const void *dummy = str; \
>> >> +    if (dummy && nla_put_string(msg, attrtype, str) < 0) { \
>> >> +        virReportError(VIR_ERR_INTERNAL_ERROR, \
>> >> +                       _("nla_put_string error [" #attrtype "]: '%s'"), \
>> >> +                       str); \
>> >
>> >I don't think that changing the error is necessary, all of the variants
>> >essentially call nla_put and that one either fails with -EINVAL (not our case,
>> >since we always provide a valid datalen) or -ENOMEM, so the original error
>> >stating that the buffer wasn't large enough is sufficient.
>>
>> Okay. Then I think that the code could be like below:
>>
>> if (dummy && nla_put_string(msg, attrtype, str) < 0) { \
>>     virReportError(VIR_ERR_NO_MEMORY, \
>>                             _("nla_put_string error [" #attrtype "]: '%s'"), \
>>                             str); \
>>     return -ENOMEM; \
>
>VIR_ERR_NO_MEMORY is for allocation errors, this is not an allocation error,
>it's simply that didn't allocate a large enough msg to fit all the payload.
>In fact, this would return: "out of memory: nla_put_string error..." but the
>daemon would continue running happily. This really is an internal error. I
>would also like to drop the per-variant specific error messages to avoid any
>redundancy. One thing that would help when debugging for sure would be to add a
>VIR_DEBUG at the beginning of virNetlinkNewLink in the previous patch, that
>would IMHO help more. 

Okay. I see.

>
>> } \
>>
>> But we should ensure that the return-value of those functions which call nla_put*
>> would accord with their declaration. I find that many of the functions
>> in util/virnetdev.c and util/virnetdev.c always return -1 when they fail, whatever the error.
>> For virNetlinkDumpLink/virNetlinkDelLink, their notes include stuff like below:
>> /* Returns 0 on success, -1 on fatal error. */
>>
>> So I would do some work on these declaration...
>>
>> >
>> >> +        return -1; \
>> >> +    } \
>> >> +} while(0)
>> >
>> >I'm not entirely sold on these MSG_PUT_X macros, but I don't have a reasonable
>> >argument against either, so let's leave them in. Alternatively, we could have
>> >
>> >#define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \
>> >    NETLINK_MSG_PUT(msg, attrtype, strlen(str) + 1, str)
>> >
>> >and that would make things more clean, afterall that is what all the
>> >nla_put_foo will do anyway...
>> >
>> >Erik
>>
>> I understand. Most of the nla_put_foo actually use nla_put simply in current implementation.
>>
>> But I have some other ideas on this point:
>> The authors of the libnl provide many nla_put_foo/nla_get_foo functions for various types
>> (e.g. s8/u8/s16/u16/.../string/msecs/...). Perhaps they are more than convenient helper-functions.
>> I suppose they might want users to use them as far as possible if the users know the attribute type
>> actually, because they would get more information about this attribute payload.
>> There's a chance that in future they could do some extra work in nla_put_foo/nla_get_foo
>> according to the exact type, such as type-check or optimization ...
>>
>> So I think it's useful to provide these NETLINK_MSG_PUT_[type] which just wrap nla_put_[type].
>
>Well, I truly don't want to end up having a NETLINK_MSG_PUT_[type] macro for
>every possible int length and signedness there is in libnl, I'd like to believe
>that NETLINK_MSG_PUT_INT with explicit size requirement should be enough,
>besides, users calling into these nla_put_uintX_t variants already know the
>correct size. Another thing is, that whatever these variants do, should be
>possible with plain nla_put with a little overhead if necessary unless libnl
>explicitly deprecates/discourages the usage of it. The reason why I'm trying to
>keep the number of macros we'll introduce reasonably small, ideally linking one
>to another is to prevent redundancy (the error code path) as well as preserve
>maintainability, otherwise this might just eventually explode into our faces. 

Okay. If there's no need to consider the things I mentioned, then I would like to
return to the macro you written in v1.
Then the only macro NETLINK_MSG_PUT with explicit size would deal with all types
(includes string uint int ...). Yes, it's clear enough.

Shi Lei

>
>Erik
>
>>
>> Thanks for your patience. Have a nice day!  :-)
>>
>> Shi Lei
>>
>> >
>> >> +
>> >> +# define NETLINK_MSG_PUT_U32PTR(msg, attrtype, ptr) \
>> >> +do { \
>> >> +    const void *dummy = ptr; \
>> >> +    if (dummy && nla_put_u32(msg, attrtype, *ptr) < 0) { \
>> >> +        virReportError(VIR_ERR_INTERNAL_ERROR, \
>> >> +                       _("nla_put_u32 error [" #attrtype "]: '%u'"), \
>> >> +                       *ptr); \
>> >> +        return -1; \
>> >> +    } \
>> >> +} while(0)
>> >> +
>> >> +
>> >>  int virNetlinkStartup(void);
>> >>  void virNetlinkShutdown(void);
>> >>
>> >> --
>> >> 2.17.1
>> >>
>> >>
>> >> --
>> >> libvir-list mailing list
>> >> libvir-list at redhat.com
>> >> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list