Re: [Qemu-devel] [PATCH v3 4/6] qemu-option: support +foo/-foo command line agruments

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 12 Nov 2013 23:39:27 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 12.11.2013 20:58, Igor Mammedov wrote:
> > On Tue, 12 Nov 2013 10:49:58 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > 
> >> On 11/12/2013 01:25 AM, Igor Mammedov wrote:
> >>> On Mon, 11 Nov 2013 13:41:05 +0100
> >>> Andreas Färber <afaerber@suse.de> wrote:
> >>>
> >>>> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
> >>>>> This converts +foo/-foo to "foo=on"/"foo=off" respectively when
> >>>>> QEMU parser is used for the command line options.
> >>>>>
> >>>>> "-cpu" parsers in x86 and other architectures should be unaffected
> >>>>> by this change.
> >>>>>
> >>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>> ---
> >>>>>  util/qemu-option.c | 6 ++++++
> >>>>>  1 file changed, 6 insertions(+)
> >>>>>
> >>>>> diff --git a/util/qemu-option.c b/util/qemu-option.c
> >>>>> index efcb5dc..6c8667c 100644
> >>>>> --- a/util/qemu-option.c
> >>>>> +++ b/util/qemu-option.c
> >>>>> @@ -890,6 +890,12 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
> >>>>>                  if (strncmp(option, "no", 2) == 0) {
> >>>>>                      memmove(option, option+2, strlen(option+2)+1);
> >>>>>                      pstrcpy(value, sizeof(value), "off");
> >>>>> +                } else if (strncmp(option, "-", 1) == 0) {
> >>>>> +                    memmove(option, option+1, strlen(option+1)+1);
> >>>>> +                    pstrcpy(value, sizeof(value), "off");
> >>>>> +                } else if (strncmp(option, "+", 1) == 0) {
> >>>>> +                    memmove(option, option+1, strlen(option+1)+1);
> >>>>> +                    pstrcpy(value, sizeof(value), "on");
> >>>>>                  } else {
> >>>>>                      pstrcpy(value, sizeof(value), "on");
> >>>>>                  }
> >>>>
> >>>> This looks like an interesting idea! However this is much too big a
> >>>> change to just CC ppc folks on...
> >>>>
> >>>> Jan, I wonder if this might break slirp's hostfwd option?
> >>>>
> >>>> Not sure what other options potentially starting with '-' might be
> >>>> affected. Test cases would be a helpful way of demonstrating that this
> >>>> change does not have undesired side effects.
> >>> on x86 there is several value fixups for compatibility reason and a manual
> >>> value parsing in cpu_x86_parse_featurestr(), so above won't just work there.
> >>
> >>
> >> What particular x86 CPU option cannot be handled the way as PPC's "VSX" is
> >> handled two patches below? As I see, even static properties will work there
> >> fine.
> > There is legacy code that is kept for CLI compatibility reasons.
> > Please, look at following features in cpu_x86_parse_featurestr():
> >   xlevel, tsc-freq hv-spinlocks
> 
> Ok, I do not know for sure if static properties support setters/getters
> (they do not if I remember correct) but what does prevent these x86
> properties from being _dynamic_?
nothing, except of:
 * it's better to keep CPU device model clean from legacy hacks so that legacy
   silent fixups of invalid values won't be available via other interfaces
   except of CLI. That will force users to use correct property names/values
   and not break old users that use legacy CLI options.


> 
> > the rest feature flags on x86 should be handled just fine by your patch,
> > once x86properties series is applied. 
> > 
> > that's why we are talking about parser hook that could be overridden
> > by target if necessary.
> 
> This part confuses me the most. I thought I added the hook and I did not
> change other than PPC archs so my patches should have gone quite easily
> to upstream but instead I was told (I think I was but I could
> misunderstand) that other folks may be unhappy that my stuff does not
> support +foo/-foo (which could be added later).
> 
> Could you please point me to the x86properties patch(es) which everybody
> is waiting for? Thanks!
latest is available at
https://github.com/imammedo/qemu/tree/x86-cpu-properties.v10.1
which basically is a rebase with fixed conflicts of v9
http://comments.gmane.org/gmane.comp.emulators.qemu/222284

> > PS:
> > extending QemuOpts to parsing +/-opts format, seems like good workaround
> > above problem. But I was under impression that general movement was to convert
> > custom formats to canonical format "prop=value".
> 
> Heh. I do not understand movements in the qemu project most of the time
> :) I thought I could have added "compat" to PowerPC CPU as others did
> but I was so wrong :)
> 
> 
> 
> -- 
> With best regards
> 
> Alexey Kardashevskiy -- icq: 52150396


-- 
Regards,
  Igor