Re: [Qemu-devel] [RFC 00/14] More patches to disable stuff

[Thomas Huth <thuth@redhat.com>]

On 17.07.2018 19:00, Juan Quintela wrote:
> Thomas Huth <thuth@redhat.com> wrote:
>> On 17.07.2018 14:04, Juan Quintela wrote:
>>> Hi
>>>
>>> Notice that this is an RFC because they don't work.  As said on my
>>> previous submmision, we need <foo>-softmmu/config-devices.h to make
>>> this work.  This series just allow us to disable the devices, but not
>>> to enable it back O:-)
>>>
>>> Notice:
>>>
>>> - scsi stuff: we are testing they in cdrom-test.c, so we need to be
>>>   able to config them out.  Notice also that #ifdefs only go in tests/<...>
>>>
>>> - virtio stuff: see how we need to also change hw/virtio/virtio-pci.c
>>>   to disable it.  The problem appears in the device-instropect-test.c.
>>>   As they are defined in the binary, but not complied in.  We can
>>>   change for a registration appreach, but that is more work that what
>>>   I intended for this series.
>>>
>>> What do you think?
>>
>> I think this is the wrong way to go. If you add #ifdefs to the sources,
>> you have to make the binaries target-specific. Currently each test
>> binary can work for each target architecture. With #ifdefs, that's not
>> possible anymore. So please don't do that.
> 
> As the system goes now, you have something enabled if it is enabled for
> any _configuration_, that is what config-all-devices.mak is supposed to
> do.

We certainly need this for common, target-independent code. Using
#ifdefs for common code will only cause confusion for people who are not
aware of the common vs. target-specific code compilation yet.

>> If you want to make the tests more flexible for configuration, please
>> use QOM instead to check whether the devices are available or not.
> 
> QOM is the problem, not the solution (TM).  Uninteresting bits deleted.
> 
> tests/device-instrospect-test.c
> 
> static void test_device_intro_concrete(void)
> {
>     ...
>     types = device_type_list(false);
>     ...
> }
> 
> static QList *device_type_list(bool abstract)
> {
>     return qom_list_types("device", abstract);
> }
> 
> static QList *qom_list_types(const char *implements, bool abstract)
> {
>     QDict *resp;
>     QList *ret;
>     QDict *args = qdict_new();
> 
>     qdict_put_bool(args, "abstract", abstract);
>     if (implements) {
>         qdict_put_str(args, "implements", implements);
>     }
>     resp = qmp("{'execute': 'qom-list-types',"
>                " 'arguments': %p }", args);
>     g_assert(qdict_haskey(resp, "return"));
>     ret = qdict_get_qlist(resp, "return");
>     qobject_ref(ret);
>     qobject_unref(resp);
>     return ret;
> }
> 
> If I disable CONFIG_VIRTIO_RNG, then I don't compile
> common-obj-$(CONFIG_VIRTIO_RNG) += virtio-rng.o
> 
> So far so good, but look at virtio-pci.c:
> 
> static void virtio_rng_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> {
>    ...
> }
> 
> static void virtio_rng_pci_class_init(ObjectClass *klass, void *data)
> {
>    ....
> }
> 
> static void virtio_rng_initfn(Object *obj)
> {
>    ...
> }
> 
> static const TypeInfo virtio_rng_pci_info = {
>     .name          = TYPE_VIRTIO_RNG_PCI,
>     .parent        = TYPE_VIRTIO_PCI,
>     .instance_size = sizeof(VirtIORngPCI),
>     .instance_init = virtio_rng_initfn,
>     .class_init    = virtio_rng_pci_class_init,
> };
> 
> static void virtio_pci_register_types(void)
> {
>     type_register_static(&virtio_rng_pci_info);
> ...
> }
> 
> See, we have defined the device "virtio-rng-pci", but there is no
> implementation.  WHen I run device-intronspection-test on that qemu with
> CONFIG_VIRTIO_RNG, it fails to run.  If we can agree that something is
> wrong, then we can search for a solution.

I agree with you that the current situation with virtio-pci. c is bad. I
think we should split it up into individual files instead
(virtio-pci-rng.c etc.).

 Thomas