| Message ID | 20231011070335.14398-24-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | plugins: Allow to read registers | expand |
On Wed, Oct 11, 2023 at 04:03:09PM +0900, Akihiko Odaki wrote: > Make qemu-plugin.h consumable for C++ platform. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > docs/devel/tcg-plugins.rst | 4 ++++ > meson.build | 2 +- > include/qemu/qemu-plugin.h | 4 ++++ > tests/plugin/cc.cc | 16 ++++++++++++++++ > tests/plugin/meson.build | 5 +++++ > tests/tcg/Makefile.target | 3 +-- > 6 files changed, 31 insertions(+), 3 deletions(-) > create mode 100644 tests/plugin/cc.cc > > diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst > index c9f8b27590..984d8012e9 100644 > --- a/docs/devel/tcg-plugins.rst > +++ b/docs/devel/tcg-plugins.rst > @@ -283,6 +283,10 @@ run:: > 160 1 0 > 135 1 0 > > +- contrib/plugins/cc.cc > + > +A pure test plugin to ensure that the plugin API is compatible with C++. > + IMHO we don't need to be adding a test just to prove the existance of the G_BEGIN_DECLS/G_END_DECLS macros in the plugin header. > - contrib/plugins/hotblocks.c > > The hotblocks plugin allows you to examine the where hot paths of > diff --git a/meson.build b/meson.build > index 9f4c4f2f1e..c289bb8948 100644 > --- a/meson.build > +++ b/meson.build > @@ -20,7 +20,7 @@ config_host = keyval.load(meson.current_build_dir() / 'config-host.mak') > > cc = meson.get_compiler('c') > all_languages = ['c'] > -if targetos == 'windows' and add_languages('cpp', required: false, native: false) > +if add_languages('cpp', required: false, native: false) > all_languages += ['cpp'] > cxx = meson.get_compiler('cpp') > endif Our goal has been to entirely eliminate C++ from our build system, even the Windows piece will ideally go away eventually. So I'm loathe to see us expand where we use C++ again, even if only for a unit test. > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > index 40aae8db68..55f514ca6c 100644 > --- a/include/qemu/qemu-plugin.h > +++ b/include/qemu/qemu-plugin.h > @@ -16,6 +16,8 @@ > #include <stdbool.h> > #include <stddef.h> > > +G_BEGIN_DECLS > + > /* > * For best performance, build the plugin with -fvisibility=hidden so that > * QEMU_PLUGIN_LOCAL is implicit. Then, just mark qemu_plugin_install with > @@ -710,4 +712,6 @@ int qemu_plugin_find_register(unsigned int vcpu_index, int file, > */ > int qemu_plugin_read_register(GByteArray *buf, int reg); > > +G_END_DECLS > + > #endif /* QEMU_QEMU_PLUGIN_H */ > diff --git a/tests/plugin/cc.cc b/tests/plugin/cc.cc > new file mode 100644 > index 0000000000..297a7e4f3f > --- /dev/null > +++ b/tests/plugin/cc.cc > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > + > +#include <qemu-plugin.h> > + > +extern "C" { > + > +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; > + > +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, > + const qemu_info_t *info, int argc, > + char **argv) > +{ > + return 0; > +} > + > +}; > diff --git a/tests/plugin/meson.build b/tests/plugin/meson.build > index 322cafcdf6..fed14aa0c2 100644 > --- a/tests/plugin/meson.build > +++ b/tests/plugin/meson.build > @@ -5,6 +5,11 @@ if get_option('plugins') > include_directories: '../../include/qemu', > dependencies: glib) > endforeach > + if 'cpp' in all_languages > + t += shared_module('cc', files('cc.cc'), > + include_directories: '../../include/qemu', > + dependencies: glib) > + endif > endif > if t.length() > 0 > alias_target('test-plugins', t) > diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target > index 462289f47c..66a20d0f2c 100644 > --- a/tests/tcg/Makefile.target > +++ b/tests/tcg/Makefile.target > @@ -145,10 +145,9 @@ RUN_TESTS=$(patsubst %,run-%, $(TESTS)) > > # If plugins exist also include those in the tests > ifeq ($(CONFIG_PLUGIN),y) > -PLUGIN_SRC=$(SRC_PATH)/tests/plugin > PLUGIN_LIB=../../plugin > VPATH+=$(PLUGIN_LIB) > -PLUGINS=$(patsubst %.c, lib%.so, $(notdir $(wildcard $(PLUGIN_SRC)/*.c))) > +PLUGINS=$(notdir $(wildcard $(PLUGIN_LIB)/*.so)) > > # We need to ensure expand the run-plugin-TEST-with-PLUGIN > # pre-requistes manually here as we can't use stems to handle it. We > -- > 2.42.0 > With regards, Daniel
On 2023/10/11 17:51, Daniel P. Berrangé wrote: > On Wed, Oct 11, 2023 at 04:03:09PM +0900, Akihiko Odaki wrote: >> Make qemu-plugin.h consumable for C++ platform. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> docs/devel/tcg-plugins.rst | 4 ++++ >> meson.build | 2 +- >> include/qemu/qemu-plugin.h | 4 ++++ >> tests/plugin/cc.cc | 16 ++++++++++++++++ >> tests/plugin/meson.build | 5 +++++ >> tests/tcg/Makefile.target | 3 +-- >> 6 files changed, 31 insertions(+), 3 deletions(-) >> create mode 100644 tests/plugin/cc.cc >> >> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst >> index c9f8b27590..984d8012e9 100644 >> --- a/docs/devel/tcg-plugins.rst >> +++ b/docs/devel/tcg-plugins.rst >> @@ -283,6 +283,10 @@ run:: >> 160 1 0 >> 135 1 0 >> >> +- contrib/plugins/cc.cc >> + >> +A pure test plugin to ensure that the plugin API is compatible with C++. >> + > > IMHO we don't need to be adding a test just to prove the > existance of the G_BEGIN_DECLS/G_END_DECLS macros in the > plugin header. Strictly speaking, if you include this header file from C++, the code will be interpreted as C++ instead of C but with C linkage. That worries me that the header file may get something not allowed in C++ in the future.
On 11/10/2023 17.48, Akihiko Odaki wrote: > On 2023/10/11 17:51, Daniel P. Berrangé wrote: >> On Wed, Oct 11, 2023 at 04:03:09PM +0900, Akihiko Odaki wrote: >>> Make qemu-plugin.h consumable for C++ platform. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> docs/devel/tcg-plugins.rst | 4 ++++ >>> meson.build | 2 +- >>> include/qemu/qemu-plugin.h | 4 ++++ >>> tests/plugin/cc.cc | 16 ++++++++++++++++ >>> tests/plugin/meson.build | 5 +++++ >>> tests/tcg/Makefile.target | 3 +-- >>> 6 files changed, 31 insertions(+), 3 deletions(-) >>> create mode 100644 tests/plugin/cc.cc >>> >>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst >>> index c9f8b27590..984d8012e9 100644 >>> --- a/docs/devel/tcg-plugins.rst >>> +++ b/docs/devel/tcg-plugins.rst >>> @@ -283,6 +283,10 @@ run:: >>> 160 1 0 >>> 135 1 0 >>> +- contrib/plugins/cc.cc >>> + >>> +A pure test plugin to ensure that the plugin API is compatible with C++. >>> + >> >> IMHO we don't need to be adding a test just to prove the >> existance of the G_BEGIN_DECLS/G_END_DECLS macros in the >> plugin header. > > Strictly speaking, if you include this header file from C++, the code will > be interpreted as C++ instead of C but with C linkage. That worries me that > the header file may get something not allowed in C++ in the future. I think it should be enough if you add the G_BEGIN_DECLS/G_END_DECLS macros here. QEMU is a C project, and it was quite difficult to get rid of the C++ code again, so please don't soften the check in meson.build and don't introduce new .cc files. If you have some code somewhere that uses C++ for plugins, I think it would be better to add a regression test there instead. Thomas
On 2023/10/12 1:21, Thomas Huth wrote: > On 11/10/2023 17.48, Akihiko Odaki wrote: >> On 2023/10/11 17:51, Daniel P. Berrangé wrote: >>> On Wed, Oct 11, 2023 at 04:03:09PM +0900, Akihiko Odaki wrote: >>>> Make qemu-plugin.h consumable for C++ platform. >>>> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> --- >>>> docs/devel/tcg-plugins.rst | 4 ++++ >>>> meson.build | 2 +- >>>> include/qemu/qemu-plugin.h | 4 ++++ >>>> tests/plugin/cc.cc | 16 ++++++++++++++++ >>>> tests/plugin/meson.build | 5 +++++ >>>> tests/tcg/Makefile.target | 3 +-- >>>> 6 files changed, 31 insertions(+), 3 deletions(-) >>>> create mode 100644 tests/plugin/cc.cc >>>> >>>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst >>>> index c9f8b27590..984d8012e9 100644 >>>> --- a/docs/devel/tcg-plugins.rst >>>> +++ b/docs/devel/tcg-plugins.rst >>>> @@ -283,6 +283,10 @@ run:: >>>> 160 1 0 >>>> 135 1 0 >>>> +- contrib/plugins/cc.cc >>>> + >>>> +A pure test plugin to ensure that the plugin API is compatible with >>>> C++. >>>> + >>> >>> IMHO we don't need to be adding a test just to prove the >>> existance of the G_BEGIN_DECLS/G_END_DECLS macros in the >>> plugin header. >> >> Strictly speaking, if you include this header file from C++, the code >> will be interpreted as C++ instead of C but with C linkage. That >> worries me that the header file may get something not allowed in C++ >> in the future. > > I think it should be enough if you add the G_BEGIN_DECLS/G_END_DECLS > macros here. QEMU is a C project, and it was quite difficult to get rid > of the C++ code again, so please don't soften the check in meson.build > and don't introduce new .cc files. > If you have some code somewhere that uses C++ for plugins, I think it > would be better to add a regression test there instead. It doesn't sound right to test the upstream header file in a downstream project. It will take time until the fix becomes readily usable for the downstream even if the downstream finds a bug. What about adding a check for CONFIG_PLUGIN before enabling C++? This will ensure nobody dares introducing C++ code again for code else plugins.
On Thu, Oct 12, 2023 at 01:42:04AM +0900, Akihiko Odaki wrote: > On 2023/10/12 1:21, Thomas Huth wrote: > > On 11/10/2023 17.48, Akihiko Odaki wrote: > > > On 2023/10/11 17:51, Daniel P. Berrangé wrote: > > > > On Wed, Oct 11, 2023 at 04:03:09PM +0900, Akihiko Odaki wrote: > > > > > Make qemu-plugin.h consumable for C++ platform. > > > > > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > > --- > > > > > docs/devel/tcg-plugins.rst | 4 ++++ > > > > > meson.build | 2 +- > > > > > include/qemu/qemu-plugin.h | 4 ++++ > > > > > tests/plugin/cc.cc | 16 ++++++++++++++++ > > > > > tests/plugin/meson.build | 5 +++++ > > > > > tests/tcg/Makefile.target | 3 +-- > > > > > 6 files changed, 31 insertions(+), 3 deletions(-) > > > > > create mode 100644 tests/plugin/cc.cc > > > > > > > > > > diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst > > > > > index c9f8b27590..984d8012e9 100644 > > > > > --- a/docs/devel/tcg-plugins.rst > > > > > +++ b/docs/devel/tcg-plugins.rst > > > > > @@ -283,6 +283,10 @@ run:: > > > > > 160 1 0 > > > > > 135 1 0 > > > > > +- contrib/plugins/cc.cc > > > > > + > > > > > +A pure test plugin to ensure that the plugin API is > > > > > compatible with C++. > > > > > + > > > > > > > > IMHO we don't need to be adding a test just to prove the > > > > existance of the G_BEGIN_DECLS/G_END_DECLS macros in the > > > > plugin header. > > > > > > Strictly speaking, if you include this header file from C++, the > > > code will be interpreted as C++ instead of C but with C linkage. > > > That worries me that the header file may get something not allowed > > > in C++ in the future. > > > > I think it should be enough if you add the G_BEGIN_DECLS/G_END_DECLS > > macros here. QEMU is a C project, and it was quite difficult to get rid [> > of the C++ code again, so please don't soften the check in meson.build > > and don't introduce new .cc files. > > If you have some code somewhere that uses C++ for plugins, I think it > > would be better to add a regression test there instead. > > It doesn't sound right to test the upstream header file in a downstream > project. It will take time until the fix becomes readily usable for the > downstream even if the downstream finds a bug. Essentially QEMU is explicitly saying that C++ support is not a core deliverable of the project. QEMU will accept patches to fix problems but won't put any effort into C++ beyond that. In such a scenario it is appropriate for a downstream to do testing itself. The delay between finding a problem and sending a fix does not need to be big - it could easily be less than a day if there is a nightly CI job that tests against latest QEMU git master. Distributing the testing burden is more scalable as QEMU also does not have the resources to test every scenario it wants to. This kind of situation already exists with the Xen project, which does CI against QEMU on an ongoing basis to identify and report bugs that affect Xen in scenarios which QEMU does not test. Libvirt also runs CI against QEMU to detect regressions in QEMU which impact libvirt's usage of QEMU, that QEMU's own CI won't catch. All that not withstanding, while you are right that someone might accidentally introduce something in the header that is not compatible with C++, the actual chances of this are low. This plugin header file is small, self contained, and is not undergoing a high volume of change. With regards, Daniel
diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst index c9f8b27590..984d8012e9 100644 --- a/docs/devel/tcg-plugins.rst +++ b/docs/devel/tcg-plugins.rst @@ -283,6 +283,10 @@ run:: 160 1 0 135 1 0 +- contrib/plugins/cc.cc + +A pure test plugin to ensure that the plugin API is compatible with C++. + - contrib/plugins/hotblocks.c The hotblocks plugin allows you to examine the where hot paths of diff --git a/meson.build b/meson.build index 9f4c4f2f1e..c289bb8948 100644 --- a/meson.build +++ b/meson.build @@ -20,7 +20,7 @@ config_host = keyval.load(meson.current_build_dir() / 'config-host.mak') cc = meson.get_compiler('c') all_languages = ['c'] -if targetos == 'windows' and add_languages('cpp', required: false, native: false) +if add_languages('cpp', required: false, native: false) all_languages += ['cpp'] cxx = meson.get_compiler('cpp') endif diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h index 40aae8db68..55f514ca6c 100644 --- a/include/qemu/qemu-plugin.h +++ b/include/qemu/qemu-plugin.h @@ -16,6 +16,8 @@ #include <stdbool.h> #include <stddef.h> +G_BEGIN_DECLS + /* * For best performance, build the plugin with -fvisibility=hidden so that * QEMU_PLUGIN_LOCAL is implicit. Then, just mark qemu_plugin_install with @@ -710,4 +712,6 @@ int qemu_plugin_find_register(unsigned int vcpu_index, int file, */ int qemu_plugin_read_register(GByteArray *buf, int reg); +G_END_DECLS + #endif /* QEMU_QEMU_PLUGIN_H */ diff --git a/tests/plugin/cc.cc b/tests/plugin/cc.cc new file mode 100644 index 0000000000..297a7e4f3f --- /dev/null +++ b/tests/plugin/cc.cc @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <qemu-plugin.h> + +extern "C" { + +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; + +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, + const qemu_info_t *info, int argc, + char **argv) +{ + return 0; +} + +}; diff --git a/tests/plugin/meson.build b/tests/plugin/meson.build index 322cafcdf6..fed14aa0c2 100644 --- a/tests/plugin/meson.build +++ b/tests/plugin/meson.build @@ -5,6 +5,11 @@ if get_option('plugins') include_directories: '../../include/qemu', dependencies: glib) endforeach + if 'cpp' in all_languages + t += shared_module('cc', files('cc.cc'), + include_directories: '../../include/qemu', + dependencies: glib) + endif endif if t.length() > 0 alias_target('test-plugins', t) diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target index 462289f47c..66a20d0f2c 100644 --- a/tests/tcg/Makefile.target +++ b/tests/tcg/Makefile.target @@ -145,10 +145,9 @@ RUN_TESTS=$(patsubst %,run-%, $(TESTS)) # If plugins exist also include those in the tests ifeq ($(CONFIG_PLUGIN),y) -PLUGIN_SRC=$(SRC_PATH)/tests/plugin PLUGIN_LIB=../../plugin VPATH+=$(PLUGIN_LIB) -PLUGINS=$(patsubst %.c, lib%.so, $(notdir $(wildcard $(PLUGIN_SRC)/*.c))) +PLUGINS=$(notdir $(wildcard $(PLUGIN_LIB)/*.so)) # We need to ensure expand the run-plugin-TEST-with-PLUGIN # pre-requistes manually here as we can't use stems to handle it. We
Make qemu-plugin.h consumable for C++ platform. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- docs/devel/tcg-plugins.rst | 4 ++++ meson.build | 2 +- include/qemu/qemu-plugin.h | 4 ++++ tests/plugin/cc.cc | 16 ++++++++++++++++ tests/plugin/meson.build | 5 +++++ tests/tcg/Makefile.target | 3 +-- 6 files changed, 31 insertions(+), 3 deletions(-) create mode 100644 tests/plugin/cc.cc