hw/virtio/virtio-pci.c | 15 ++++++++++++--- tests/Makefile.include | 36 ++++++++++++++++++------------------ tests/bios-tables-test.c | 8 ++++++++ tests/boot-serial-test.c | 2 ++ tests/cdrom-test.c | 10 ++++++++++ tests/display-vga-test.c | 4 ++++ tests/drive_del-test.c | 6 ++++++ tests/usb-hcd-uhci-test.c | 5 ++++- tests/usb-hcd-xhci-test.c | 9 ++++++++- 9 files changed, 72 insertions(+), 23 deletions(-)
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? Later, Juan. Based-on: 20180717113402.5510-1-quintela@redhat.com Juan Quintela (14): check: Only test ipmi when it is compiled in check: Only test megasas when it is compiled in check: Only test lsi when it is compiled in check: Only test esp when it is compiled in check: Only test sga when it is compiled in check: Only test usb smartcard when it is compiled in check: Only test usb storage when it is compiled in check: Only test nvdim_acpi when it is compiled in check: Only test virtio-balloon when it is compiled in check: Only test virtio-serial when it is compiled in check: Only test virtio-rng when it is compiled in check: Only test virtio-gpu when it is compiled in check: Only test virtio-input when it is compiled in check: Only test virtio-scsi when it is compiled in hw/virtio/virtio-pci.c | 15 ++++++++++++--- tests/Makefile.include | 36 ++++++++++++++++++------------------ tests/bios-tables-test.c | 8 ++++++++ tests/boot-serial-test.c | 2 ++ tests/cdrom-test.c | 10 ++++++++++ tests/display-vga-test.c | 4 ++++ tests/drive_del-test.c | 6 ++++++ tests/usb-hcd-uhci-test.c | 5 ++++- tests/usb-hcd-xhci-test.c | 9 ++++++++- 9 files changed, 72 insertions(+), 23 deletions(-) -- 2.17.1
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. If you want to make the tests more flexible for configuration, please use QOM instead to check whether the devices are available or not. Thomas
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.
> 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 send this patches as an RFC
to ask for what people think is the best solution, or if we should even
bother in fix that.
Later, JUan.
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
On Thu, 19 Jul 2018 13:06:59 +0200
Thomas Huth <thuth@redhat.com> wrote:
> On 17.07.2018 19:00, Juan Quintela wrote:
> > 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.).
We should then do the same thing for virtio-ccw as well.
On 19.07.2018 13:45, Cornelia Huck wrote:
> On Thu, 19 Jul 2018 13:06:59 +0200
> Thomas Huth <thuth@redhat.com> wrote:
>
>> On 17.07.2018 19:00, Juan Quintela wrote:
>
>>> 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.).
>
> We should then do the same thing for virtio-ccw as well.
Yes. I can do that if you like.
Thomas
On Thu, 19 Jul 2018 13:58:02 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 19.07.2018 13:45, Cornelia Huck wrote: > > On Thu, 19 Jul 2018 13:06:59 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > >> 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.). > > > > We should then do the same thing for virtio-ccw as well. > > Yes. I can do that if you like. Sure, feel free to go ahead.
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. Emanuele's GSoC project will be able to do this automatically; tests are defined to "consume" some devices, and are not enabled unless the device is present. Thanks, Paolo
Paolo Bonzini <pbonzini@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. > > Emanuele's GSoC project will be able to do this automatically; tests are > defined to "consume" some devices, and are not enabled unless the device > is present. Nice. Thanks. > > Thanks, > > Paolo
© 2016 - 2025 Red Hat, Inc.