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

... > > > >> +    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.

> } \ > > 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.

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(a)redhat.com > >> https://www.redhat.com/mailman/listinfo/libvir-list > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list