Message ID | 20201005105442.2093105-2-philmd@redhat.com |
---|---|
State | New |
Headers | show |
Series | qom: Fix missing interface in qemu-storage-daemon | expand |
On Mon, Oct 05, 2020 at 12:54:40PM +0200, Philippe Mathieu-Daudé wrote: > While the FW_CFG_DATA_GENERATOR_INTERFACE is only consumed > by a device only available using system-mode (fw_cfg), it is > implemented by a crypto component (tls-cipher-suites) which > is always available when crypto is used. > > Commit 69699f3055 introduced the following error in the > qemu-storage-daemon binary: > > $ echo -e \ > '{"execute": "qmp_capabilities"}\r\n{"execute": "qom-list-types"}\r\n{"execute": "quit"}\r\n' \ > | storage-daemon/qemu-storage-daemon --chardev stdio,id=qmp0 --monitor qmp0 > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 5}, "package": ""}, "capabilities": ["oob"]}} > {"return": {}} > missing interface 'fw_cfg-data-generator' for object 'tls-creds' > Aborted (core dumped) > > Since QOM dependencies are resolved at runtime, this issue > could not be triggered at linktime, and we don't have test > running the qemu-storage-daemon binary. > > Fix by always registering the QOM interface. > > Reported-by: Kevin Wolf <kwolf@redhat.com> > Fixes: 69699f3055 ("crypto/tls-cipher-suites: Produce fw_cfg consumable blob") > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > I first used: > > +if config_host.has_key('CONFIG_GNUTLS') or have_system > + qom_ss.add(files('fw_cfg_interface.c')) > +endif > > but then realized anything could implement a QOM interface, > so better keep this generic. > --- > hw/nvram/fw_cfg.c | 7 ------- > qom/fw_cfg_interface.c | 15 +++++++++++++++ I feel this should be left in hw/nvram, but still added to qom_ss. The code location should reflect the functional area and maintainership, so we shouldn't move code just to satisfy linkage problems. > MAINTAINERS | 1 + > qom/meson.build | 5 +++++ > 4 files changed, 21 insertions(+), 7 deletions(-) > create mode 100644 qom/fw_cfg_interface.c > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 0e95d057fd..08539a1aab 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -1360,18 +1360,11 @@ static const TypeInfo fw_cfg_mem_info = { > .class_init = fw_cfg_mem_class_init, > }; > > -static const TypeInfo fw_cfg_data_generator_interface_info = { > - .parent = TYPE_INTERFACE, > - .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, > - .class_size = sizeof(FWCfgDataGeneratorClass), > -}; > - > static void fw_cfg_register_types(void) > { > type_register_static(&fw_cfg_info); > type_register_static(&fw_cfg_io_info); > type_register_static(&fw_cfg_mem_info); > - type_register_static(&fw_cfg_data_generator_interface_info); > } > > type_init(fw_cfg_register_types) > diff --git a/qom/fw_cfg_interface.c b/qom/fw_cfg_interface.c > new file mode 100644 > index 0000000000..2b19502ffe > --- /dev/null > +++ b/qom/fw_cfg_interface.c > @@ -0,0 +1,15 @@ > +#include "qemu/osdep.h" > +#include "hw/nvram/fw_cfg.h" > + > +static const TypeInfo fw_cfg_data_generator_interface_info = { > + .parent = TYPE_INTERFACE, > + .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, > + .class_size = sizeof(FWCfgDataGeneratorClass), > +}; > + > +static void fw_cfg_register_types(void) > +{ > + type_register_static(&fw_cfg_data_generator_interface_info); > +} > + > +type_init(fw_cfg_register_types) > diff --git a/MAINTAINERS b/MAINTAINERS > index b76fb31861..9c89d54b41 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2055,6 +2055,7 @@ R: Gerd Hoffmann <kraxel@redhat.com> > S: Supported > F: docs/specs/fw_cfg.txt > F: hw/nvram/fw_cfg.c > +F: qom/fw_cfg_interface.c > F: stubs/fw_cfg.c > F: include/hw/nvram/fw_cfg.h > F: include/standard-headers/linux/qemu_fw_cfg.h > diff --git a/qom/meson.build b/qom/meson.build > index a1cd03c82c..7335f8c8a2 100644 > --- a/qom/meson.build > +++ b/qom/meson.build > @@ -7,6 +7,11 @@ qom_ss.add(files( > 'qom-qobject.c', > )) > > +# interfaces any object might implement > +qom_ss.add(files( > + 'fw_cfg_interface.c', > +)) > + > qmp_ss.add(files('qom-qmp-cmds.c')) > softmmu_ss.add(files('qom-hmp-cmds.c')) > > -- > 2.26.2 > Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 10/05/20 12:54, Philippe Mathieu-Daudé wrote: > While the FW_CFG_DATA_GENERATOR_INTERFACE is only consumed > by a device only available using system-mode (fw_cfg), it is > implemented by a crypto component (tls-cipher-suites) which > is always available when crypto is used. > > Commit 69699f3055 introduced the following error in the > qemu-storage-daemon binary: > > $ echo -e \ > '{"execute": "qmp_capabilities"}\r\n{"execute": "qom-list-types"}\r\n{"execute": "quit"}\r\n' \ > | storage-daemon/qemu-storage-daemon --chardev stdio,id=qmp0 --monitor qmp0 > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 5}, "package": ""}, "capabilities": ["oob"]}} > {"return": {}} > missing interface 'fw_cfg-data-generator' for object 'tls-creds' > Aborted (core dumped) > > Since QOM dependencies are resolved at runtime, this issue > could not be triggered at linktime, O_o I'll defer the review of this work to others with actual QOM knowledge. Please ping me when the series is otherwise ready; I'll be happy to ACK the fw_cfg parts (if any). Laszlo > and we don't have test > running the qemu-storage-daemon binary. > > Fix by always registering the QOM interface. > > Reported-by: Kevin Wolf <kwolf@redhat.com> > Fixes: 69699f3055 ("crypto/tls-cipher-suites: Produce fw_cfg consumable blob") > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > I first used: > > +if config_host.has_key('CONFIG_GNUTLS') or have_system > + qom_ss.add(files('fw_cfg_interface.c')) > +endif > > but then realized anything could implement a QOM interface, > so better keep this generic. > --- > hw/nvram/fw_cfg.c | 7 ------- > qom/fw_cfg_interface.c | 15 +++++++++++++++ > MAINTAINERS | 1 + > qom/meson.build | 5 +++++ > 4 files changed, 21 insertions(+), 7 deletions(-) > create mode 100644 qom/fw_cfg_interface.c > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 0e95d057fd..08539a1aab 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -1360,18 +1360,11 @@ static const TypeInfo fw_cfg_mem_info = { > .class_init = fw_cfg_mem_class_init, > }; > > -static const TypeInfo fw_cfg_data_generator_interface_info = { > - .parent = TYPE_INTERFACE, > - .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, > - .class_size = sizeof(FWCfgDataGeneratorClass), > -}; > - > static void fw_cfg_register_types(void) > { > type_register_static(&fw_cfg_info); > type_register_static(&fw_cfg_io_info); > type_register_static(&fw_cfg_mem_info); > - type_register_static(&fw_cfg_data_generator_interface_info); > } > > type_init(fw_cfg_register_types) > diff --git a/qom/fw_cfg_interface.c b/qom/fw_cfg_interface.c > new file mode 100644 > index 0000000000..2b19502ffe > --- /dev/null > +++ b/qom/fw_cfg_interface.c > @@ -0,0 +1,15 @@ > +#include "qemu/osdep.h" > +#include "hw/nvram/fw_cfg.h" > + > +static const TypeInfo fw_cfg_data_generator_interface_info = { > + .parent = TYPE_INTERFACE, > + .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, > + .class_size = sizeof(FWCfgDataGeneratorClass), > +}; > + > +static void fw_cfg_register_types(void) > +{ > + type_register_static(&fw_cfg_data_generator_interface_info); > +} > + > +type_init(fw_cfg_register_types) > diff --git a/MAINTAINERS b/MAINTAINERS > index b76fb31861..9c89d54b41 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2055,6 +2055,7 @@ R: Gerd Hoffmann <kraxel@redhat.com> > S: Supported > F: docs/specs/fw_cfg.txt > F: hw/nvram/fw_cfg.c > +F: qom/fw_cfg_interface.c > F: stubs/fw_cfg.c > F: include/hw/nvram/fw_cfg.h > F: include/standard-headers/linux/qemu_fw_cfg.h > diff --git a/qom/meson.build b/qom/meson.build > index a1cd03c82c..7335f8c8a2 100644 > --- a/qom/meson.build > +++ b/qom/meson.build > @@ -7,6 +7,11 @@ qom_ss.add(files( > 'qom-qobject.c', > )) > > +# interfaces any object might implement > +qom_ss.add(files( > + 'fw_cfg_interface.c', > +)) > + > qmp_ss.add(files('qom-qmp-cmds.c')) > softmmu_ss.add(files('qom-hmp-cmds.c')) > >
On 10/5/20 3:22 PM, Daniel P. Berrangé wrote: > On Mon, Oct 05, 2020 at 12:54:40PM +0200, Philippe Mathieu-Daudé wrote: >> While the FW_CFG_DATA_GENERATOR_INTERFACE is only consumed >> by a device only available using system-mode (fw_cfg), it is >> implemented by a crypto component (tls-cipher-suites) which >> is always available when crypto is used. >> >> Commit 69699f3055 introduced the following error in the >> qemu-storage-daemon binary: >> >> $ echo -e \ >> '{"execute": "qmp_capabilities"}\r\n{"execute": "qom-list-types"}\r\n{"execute": "quit"}\r\n' \ >> | storage-daemon/qemu-storage-daemon --chardev stdio,id=qmp0 --monitor qmp0 >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 5}, "package": ""}, "capabilities": ["oob"]}} >> {"return": {}} >> missing interface 'fw_cfg-data-generator' for object 'tls-creds' >> Aborted (core dumped) >> >> Since QOM dependencies are resolved at runtime, this issue >> could not be triggered at linktime, and we don't have test >> running the qemu-storage-daemon binary. >> >> Fix by always registering the QOM interface. >> >> Reported-by: Kevin Wolf <kwolf@redhat.com> >> Fixes: 69699f3055 ("crypto/tls-cipher-suites: Produce fw_cfg consumable blob") >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> I first used: >> >> +if config_host.has_key('CONFIG_GNUTLS') or have_system >> + qom_ss.add(files('fw_cfg_interface.c')) >> +endif >> >> but then realized anything could implement a QOM interface, >> so better keep this generic. >> --- >> hw/nvram/fw_cfg.c | 7 ------- >> qom/fw_cfg_interface.c | 15 +++++++++++++++ > > I feel this should be left in hw/nvram, but still added to qom_ss. > > The code location should reflect the functional area and maintainership, > so we shouldn't move code just to satisfy linkage problems. I thought Meson would conditionally include subdir() meson.build like the Makefile based previous buildsys, but I was wrong, all subdir are processed. I'll respin with your suggestion, thanks. Phil.
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 0e95d057fd..08539a1aab 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -1360,18 +1360,11 @@ static const TypeInfo fw_cfg_mem_info = { .class_init = fw_cfg_mem_class_init, }; -static const TypeInfo fw_cfg_data_generator_interface_info = { - .parent = TYPE_INTERFACE, - .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, - .class_size = sizeof(FWCfgDataGeneratorClass), -}; - static void fw_cfg_register_types(void) { type_register_static(&fw_cfg_info); type_register_static(&fw_cfg_io_info); type_register_static(&fw_cfg_mem_info); - type_register_static(&fw_cfg_data_generator_interface_info); } type_init(fw_cfg_register_types) diff --git a/qom/fw_cfg_interface.c b/qom/fw_cfg_interface.c new file mode 100644 index 0000000000..2b19502ffe --- /dev/null +++ b/qom/fw_cfg_interface.c @@ -0,0 +1,15 @@ +#include "qemu/osdep.h" +#include "hw/nvram/fw_cfg.h" + +static const TypeInfo fw_cfg_data_generator_interface_info = { + .parent = TYPE_INTERFACE, + .name = TYPE_FW_CFG_DATA_GENERATOR_INTERFACE, + .class_size = sizeof(FWCfgDataGeneratorClass), +}; + +static void fw_cfg_register_types(void) +{ + type_register_static(&fw_cfg_data_generator_interface_info); +} + +type_init(fw_cfg_register_types) diff --git a/MAINTAINERS b/MAINTAINERS index b76fb31861..9c89d54b41 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2055,6 +2055,7 @@ R: Gerd Hoffmann <kraxel@redhat.com> S: Supported F: docs/specs/fw_cfg.txt F: hw/nvram/fw_cfg.c +F: qom/fw_cfg_interface.c F: stubs/fw_cfg.c F: include/hw/nvram/fw_cfg.h F: include/standard-headers/linux/qemu_fw_cfg.h diff --git a/qom/meson.build b/qom/meson.build index a1cd03c82c..7335f8c8a2 100644 --- a/qom/meson.build +++ b/qom/meson.build @@ -7,6 +7,11 @@ qom_ss.add(files( 'qom-qobject.c', )) +# interfaces any object might implement +qom_ss.add(files( + 'fw_cfg_interface.c', +)) + qmp_ss.add(files('qom-qmp-cmds.c')) softmmu_ss.add(files('qom-hmp-cmds.c'))
While the FW_CFG_DATA_GENERATOR_INTERFACE is only consumed by a device only available using system-mode (fw_cfg), it is implemented by a crypto component (tls-cipher-suites) which is always available when crypto is used. Commit 69699f3055 introduced the following error in the qemu-storage-daemon binary: $ echo -e \ '{"execute": "qmp_capabilities"}\r\n{"execute": "qom-list-types"}\r\n{"execute": "quit"}\r\n' \ | storage-daemon/qemu-storage-daemon --chardev stdio,id=qmp0 --monitor qmp0 {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 5}, "package": ""}, "capabilities": ["oob"]}} {"return": {}} missing interface 'fw_cfg-data-generator' for object 'tls-creds' Aborted (core dumped) Since QOM dependencies are resolved at runtime, this issue could not be triggered at linktime, and we don't have test running the qemu-storage-daemon binary. Fix by always registering the QOM interface. Reported-by: Kevin Wolf <kwolf@redhat.com> Fixes: 69699f3055 ("crypto/tls-cipher-suites: Produce fw_cfg consumable blob") Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- I first used: +if config_host.has_key('CONFIG_GNUTLS') or have_system + qom_ss.add(files('fw_cfg_interface.c')) +endif but then realized anything could implement a QOM interface, so better keep this generic. --- hw/nvram/fw_cfg.c | 7 ------- qom/fw_cfg_interface.c | 15 +++++++++++++++ MAINTAINERS | 1 + qom/meson.build | 5 +++++ 4 files changed, 21 insertions(+), 7 deletions(-) create mode 100644 qom/fw_cfg_interface.c