Re: [Qemu-devel] [PATCH] add "info ioapic" monitor command

[Gleb Natapov]

On Tue, Dec 29, 2009 at 06:49:14PM -0200, Luiz Capitulino wrote:
> On Tue, 29 Dec 2009 20:49:53 +0200
> Gleb Natapov <address@hidden> wrote:
> 
> > On Tue, Dec 29, 2009 at 03:39:29PM -0200, Luiz Capitulino wrote:
> > > On Tue, 29 Dec 2009 18:53:44 +0200
> > > Gleb Natapov <address@hidden> wrote:
> > > 
> > > > On Tue, Dec 29, 2009 at 02:37:20PM -0200, Luiz Capitulino wrote:
> > > > > On Thu, 24 Dec 2009 17:02:42 +0200
> > > > > Gleb Natapov <address@hidden> wrote:
> > > > > 
> > > > > > +
> > > > > > +static const char *delivery_mode_string[] = {"fixed", "lowprio", 
> > > > > > "smi", "res",
> > > > > > +                                             "nmi", "init", "res", 
> > > > > > "extint"};
> > > > > > +
> > > > > > +void do_info_ioapic(Monitor *mon)
> > > > > > +{
> > > > > > +    int i;
> > > > > > +
> > > > > > +    if (!ioapic)
> > > > > > +        return;
> > > > > > +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> > > > > > +        uint64 e = ioapic->ioredtbl[i];
> > > > > > +        monitor_printf(mon, "%2d: ", i);
> > > > > > +        if (e & IOAPIC_LVT_MASKED) {
> > > > > > +            monitor_printf(mon, "masked\n");
> > > > > > +        } else {
> > > > > > +            uint8_t vec = e & 0xff;
> > > > > > +            uint8_t trig_mode = ((e >> 15) & 1);
> > > > > > +            uint8_t dest = e >> 56;
> > > > > > +            uint8_t dest_mode = (e >> 11) & 1;
> > > > > > +            uint8_t delivery_mode = (e >> 8) & 7;
> > > > > > +            uint8_t polarity = (e >> 13) & 1;
> > > > > > +            monitor_printf(mon, "vec=%3d %s %s acive-%s %s 
> > > > > > dest=%d\n",
> > > > > > +                           vec,
> > > > > > +                           delivery_mode_string[delivery_mode],
> > > > > > +                           dest_mode ? "logical":"physical",
> > > > > > +                           polarity ? "low" : "high",
> > > > > > +                           trig_mode ? "level": "edge",
> > > > > > +                           dest);
> > > > > > +        }
> > > > > > +    }
> > > > > > +}
> > > > > 
> > > > >  New Monitor handlers should return QObjects, monitor_printf()
> > > > > can only be used in the print handler.
> > > > > 
> > > > So I want to add only print handler. This is debug info only, no
> > > > management should ever care about it.
> > > 
> > >  Well, this is possible (at least for now) but there are plans to
> > > have an external shell.
> > > 
> > >  Also, I don't want people to avoid writing handlers because
> > > they feel it's difficult.
> > > 
> > > > >  You can take a look at qemu_chr_info() for an example, as it does
> > > > > what I think you should do here: build a qdict for each pin and
> > > > > return a qlist or return another qdict if it makes sense (or will
> > > > > make in the future) to have more the one ioapic.
> > > > I don't understand a single word from what you are saying :(, but
> > > > qemu_chr_info() looks scary. Actually I don't understand what it does at
> > > > all.
> > > 
> > >  Ouch. :((
> > > 
> > Well, after looking at it for 10 more minutes I can tell that it build
> > some kind of object hierarchy, but it is too low level and requires from
> > casual command writer too deep knowledge of the infrastructure.
> 
>  "too deep" is too strong :) a simple text explaining how to use the
> API would be enough, imho.
> 
May be. May be adding helpers functions to handle common cases would be 
helpful too. For instance iterating over some data structure while creating
qdict object for each one of them and collecting all of them in one list
is used all over the palace as far as I can see.

> > > > >  I'm still thinking in ways to make the work of writing the new
> > > > > Monitor handlers easier..
> > > > Something more simple is definitely needed. If I need to pars the data
> > > > structure to some intermediate language and then parse it back again 
> > > > just to add
> > > > debug command I will not event consider adding it. One more tracepoint
> > > > in the kernel will take 10min to add and will accomplish the same goal
> > > > to me albeit in less elegant way. 
> > > 
> > >  Actually, you're building objects and iterating over them to get them
> > > printed, it's not parsing and it's common to do that in high level
> > > languages.
> > I high level language the syntax hides all the complexity.
> 
>  Right, but the handling of objects is still done by the programmer.
>
The handling of objects is very different when it is not part of the
language.

There is a difference between this:

static void qemu_chr_qlist_iter(QObject *obj, void *opaque)
{
    QDict *chr_dict;
    Monitor *mon = opaque;

    chr_dict = qobject_to_qdict(obj);
    monitor_printf(mon, "%s: filename=%s\n", qdict_get_str(chr_dict, "label"),
                                         qdict_get_str(chr_dict, "filename"));
}

void qemu_chr_info_print(Monitor *mon, const QObject *ret_data)
{
    qlist_iter(qobject_to_qlist(ret_data), qemu_chr_qlist_iter, mon);
}

And this:
 for i in chardev:
   print "%(label)s: filename=%(filename)s" % i

> > >  But I agree that it's not as easy as calling monitor_printf().
> > > 
> > >  Something that is on my TODO list is to add a default printing
> > > function. This will make things easier a bit, but you'll have to
> > > build the objects and the user output won't be pretty.
> > > 
> > Why not have helper function that builds objects for common data
> > structures? For ioapic each entry is an array of strings and integers.
> > Why not have printf like function that will build the object for me?
> 
>  We have qobject_from_jsonf(), it does exactly that.
BTW that is the other question I wanted to ask. What json has to do with
it? As far as I underhand (and I haven't followed QMP discussion
closely) json is used by QMP protocol for outside communication. Why it
is used internally for object creation? Other then that it definitely looks
like the function I want to use. Will try.

> 
>  But I think your point is that the complexity lays in the fact that the
> programmer has to come up with an object hierarchy and keep track of it,
> right?
Keeping track of it is fine. Creating it looks a bit tedious. What if we
had a way to describe what properties the chrdev has and how to extract
them and then common function would iterate over all chrdevs creating
qobject for each one of them and collecting result in a list?


> 
>  I'm looking forward in ways of making the API simple, but delegating this
> work to some mechanism seems magic to me. I mean, having an API which accepts
> an arbitrary data structure and converts it to an object hierarchy.
> 
>  I'm open to suggestions, though.

--
                        Gleb.