On 17.07.2018 19:00, Juan Quintela wrote:
> Thomas Huth <th...@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
