Re: [PATCH] tools: Refactor code in libxl/xl_cmdimpl.c

[Ian Campbell <Ian.Campbell@citrix.com>]

On Mon, 2014-10-20 at 03:01 +0300, Alexandra Sandulescu wrote:
> This patch removes duplicate code in /tools/libxl/xl_cmdimpl.c by
> adding set_device_data function. This function parses configuration
> data and adds the information into libxl_device_nic struct. It is
> called in both main_networkattach and parse_config_data functions
> to replace duplicate code.

Thanks. There's a couple of procedural things which need sorting and
some pretty minor coding style nits but on the whole this is a good
first submission.

The first thing this needs is a signed-off-by line as described at
http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch.
This is required to ensure that you have agreed to license the code
correctly for the Xen project.

Secondly both the subject line and the new function name should use the
word "network" or "net"/"nic" etc in them to make it clear what they
relate to. e.g. for the subject "tools: xl: refactor code to parse
network device options".

> ---
>  tools/libxl/xl_cmdimpl.c | 160 +++++++++++++++++------------------------------
>  1 file changed, 57 insertions(+), 103 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 988ee28..449aa91 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -823,6 +823,8 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
>      }
>  }
>  
> +static int set_device_data(libxl_device_nic *nic, XLU_Config **config, char *token);

parse_nic_config() perhaps? Or since this appears to deal with a single
foo=bar value perhaps parse_nic_keyvalue()? A doc comment to confirm the
expected behaviour would be good, since the two callers do have subtly
different semantics and expectations. Did you test both code paths?

Generally we prefer to order things such that these forward declarations
of static functions are not needed. IOW you could move the
implementation here instead.

 +[...]
> +                set_device_data(nic, &config, p);

Check the return value?

> +      if (set_device_data(&nic, &config, *argv)) { return 1;}

The xl  coding style (see, tools/libxl/CODING_STYLE) is 4 spaces, not 2
as you have here. The "return"  would normally be on  the next line and
the {}'s aren't needed e.g.:
            if (set_device_data(&nic, &config, *argv))
                return 1;

Although it is suggested in coding style that
            if (set_device_data(&nic, &config, *argv)) return 1;
might be allowable I think this is best avoided when the condition is
not a trivial comparison of an rc variable.

Ian.