qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 4/6] plugin: define QEMU_PLUGIN_API_IMPLEMENTATION first


From: Yonggang Luo
Subject: Re: [PATCH v3 4/6] plugin: define QEMU_PLUGIN_API_IMPLEMENTATION first
Date: Mon, 5 Oct 2020 23:58:59 +0800

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]