Re: [Qemu-devel] [PATCH 04/11] qdev: pcibus_dev_info

[Luiz Capitulino]

On Sat, 26 Dec 2009 21:19:15 +0000
Nathan Baum <address@hidden> wrote:

> This returns a QObject detailing the PCI-specific data about the device.

 Back to this again, turns out I'm converting pci_info() and I think we
should agree on some standards for PCI keys.

> +static QObject *pcibus_dev_info(Monitor *mon, DeviceState *dev)
> +{
> +    PCIDevice *d = (PCIDevice *)dev;
> +    const pci_class_desc *desc;
> +    PCIIORegion *r;
> +    int i, class;
> +    QObject *retval;
> +    QList *regions;
> +    
> +    retval = qobject_from_jsonf("{ 'addr': { 'bus' : %d, 'slot' : %d, 
> 'func': %d }, "
> +                                "  'device': { 'vendor': %d, 'id': %d }, "
> +                                "  'subsystem': { 'vendor': %d, 'id': %d } "
> +                                "}",

 I'm using 'function' instead of 'func' and I have a dict called 'id' with keys
'device' and 'vendor'. Maybe in this case you could call it 'device_info' as
you have subsystem as well.

 These are only general suggestions, it's not always easy to choose them.

> +    class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> +    desc = pci_class_descriptions;
> +    while (desc->desc && class != desc->class)
> +        desc++;
> +    if (desc->desc) {
> +        qdict_put(qobject_to_qdict(retval), "class", 
> qstring_from_str(desc->desc));
> +    } else {
> +        qdict_put(qobject_to_qdict(retval), "class", qint_from_int(class));
> +    }

 We can share code here, I've added a key called 'class_info' for this, it's
a dict with exact the same info ('desc' and 'class').

> +    
> +    regions = qlist_new();
> +    qdict_put(qobject_to_qdict(retval), "regions", regions);
> +
> +    for (i = 0; i < PCI_NUM_REGIONS; i++) {
> +        r = &d->io_regions[i];
> +        if (!r->size)
> +            continue;
> +        qlist_append_obj(regions,
> +                         
> qobject_from_jsonf("{'type':%s,'addr':%d,'size':%d}",
> +                                            r->type & 
> PCI_BASE_ADDRESS_SPACE_IO ? "i/o" : "mem",
> +                                            (int) r->addr,
> +                                            (int) r->size));

 You should never cast to int, you should use PRId64 like this example:

obj = qobject_from_jsonf("{ 'BAR': %d, 'type': 'io', "
                        "'address': %" PRId64 ", "
                        "'current': %" PRId64 " }",
                        i, r->addr, r->addr + r->size - 1);

 Also, I'm using 'io' and 'memory' for the 'type' key, usually I prefer the
full word if it's something simple.

 /me remember to write a 'dict best' practices doc.