Re: [Qemu-devel] [PATCH 10/19] monitor: Convert do_info_name() to QObject

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 10 Dec 2009 19:02:37 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 12/10/2009 06:54 PM, Luiz Capitulino wrote:
> > On Thu, 10 Dec 2009 18:24:38 +0200
> > Avi Kivity<avi@redhat.com>  wrote:
> >
> >    
> >>> Let me put it another way, I don't think adding null to the json
> >>> parser and incorporating it into this command is a good idea at this
> >>> stage in the release so if we want to do something like this, we need
> >>> to defer it to 0.13.
> >>>
> >>> I agree there are some instances where null could be useful.  I think
> >>> we can get away without it here though.
> >>>        
> >> For 'name', definitely, since it's known to exist.  It would be nice to
> >> have consistency in how features are presented, though.
> >>      
> >   Following what you propose, if it's known to exist then we should
> > never return an empty dict.
> >    
> 
> Right, but if we can't support null (why?) then we can make an exception 
> for 'name'.

 I'm ok with the exception too and the problem with adding null support
now is that it requires some changes to the parser which seem a bit
late to be done.

> >   There are other commands that might require adjustments, for example
> > 'info kvm' has a 'present' key. If QEMU is built w/o KVM support, then
> > this key will be 'false'. Should we return an empty dict then?
> >    
> 
> No.
> 
> >   HPET is another example, currently it's only compiled in if the
> > target is i386. Otherwise the command won't even be available, and
> > we have more commands with conditional features/compilation.
> >
> >    
> 
> That's fine, as long as command availability is discoverable.

 Yes, it's.

> >   So, what I arguably did wrong here was starting the conversion
> > work before defining all these rules.
> >    
> 
> On the other hand, we can often only find out something by implementing 
> it incorrectly first.

 Sure and even being inconsistent a bit I'm quite sure that it's a lot
better than parsing the old user monitor. :)

> >   An option we have is: libvirt actually uses four or five of those
> > info commands. So, we could drop all the rest and guarantee that
> > only those libvirt ones are 100% correct.
> >    
> 
> My worry is with the commands that parse or emit comma-separated option 
> strings.

 I tried to fix all emissions, but we accept it like in the uri
argument of migrate.

 I don't think it's a big deal though, as it's easy to add a new
key for that (if needed) w/o breaking compatibility.