WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH] xl: refactor common parts of command line parsin

To: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] xl: refactor common parts of command line parsing
From: Andre Przywara <andre.przywara@xxxxxxx>
Date: Fri, 15 Apr 2011 14:44:43 +0200
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Delivery-date: Fri, 15 Apr 2011 05:43:38 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1302860195.5997.92.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4DA7F4CE.40009@xxxxxxx> <1302860195.5997.92.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.14) Gecko/20101020 Thunderbird/3.0.9
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@xxxxxxx>

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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>