Re: [KVM-AUTOTEST PATCH] KVM test: use command line option wrapper functions

[Feng Yang <fyang@redhat.com>]

----- "Lucas Meneghel Rodrigues" <lmr@redhat.com> wrote:

> From: "Lucas Meneghel Rodrigues" <lmr@redhat.com>
> To: "Michael Goldish" <mgoldish@redhat.com>
> Cc: "Feng Yang" <fyang@redhat.com>, autotest@test.kernel.org, kvm@vger.kernel.org
> Sent: Thursday, May 20, 2010 6:57:23 PM GMT +08:00 Beijing / Chongqing / Hong Kong / Urumqi
> Subject: Re: [KVM-AUTOTEST PATCH] KVM test: use command line option wrapper functions
>
> On Thu, 2010-05-20 at 12:50 +0300, Michael Goldish wrote:
> > On 05/19/2010 11:25 AM, Feng Yang wrote:
> > > Hi, Michael
> > > 
> > > Thanks for your patch.
> > > We plan add "netdev" parameter support in make_qemu_command. 
> Since you are working on this part. Could you add netdev support in
> your patch? hopeful netdev can be default supported in
> make_qemu_command if qemu support it. Thanks very much!
> > 
> > Sure, I'll look into it.
> > 
> > > I think the point of this patch is good and we need this kinds of
> patch.
> > > But I think we need not add so many new function.  Especially some
> function only directly return the string and do nothing more.
> > > This will increase the function call consumption.
> > > 
> > All these helper functions are meant to be extended and modified in
> the
> > future.  They're only there to minimize future effort involved in
> adding
> > support for new command line syntaxes.
> > Right now add_smp() just returns " -smp %s", but in the future we
> may
> > have to support different syntaxes for -smp, and then add_smp()
> will
> > consult the output of 'qemu -help' and return the proper string.
> > What do you mean by function call consumption?  I don't think these
> > functions cause a measurable slowdown, and make_qemu_command() is
> called
> > very few times, so this really isn't a concern IMO.
> 
> Agreed, the wrappers are a good strategy in the case we have to
> support
> different feature sets and syntax.

I know your meaning. Yes, the wrapper is a good strategy for some parameters.
For the new added feature which could not work on old kvm, We need check support 
before using it.  So i say we need this patch. But i still think so many one line function 
is not a good code style.

For many parameters that already support by all the kvm build, I do not think 
it will have big change on syntax later.
Because a mature software should ensure the stability of the interface.
So we need not add one line function for these parameters now.
Even these parameters change its interface later, then we update the code is ok.


Again. Thanks for your patch and comments.


> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html