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
|
Home