Re: [PATCH] xl: refactor common parts of command line parsing

[Andre Przywara <andre.przywara@amd.com>]

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