Xen devel - Apr 2011 - [PATCH] xl: refactor common parts of command line parsing

Hi,

xl command options are currently handled in each command''s sub
function,
leading to a lot of duplicate code.
This patch moves the common part of it into a separate function,
which handles the help switch, unknown options and an insufficient
number of parameters. This removes a lot of redundant code.

Due to the high number of commands this patch is rather large. If that 
would help reviewers, I could split it up, though this would be rather 
artificial. Just tell me.


Signed-off-by: Andre Przywara <andre.przywara@amd.com>

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 2011-04-15 at 08:33 +0100, Andre Przywara wrote:
> Hi,
> 
> xl command options are currently handled in each command''s sub
function,
> leading to a lot of duplicate code.
> This patch moves the common part of it into a separate function,
> which handles the help switch, unknown options and an insufficient
> number of parameters. This removes a lot of redundant code.
> 
> Due to the high number of commands this patch is rather large. If that 
> would help reviewers, I could split it up, though this would be rather 
> artificial. Just tell me.
> 
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
Thanks, I bet the diffstat looks awesome!

One thing I noticed is that the def_getopt function doesn''t add
''h'' to
the optstring, which confused me at first but I see now that you handle
that by handling it in the case where getopt returns ''?'',
clever.

Looks like you also found a few unused options along they way ("n:"
seems to have been cut-and-pasted into a bunch of incorrect places).

Due to the length I''ve not reviewed in great detail but I think we
should just take this and fix up any fallout as it is discovered.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/15/2011 11:36 AM, Ian Campbell wrote:
> On Fri, 2011-04-15 at 08:33 +0100, Andre Przywara wrote:
>> Hi,
>>
>> xl command options are currently handled in each command''s sub
function,
>> leading to a lot of duplicate code.
>> This patch moves the common part of it into a separate function,
>> which handles the help switch, unknown options and an insufficient
>> number of parameters. This removes a lot of redundant code.
>>
>> Due to the high number of commands this patch is rather large. If that
>> would help reviewers, I could split it up, though this would be rather
>> artificial. Just tell me.
>>
>>
>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
Ian,

thanks for the review.
> Thanks, I bet the diffstat looks awesome!
  tools/libxl/xl_cmdimpl.c |  837 
+++++++++-------------------------------------
  1 files changed, 162 insertions(+), 675 deletions(-)
> One thing I noticed is that the def_getopt function doesn''t add
''h'' to
> the optstring, which confused me at first but I see now that you handle
> that by handling it in the case where getopt returns ''?'',
clever.
I tried at least three different approaches, most of them had serious 
drawbacks. But this one looks finally sane.
> Looks like you also found a few unused options along they way
("n:"
> seems to have been cut-and-pasted into a bunch of incorrect places).
I guessed that these would be copy & paste errors. I am not sure if any 
of these were intentionally left in to maintain compatibility with xm, 
but since we only warn on extra options (and don''t break) I decided to 
remove them.
While testing I found some artifacts in the help messages, I will check 
if they are not yet implemented options or if they can go away, too.

One thing I was not sure about is whether we want to break on not 
recognized options. Currently we just warn, which could be easily 
overseen by users, especially with longish outputs.
This is now a trivial one-liner, though.
> Due to the length I''ve not reviewed in great detail but I think we
> should just take this and fix up any fallout as it is discovered.
Sounds fair. I also only tested some selected commands and checked the 
rest with some greps and diffs.

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Andre Przywara writes ("[Xen-devel] [PATCH] xl: refactor common parts of
command line parsing"):
> xl command options are currently handled in each command''s sub
function,
> leading to a lot of duplicate code.
> This patch moves the common part of it into a separate function,
> which handles the help switch, unknown options and an insufficient
> number of parameters. This removes a lot of redundant code.
Thanks, I have applied this.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel