Re: [PATCH iproute2-next v3 8/8] iplink: Reduce number of arguments to iplink_parse()

[Stephen Hemminger <stephen@networkplumber.org>]

[-- Attachment #1: Type: text/plain, Size: 2749 bytes --]

On Mon, 26 Feb 2018 22:09:03 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> Serhey Popovych wrote:
> > David Ahern wrote:  
> >> On 2/26/18 11:20 AM, Serhey Popovych wrote:  
> >>> Stephen Hemminger wrote:  
> >>>> On Thu, 22 Feb 2018 15:02:06 +0200
> >>>> Serhey Popovych <serhe.popovych@gmail.com> wrote:
> >>>>  
> >>>>> +struct iplink_parse_args {
> >>>>> +	const char *dev;
> >>>>> +	const char *name;
> >>>>> +	const char *type;
> >>>>> +
> >>>>> +	/* This definitely must be the last one and initialized
> >>>>> +	 * by the caller of iplink_parse() that will initialize rest.
> >>>>> +	 */
> >>>>> +	struct iplink_req *req;
> >>>>> +};
> >>>>> +  
> >>>>
> >>>> No control block please.  
> >>> Accepted.
> >>>  
> >>>> If you have too many arguments, then that means you need to do
> >>>> some refactoring.  
> >>>
> >>> So using structure as single argument to a function isn't an option?
> >>>  
> >>>>  
> >>>
> >>>  
> >>
> >> As I mentioned before, iplink_parse should not be used by vxcan or veth
> >> as they only want a subset of the parsing. Once you take those users
> >> out, iplink_parse becomes local to iplink.c with a single user. In which
> >> case I suspect the compiler will always inline the function so no
> >> refactoring on the number of arguments is needed.
> >>  
> > I will implement cut down function to parse vxcan and veth peer device
> > parameters and reuse it in iplink_parse() to avoid code duplications.
> > 
> > But my final goal not to refactor on number of arguments to parse,
> > that's side product of this series, I want to take @name, @dev and
> > other parameters for later use. In ->parse_opt() modules @name, @dev
> > and others are not available easily. It seems only way to get them is
> > to parse supplied netlink buffer.
> > 
> >   
> While looking on how to make iplink_parse_light() that will be used with
> vxcan and veth I found following problems:
> 
>   1) need to copy nearly all parameters parsing code (except vf, alias,
>      carrier, master, protodown, link-netnsid, addrgenmode). this will
>      be mitigated by re using iplink_parse_light() in iplink_parse()
> 
>   2) how to add attributes like IFLA_GROUP? in caller? this will give
>      even more code duplications.
> 
>   3) there is high risk of adding regression either via conflicting
>      matches() parameter in userspace or missing kernel attribute
>      previously supported.
> 
> Sorry, I do not like this approach. I do not want to broke anything,
> I just want @name and @dev parameters in ->parse_opt().
> 
> 
>   2)
> >   

If the work to use common code is bigger than the code itself
then why bother. There is no benefit.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]