Re: [PATCH 2/3] 9pfs: Use g_new() & friends where that makes obvious sense
On Mon, 14 Mar 2022 17:01:07 +0100
Markus Armbruster wrote:
> g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer,
> for two reasons. One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
>
> This commit only touches allocations with size arguments of the form
> sizeof(T).
>
> Patch created mechanically with:
>
> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>--macro-file scripts/cocci-macro-file.h FILES...
>
> Except this uncovers a typing error:
>
> ../hw/9pfs/9p.c:855:13: warning: incompatible pointer types assigning to
> 'QpfEntry *' from 'QppEntry *' [-Wincompatible-pointer-types]
> val = g_new0(QppEntry, 1);
> ^ ~~~
> 1 warning generated.
>
> Harmless, because QppEntry is larger than QpfEntry. Fix to allocate a
> QpfEntry instead.
>
> Cc: Greg Kurz
> Cc: Christian Schoenebeck
> Signed-off-by: Markus Armbruster
> ---
Reviewed-by: Greg Kurz
> hw/9pfs/9p-proxy.c | 2 +-
> hw/9pfs/9p-synth.c | 4 ++--
> hw/9pfs/9p.c | 8
> hw/9pfs/codir.c | 6 +++---
> tests/qtest/virtio-9p-test.c | 4 ++--
> 5 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index 8b4b5cf7dc..4c5e0fc217 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -1187,7 +1187,7 @@ static int proxy_parse_opts(QemuOpts *opts,
> FsDriverEntry *fs, Error **errp)
>
> static int proxy_init(FsContext *ctx, Error **errp)
> {
> -V9fsProxy *proxy = g_malloc(sizeof(V9fsProxy));
> +V9fsProxy *proxy = g_new(V9fsProxy, 1);
> int sock_id;
>
> if (ctx->export_flags & V9FS_PROXY_SOCK_NAME) {
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index b3080e415b..d99d263985 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -49,7 +49,7 @@ static V9fsSynthNode *v9fs_add_dir_node(V9fsSynthNode
> *parent, int mode,
>
> /* Add directory type and remove write bits */
> mode = ((mode & 0777) | S_IFDIR) & ~(S_IWUSR | S_IWGRP | S_IWOTH);
> -node = g_malloc0(sizeof(V9fsSynthNode));
> +node = g_new0(V9fsSynthNode, 1);
> if (attr) {
> /* We are adding .. or . entries */
> node->attr = attr;
> @@ -128,7 +128,7 @@ int qemu_v9fs_synth_add_file(V9fsSynthNode *parent, int
> mode,
> }
> /* Add file type and remove write bits */
> mode = ((mode & 0777) | S_IFREG);
> -node = g_malloc0(sizeof(V9fsSynthNode));
> +node = g_new0(V9fsSynthNode, 1);
> node->attr = >actual_attr;
> node->attr->inode = synth_node_count++;
> node->attr->nlink = 1;
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index a6d6b3f835..8e9d4aea73 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -324,7 +324,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
> return NULL;
> }
> }
> -f = g_malloc0(sizeof(V9fsFidState));
> +f = g_new0(V9fsFidState, 1);
> f->fid = fid;
> f->fid_type = P9_FID_NONE;
> f->ref = 1;
> @@ -804,7 +804,7 @@ static int qid_inode_prefix_hash_bits(V9fsPDU *pdu, dev_t
> dev)
>
> val = qht_lookup(>s->qpd_table, , hash);
> if (!val) {
> -val = g_malloc0(sizeof(QpdEntry));
> +val = g_new0(QpdEntry, 1);
> *val = lookup;
> affix = affixForIndex(pdu->s->qp_affix_next);
> val->prefix_bits = affix.bits;
> @@ -852,7 +852,7 @@ static int qid_path_fullmap(V9fsPDU *pdu, const struct
> stat *stbuf,
> return -ENFILE;
> }
>
> -val = g_malloc0(sizeof(QppEntry));
> +val = g_new0(QpfEntry, 1);
> *val = lookup;
>
> /* new unique inode and device combo */
> @@ -928,7 +928,7 @@ static int qid_path_suffixmap(V9fsPDU *pdu, const struct
> stat *stbuf,
> return -ENFILE;
> }
>
> -val = g_malloc0(sizeof(QppEntry));
> +val = g_new0(QppEntry, 1);
> *val = lookup;
>
> /* new unique inode affix and device combo */
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index 75148bc985..93ba44fb75 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -141,9 +141,9 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> *fidp,
>
> /* append next node to result chain */
> if (!e) {
> -*entries = e = g_malloc0(sizeof(V9fsDirEnt));
> +
Re: [PATCH-for-5.2 v3 2/4] hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen
On Thu, 5 Nov 2020 13:15:59 +0100
Philippe Mathieu-Daudé wrote:
> On 11/4/20 6:54 PM, Greg Kurz wrote:
> > On Wed, 04 Nov 2020 13:18:09 +0100
> > Christian Schoenebeck wrote:
> >
> >> On Mittwoch, 4. November 2020 12:57:04 CET Philippe Mathieu-Daudé wrote:
> >>> Commit b2c00bce54c ("meson: convert hw/9pfs, cleanup") introduced
> >>> CONFIG_9PFS (probably a wrong conflict resolution). This config is
> >>> not used anywhere. Backends depend on CONFIG_FSDEV_9P which itself
> >>> depends on CONFIG_VIRTFS.
> >>>
> >>> Remove the invalid CONFIG_9PFS and use CONFIG_FSDEV_9P instead, to
> >>> fix the './configure --without-default-devices --enable-xen' build:
> >>>
> >>> /usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function
> >>> `xen_be_register_common': hw/xen/xen-legacy-backend.c:754: undefined
> >>> reference to `xen_9pfs_ops' /usr/bin/ld:
> >>> libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x8): undefined reference
> >>> to
> >>> `local_ops' /usr/bin/ld:
> >>> libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x20): undefined reference
> >>> to `synth_ops' /usr/bin/ld:
> >>> libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x38): undefined reference
> >>> to `proxy_ops' collect2: error: ld returned 1 exit status
> >>>
> >>> Fixes: b2c00bce54c ("meson: convert hw/9pfs, cleanup")
> >>> Suggested-by: Paolo Bonzini
> >>> Acked-by: Greg Kurz
> >>> Tested-by: Greg Kurz
> >>> Signed-off-by: Philippe Mathieu-Daudé
> >>
> >> Acked-by: Christian Schoenebeck
> >>
> >
> > Phil,
> >
> > Same questioning as Connie. Do you intend to get this merged or should
> > Christian or I take care of that ?
>
> Same answer too =) If you are preparing a pull request, please go ahead!
>
Heh I've just seen your answer.
Christian,
Maybe you can add this patch in your next PR ?
> Thanks,
>
> Phil.
>
Re: [PATCH-for-5.2 v3 2/4] hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen
On Wed, 04 Nov 2020 13:18:09 +0100
Christian Schoenebeck wrote:
> On Mittwoch, 4. November 2020 12:57:04 CET Philippe Mathieu-Daudé wrote:
> > Commit b2c00bce54c ("meson: convert hw/9pfs, cleanup") introduced
> > CONFIG_9PFS (probably a wrong conflict resolution). This config is
> > not used anywhere. Backends depend on CONFIG_FSDEV_9P which itself
> > depends on CONFIG_VIRTFS.
> >
> > Remove the invalid CONFIG_9PFS and use CONFIG_FSDEV_9P instead, to
> > fix the './configure --without-default-devices --enable-xen' build:
> >
> > /usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function
> > `xen_be_register_common': hw/xen/xen-legacy-backend.c:754: undefined
> > reference to `xen_9pfs_ops' /usr/bin/ld:
> > libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x8): undefined reference to
> > `local_ops' /usr/bin/ld:
> > libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x20): undefined reference
> > to `synth_ops' /usr/bin/ld:
> > libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x38): undefined reference
> > to `proxy_ops' collect2: error: ld returned 1 exit status
> >
> > Fixes: b2c00bce54c ("meson: convert hw/9pfs, cleanup")
> > Suggested-by: Paolo Bonzini
> > Acked-by: Greg Kurz
> > Tested-by: Greg Kurz
> > Signed-off-by: Philippe Mathieu-Daudé
>
> Acked-by: Christian Schoenebeck
>
Phil,
Same questioning as Connie. Do you intend to get this merged or should
Christian or I take care of that ?
> > ---
> > v2: Reworded description (Greg)
> >
> > Cc: Stefano Stabellini
> > Cc: Anthony Perard
> > Cc: Paul Durrant
> > Cc: xen-devel@lists.xenproject.org
> > Cc: Greg Kurz
> > Cc: Christian Schoenebeck
> > ---
> > hw/9pfs/Kconfig | 4
> > hw/9pfs/meson.build | 2 +-
> > 2 files changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/hw/9pfs/Kconfig b/hw/9pfs/Kconfig
> > index d3ebd737301..3ae57496613 100644
> > --- a/hw/9pfs/Kconfig
> > +++ b/hw/9pfs/Kconfig
> > @@ -2,12 +2,8 @@ config FSDEV_9P
> > bool
> > depends on VIRTFS
> >
> > -config 9PFS
> > -bool
> > -
> > config VIRTIO_9P
> > bool
> > default y
> > depends on VIRTFS && VIRTIO
> > select FSDEV_9P
> > -select 9PFS
> > diff --git a/hw/9pfs/meson.build b/hw/9pfs/meson.build
> > index cc094262122..99be5d91196 100644
> > --- a/hw/9pfs/meson.build
> > +++ b/hw/9pfs/meson.build
> > @@ -15,6 +15,6 @@
> >'coxattr.c',
> > ))
> > fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c'))
> > -softmmu_ss.add_all(when: 'CONFIG_9PFS', if_true: fs_ss)
> > +softmmu_ss.add_all(when: 'CONFIG_FSDEV_9P', if_true: fs_ss)
> >
> > specific_ss.add(when: 'CONFIG_VIRTIO_9P', if_true:
> > files('virtio-9p-device.c'))
>
> Best regards,
> Christian Schoenebeck
>
>
Re: [PATCH-for-5.2 v2 2/4] hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen
On Wed, 4 Nov 2020 09:43:25 +0100
Philippe Mathieu-Daudé wrote:
> Fixes './configure --without-default-devices --enable-xen' build:
>
> /usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function
> `xen_be_register_common':
> hw/xen/xen-legacy-backend.c:754: undefined reference to `xen_9pfs_ops'
> /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x8): undefined
> reference to `local_ops'
> /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x20):
> undefined reference to `synth_ops'
> /usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x38):
> undefined reference to `proxy_ops'
> collect2: error: ld returned 1 exit status
>
> Fixes: b2c00bce54c ("meson: convert hw/9pfs, cleanup")
> Suggested-by: Paolo Bonzini
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> I'm not sure b2c00bce54c is the real culprit
>
FWIW this commit introduced the 9PFS config which isn't used
anywhere. Backends depend on FSDEV_9P which itself depends
on VIRTFS. So I tend to think b2c00bce54c is the culprit
but _of couse_ I could be wrong :)
Anyway, this patch (+ patch 1) fix the build break mentioned
in the changelog so:
Acked-by: Greg Kurz
Tested-by: Greg Kurz
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: Paul Durrant
> Cc: xen-devel@lists.xenproject.org
> Cc: Greg Kurz
> Cc: Christian Schoenebeck
> ---
> hw/9pfs/Kconfig | 4
> hw/9pfs/meson.build | 2 +-
> 2 files changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/hw/9pfs/Kconfig b/hw/9pfs/Kconfig
> index d3ebd737301..3ae57496613 100644
> --- a/hw/9pfs/Kconfig
> +++ b/hw/9pfs/Kconfig
> @@ -2,12 +2,8 @@ config FSDEV_9P
> bool
> depends on VIRTFS
>
> -config 9PFS
> -bool
> -
> config VIRTIO_9P
> bool
> default y
> depends on VIRTFS && VIRTIO
> select FSDEV_9P
> -select 9PFS
> diff --git a/hw/9pfs/meson.build b/hw/9pfs/meson.build
> index cc094262122..99be5d91196 100644
> --- a/hw/9pfs/meson.build
> +++ b/hw/9pfs/meson.build
> @@ -15,6 +15,6 @@
>'coxattr.c',
> ))
> fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c'))
> -softmmu_ss.add_all(when: 'CONFIG_9PFS', if_true: fs_ss)
> +softmmu_ss.add_all(when: 'CONFIG_FSDEV_9P', if_true: fs_ss)
>
> specific_ss.add(when: 'CONFIG_VIRTIO_9P', if_true:
> files('virtio-9p-device.c'))
Re: [PATCH v10 1/9] error: auto propagated local_err
On Wed, 24 Jun 2020 18:53:05 +0200 Markus Armbruster wrote: > Greg Kurz writes: > > > On Mon, 15 Jun 2020 07:21:03 +0200 > > Markus Armbruster wrote: > > > >> Greg Kurz writes: > >> > >> > On Tue, 17 Mar 2020 18:16:17 +0300 > >> > Vladimir Sementsov-Ogievskiy wrote: > >> > > >> >> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of > >> >> functions with an errp OUT parameter. > >> >> > >> >> It has three goals: > >> >> > >> >> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user > >> >> can't see this additional information, because exit() happens in > >> >> error_setg earlier than information is added. [Reported by Greg Kurz] > >> >> > >> > > >> > I have more of these coming and I'd really like to use > >> > ERRP_AUTO_PROPAGATE. > >> > > >> > It seems we have a consensus on the macro itself but this series is gated > >> > by the conversion of the existing code base. > >> > > >> > What about merging this patch separately so that people can start using > >> > it at least ? > >> > >> Please give me a few more days to finish the work I feel should go in > >> before the conversion. With any luck, Vladimir can then rebase / > >> recreate the conversion easily, and you can finally use the macro for > >> your own work. > >> > > > > Sure. Thanks. > > Just posted "[PATCH 00/46] Less clumsy error checking". The sheer size > of the thing and the length of its dependency chain explains why it took > me so long. I feel bad about delaying you all the same. Apologies! > No problem. This series of yours is impressive. Putting an end to the highjacking of the Error ** argument is really a beneficial move. > I hope we can converge quickly enough to get Vladimir's work on top > ready in time for the soft freeze. > I'll find some cycles for reviewing. Cheers, -- Greg
Re: [PATCH v10 1/9] error: auto propagated local_err
On Mon, 15 Jun 2020 07:21:03 +0200 Markus Armbruster wrote: > Greg Kurz writes: > > > On Tue, 17 Mar 2020 18:16:17 +0300 > > Vladimir Sementsov-Ogievskiy wrote: > > > >> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of > >> functions with an errp OUT parameter. > >> > >> It has three goals: > >> > >> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user > >> can't see this additional information, because exit() happens in > >> error_setg earlier than information is added. [Reported by Greg Kurz] > >> > > > > I have more of these coming and I'd really like to use ERRP_AUTO_PROPAGATE. > > > > It seems we have a consensus on the macro itself but this series is gated > > by the conversion of the existing code base. > > > > What about merging this patch separately so that people can start using > > it at least ? > > Please give me a few more days to finish the work I feel should go in > before the conversion. With any luck, Vladimir can then rebase / > recreate the conversion easily, and you can finally use the macro for > your own work. > Sure. Thanks.
Re: [PATCH v10 1/9] error: auto propagated local_err
On Tue, 17 Mar 2020 18:16:17 +0300 Vladimir Sementsov-Ogievskiy wrote: > Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of > functions with an errp OUT parameter. > > It has three goals: > > 1. Fix issue with error_fatal and error_prepend/error_append_hint: user > can't see this additional information, because exit() happens in > error_setg earlier than information is added. [Reported by Greg Kurz] > I have more of these coming and I'd really like to use ERRP_AUTO_PROPAGATE. It seems we have a consensus on the macro itself but this series is gated by the conversion of the existing code base. What about merging this patch separately so that people can start using it at least ? > 2. Fix issue with error_abort and error_propagate: when we wrap > error_abort by local_err+error_propagate, the resulting coredump will > refer to error_propagate and not to the place where error happened. > (the macro itself doesn't fix the issue, but it allows us to [3.] drop > the local_err+error_propagate pattern, which will definitely fix the > issue) [Reported by Kevin Wolf] > > 3. Drop local_err+error_propagate pattern, which is used to workaround > void functions with errp parameter, when caller wants to know resulting > status. (Note: actually these functions could be merely updated to > return int error code). > > To achieve these goals, later patches will add invocations > of this macro at the start of functions with either use > error_prepend/error_append_hint (solving 1) or which use > local_err+error_propagate to check errors, switching those > functions to use *errp instead (solving 2 and 3). > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Paul Durrant > Reviewed-by: Greg Kurz > Reviewed-by: Eric Blake > --- > > Cc: Eric Blake > Cc: Kevin Wolf > Cc: Max Reitz > Cc: Greg Kurz > Cc: Christian Schoenebeck > Cc: Stefan Hajnoczi > Cc: Stefano Stabellini > Cc: Anthony Perard > Cc: Paul Durrant > Cc: "Philippe Mathieu-Daudé" > Cc: Laszlo Ersek > Cc: Gerd Hoffmann > Cc: Stefan Berger > Cc: Markus Armbruster > Cc: Michael Roth > Cc: qemu-de...@nongnu.org > Cc: qemu-bl...@nongnu.org > Cc: xen-devel@lists.xenproject.org > > include/qapi/error.h | 205 --- > 1 file changed, 173 insertions(+), 32 deletions(-) > > diff --git a/include/qapi/error.h b/include/qapi/error.h > index ad5b6e896d..30140d9bfe 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -15,6 +15,8 @@ > /* > * Error reporting system loosely patterned after Glib's GError. > * > + * = Deal with Error object = > + * > * Create an error: > * error_setg(, "situation normal, all fouled up"); > * > @@ -47,28 +49,91 @@ > * reporting it (primarily useful in testsuites): > * error_free_or_abort(); > * > - * Pass an existing error to the caller: > - * error_propagate(errp, err); > - * where Error **errp is a parameter, by convention the last one. > + * = Deal with Error ** function parameter = > * > - * Pass an existing error to the caller with the message modified: > - * error_propagate_prepend(errp, err); > + * A function may use the error system to return errors. In this case, the > + * function defines an Error **errp parameter, by convention the last one > (with > + * exceptions for functions using ... or va_list). > * > - * Avoid > - * error_propagate(errp, err); > - * error_prepend(errp, "Could not frobnicate '%s': ", name); > - * because this fails to prepend when @errp is _fatal. > + * The caller may then pass in the following errp values: > * > - * Create a new error and pass it to the caller: > + * 1. _abort > + *Any error will result in abort(). > + * 2. _fatal > + *Any error will result in exit() with a non-zero status. > + * 3. NULL > + *No error reporting through errp parameter. > + * 4. The address of a NULL-initialized Error *err > + *Any error will populate errp with an error object. > + * > + * The following rules then implement the correct semantics desired by the > + * caller. > + * > + * Create a new error to pass to the caller: > * error_setg(errp, "situation normal, all fouled up"); > * > - * Call a function and receive an error from it: > + * Calling another errp-based function: > + * f(..., errp); > + * > + * == Checking success of subcall == > + * > + * If a function returns a value indicating an error in addition to setting > + * errp (which is recommended), then you don't need any additional code, just > + * do: > + * > + * int ret = f(..., errp); > + *
Re: [Xen-devel] [PATCH v8 01/10] error: auto propagated local_err
On Fri, 6 Mar 2020 08:15:27 +0300
Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with an errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort and error_propagate: when we wrap
> error_abort by local_err+error_propagate, the resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
> the local_err+error_propagate pattern, which will definitely fix the
> issue) [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, later patches will add invocations
> of this macro at the start of functions with either use
> error_prepend/error_append_hint (solving 1) or which use
> local_err+error_propagate to check errors, switching those
> functions to use *errp instead (solving 2 and 3).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>
Thanks for this impressive work Vladimir !
Reviewed-by: Greg Kurz
> Cc: Eric Blake
> Cc: Kevin Wolf
> Cc: Max Reitz
> Cc: Greg Kurz
> Cc: Christian Schoenebeck
> Cc: Stefano Stabellini
> Cc: Anthony Perard
> Cc: Paul Durrant
> Cc: Stefan Hajnoczi
> Cc: "Philippe Mathieu-Daudé"
> Cc: Laszlo Ersek
> Cc: Gerd Hoffmann
> Cc: Stefan Berger
> Cc: Markus Armbruster
> Cc: Michael Roth
> Cc: qemu-bl...@nongnu.org
> Cc: qemu-de...@nongnu.org
> Cc: xen-devel@lists.xenproject.org
>
> include/qapi/error.h | 203 ---
> 1 file changed, 170 insertions(+), 33 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index ad5b6e896d..bb9bcf02fb 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -15,6 +15,8 @@
> /*
> * Error reporting system loosely patterned after Glib's GError.
> *
> + * = Deal with Error object =
> + *
> * Create an error:
> * error_setg(, "situation normal, all fouled up");
> *
> @@ -47,28 +49,88 @@
> * reporting it (primarily useful in testsuites):
> * error_free_or_abort();
> *
> - * Pass an existing error to the caller:
> - * error_propagate(errp, err);
> - * where Error **errp is a parameter, by convention the last one.
> + * = Deal with Error ** function parameter =
> *
> - * Pass an existing error to the caller with the message modified:
> - * error_propagate_prepend(errp, err);
> + * Function may use error system to return errors. In this case function
> + * defines Error **errp parameter, which should be the last one (except for
> + * functions which varidic argument list), which has the following API:
> *
> - * Avoid
> - * error_propagate(errp, err);
> - * error_prepend(errp, "Could not frobnicate '%s': ", name);
> - * because this fails to prepend when @errp is _fatal.
> + * Caller may pass as errp:
> + * 1. _abort
> + *This means abort on any error
> + * 2. _fatal
> + *Exit with non-zero return code on error
> + * 3. NULL
> + *Ignore errors
> + * 4. Another value
> + *On error allocate error object and set errp
> *
> - * Create a new error and pass it to the caller:
> - * error_setg(errp, "situation normal, all fouled up");
> + * Error API functions with Error ** (like error_setg) argument supports
> these
> + * rules, so user functions just need to use them appropriately (read below).
> *
> - * Call a function and receive an error from it:
> + * Simple pass error to the caller:
> + * error_setg(errp, "Some error");
> + *
> + * Subcall of another errp-based function, passing the error to the caller
> + * f(..., errp);
> + *
> + * == Checking success of subcall ==
> + *
> + * If function returns error code in addition to errp (which is recommended),
> + * you don't need any additional code, just do:
> + * int ret = f(..., errp);
> + * if (ret < 0) {
> + * ... handle error ...
> + * return ret;
> + * }
> + *
> + * If function returns nothing (which is not recommended API) and the only
> way
> + * to check success is checking errp, we must care about cases [1-3] above.
> We
> + * need to use
Re: [Xen-devel] [PATCH v6 02/11] error: auto propagated local_err
On Fri, 10 Jan 2020 22:41:49 +0300
Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>
LGTM
Reviewed-by: Greg Kurz
> CC: Cornelia Huck
> CC: Eric Blake
> CC: Kevin Wolf
> CC: Max Reitz
> CC: Greg Kurz
> CC: Stefan Hajnoczi
> CC: Stefano Stabellini
> CC: Anthony Perard
> CC: Paul Durrant
> CC: "Philippe Mathieu-Daudé"
> CC: Laszlo Ersek
> CC: Gerd Hoffmann
> CC: Stefan Berger
> CC: Markus Armbruster
> CC: Michael Roth
> CC: qemu-bl...@nongnu.org
> CC: xen-devel@lists.xenproject.org
>
> include/qapi/error.h | 84 +++-
> 1 file changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index fa8d51fd6d..532b9afb9e 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -78,7 +78,7 @@
> * Call a function treating errors as fatal:
> * foo(arg, _fatal);
> *
> - * Receive an error and pass it on to the caller:
> + * Receive an error and pass it on to the caller (DEPRECATED*):
> * Error *err = NULL;
> * foo(arg, );
> * if (err) {
> @@ -98,6 +98,50 @@
> * foo(arg, errp);
> * for readability.
> *
> + * DEPRECATED* This pattern is deprecated now, use ERRP_AUTO_PROPAGATE macro
> + * instead (defined below).
> + * It's deprecated because of two things:
> + *
> + * 1. Issue with error_abort & error_propagate: when we wrap error_abort by
> + * local_err+error_propagate, resulting coredump will refer to
> error_propagate
> + * and not to the place where error happened.
> + *
> + * 2. A lot of extra code of the same pattern
> + *
> + * How to update old code to use ERRP_AUTO_PROPAGATE?
> + *
> + * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
> + * than you may safely dereference errp to check errors and do not need any
> + * additional local Error variables or calls to error_propagate().
> + *
> + * Example:
> + *
> + * old code
> + *
> + * void fn(..., Error **errp) {
> + * Error *err = NULL;
> + * foo(arg, );
> + * if (err) {
> + * handle the error...
> + * error_propagate(errp, err);
> + * return;
> + * }
> + * ...
> + * }
> + *
> + * updated code
> + *
> + * void fn(..., Error **errp) {
> + * ERRP_AUTO_PROPAGATE();
> + * foo(arg, errp);
> + * if (*errp) {
> + * handle the error...
> + * return;
> + * }
> + * ...
> + * }
> + *
> + *
> * Receive and accumulate multiple errors (first one wins):
> * Error *err = NULL, *local_err = NULL;
> * foo(arg, );
> @@ -348,6 +392,44 @@ void error_set_internal(Error **errp,
> ErrorClass err_class, const char *fmt, ...)
> GCC_FMT_ATTR(6, 7);
>
> +typedef struct ErrorPropagator {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the fi
Re: [Xen-devel] [PATCH v6 01/11] qapi/error: add (Error **errp) cleaning APIs
On Fri, 10 Jan 2020 22:41:48 +0300
Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>
Reviewed-by: Greg Kurz
> CC: Cornelia Huck
> CC: Eric Blake
> CC: Kevin Wolf
> CC: Max Reitz
> CC: Greg Kurz
> CC: Stefan Hajnoczi
> CC: Stefano Stabellini
> CC: Anthony Perard
> CC: Paul Durrant
> CC: "Philippe Mathieu-Daudé"
> CC: Laszlo Ersek
> CC: Gerd Hoffmann
> CC: Stefan Berger
> CC: Markus Armbruster
> CC: Michael Roth
> CC: qemu-bl...@nongnu.org
> CC: xen-devel@lists.xenproject.org
>
> include/qapi/error.h | 26 ++
> 1 file changed, 26 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index ad5b6e896d..fa8d51fd6d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -309,6 +309,32 @@ void warn_reportf_err(Error *err, const char *fmt, ...)
> void error_reportf_err(Error *err, const char *fmt, ...)
> GCC_FMT_ATTR(2, 3);
>
> +/*
> + * Functions to clean Error **errp: call corresponding Error *err cleaning
> + * function an set pointer to NULL
> + */
> +static inline void error_free_errp(Error **errp)
> +{
> +assert(errp && *errp);
> +error_free(*errp);
> +*errp = NULL;
> +}
> +
> +static inline void error_report_errp(Error **errp)
> +{
> +assert(errp && *errp);
> +error_report_err(*errp);
> +*errp = NULL;
> +}
> +
> +static inline void warn_report_errp(Error **errp)
> +{
> +assert(errp && *errp);
> +warn_report_err(*errp);
> +*errp = NULL;
> +}
> +
> +
> /*
> * Just like error_setg(), except you get to specify the error class.
> * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PULL 0/4] Trivial branch patches
On Tue, 5 Nov 2019 16:56:11 +0100 Laurent Vivier wrote: > Greg, Dave, > > could you fix that? > > Thanks, > Laurent > > Le 05/11/2019 à 16:48, no-re...@patchew.org a écrit : > > Patchew URL: > > https://patchew.org/QEMU/20191105144247.10301-1-laur...@vivier.eu/ > > > > > > > > Hi, > > > > This series seems to have some coding style problems. See output below for > > more information: > > > > Subject: [PULL 0/4] Trivial branch patches > > Type: series > > Message-id: 20191105144247.10301-1-laur...@vivier.eu > > > > === TEST SCRIPT BEGIN === > > #!/bin/bash > > git rev-parse base > /dev/null || exit 0 > > git config --local diff.renamelimit 0 > > git config --local diff.renames True > > git config --local diff.algorithm histogram > > ./scripts/checkpatch.pl --mailback base.. > > === TEST SCRIPT END === > > > > Switched to a new branch 'test' > > 85ac453 global: Squash 'the the' > > 9dd7da4 qom: Fix error message in object_class_property_add() > > 2b76b45 hw/misc/grlib_ahb_apb_pnp: Fix 8-bit accesses > > bddcfd9 hw/misc/grlib_ahb_apb_pnp: Avoid crash when writing to PnP registers > > > > === OUTPUT BEGIN === > > 1/4 Checking commit bddcfd9b6b24 (hw/misc/grlib_ahb_apb_pnp: Avoid crash > > when writing to PnP registers) > > 2/4 Checking commit 2b76b451f9b7 (hw/misc/grlib_ahb_apb_pnp: Fix 8-bit > > accesses) > > 3/4 Checking commit 9dd7da421bfb (qom: Fix error message in > > object_class_property_add()) > > WARNING: line over 80 characters > > #31: FILE: qom/object.c:1109: > > +error_setg(errp, "attempt to add duplicate property '%s' to object > > (type '%s')", > > > > WARNING: line over 80 characters > > #43: FILE: qom/object.c:1141: > > +error_setg(errp, "attempt to add duplicate property '%s' to class > > (type '%s')", > > As mentioned in the changelog, this is deliberate. AFAIK better grep-ability has precedence over the 80 characters rule when it comes to error messages. Maybe we should teach checkpatch about that ? Cheers, -- Greg > > total: 0 errors, 2 warnings, 22 lines checked > > > > Patch 3/4 has style problems, please review. If any of these errors > > are false positives report them to the maintainer, see > > CHECKPATCH in MAINTAINERS. > > 4/4 Checking commit 85ac453d1520 (global: Squash 'the the') > > ERROR: do not use C99 // comments > > #26: FILE: disas/libvixl/vixl/invalset.h:105: > > + // Note that this does not mean the backing storage is empty: it can > > still > > > > total: 1 errors, 0 warnings, 56 lines checked > > > > Patch 4/4 has style problems, please review. If any of these errors > > are false positives report them to the maintainer, see > > CHECKPATCH in MAINTAINERS. > > > > === OUTPUT END === > > > > Test command exited with code: 1 > > > > > > The full log is available at > > http://patchew.org/logs/20191105144247.10301-1-laur...@vivier.eu/testing.checkpatch/?type=message. > > --- > > Email generated automatically by Patchew [https://patchew.org/]. > > Please send your feedback to patchew-de...@redhat.com > > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [SPAM] [Qemu-ppc] [PATCH 2/2] avoid TABs in files that only contain a few
On Thu, 13 Dec 2018 23:37:37 +0100 Paolo Bonzini wrote: > Most files that have TABs only contain a handful of them. Change > them to spaces so that we don't confuse people. > > disas, standard-headers, linux-headers and libdecnumber are imported > from other projects and probably should be exempted from the check. > Outside those, after this patch the following files still contain both > 8-space and TAB sequences at the beginning of the line. Many of them > have a majority of TABs, or were initially committed with all tabs. > > [...] > > Signed-off-by: Paolo Bonzini > --- This a lot of places to review... FWIW stgit spotted a trailing whitespace for free :) > > diff --git a/target/cris/helper.c b/target/cris/helper.c > index d2ec349191..754fa00d06 100644 > --- a/target/cris/helper.c > +++ b/target/cris/helper.c > @@ -240,7 +240,7 @@ void cris_cpu_do_interrupt(CPUState *cs) > /* Exception starts with dslot cleared. */ > env->dslot = 0; > } > - > + ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
