diff mbox series

[v3,4/6] plugin: define QEMU_PLUGIN_API_IMPLEMENTATION first

Message ID 20201001163429.1348-5-luoyonggang@gmail.com
State New
Headers show
Series Enable plugin support on msys2/mingw | expand

Commit Message

Yonggang Luo Oct. 1, 2020, 4:34 p.m. UTC
This is used to distinguish from the qemu and plugin in
header qemu-plugin.h

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
---
 plugins/api.c  | 1 +
 plugins/core.c | 1 +
 2 files changed, 2 insertions(+)

Comments

Alex Bennée Oct. 5, 2020, 10:44 a.m. UTC | #1
Yonggang Luo <luoyonggang@gmail.com> writes:

> This is used to distinguish from the qemu and plugin in
> header qemu-plugin.h
>
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  plugins/api.c  | 1 +
>  plugins/core.c | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/plugins/api.c b/plugins/api.c
> index bbdc5a4eb4..f16922ca8b 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -35,6 +35,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#define QEMU_PLUGIN_API_IMPLEMENTATION
>  #include "qemu/plugin.h"

This doesn't do anything in this patch. You should split the special
handling in the plugin header and combine it in this patch. Also I'm not
quite sure of the logic you are trying to achieve later on:

  #if !defined(QEMU_PLUGIN_API_IMPLEMENTATION)
  #if defined(QEMU_PLUGIN_IMPLEMENTATION)
  #define QEMU_PLUGIN_EXTERN
  #else
  #define QEMU_PLUGIN_EXTERN extern
  #endif

It seems to me you could get away with only one symbol - ideally just
defined in plugins/api.c so you don't need to churn the plugins with
changes, e.g.

  #ifdef QEMU_PLUGIN_API_IMPLEMENTATION
  #define QEMU_PLUGIN_EXTERN
  #else
  #define QEMU_PLUGIN_EXTERN extern
  #endif

But I'd still like to have a better answer as to why we need the extern?
Is this a limitation of the mingw compiler or some windows special?

>  #include "cpu.h"
>  #include "sysemu/sysemu.h"
> diff --git a/plugins/core.c b/plugins/core.c
> index 51bfc94787..7a79ea4179 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -12,6 +12,7 @@
>   * SPDX-License-Identifier: GPL-2.0-or-later
>   */
>  #include "qemu/osdep.h"
> +#define QEMU_PLUGIN_API_IMPLEMENTATION
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  #include "qapi/error.h"

I don't think we include qemu/plugin.h from this file although it does
raise the question of what happens when other parts of QEMU include the
internal qemu/plugins.h header.
Yonggang Luo Oct. 5, 2020, 3:58 p.m. UTC | #2
Hi, I split this out just for easier review, so the lines changed in api.c
and core.c
equales to the number of function exported, anyway

On Mon, Oct 5, 2020 at 6:44 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Yonggang Luo <luoyonggang@gmail.com> writes:
>
> > This is used to distinguish from the qemu and plugin in
> > header qemu-plugin.h
> >
> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> > ---
> >  plugins/api.c  | 1 +
> >  plugins/core.c | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/plugins/api.c b/plugins/api.c
> > index bbdc5a4eb4..f16922ca8b 100644
> > --- a/plugins/api.c
> > +++ b/plugins/api.c
> > @@ -35,6 +35,7 @@
> >   */
> >
> >  #include "qemu/osdep.h"
> > +#define QEMU_PLUGIN_API_IMPLEMENTATION
> >  #include "qemu/plugin.h"
>
> This doesn't do anything in this patch. You should split the special
> handling in the plugin header and combine it in this patch. Also I'm not
> quite sure of the logic you are trying to achieve later on:
>
>   #if !defined(QEMU_PLUGIN_API_IMPLEMENTATION)
>   #if defined(QEMU_PLUGIN_IMPLEMENTATION)
>   #define QEMU_PLUGIN_EXTERN
>   #else
>   #define QEMU_PLUGIN_EXTERN extern
>   #endif
>
> It seems to me you could get away with only one symbol - ideally just
> defined in plugins/api.c so you don't need to churn the plugins with
> changes, e.g.
>
>   #ifdef QEMU_PLUGIN_API_IMPLEMENTATION
>   #define QEMU_PLUGIN_EXTERN
>   #else
>   #define QEMU_PLUGIN_EXTERN extern
>   #endif
>
> But I'd still like to have a better answer as to why we need the extern?
> Is this a limitation of the mingw compiler or some windows special?
>
> >  #include "cpu.h"
> >  #include "sysemu/sysemu.h"
> > diff --git a/plugins/core.c b/plugins/core.c
> > index 51bfc94787..7a79ea4179 100644
> > --- a/plugins/core.c
> > +++ b/plugins/core.c
> > @@ -12,6 +12,7 @@
> >   * SPDX-License-Identifier: GPL-2.0-or-later
> >   */
> >  #include "qemu/osdep.h"
> > +#define QEMU_PLUGIN_API_IMPLEMENTATION
> >  #include "qemu/error-report.h"
> >  #include "qemu/config-file.h"
> >  #include "qapi/error.h"
>
> I don't think we include qemu/plugin.h from this file although it does
> raise the question of what happens when other parts of QEMU include the
> internal qemu/plugins.h header.
>
> --
> Alex Bennée



--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo
diff mbox series

Patch

diff --git a/plugins/api.c b/plugins/api.c
index bbdc5a4eb4..f16922ca8b 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -35,6 +35,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#define QEMU_PLUGIN_API_IMPLEMENTATION
 #include "qemu/plugin.h"
 #include "cpu.h"
 #include "sysemu/sysemu.h"
diff --git a/plugins/core.c b/plugins/core.c
index 51bfc94787..7a79ea4179 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -12,6 +12,7 @@ 
  * SPDX-License-Identifier: GPL-2.0-or-later
  */
 #include "qemu/osdep.h"
+#define QEMU_PLUGIN_API_IMPLEMENTATION
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 #include "qapi/error.h"