Re: [Xen-devel] [RFC v2 6/9] scripts: add coccinelle script to use auto propagated errp
On 9/23/19 3:05 PM, Eric Blake wrote:
> Does running this Coccinelle script 2 times in a row add a second
> ERRP_FUNCTION_BEGIN() line? We want it to be idempotent (no changes on
> a second run). (Admittedly, I did not actually test that yet). Also, I
> don't know if this can be tweaked to avoid adding the line to a function
> with an empty body, maybe:
>
> fn(..., Error **errp, ...)
> {
> +ERRP_FUNCTION_BEGIN();
> ...
> }
Also untested:
fn(..., Error **errp, ...)
{
(
|
ERRP_FUNCTION_BEGIN();
...
|
+ERRP_FUNCTION_BEGIN()
...
)
}
> Overall, the script makes sense in my reading (but no idea if it
> actually catches everything we want, or if it missed something).
Having spot-checked 7, it definitely misses cases where it was supposed
to add ERRP_FUNCTION_BEGIN().
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 7/9] Use auto-propagated errp
lSocket *sioc;
> -Error *local_err = NULL;
>
> sioc = qio_channel_socket_new();
> qio_channel_set_name(QIO_CHANNEL(sioc), "nbd-client");
>
> -qio_channel_socket_connect_sync(sioc, saddr, &local_err);
> -if (local_err) {
> +qio_channel_socket_connect_sync(sioc, saddr, errp);
> +if (*errp) {
> object_unref(OBJECT(sioc));
> -error_propagate(errp, local_err);
> return NULL;
But getting rid of error_propagate() is nice.
Did you grep for instances of error_propagate() after your mechanical
patch, to see what else the Coccinelle script might have missed?
> +++ b/qapi/opts-visitor.c
> @@ -275,6 +275,7 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
> static void
> opts_check_list(Visitor *v, Error **errp)
> {
> + ERRP_FUNCTION_BEGIN();
> /*
> * Unvisited list elements will be reported later when checking
> * whether unvisited struct members remain.
Here the macro got added, but with no obvious benefit later on (although
we also argued that adding it even when it makes no difference is not
bad, if that's easier to automate for style checking).
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index d192724b13..3ee4c7a2e7 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -25,6 +25,7 @@ struct QapiDeallocVisitor
> static void qapi_dealloc_start_struct(Visitor *v, const char *name, void
> **obj,
>size_t unused, Error **errp)
> {
> + ERRP_FUNCTION_BEGIN();
> }
Here's an example where exempting empty functions would be nicer.
> +++ b/qapi/qapi-visit-core.c
> @@ -39,18 +39,15 @@ void visit_free(Visitor *v)
> void visit_start_struct(Visitor *v, const char *name, void **obj,
> size_t size, Error **errp)
> {
> -Error *err = NULL;
> -
Oops, macro not added.
> trace_visit_start_struct(v, name, obj, size);
> if (obj) {
> assert(size);
> assert(!(v->type & VISITOR_OUTPUT) || *obj);
> }
> -v->start_struct(v, name, obj, size, &err);
> +v->start_struct(v, name, obj, size, errp);
> if (obj && (v->type & VISITOR_INPUT)) {
> -assert(!err != !*obj);
> +assert(!*errp != !*obj);
> }
> -error_propagate(errp, err);
> }
But the cleanup is sane, once the macro is present.
> @@ -152,12 +143,10 @@ void visit_type_int(Visitor *v, const char *name,
> int64_t *obj, Error **errp)
> static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name,
> uint64_t max, const char *type, Error **errp)
> {
> -Error *err = NULL;
> uint64_t value = *obj;
>
> -v->type_uint64(v, name, &value, &err);
> -if (err) {
> -error_propagate(errp, err);
> +v->type_uint64(v, name, &value, errp);
> +if (*errp) {
> } else if (value > max) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> name ? name : "null", type);
Results in an empty if which looks funny. This one could be a manual
touchup later.
> @@ -211,12 +200,10 @@ static void visit_type_intN(Visitor *v, int64_t *obj,
> const char *name,
> int64_t min, int64_t max, const char *type,
> Error **errp)
> {
> -Error *err = NULL;
> int64_t value = *obj;
>
> -v->type_int64(v, name, &value, &err);
> -if (err) {
> -error_propagate(errp, err);
> +v->type_int64(v, name, &value, errp);
> +if (*errp) {
> } else if (value < min || value > max) {
and again
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 1/9] error: auto propagated local_err
On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
> any function with errp parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_append_hint: user can't see these
> hints, because exit() happens in error_setg earlier than hint is
> appended. [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 don't fix the issue, but it allows to [3.] drop all
doesn't
> 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 should be merely updated to
Maybe:
s/should/could/
> return int error code).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> include/qapi/error.h | 37 +
> 1 file changed, 37 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..f6f4fa0fac 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,43 @@ 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_FUNCTION_BEGIN
> + *
> + * This macro MUST be the first line of EACH function with Error **errp
> + * parameter.
Maybe s/EACH function/ANY non-empty function/ (allowing our stub
functions to be exemptions).
> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to
s/to/to a/
> + * local Error object, which will be automatically propagated to original
s/to/to the/
> + * errp on function exit (see error_propagator_cleanup).
> + *
> + * After invocation of this macro it is always safe to dereference errp
> + * (as it's not NULL anymore) and to append hint (by error_append_hint)
s/append hint/append hints/
> + * (as, if it was error_fatal, we swapped it by local_error to be
s/by local_error/with a local error/
> + * propagated on cleanup).
> + *
> + * Note: we don't wrap error_abort case, as we want resulting coredump
> + * to point to the place where the error happened, not to error_propagate.
s/wrap/wrap the/
> + */
> +#define ERRP_FUNCTION_BEGIN() \
> +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
> +Error **__local_errp_unused __attribute__ ((unused)) = \
> +(errp = (errp == NULL || *errp == error_fatal ? \
> + &__auto_errp_prop.local_err : errp))
I'm not sold on why we need the dummy declaration (yeah, I know it's so
that you don't have to fight the battle of mixing declarations and
statements - but this is in a macro call whose usage LOOKS like a
statement rather than a declaration, so we're already on fuzzy ground).
We could make this macro expansion one line shorter and still be
correct, but I'm not going to insist (we'll see what consensus is,
and/or what Markus says).
> +
> +
> /*
> * Special error destination to abort on error.
> * See error_setg() and error_propagate() for details.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 6/9] scripts: add coccinelle script to use auto propagated errp
On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> scripts/coccinelle/auto-propagated-errp.cocci | 82 +++
> 1 file changed, 82 insertions(+)
> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>
> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci
> b/scripts/coccinelle/auto-propagated-errp.cocci
> new file mode 100644
> index 00..1a3f006f0b
> --- /dev/null
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -0,0 +1,82 @@
> +@@
> +identifier fn;
> +identifier local_err;
> +@@
> +
> + fn(..., Error **errp)
> + {
> ++ERRP_FUNCTION_BEGIN();
> + }
This doesn't catch functions where Error **errp is not the last
parameter. Some examples (some of which may need independent tweaking
in their own right for being inconsistent, although we DO want errp to
appear before any 'format, ...' arguments):
block/ssh.c:sftp_error_setg(Error **errp, BDRVSSHState *s, const char
*fs, ...)
exec.c:static void ram_block_add(RAMBlock *new_block, Error **errp, bool
shared)
Does running this Coccinelle script 2 times in a row add a second
ERRP_FUNCTION_BEGIN() line? We want it to be idempotent (no changes on
a second run). (Admittedly, I did not actually test that yet). Also, I
don't know if this can be tweaked to avoid adding the line to a function
with an empty body, maybe:
fn(..., Error **errp, ...)
{
+ERRP_FUNCTION_BEGIN();
...
}
to only add it to a function that already has a body (thanks to the ...)
- but I'm fuzzy enough on Coccinelle that I may be saying something
totally wrong.
> +
> +@rule1@
> +identifier fn;
> +identifier local_err;
> +@@
> +
> + fn(..., Error **errp)
> + {
> + <...
> +-Error *local_err = NULL;
> + ...>
> + }
> +
> +@@
> +identifier rule1.fn;
> +identifier rule1.local_err;
> +identifier out;
> +@@
> +
> + fn(...)
> + {
> + <...
> +-goto out;
> ++return;
> + ...>
> +- out:
> +-error_propagate(errp, local_err);
> + }
> +
> +@@
> +identifier rule1.fn;
> +identifier rule1.local_err;
> +@@
> +
> + fn(...)
> + {
> + <...
> +(
> +-error_free(local_err);
> +-local_err = NULL;
> ++error_free_errp(errp);
> +|
> +-error_free(local_err);
> ++error_free_errp(errp);
> +|
> +-error_report_err(local_err);
> ++error_report_errp(errp);
> +|
> +-warn_report_err(local_err);
> ++warn_report_errp(errp);
> +|
> +-error_propagate(errp, local_err);
> +)
> + ...>
> + }
> +
> +@@
> +identifier rule1.fn;
> +identifier rule1.local_err;
> +@@
> +
> + fn(...)
> + {
> + <...
> +(
> +-&local_err
> ++errp
> +|
> +-local_err
> ++*errp
> +)
> + ...>
> + }
>
Overall, the script makes sense in my reading (but no idea if it
actually catches everything we want, or if it missed something). At any
rate, once patch 7 is split into more manageable chunks, we can
definitely spot-check results to make sure they all look reasonable.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 3/9] errp: rename errp to errp_in where it is IN-argument
On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote: > Error **errp is almost always OUT-argument: it's assumed to be NULL, or > pointer to NULL-initialized pointer, or pointer to error_abort or > error_fatal, for callee to report error. > > But very few functions (most of the are error API) instead get Error > **errp as IN-argument: it's assumed to be set, and callee should clean > it. In such cases, rename errp to errp_in. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > include/monitor/hmp.h | 2 +- > include/qapi/error.h | 8 > ui/vnc.h | 2 +- > monitor/hmp-cmds.c| 8 > ui/vnc.c | 10 +- > util/error.c | 2 +- > 6 files changed, 16 insertions(+), 16 deletions(-) This is worthwhile, regardless of the fate of the rest of the series. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 4/9] hw/core/loader-fit: fix freeing errp in fit_load_fdt
On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> fit_load_fdt forget to zero errp. Fix it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> hw/core/loader-fit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Independent bug fix. Either we take the (fixed) 2-3 (to rely on the new
error_free_errp), or you could open-code the assignment of errp=NULL to
take this on its own, regardless of the hfate of the rest of the series.
Reviewed-by: Eric Blake
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 953b16bc82..fe5bcc6700 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -200,7 +200,7 @@ static int fit_load_fdt(const struct fit_loader *ldr,
> const void *itb,
> err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
> if (err == -ENOENT) {
> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
> -error_free(*errp);
> +error_free_errp(errp);
> } else if (err) {
> error_prepend(errp, "unable to read FDT load address from FIT: ");
> ret = err;
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 2/9] qapi/error: add (Error **errp) cleaning APIs
On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
A "why" as the commit message body wouldn't hurt.
> include/qapi/error.h | 22 ++
> util/error.c | 6 +++---
> 2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index f6f4fa0fac..551385aa91 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -297,6 +297,28 @@ void warn_report_err(Error *err);
> */
> void error_report_err(Error *err);
>
> +/*
> + * Functions to clean Error **errp: call corresponding Error *err cleaning
> + * function an set pointer to NULL
s/an/and/
> + */
> +static inline void error_free_errp(Error **errp)
> +{
> +error_free(*errp_in);
Fails to compile. Did you mean for this to come after 3/9?
> +*errp_in = NULL;
> +}
> +
> +static inline void error_report_errp(Error **errp)
> +{
> +error_report_err(*errp_in);
> +*errp_in = NULL;
> +}
> +
> +static inline void warn_report_errp(Error **errp)
> +{
> +warn_report_err(*errp_in);
> +*errp_in = NULL;
> +}
> +
> /*
> * Convenience function to error_prepend(), warn_report() and free @err.
> */
> diff --git a/util/error.c b/util/error.c
> index d4532ce318..dfba091757 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -273,9 +273,9 @@ void error_free(Error *err)
>
> void error_free_or_abort(Error **errp)
> {
> -assert(errp && *errp);
> -error_free(*errp);
> -*errp = NULL;
> +assert(errp_in && *errp_in);
> +error_free(*errp_in);
> +*errp_in = NULL;
Did you mean to use error_free_errp() instead of these last two lines?
> }
>
> void error_propagate(Error **dst_errp, Error *local_err)
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 5/9] net/net: fix local variable shadowing in net_client_init
On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Don't shadow Error *err: it's a bad thing. This patch also simplifies
> following Error propagation conversion.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> net/net.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Can be applied independently of the rest of this series.
>
> diff --git a/net/net.c b/net/net.c
> index 84aa6d8d00..5fc72511c1 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1128,10 +1128,10 @@ static int net_client_init(QemuOpts *opts, bool
> is_netdev, Error **errp)
>
> if (substrings[1]) {
> /* User-specified prefix length. */
> -int err;
> +int ret2;
>
> -err = qemu_strtoul(substrings[1], NULL, 10, &prefix_len);
> -if (err) {
> +ret2 = qemu_strtoul(substrings[1], NULL, 10, &prefix_len);
> +if (ret2) {
In fact, you don't need ret2; you could just:
if (qemu_strtoul(...)) {
at which point you could then join:
if (substrings[1] &&
qemus_strtoul(...)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "ipv6-prefixlen", "a number");
> goto out;
>
Either way,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 0/9] error: auto propagated local_err
On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a proposal of auto propagation for local_err, to not call
> error_propagate on every exit point, when we deal with local_err.
>
> It also fixes two issues:
> 1. Fix issue with error_fatal & error_append_hint: user can't see these
> hints, because exit() happens in error_setg earlier than hint is
> appended. [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 don't fix the issue, but it allows to [3.] drop all
doesn't
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
>
> It's still an RFC, due to the following reasons:
>
> 1. I'm new to coccinella, so I failed to do the following pattern:
>
> <...
> - goto out;
> + return;
> ...>
> - out:
> - error_propagate(errp, local_err)
>
> So, here is compilation fix 08.. Who can help with it? If nobody, 08 is
> to be merged to 07 by hand.
I'm not sure either; but I agree that if we can't figure out how to make
Coccinelle do quite what we want, that we are better off squashing in
compile fixes.
Also, while I like Coccinelle for automating the conversion, it's harder
to get everyone to run it; it would be nice if we could also figure out
a patch to scripts/checkpatch.pl that for any instance of 'Error
**errp)\n{\n' not followed by either } or the new macro, we flag that as
a checkpatch warning or error.
>
> 2. Question about using new macro in empty stub functions - see 09
It would be nice if we could exempt empty functions - no need to use the
macro if there is no function body otherwise. I'm not sure if
Coccinelle can do that filtering during the conversion, or if we clean
up by hand after the fact.
>
> 3. What to do with huge auto-generated commit 07? Should I split it
> per-maintainer or per-subsystem, or leave it as-is?
It's big. I'd split it into multiple patches (and reduce the cc - except
for the cover letter, the rest of the patches can be limited to the
actual maintainer/subsystem affected rather than everyone involved
anywhere else in the series. With the current large cc, anyone that
replies gets several mail bounces about "too many recipients"). It may
be easier to split along directory boundaries than by maintainer
boundaries. Markus has applied large tree-wide Coccinelle cleanups
before, maybe he has some advice.
>
> 4. Also, checkpatch has some complains about 07 patch:
> - using tabs.. (hmm exactly stubs functions..)
> - empty ifs
> Again, I don't see any ways to fix it other that by hand and merge to
> 07..
Hand cleanups for formatting or compilation fixes to Coccinelle's work
is not an uncommon issue after large patches; thankfully it's also not
very difficult (and surprisingly needed in very few places compared to
how much actually gets touched).
>
> ==
>
> Also, if we decide, that this all is too huge, here is plan B:
>
> 1. apply 01
> 2. fix only functions that don't use local_err and use
> error_append_hint, by just invocation of new macro at function start -
> it will substitute Greg's series with no pain.
> 3[optional]. Do full update for some subsystems, for example, only for
> block* and nbd*
Even if we go with plan B, it's still worth checking in a Coccinelle
script that we can periodically run to make sure we aren't missing out
on the use of the macro where it is needed.
>
> Vladimir Sementsov-Ogievskiy (9):
> error: auto propagated local_err
> qapi/error: add (Error **errp) cleaning APIs
> errp: rename errp to errp_in where it is IN-argument
> hw/core/loader-fit: fix freeing errp in fit_load_fdt
> net/net: fix local variable shadowing in net_client_init
> scripts: add coccinelle script to use auto propagated errp
> Use auto-propagated errp
> fix-compilation: empty goto
> fix-compilation: includes
>
> include/hw/pci-host/spapr.h | 2 +
> include/monitor/hmp.h | 2 +-
> include/qapi/error.h | 61 -
> target/ppc/kvm_ppc.h | 3 +
> target/s390x/cpu_models.h | 3 +
> ui/vnc.h | 2 +-
> vl.c | 13 +-
> scripts/coccinelle/auto-propagated-errp.cocci | 82 +++
> 319 files changed, 2729 insertions(+), 4245 deletions(-)
> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
The diffstat is huge, but promising.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v2 0/9] error: auto propagated local_err
On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 3. What to do with huge auto-generated commit 07? Should I split it >>> per-maintainer or per-subsystem, or leave it as-is? >> >> It's big. I'd split it into multiple patches (and reduce the cc - except >> for the cover letter, the rest of the patches can be limited to the >> actual maintainer/subsystem affected rather than everyone involved >> anywhere else in the series. With the current large cc, anyone that >> replies gets several mail bounces about "too many recipients"). It may >> be easier to split along directory boundaries than by maintainer >> boundaries. Markus has applied large tree-wide Coccinelle cleanups >> before, maybe he has some advice. > > > If split by subsystem it would be 200+ patches: > git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f > --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | > uniq | wc -l > 205 > > > Try to look at larger subsystem: > git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f > --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done > | sort | uniq | wc -l > 139 > > still too many.. Or is it OK? Hmm - that becomes a tradeoff in length of the series (where individual patches may be reviewed fast, but where the overall process may be bogged down by sheer length), vs. length of individual emails (where the email itself is daunting, but as the review is mechanical and done by automation, it becomes a matter of spot-checking if we trust that the automation was done correctly). You can probably group it in fewer patches, by joining smaller patches across a couple of subsystems. It's an art form, there's probably several ways to do it that would work, and it comes down to a judgment call on how much work you want to do to try and reduce other's work in reviewing it. Maybe even an off-hand split of gathering files until you reach about 500 or so lines per diff. I wish I had easier advice on how to tackle this sort of project in the way that will get the fastest response time. >>> vl.c | 13 +- >>> scripts/coccinelle/auto-propagated-errp.cocci | 82 +++ >>> 319 files changed, 2729 insertions(+), 4245 deletions(-) >>> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci >> >> The diffstat is huge, but promising. We also learned in reviews of 7/9 that the diffstat here is misleading, the number of insertions will definitely be increasing once the Coccinelle script is fixed to insert the macro in more functions, but hopefully it's still a net reduction in overall lines. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH] configure: Don't add Xen's libs to LDFLAGS
On 1/24/19 2:45 AM, Markus Armbruster wrote: >> Signed-off-by: Michael Tokarev >> Revieved-by: Michael Tokarev > > Typo in Reviewed-by. Should we tighten checkpatch.pl to flag suspicious-looking 'xxx-by:' tags, to catch instances of typos? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH] configure: Don't add Xen's libs to LDFLAGS
On 1/24/19 12:29 PM, Peter Maydell wrote: > On Thu, 24 Jan 2019 at 17:40, Eric Blake wrote: >> >> On 1/24/19 2:45 AM, Markus Armbruster wrote: >> >>>> Signed-off-by: Michael Tokarev >>>> Revieved-by: Michael Tokarev >>> >>> Typo in Reviewed-by. >> >> Should we tighten checkpatch.pl to flag suspicious-looking 'xxx-by:' >> tags, to catch instances of typos? > > Yes, I would vote for having it whitelist the half a dozen > expected ones and complain about the rest. I think we > kind of discussed this in the past... I submitted a patch along those lines: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg00910.html -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument
On 2/20/19 5:30 AM, Daniel P. Berrangé wrote: >> Since Paolo you suggested the change, could you give some convincing >> arguments that it's worth taking the plunge? > > The chardev write/read methods will end up calling libc read/write > methods, whose parameters are "size_t count". In my mind, that's the convincing reason. We should model our read/write after the libc read/write, which means size_t input and ssize_t returns. > > Thus if there is QEMU code that could currently (mistakenly) pass a > negative value for length to qemu_chr_write, unless something stops > it, this is going to be cast to a size_t when we finally call read/ > write on the FD, leading to a large positive value & array out of > bounds read/write. > > IOW we already have inconsistent use of signed vs unsigned in our code > which has potential to cause bugs. Converting chardev to use size_t > we get rid fo the mismatch with the underlying libc APIs we call, > which ultimately eliminates an area of risk longer term. There is a > chance it could uncover some pre-existing dormant bugs, but provided > we do due diligence to check callers I think its a win to be consistent > with libc APIs in size_t usage for read/write. And hopefully this exercise of making the conversion serves as a good audit to help us gain confidence in our code and/or fix bugs it uncovers. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH] xen-block: Replace qdict_put_obj() by qdict_put() where appropriate
On 3/13/19 12:44 PM, Markus Armbruster wrote: > Patch created mechanically by rerunning: > > $ spatch --sp-file scripts/coccinelle/qobject.cocci \ > --macro-file scripts/cocci-macro-file.h \ > --dir hw/block --in-place > > Signed-off-by: Markus Armbruster > --- > hw/block/xen-block.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Eric Blake Safe for 4.0 softfreeze in my opinion, but also harmless if it slips to 4.1. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 000/126] error: auto propagated local_err
On 11/20/19 3:50 AM, Vladimir Sementsov-Ogievskiy wrote: Okay... I think that: 1. A lot of efforts (not only my, I think reviewing is already exceeded generation efforts) are made, it would be sad to lose them. 2. It's safe enough to apply only part of generated patches: we just fix error_abort/error_fatal in more popular subsystems, what's wrong with that? Why not to cover 80% cases by 20% efforts? 3. It's obviously impossible to merge the whole series. A lot of time passed, series diverges. So I propose the following plan: 1. I resend small separate series of preparation patches per maintainer. They are good anyway. 2. We commit patch with macro (changing MUST to SHOULD in documentation) and coccinelle script. (or that may be combined with the first series from [3.]) 3. When one of preparations taken to maintainer's tree, I send generated patches for its maintainer. I'd still prefer waiting for direction from Markus. We've been tied up by other things (KVM Forum, 4.2 release), but now that we are in freeze, this is actually a GOOD time for Markus to finally get back to this series, and there is going to be less rebasing needed if we can apply the entire cleanup right as 5.0 development opens in a couple of weeks. If no objections during a week, I'll start that plan, hope someone will support it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 024/126] error: auto propagated local_err
On 12/5/19 8:58 AM, Vladimir Sementsov-Ogievskiy wrote:
What about you provide the examples, and then I try to polish the prose?
1: error_fatal problem
Assume the following code flow:
int f1(errp) {
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
Now, if we call f1 with &error_fatal argument and f2 fails, the program
will exit immediately inside f2, when setting the errp. User will not
see the hint.
So, in this case we should use local_err.
How does this example look after the transformation?
Without ERRP_AUTO_PROPAGATE(), the transformation is a lot of boilerplate:
int f1(errp) {
Error *err = NULL;
ret = f2(&err);
if (ret < 0) {
error_append_hint(&err, "very useful hint");
error_propagate(errp, err);
return ret;
}
}
what's worse, that boilerplate to solve problem 1 turns out to be...
Good point.
int f1(errp) {
ERRP_AUTO_PROPAGATE();
...
ret = f2(errp);
if (ret < 0) {
error_append_hint(errp, "very useful hint");
return ret;
}
...
}
- nothing changed, only add macro at start. But now errp is safe, if it was
error_fatal it is wrapped by local error, and will only call exit on automatic
propagation on f1 finish.
2: error_abort problem
Now, consider functions without return value. We normally use local_err
variable to catch failures:
void f1(errp) {
Error *local_err = NULL;
...
f2(&local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
...
}
the very same code as the cause of problem 2.
Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
crash dump will point to error_propagate, not to the failure point in f2,
which complicates debugging.
So, we should never wrap error_abort by local_err.
Likewise.
And here:
void f1(errp) {
ERRP_AUTO_PROPAGATE();
...
f2(errp);
if (*errp) {
return;
}
...
- if errp was NULL, it is wrapped, so dereferencing errp is safe. On return,
local error is automatically propagated to original one.
So, the use of ERRP_AUTO_PROPAGATE() solves BOTH problems 1 and 2 - we
avoid the boilerplate that trades one problem for another, by
consolidating ALL of the boilerplate into a single-line macro, such that
error_propagate() no longer needs to be called anywhere except inside
the ERRP_AUTO_PROPAGATE macro.
===
Our solution:
- Fixes [1.], adding invocation of new macro into functions with
error_appen_hint/error_prepend,
New macro will wrap error_fatal.
- Fixes [2.], by switching from hand-written local_err to smart macro, which
never
wraps error_abort.
- Handles [3.], by switching to macro, which is less code
- Additionally, macro doesn't wrap normal non-zero errp, to avoid extra
propagations
(in fact, error_propagate is called, but returns immediately on first if
(!local_err))
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 025/126] scripts: add coccinelle script to use auto propagated errp
On 10/11/19 12:12 PM, Eric Blake wrote:
On 10/11/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
+@@
+identifier rule1.fn;
+identifier rule1.local_err;
+@@
+
+ fn(...)
+ {
+ <...
+(
+- error_free(local_err);
+- local_err = NULL;
++ error_free_errp(errp);
This does not make sense - error_free_errp() is not defined prior to
this series or anywhere in patches 1-24, if I'm reading it correctly.
Apologies - I sent my reply to this message before 21/126 showed up in
my inbox, and didn't realize that I had skipped over sequencing. This
part is fine after all, but it points to the perils of reviewing a
thread as it comes in piecemeal, vs. reviewing an actual branch with all
patches instantly available.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 025/126] scripts: add coccinelle script to use auto propagated errp
On 10/11/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/coccinelle/auto-propagated-errp.cocci | 118 ++
1 file changed, 118 insertions(+)
create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
diff --git a/scripts/coccinelle/auto-propagated-errp.cocci
b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 00..d9731620aa
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
+@rule1@
+// Drop local_err
+identifier fn, local_err;
+symbol errp;
+@@
+
+ fn(..., Error **errp, ...)
+ {
+ <...
+-Error *local_err = NULL;
+ ...>
+ }
+
So our goal is to automate removal of all local_err (including when it
is spelled err)...
+@@
+// Handle pattern with goto, otherwise we'll finish up
+// with labels at function end which will not compile.
+identifier rule1.fn;
+identifier rule1.local_err;
+identifier OUT;
+@@
+
+ fn(...)
+ {
+ <...
+-goto OUT;
++return;
+ ...>
+- OUT:
+-error_propagate(errp, local_err);
+ }
+
this dangling label cleanup makes sense
+@@
+identifier rule1.fn;
+identifier rule1.local_err;
+@@
+
+ fn(...)
+ {
+ <...
+(
+-error_free(local_err);
+-local_err = NULL;
++error_free_errp(errp);
This does not make sense - error_free_errp() is not defined prior to
this series or anywhere in patches 1-24, if I'm reading it correctly.
+|
+-error_free(local_err);
++error_free_errp(errp);
and again
+|
+-error_report_err(local_err);
++error_report_errp(errp);
+|
+-warn_report_err(local_err);
++warn_report_errp(errp);
+|
+-error_propagate_prepend(errp, local_err,
++error_prepend(errp,
+ ...);
+|
+-error_propagate(errp, local_err);
+)
+ ...>
+ }
+
It looks like once this script is run, error_propagate_prepend() will
have no clients. Is there a non-generated cleanup patch that removes it
(and once it is removed, it can also be removed from the .cocci script
as no further clients will reappear later)?
+@@
+identifier rule1.fn;
+identifier rule1.local_err;
+@@
+
+ fn(...)
+ {
+ <...
+(
+-&local_err
++errp
+|
+-local_err
++*errp
+)
+ ...>
+ }
+
+@@
+symbol errp;
+@@
+
+- *errp != NULL
++ *errp
Seems to make sense.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 000/126] error: auto propagated local_err
On 10/11/19 11:03 AM, Vladimir Sementsov-Ogievskiy wrote: Hi all! At the request of Markus: full version of errp propagation. Let's look at it. Cover as much as possible, except inserting macro invocation where it's not necessary. It's huge, and so it's an RFC. Is there a repo containing these patches, to make it easier to play with them locally without having to 'git am' the entire 126 messages? util/qemu-sockets.c | 31 +-- vl.c | 14 +- python/commit-per-subsystem.py| 204 ++ scripts/coccinelle/auto-propagated-errp.cocci | 118 341 files changed, 3851 insertions(+), 4455 deletions(-) create mode 100755 python/commit-per-subsystem.py create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci So, whether or not we take commit-per-subsystem.py, the overall series appears to be a nice reduction in lines of code. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 025/126] scripts: add coccinelle script to use auto propagated errp
On 10/14/19 3:19 AM, Vladimir Sementsov-Ogievskiy wrote: +| +- error_propagate(errp, local_err); +) + ...> + } + It looks like once this script is run, error_propagate_prepend() will have no clients. No, it still have a bit, when working with error_copy, and/or moving errors from/to structures. No public clients. So can we turn it into a static function only used by error.c? Is there a non-generated cleanup patch that removes it (and once it is removed, it can also be removed from the .cocci script as no further clients will reappear later)? Maybe. Basically, if we can get error_propagate out of error.h, that's a good sign. But it's not a show-stopper if we can't do it for some legitimate reason (such a reason might include that the definition of the ERRP_AUTO_PROPAGATE macro is easier to write if error_propagate remains in the .h), so we'll just have to see what is possible. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v5 024/126] error: auto propagated local_err
On 11/8/19 3:10 PM, Marc-André Lureau wrote: +/* + * ERRP_AUTO_PROPAGATE + * + * This macro is created to be the first line of a function with Error **errp + * OUT parameter. It's needed only in cases where we want to use error_prepend, + * error_append_hint or dereference *errp. It's still safe (but useless) in + * other cases. + * + * If errp is NULL or points to error_fatal, it is rewritten to point to a + * local Error object, which will be automatically propagated to the original + * errp on function exit (see error_propagator_cleanup). + * + * After invocation of this macro it is always safe to dereference errp + * (as it's not NULL anymore) and to add information (by error_prepend or + * error_append_hint) + * (as, if it was error_fatal, we swapped it with a local_error to be + * propagated on cleanup). Nice improvements. Minor drawback, the abort()/exit() will now take place when going out of scope and running the cleanup instead of error location. Not a big problem I guess. Your assessment is not quite right: Any abort() will happen at the leaf node (because we are no longer wrapping thing into a local err and skipping error_propagate altogether for &error_abort). You are correct that any exit() will now happen during cleanup, but that is an undetectable change (there is no stack trace present for &error_fatal, so calling error_propagate at a later point in time does not affect the observable end behavior). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [Qemu-block] [PATCH 02/18] xen: introduce new 'XenBus' and 'XenDevice' object hierarchy
On 11/28/18 10:26 AM, Paul Durrant wrote: +++ b/hw/xen/xen-bus.c @@ -0,0 +1,125 @@ +/* + * Copyright (c) Citrix Systems Inc. + * All rights reserved. + */ This doesn't look very compatible with the GPL. In fact it might even make it illegal for the QEMU project to distribute this code. :-) Other files you add throughout the series seem to have the same problem. I was working on the assumption that a lack of explicit license meant that the overall project license as described in item 2 in LICENSE. Did I misinterpret that text? No, but you missed the fact that "All rights reserved" is an explicit license (or rather, an explicit anti-license that states you are not granting rights that the GPL would normally grant), and an implicit license does not apply when an explicit (anti-)license is present. What's more, relying on implicit licenses is prone to misinterpretation, so even though the overall project documentation tries to cover what will happen, it's much nicer if you DO use an explicit license mention in your file so that we don't HAVE to rely on the implicit license. Yes, the phrase "All rights reserved" exists in several existing files: $ git grep -il 'all rights reserved' |wc 138 1383557 but we should be striving to clean those up, not adding to the mess. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] avoid TABs in files that only contain a few
On 12/13/18 4:37 PM, 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 --- nbd/client.c | 2 +- NBD part: Acked-by: Eric Blake but I have patches that remove the line in question entirely as part of adding 'qemu-nbd --list'. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 16/16] configure: do_compiler: Dump some extra info under bash
On 04/23/2018 11:38 AM, Ian Jackson wrote:
> You are right. Perhaps my testing was inadequate. I wrote this a
> long while ago, and if there was a syntax along these lines that DTRT
> in both bash and dash in my tests it is long gone. Starting de novo,
> the following code works for me:
>
> (echo >>config.log "
> funcs: ${FUNCNAME[*]}
> lines: ${BASH_LINENO[*]}
> files: ${BASH_SOURCE[*]}") 2>/dev/null ||:
That's still fork-heavy. You could do:
test -n "$BASH_VERSION" && eval '
echo >>config.log "
funcs: ${FUNCNAME[*]}
lines: ${BASH_LINENO[*]}
files: ${BASH_SOURCE[*]}"'
which avoids the fork, but remains silent on dash.
>
> With bash I get the expected information in config.log, which looks
> like this:
>
> funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
> lines: 91 124 1720 1724 0
> files: ./configure ./configure ./configure ./configure ./configure
Is files: really useful information? The other two are (as it gives a
full stack trace), but if we aren't actively sourcing lots of other
files, seeing a bunch of ./configure doesn't add much.
>
> With dash the script runs but there is nothing from this segment in
> the log. Without the 2>/dev/null, it prints
> ./configure: 63: ./configure: Bad substitution
> so the syntax error is indeed being suprresed and ignored.
>
> The ( ) is necessary because syntax errors are not like set -e errors:
> they cause the shell process to exit.
See above - a well-quoted eval is sufficient to avoid a subshell.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report
On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote:
>> - static void vreport(report_type type, const char *fmt, va_list ap)
>> + static void vreport(report_type type, int errnoval, const char *fmt,
>> va_list ap)
>> ...
>> + if (errnoval >= 0) {
>> + error_printf(": %s", strerror(errnoval);
>> + }
>>
>> and then add both
>> error_report_errno
>> error_vreport_errno
>> with the obvious semantics.
>
> That would be nice, because then we can make these two functions actually
> use strerror_r() instead of strerror(), for thread safety on all platforms.
Except that strerror_r() is a bear to use portably, given that glibc's
default declaration differs from the POSIX requirement (you can force
glibc to give you the POSIX version, but doing so causes you to lose
access to many other useful extensions). It's rather telling that 'git
grep strerror_r' currently comes up empty.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report
On 04/24/2018 10:40 AM, Eric Blake wrote:
> On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote:
>
>>> - static void vreport(report_type type, const char *fmt, va_list ap)
>>> + static void vreport(report_type type, int errnoval, const char *fmt,
>>> va_list ap)
>>> ...
>>> + if (errnoval >= 0) {
>>> + error_printf(": %s", strerror(errnoval);
>>> + }
>>>
>>> and then add both
>>> error_report_errno
>>> error_vreport_errno
>>> with the obvious semantics.
>>
>> That would be nice, because then we can make these two functions actually
>> use strerror_r() instead of strerror(), for thread safety on all platforms.
>
> Except that strerror_r() is a bear to use portably, given that glibc's
> default declaration differs from the POSIX requirement (you can force
> glibc to give you the POSIX version, but doing so causes you to lose
> access to many other useful extensions). It's rather telling that 'git
> grep strerror_r' currently comes up empty.
That said, glib's g_strerror() may be suitable for this purpose,
although we are currently using it only in tests/ivshmem-test.c.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 01/16] checkpatch: Add xendevicemodel_handle to the list of types
On 04/26/2018 06:06 AM, Ian Jackson wrote:
> (expanding the CC to include everyone that get_maintainer suggests)
>
> Ian Jackson writes ("[PATCH 01/16] checkpatch: Add xendevicemodel_handle to
> the list of types"):
>> This avoids checkpatch misparsing (as statements) long function
>> definitions or declarations, which sometimes start with constructs
>> like this:
>>
>> static inline int xendevicemodel_relocate_memory(
>> xendevicemodel_handle *dmod, domid_t domid, ...
>>
>> The type xendevicemodel_handle does not conform to Qemu CODING_STYLE,
>> which would suggest CamelCase. However, it is a type defined by the
>> Xen Project in xen.git. It would be possible to introduce a typedef
>> to allow the qemu code to refer to it by a differently-spelled name,
>> but that would obfuscate more than it would clarify.
>
> This patch has been posted in substantially similar form quite a few
> times now. Paolo Bonzini understandably suggested that renaming the
> variable would be better but that's not within qemu's bailiwick as I
> say above.
>
> I think everything else in this series has a review and/or an ack.
> So I would like to send a pull request.
>
> Does someone want to review this patch ? Should I drop it and just
> let checkpatch complain ? Shold I include it in my pull request
> anyway ?
If no one has commented on what seems pretty trivial (especially since
checkpatch.pl has no official maintainer, but is more of a
"whoever-touched-it-last" file at the moment), then including the patch
in your pull request is perfectly acceptable. As a maintainer, it is
also perfectly acceptable for you to ignore false positives from
checkpatch (although documenting it in the commit message and/or cover
letter never hurts, when you are intentionally ignoring a false positive).
But, as there has also been a recent patch to teach checkpatch about
glib types [1], your patch makes sense (any merge conflict between your
patch and that one will be obvious to resolve). So on that grounds,
Reviewed-by: Eric Blake
[1] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg04179.html
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH 0/2] xen-hvm: use new resource mapping API
On 05/09/2018 11:05 AM, Paul Durrant wrote: xenforeignmemory_map_resource() to map ioreq pages... ERROR: spaces required around that '*' (ctx:WxV) #164: FILE: include/hw/xen/xen_common.h:128: +xenforeignmemory_handle *fmem, domid_t domid, unsigned int type, ^ total: 1 errors, 0 warnings, 138 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. This style warning appears to be spurious. Yep, and it's because xenforeignmemory_handle doesn't follow our usual conventions for a type name. See commit 5ac067a if you want to add it to the list of whitelisted exception type names, to silence messages like this. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] checkpatch: generalize xen handle matching in the list of types
On 05/10/2018 04:15 AM, Paul Durrant wrote:
All the xen stable APIs define handle types of the form:
_handle
and some define additional handle types of the form:
__handle
Maybe worth mentioning that always has a 'xen' prefix,
and/or spelling it:
xen_handle
xen__handle
Examples of these are xenforeignmemory_handle and
xenforeignmemory_resource_handle.
Both of these types will be misparsed by checkpatch if they appear as the
first token in a line since, as types defined by an external library, they
do not conform to the QEMU CODING_STYLE, which suggests CamelCase.
A previous patch (5ac067a24a8) added xendevicemodel_handle to the list
of types. This patch changes that to xen\w+_handle such that it will
match all Xen stable API handles of the forms detailed above.
Nice use of a regex.
Signed-off-by: Paul Durrant
---
Cc: Eric Blake
Cc: Paolo Bonzini
Cc: Daniel P. Berrange
v2:
- New in this version
Reviewed-by: Eric Blake
---
scripts/checkpatch.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5b8735defb..98ed799f29 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -266,7 +266,7 @@ our @typeList = (
qr{target_(?:u)?long},
qr{hwaddr},
qr{xml${Ident}},
- qr{xendevicemodel_handle},
+ qr{xen\w+_handle},
);
# This can be modified by sub possible. Since it can be empty, be careful
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH v2 3/3] xen-hvm: try to use xenforeignmemory_map_resource() to map ioreq pages
On 05/15/2018 11:16 AM, Anthony PERARD wrote: +errno = EOPNOTSUPP; I think ENOSYS would be better. EOPNOTSUPP seems to be for sockets. No, EOPNOTSUPP is more general than that and is convention for unimplemented API operations elsewhere. ENOSYS is supposed to strictly mean 'system call not implemented' but we use it for hypercalls in Xen, leading to occasional fun with Linux checkpatch.pl. In man errno, I have: ENOTSUP Operation not supported (POSIX.1-2001) EOPNOTSUPP Operation not supported on socket (POSIX.1-2001). POSIX allows (and Linux exploits) ENOTSUP and EOPNOTSUPP to be synonyms for the same error value. I somewhat prefer the ENOTSUP spelling; and it's probably a bit nicer between the two when porting to platforms where the two spellings are not synonyms. ENOSYS Function not implemented (POSIX.1-2001). But I guess any of these would work. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""
On 03/19/2018 08:54 PM, Michael S. Tsirkin wrote: QEMU coding style at the moment asks for all non-system include files to be used with #include "foo.h". [I'm replying without having read the rest of the thread, so bear with me if I repeat some of the other comments that have already been made] And Markus even just did a cleanup along those lines. However this rule actually does not make sense and creates issues for when the included file is generated. In C, include "file" means look in current directory, then on include search path. Current directory here means the source file directory. By comparison include means look on include search path. It's also nice when "file" means file belonging to our project, and means 3rd-party file. So we have to choose which semantics are easier; perhaps better Makefile rules that prevent us from seeing stale files is a better solution than figuring out which files are generated. As generated files are not in the search directory (unless the build directory happens to match the source directory), Why can't we fix Makefile to include BOTH the build and the source directories (to pick up generated files first, and then version-controlled files), and possibly include logic to simplify to a single -I instead of two when doing in-tree builds? it does not make sense to include them with "" - doing so is merely more work for preprocessor and a source or errors if a stale file happens to exist in the source directory. This changes include directives for all generated files, across the tree. The idea is to avoid sending a huge amount of email. But when merging, the changes will be split with one commit per file, e.g. for ease of bisect in case of build failures, and to ease merging. Note that should some generated files be missed by this tree-wide refactoring, it isn't a big deal - this merely maintains the status quo, and this can be addressed by a separate patch on top. I'm not sure I'm a fan of this patch yet, if there may be other alternatives that solve the real issue of stale generated files breaking incremental builds. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/2] make: move generated headers to qemu-build/
On 03/22/2018 02:27 PM, Michael S. Tsirkin wrote: Make sure all generated files go into qemu-build subdirectory. We can then include them like this: #include "qemu-build/trace.h" This serves two purposes: - make it easy to detect which files are in the source directory (a bit more work for writers, easier for readers) - reduce chances of conflicts with possible stale files in source directory (which could be left over from e.g. old patches, etc) This patch needs to be merged with patch 2 of series updating all files: sending it separately to avoid spamming the list. Signed-off-by: Michael S. Tsirkin --- +++ b/Makefile @@ -89,102 +89,102 @@ endif include $(SRC_PATH)/rules.mak -GENERATED_FILES = qemu-version.h config-host.h qemu-options.def -GENERATED_FILES += qapi/qapi-builtin-types.h qapi/qapi-builtin-types.c Uggh - I really need to follow up on my threat to make smarter use of make variables and string manipulation to cut down on the boilerplate involved here. Sadly, I'm not convinced that doing so is a 2.12 bugfix priority, so it isn't at the top of my work queue. Overall, the patch is an interesting idea. I'm still not 100% sold on it (as you say, it's now slightly more work for writers), but I'm not coming up with any solid reasons why it should not be applied (at least, for 2.13 - doing it during freeze for 2.12 is a bit harder to justify). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [libvirt] Likely build race, "/usr/bin/ld: cannot find -lvirt"
On 06/12/2018 06:11 AM, Jiri Denemark wrote: I hit the same race twice on aarch64 and ppc64 and I can confirm the installation phase fails if libvirt.la is installed later than libraries which link to it. However, the dependencies seem to be set correctly in the Makefiles. But it looks like they are only honored when linking the library during the build phase. During make install libvirt.la and libraries which link to it are installed independently. That is, install-modLTLIBRARIES does not depend on anything except for the mod_LTIBRARIES themselves. Thus when libtool decides to relink the libraries libvirt.la may still be missing at this point. Manually changing install-modLTLIBRARIES: $(mod_LTLIBRARIES) to install-modLTLIBRARIES: $(mod_LTLIBRARIES) install-libLTLIBRARIES fixed the problem for me (tested with an artificial delay added to install-libLTLIBRARIES target), but I have no idea how to persuade automake to generate something like that for us. Eric, is my investigation correct and do you have any ideas on how to fix the race? Can you add that line directly into Makefile.am, or does doing that cause automake to complain and/or omit its normal rules because it thinks you are overriding its defaults? I know that getting automake to add a dependency is not always trivial, but that it should be possible (my strengths lie more on autoconf than on automake). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH v4 11/40] hw/xen: Use the IEC binary prefix definitions
On 06/12/2018 03:51 PM, Richard Henderson wrote: On 06/10/2018 03:14 PM, Philippe Mathieu-Daudé wrote: xen_pv_printf(xendev, 1, "type \"%s\", fileproto \"%s\", filename \"%s\"," - " size %" PRId64 " (%" PRId64 " MB)\n", + " size %" PRId64 " (%llu MB)\n", blkdev->type, blkdev->fileproto, blkdev->filename, - blkdev->file_size, blkdev->file_size >> 20); + blkdev->file_size, blkdev->file_size / MiB); Having to change printf markup is exactly why you shouldn't use ULL in MiB. Conversely, M_BYTE was already ULL, so if you don't use it in MiB, you'll have to change other printf markup where you were changing those uses. One benefit of using the widest possible type: we avoid risk of silent truncation. Potential downsides: wasted processing time (when 32 bits was sufficient), and compilers might start warning when we narrow a 64-bit value into a 32-bit variable (but I think we already ignore that). One benefit of using the natural type that holds the value: use of 64-bit math is explicit based on the type of what else is being multiplied by the macro. Potential downside: 32*32 assigned to a 64-bit result may be botched (but hopefully Coverity will flag it). So there's tradeoffs either way, and you at least need to document in your commit messages what auditing you have done that any type changes introduced by your changes are safe. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH v4 11/40] hw/xen: Use the IEC binary prefix definitions
On 06/12/2018 04:10 PM, Richard Henderson wrote: So there's tradeoffs either way, and you at least need to document in your commit messages what auditing you have done that any type changes introduced by your changes are safe. I'm more concerned about unnecessary or unintended signed vs unsigned changes than I am about width. But if we're going to force a 64-bit type, use (int64_t)1 not 1LL. That way the type will match the existing PRId64 printf markup. Or spell it UINT64_C(1) if you don't want a cast. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH] RFC: chardev: mark the calls that allow an implicit mux monitor
On 08/22/2018 01:02 PM, Marc-André Lureau wrote:
This is mostly for readability of the code. Let's make it clear which
callers can create an implicit monitor when the chardev is muxed.
This will also enforce a safer behaviour, as we don't really support
creating monitor anywhere/anytime at the moment.
There are documented cases, such as: -serial/-parallel/-virtioconsole
and to less extent -debugcon.
Less obvious and questionnable ones are -gdb and Xen console. Add a
s/questionnable/questionable/
FIXME note for those, but keep the support for now.
Other qemu_chr_new() callers either have a fixed parameter/filename
string, or do preliminary checks on the string (such as slirp), or do
not need it, such as -qtest.
On a related note, the list of monitor creation places:
- the chardev creators listed above: all from command line (except
perhaps Xen console?)
- -gdb & hmp gdbserver will create a "GDB monitor command" chardev
that is wired to an HMP monitor.
- -mon command line option
From this short study, I would like to think that a monitor may only
be created in the main thread today, though I remain skeptical :)
Signed-off-by: Marc-André Lureau
---
include/chardev/char.h | 18 +-
chardev/char.c | 21 +
gdbstub.c | 6 +-
hw/char/xen_console.c | 5 -
vl.c | 8
5 files changed, 47 insertions(+), 11 deletions(-)
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 6f0576e214..be2b9b817e 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -105,6 +105,7 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
* @qemu_chr_new:
*
* Create a new character backend from a URI.
+ * Do not implicitely initialize a monitor if the chardev is muxed.
s/implicitely/implicitly/
*
* @label the name of the backend
* @filename the URI
@@ -113,6 +114,19 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
*/
Chardev *qemu_chr_new(const char *label, const char *filename);
+/**
+ * @qemu_chr_new_mux_mon:
+ *
+ * Create a new character backend from a URI.
+ * Implicitely initialize a monitor if the chardev is muxed.
and again
+++ b/gdbstub.c
@@ -2038,7 +2038,11 @@ int gdbserver_start(const char *device)
sigaction(SIGINT, &act, NULL);
}
#endif
-chr = qemu_chr_new_noreplay("gdb", device);
+/*
+ * FIXME: it's a bit weird to allow using a mux chardev here
+ * and setup implicitely a monitor. We may want to break this.
s/setup implicitely/implicitly set up/
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PULL 29/30] Fix up dangling references to qmp-commands.* in comment and doc
From: Markus Armbruster
Fix up the reference to qmp-commands.hx in qmp.c. Missed in commit
5032a16d1d.
Fix up the reference to qmp-commands.txt in
docs/xen-save-devices-state.txt. Missed in commit 4d8bb958fa.
Signed-off-by: Markus Armbruster
Message-Id: <20180211093607.27351-29-arm...@redhat.com>
Reviewed-by: Eric Blake
Reviewed-by: Marc-André Lureau
Reviewed-by: Michael Roth
Signed-off-by: Eric Blake
---
docs/xen-save-devices-state.txt | 3 +--
qmp.c | 14 +++---
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/docs/xen-save-devices-state.txt b/docs/xen-save-devices-state.txt
index a72ecc80818..1912ecad258 100644
--- a/docs/xen-save-devices-state.txt
+++ b/docs/xen-save-devices-state.txt
@@ -8,8 +8,7 @@ These operations are normally used with migration (see
migration.txt),
however it is also possible to save the state of all devices to file,
without saving the RAM or the block devices of the VM.
-This operation is called "xen-save-devices-state" (see
-qmp-commands.txt)
+The save operation is available as QMP command xen-save-devices-state.
The binary format used in the file is the following:
diff --git a/qmp.c b/qmp.c
index a8d4eba973e..ba82e1df9f4 100644
--- a/qmp.c
+++ b/qmp.c
@@ -147,13 +147,13 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
#ifndef CONFIG_SPICE
/*
- * qmp-commands.hx ensures that QMP command query-spice exists only
- * #ifdef CONFIG_SPICE. Necessary for an accurate query-commands
- * result. However, the QAPI schema is blissfully unaware of that,
- * and the QAPI code generator happily generates a dead
- * qmp_marshal_query_spice() that calls qmp_query_spice(). Provide it
- * one, or else linking fails. FIXME Educate the QAPI schema on
- * CONFIG_SPICE.
+ * qmp_unregister_commands_hack() ensures that QMP command query-spice
+ * exists only #ifdef CONFIG_SPICE. Necessary for an accurate
+ * query-commands result. However, the QAPI schema is blissfully
+ * unaware of that, and the QAPI code generator happily generates a
+ * dead qmp_marshal_query_spice() that calls qmp_query_spice().
+ * Provide it one, or else linking fails. FIXME Educate the QAPI
+ * schema on CONFIG_SPICE.
*/
SpiceInfo *qmp_query_spice(Error **errp)
{
--
2.14.3
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
!4uZP)ZEhe?LdZ@5QIJ5?Hg+mgxS918!HgAZQt>Y
ceSHN~X?i|K5fhYsvykI97nHrFhGPaN0Hp#a^8f$<
delta 62
zcmaFWqjaW6siB3jg{g(Pg=Gt?!ISN#Pguo-rU!guEowjhjTMO5wjck-zP@66bv{QC
S)Amn=9Jjf)U#j7l!3h9Zj2qGb
diff --git a/pc-bios/opensbi-riscv64-generic-fw_dynamic.elf
b/pc-bios/opensbi-riscv64-generic-fw_dynamic.elf
index
642a64e240d09b2ddb9fc12c74718ae8d386b9d3..9cf2cf23b747cb5be1d3389a0611d8697609c6f8
100644
GIT binary patch
delta 102
zcmeBpue$8LYC{WS3sVbo3(FSPMGsgDQ*%q>w*wh6LJ;=!eV!
e4L)`&8@K>*RVYvZ
delta 66
zcmZ4XUbW-BYC{WS3sVbo3(FSPMGv+wf50juH2uUU)}nU%&#XYq2E^>!?LTwO&)NPs
Up0kK)dsGtVajxxZxttAL0Np
Does the result of sphinx still look good, as mentioned in this comment?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [Xen-devel] [PATCH v3 01/20] scripts/git.orderfile: Display Cocci scripts before code modifications
On 2/20/20 7:05 AM, Philippe Mathieu-Daudé wrote: When we use a Coccinelle semantic script to do automatic code modifications, it makes sense to look at the semantic patch first. Signed-off-by: Philippe Mathieu-Daudé --- scripts/git.orderfile | 3 +++ 1 file changed, 3 insertions(+) Reviewed-by: Eric Blake diff --git a/scripts/git.orderfile b/scripts/git.orderfile index 1f747b583a..7cf22e0bf5 100644 --- a/scripts/git.orderfile +++ b/scripts/git.orderfile @@ -22,6 +22,9 @@ Makefile* qapi/*.json qga/*.json +# semantic patches +*.cocci + # headers *.h -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 02/20] hw: Remove unnecessary cast when calling dma_memory_read()
On 2/20/20 7:05 AM, Philippe Mathieu-Daudé wrote: Since its introduction in commit d86a77f8abb, dma_memory_read() always accepted void pointer argument. Remove the unnecessary casts. This commit was produced with the included Coccinelle script scripts/coccinelle/exec_rw_const. Signed-off-by: Philippe Mathieu-Daudé --- scripts/coccinelle/exec_rw_const.cocci | 15 +++ hw/arm/smmu-common.c | 3 +-- hw/arm/smmuv3.c| 10 -- hw/sd/sdhci.c | 15 +-- 4 files changed, 25 insertions(+), 18 deletions(-) create mode 100644 scripts/coccinelle/exec_rw_const.cocci diff --git a/scripts/coccinelle/exec_rw_const.cocci b/scripts/coccinelle/exec_rw_const.cocci new file mode 100644 index 00..a0054f009d --- /dev/null +++ b/scripts/coccinelle/exec_rw_const.cocci @@ -0,0 +1,15 @@ +// Usage: +// spatch --sp-file scripts/coccinelle/exec_rw_const.cocci --dir . --in-place This command line should also use '--macro-file scripts/cocci-macro-file.h' to cover more of the code base (Coccinelle skips portions of the code that uses macros it doesn't recognize). @@ -726,13 +724,10 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr) } break; case SDHC_CTRL_ADMA2_64: -dma_memory_read(s->dma_as, entry_addr, -(uint8_t *)(&dscr->attr), 1); -dma_memory_read(s->dma_as, entry_addr + 2, -(uint8_t *)(&dscr->length), 2); +dma_memory_read(s->dma_as, entry_addr, (&dscr->attr), 1); +dma_memory_read(s->dma_as, entry_addr + 2, (&dscr->length), 2); The () around &dscr->length are now pointless. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 01/11] qapi/error: add (Error **errp) cleaning APIs
On 2/21/20 3:20 AM, Vladimir Sementsov-Ogievskiy wrote:
+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
These appear to be unused apart from the Coccinelle script in PATCH 03.
They are used in the full "[RFC v5 000/126] error: auto propagated
local_err" series. Options:
1. Pick a few more patches into this part I series, so these guys come
with users.
It needs some additional effort for this series.. But it's possible. Still,
I think that we at least should not pull out patches which should be in
future series (for example from ppc or block/)..
2. Punt this patch to the first part that has users, along with the
part of the Coccinelle script that deals with them.
But coccinelle script would be wrong, if we drop this part from it. I
think,
that after commit which adds coccinelle script, it should work with any
file,
not only subset of these series.
So, it's probably OK for now to drop these functions, forcing their
addition if
coccinelle script will be applied where these functions are needed. We
can, for
example comment these three functions.
Splitting coccinelle script into two parts, which will be in different
series will
not help any patch-porting processes.
Splitting the coccinelle script across multiple patches is actually
quite reviewable, and still easy to backport. Consider this series by
Philippe:
https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg05554.html
which makes multiple additions to scripts/coccinelle/exec_rw_const.cocci
over the course of the series.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 02/11] error: auto propagated local_err
On 2/21/20 3:42 AM, Vladimir Sementsov-Ogievskiy wrote:
+#define ERRP_AUTO_PROPAGATE() \
+ g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
+ errp = ((errp == NULL || *errp == error_fatal) \
+ ? &_auto_errp_prop.local_err : errp)
+
/*
* Special error destination to abort on error.
* See error_setg() and error_propagate() for details.
*errp == error_fatal tests *errp == NULL, which is not what you want.
You need to test errp == &error_fatal, just like error_handle_fatal().
Oops, great bug) And nobody noticed before) Of course, you are right.
Sorry I missed it in my earlier looks.
Superfluous parenthesis around the first operand of ?:.
Wouldn't
#define ERRP_AUTO_PROPAGATE() \
g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
if (!errp || errp == &error_fatal) { \
errp = &_auto_errp_prop.local_err; \
}
be clearer?
Hmm, notation with "if" will allow omitting ';' after macro invocation,
which seems not good..
Then wrap it:
g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
do { \
if (!errp || errp == &error_fata) {
errp = &_auto_errp_prop.local_err; \
} \
while (0)
And if I'm not wrong we've already discussed it somewhere in previous
versions.
The original use of ?: stems from my suggestion on an earlier revision
when we were still trying to pack everything into two consecutive
declaration lines, rather than a declaration and a statement (as ?: is
necessary for conditionals in declarations). But since then, we decided
to go with a statement (we require a C99 compiler, so declaration after
statement is supported by our compiler, even if our coding style
currently avoids it where possible), so as long as we support
statements, we might as well go with a legible statement instead of
insisting on the compact ?: form.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v8 01/10] error: auto propagated local_err
t 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, &err);
+ * 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;
+ * }
+ * ...
+ * }
Again, I'm thinking the above example is more useful in the commit
message instead of permanently in the .h file.
*/
#ifndef ERROR_H
@@ -322,6 +419,46 @@ 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 first line of a function which use
+ * Error **errp parameter to report error. It's needed only in cases where we
+ * want to use error_prepend, error_append_hint or dereference *errp. It's
+ * still safe (but useless) in other cases.
This macro exists to assist with proper error handling in a function
which uses an Error **errp parameter. It must be used as the first line
of a function which modifies an error (with error_prepend,
error_append_hint, or similar) or which wants to dereference *errp. It
is still safe (but useless) to use in other functions.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to add information by error_prepend or
+ * error_append_hint (as, if it was error_fatal, we swapped it with a
+ * local_error to be propagated on cleanup).
+ *
+ * Note: we don't wrap the error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.
+ */
+#define ERRP_AUTO_PROPAGATE() \
+g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
+do { \
+if (!errp || errp == &error_fatal) { \
+errp = &_auto_errp_prop.local_err; \
+} \
+} while (0)
+
/*
* Special error destination to abort on error.
* See error_setg() and error_propagate() for details.
The macro itself looks correct. I'll leave it up to Markus how to handle
the comment text, but you can add:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v8 02/10] scripts: add coccinelle script to use auto propagated errp
On 3/5/20 11:15 PM, Vladimir Sementsov-Ogievskiy wrote:
Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
does corresponding changes in code (look for details in
include/qapi/error.h)
Usage example:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
I'll let Markus do the final review of this, but my personal take is
that if the subsequent patches created by using this script are
reasonable, then this script was good enough.
+// Always use the same patter for checking error
pattern
+@@
+identifier rule1.fn;
+symbol errp;
+@@
+
+ fn(...)
+ {
+ <...
+-*errp != NULL
++*errp
+ ...>
+ }
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
t;< check2.fn;
p1 << check2.p1;
p2 << check2.p2;
@@
print('Warning: function {} propagates to errp several times in '
'one control flow: at {}:{} and then at {}:{}'.format(
fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line))
// Convert special case with goto separately.
// I tried merging this into the following rule the obvious way, but
// it made Coccinelle hang on block.c
//
// Note interesting thing: if we don't do it here, and try to fixup
// "out: }" things later after all transformations (the rule will be
// the same, just without error_propagate() call), coccinelle fails to
// match this "out: }".
@ disable optional_qualifier@
identifier fn, rule1.local_err, out;
symbol errp;
@@
fn(..., Error ** , ...)
{
<...
-goto out;
+return;
...>
- out:
-error_propagate(errp, local_err);
}
// Convert most of local_err related stuff.
//
// Note, that we update everything related to matched by rule1
either 'related to' or 'matched by', but not both.
// function name and local_err name. We may match something not
// related to the pattern matched by rule1. For example, local_err may
// be defined with the same name in different blocks inside one
// function, and in one block follow the propagation pattern and in
// other block doesn't. Or we may have several functions with the same
// name (for different configurations).
//
// Note also that errp-cleaning functions
// error_free_errp
// error_report_errp
// error_reportf_errp
// warn_report_errp
// warn_reportf_errp
// are not yet implemented. They must call corresponding Error* -
// freeing function and then set *errp to NULL, to avoid further
// propagation to original errp (consider ERRP_AUTO_PROPAGATE in use).
Do we need this part of the patch if we aren't using it? Or can it be
added incrementally later when we actually do have those functions added?
// For example, error_free_errp may look like this:
//
//void error_free_errp(Error **errp)
//{
//error_free(*errp);
//*errp = NULL;
//}
@ disable optional_qualifier exists@
identifier fn, rule1.local_err;
expression list args;
symbol errp;
@@
fn(..., Error ** , ...)
{
<...
(
-Error *local_err = NULL;
|
// Convert error clearing functions
(
-error_free(local_err);
+error_free_errp(errp);
|
-error_report_err(local_err);
+error_report_errp(errp);
|
-error_reportf_err(local_err, args);
+error_reportf_errp(errp, args);
|
-warn_report_err(local_err);
+warn_report_errp(errp);
|
-warn_reportf_err(local_err, args);
+warn_reportf_errp(errp, args);
)
?-local_err = NULL;
|
-error_propagate_prepend(errp, local_err, args);
+error_prepend(errp, args);
|
-error_propagate(errp, local_err);
|
-&local_err
+errp
)
...>
}
// Convert remaining local_err usage. For example, different kinds of
// error checking in if conditionals. We can't merge this into
// previous hunk, as this conflicts with other substitutions in it (at
// least with "- local_err = NULL").
@ disable optional_qualifier@
identifier fn, rule1.local_err;
symbol errp;
@@
fn(..., Error ** , ...)
{
<...
-local_err
+*errp
...>
}
// Always use the same pattern for checking error
@ disable optional_qualifier@
identifier fn;
symbol errp;
@@
fn(..., Error ** , ...)
{
<...
-*errp != NULL
+*errp
...>
}
// Revert temporary ___ identifier.
@ disable optional_qualifier@
identifier fn;
@@
fn(..., Error **
-
+ errp
, ...)
{
...
}
Ultimately, the proof is in the pudding - if we are happy with the
conversion and the warnings produced by this script, and the amount of
manual touchup to address those warnings, then I'm happy to accept the
script even if I didn't fully check what it does (here, I'm trusting
what Vladimir and Markus have been doing in their back-and-forth
refinements of the script).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 01/11] qapi/error: add (Error **errp) cleaning APIs
On 1/10/20 1:41 PM, Vladimir Sementsov-Ogievskiy wrote: Signed-off-by: Vladimir Sementsov-Ogievskiy --- Sparse commit message; it might be nice (for future 'git log' greppability) to at least mention the names of the functions being added. +/* + * Functions to clean Error **errp: call corresponding Error *err cleaning + * function an set pointer to NULL s/ an/, then/ Missing a '.' at the end of the sentence. Otherwise, Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 02/11] error: auto propagated local_err
On 1/10/20 1:41 PM, Vladimir Sementsov-Ogievskiy wrote:
Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with errp OUT parameter.
s/with/with an/
It has three goals:
1. Fix issue with error_fatal & error_prepend/error_append_hint: user
maybe s/&/and/ so it doesn't look like the C & operator.
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
and again
error_abort by local_err+error_propagate, resulting coredump will
s/,/, the/
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
s/allows/allows us/
s/all/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, 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.).
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
---
- * 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, &err);
* if (err) {
@@ -98,6 +98,50 @@
* foo(arg, errp);
* for readability.
*
+ * DEPRECATED* This pattern is deprecated now, use ERRP_AUTO_PROPAGATE macro
s/use/use the/
+ * instead (defined below).
+ * It's deprecated because of two things:
+ *
+ * 1. Issue with error_abort & error_propagate: when we wrap error_abort by
s/&/and/
+ * local_err+error_propagate, resulting coredump will refer to error_propagate
s/,/, the/
+ * and not to the place where error happened.
+ *
+ * 2. A lot of extra code of the same pattern
+ *
+/*
+ * ERRP_AUTO_PROPAGATE
+ *
+ * This macro is created to be the first line of a function which use
+ * Error **errp parameter to report error. It's needed only in cases where we
+ * want to use error_prepend, error_append_hint or dereference *errp. It's
+ * still safe (but useless) in other cases.
It doesn't _have_ to be the first line to compile (we require C99+
compilers, which allow declarations after statements); but rather
because it makes it easier for our Coccinelle conversion script to catch
outliers. But I think this text is okay, without calling out that extra
information (maybe the commit message should mention it, though).
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to add information (by error_prepend or
+ * error_append_hint)
+ * (as, if it was error_fatal, we swapped it with a local_error to be
+ * propagated on cleanup).
double () () looks odd, as does the mid-sentence newline.
+ *
+ * Note: we don't wrap the error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.
+ */
+#define ERRP_AUTO_PROPAGATE() \
+g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
+errp = ((errp == NULL || *errp == error_fatal) \
+? &_auto_errp_prop.local_err : errp)
+
/*
* Special error destination to abort on error.
* See error_setg() and error_propagate() for details.
The macro itself is fine, my comments are solely on the commit message
and comments. Depending on how much cleanup Markus is willing to do
rather than require a respin, you can add:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 03/11] scripts: add coccinelle script to use auto propagated errp
On 1/10/20 1:41 PM, Vladimir Sementsov-Ogievskiy wrote:
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
Rather light on the commit message. If nothing else, a comment about
typical command-line usage would be helpful (yes, it's in the patch
body, but sometimes I just refer to git log).
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 532b9afb9e..dcfb77e107 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -141,6 +141,9 @@
* ...
* }
*
+ * For mass conversion use script
+ * scripts/coccinelle/auto-propagated-errp.cocci
+ *
*
* Receive and accumulate multiple errors (first one wins):
* Error *err = NULL, *local_err = NULL;
diff --git a/scripts/coccinelle/auto-propagated-errp.cocci
b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 00..6c72a5049f
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,139 @@
+// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
+//
+// Usage example:
+// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
+// --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
+// blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
+
+@@
+// Add invocation to errp-functions where necessary
+// We should skip functions with "Error *const *errp"
+// parameter, but how to do it with coccinelle?
+// I don't know, so, I skip them by function name regex.
+// It's safe: if we not skip some functions with
s/not/did not/
+// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
+// will fail to compile, because of const violation.
+identifier fn !~ "error_append_.*_hint";
+identifier local_err, errp;
+@@
+
+ fn(..., Error **errp, ...)
+ {
++ ERRP_AUTO_PROPAGATE();
+<+...
+when != ERRP_AUTO_PROPAGATE();
+(
+error_append_hint(errp, ...);
+|
+error_prepend(errp, ...);
+|
+Error *local_err = NULL;
+)
+...+>
+ }
+
Looks like it should catch all functions that require adding the macro.
+@rule1@
+// We do not inherit from previous rule, as we want to match
+// also functions, which already had ERRP_AUTO_PROPAGATE
+// invocation.
Grammar suggestion:
// We want to patch error propagation in functions regardless of
// whether the function already uses ERRP_AUTO_PROPAGATE, hence
// this one does not inherit from the first rule.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH v2 0/4] nbd/server: Quiesce coroutines on context switch
On 12/14/20 11:05 AM, Sergio Lopez wrote: > This series allows the NBD server to properly switch between AIO contexts, > having quiesced recv_coroutine and send_coroutine before doing the transition. > > We need this because we send back devices running in IO Thread owned contexts > to the main context when stopping the data plane, something that can happen > multiple times during the lifetime of a VM (usually during the boot sequence > or > on a reboot), and we drag the NBD server of the correspoing export with it. > > While there, fix also a problem caused by a cross-dependency between > closing the export's client connections and draining the block > layer. The visible effect of this problem was QEMU getting hung when > the guest request a power off while there's an active NBD client. > > v2: > - Replace "virtio-blk: Acquire context while switching them on > dataplane start" with "block: Honor blk_set_aio_context() context > requirements" (Kevin Wolf) > - Add "block: Avoid processing BDS twice in > bdrv_set_aio_context_ignore()" > - Add "block: Close block exports in two steps" > - Rename nbd_read_eof() to nbd_server_read_eof() (Eric Blake) > - Fix double space and typo in comment. (Eric Blake) ping - where do we stand on this series? Patches 1 and 3 have positive reviews, I'll queue them now; patches 2 and 4 had enough comments that I'm guessing I should wait for a v3? > > Sergio Lopez (4): > block: Honor blk_set_aio_context() context requirements > block: Avoid processing BDS twice in bdrv_set_aio_context_ignore() > nbd/server: Quiesce coroutines on context switch > block: Close block exports in two steps > > block.c | 27 ++- > block/export/export.c | 10 +-- > blockdev-nbd.c | 2 +- > hw/block/dataplane/virtio-blk.c | 4 ++ > hw/block/dataplane/xen-block.c | 7 +- > hw/scsi/virtio-scsi.c | 6 +- > include/block/export.h | 4 +- > nbd/server.c| 120 ++++++++ > qemu-nbd.c | 2 +- > stubs/blk-exp-close-all.c | 2 +- > 10 files changed, 156 insertions(+), 28 deletions(-) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PULL 03/13] block: Honor blk_set_aio_context() context requirements
From: Sergio Lopez
The documentation for bdrv_set_aio_context_ignore() states this:
* The caller must own the AioContext lock for the old AioContext of bs, but it
* must not own the AioContext lock for new_context (unless new_context is the
* same as the current context of bs).
As blk_set_aio_context() makes use of this function, this rule also
applies to it.
Fix all occurrences where this rule wasn't honored.
Suggested-by: Kevin Wolf
Signed-off-by: Sergio Lopez
Message-Id: <20201214170519.223781-2-...@redhat.com>
Reviewed-by: Kevin Wolf
Signed-off-by: Eric Blake
---
hw/block/dataplane/virtio-blk.c | 4
hw/block/dataplane/xen-block.c | 7 ++-
hw/scsi/virtio-scsi.c | 6 --
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 37499c556482..e9050c8987e7 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -172,6 +172,7 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
VirtIOBlockDataPlane *s = vblk->dataplane;
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vblk)));
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+AioContext *old_context;
unsigned i;
unsigned nvqs = s->conf->num_queues;
Error *local_err = NULL;
@@ -214,7 +215,10 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
vblk->dataplane_started = true;
trace_virtio_blk_data_plane_start(s);
+old_context = blk_get_aio_context(s->conf->conf.blk);
+aio_context_acquire(old_context);
r = blk_set_aio_context(s->conf->conf.blk, s->ctx, &local_err);
+aio_context_release(old_context);
if (r < 0) {
error_report_err(local_err);
goto fail_guest_notifiers;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 71c337c7b7e7..3675f8deaf9d 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -725,6 +725,7 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
{
ERRP_GUARD();
XenDevice *xendev = dataplane->xendev;
+AioContext *old_context;
unsigned int ring_size;
unsigned int i;
@@ -808,10 +809,14 @@ void xen_block_dataplane_start(XenBlockDataPlane
*dataplane,
goto stop;
}
-aio_context_acquire(dataplane->ctx);
+old_context = blk_get_aio_context(dataplane->blk);
+aio_context_acquire(old_context);
/* If other users keep the BlockBackend in the iothread, that's ok */
blk_set_aio_context(dataplane->blk, dataplane->ctx, NULL);
+aio_context_release(old_context);
+
/* Only reason for failure is a NULL channel */
+aio_context_acquire(dataplane->ctx);
xen_device_set_event_channel_context(xendev, dataplane->event_channel,
dataplane->ctx, &error_abort);
aio_context_release(dataplane->ctx);
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 9690bc63c860..99ff261cead1 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -849,15 +849,17 @@ static void virtio_scsi_hotplug(HotplugHandler
*hotplug_dev, DeviceState *dev,
VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
VirtIOSCSI *s = VIRTIO_SCSI(vdev);
SCSIDevice *sd = SCSI_DEVICE(dev);
+AioContext *old_context;
int ret;
if (s->ctx && !s->dataplane_fenced) {
if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
return;
}
-virtio_scsi_acquire(s);
+old_context = blk_get_aio_context(sd->conf.blk);
+aio_context_acquire(old_context);
ret = blk_set_aio_context(sd->conf.blk, s->ctx, errp);
-virtio_scsi_release(s);
+aio_context_release(old_context);
if (ret < 0) {
return;
}
--
2.30.0
Re: [PATCH v6 2/5] block: consolidate blocksize properties consistency checks
On 5/27/20 7:45 AM, Roman Kagan wrote: Several block device properties related to blocksize configuration must be in certain relationship WRT each other: physical block must be no smaller than logical block; min_io_size, opt_io_size, and discard_granularity must be a multiple of a logical block. To ensure these requirements are met, add corresponding consistency checks to blkconf_blocksizes, adjusting its signature to communicate possible error to the caller. Also remove the now redundant consistency checks from the specific devices. Signed-off-by: Roman Kagan --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v6 3/5] qdev-properties: blocksize: use same limits in code and description
On 5/27/20 7:45 AM, Roman Kagan wrote: Make it easier (more visible) to maintain the limits on the blocksize properties in sync with the respective description, by using macros both in the code and in the description. Signed-off-by: Roman Kagan --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v6 4/5] block: make size-related BlockConf properties accept size suffixes
On 5/27/20 7:45 AM, Roman Kagan wrote:
Several BlockConf properties represent respective sizes in bytes so it
makes sense to accept size suffixes for them.
Turn them all into uint32_t and use size-suffix-capable setters/getters
on them; introduce DEFINE_PROP_SIZE32 and adjust DEFINE_PROP_BLOCKSIZE
for that. (Making them uint64_t and reusing DEFINE_PROP_SIZE isn't
justified because guests expect at most 32bit values.)
Also, since min_io_size is exposed to the guest by scsi and virtio-blk
devices as an uint16_t in units of logical blocks, introduce an
additional check in blkconf_blocksizes to prevent its silent truncation.
Signed-off-by: Roman Kagan
---
v5 -> v6:
- add prop_size32 instead of going with 64bit
Would it be worth adding prop_size32 as its own patch, before using it
here? But I'll review this as-is.
+++ b/hw/block/block.c
@@ -96,6 +96,17 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
return false;
}
+/*
+ * all devices which support min_io_size (scsi and virtio-blk) expose it to
+ * the guest as a uint16_t in units of logical blocks
+ */
+if (conf->min_io_size > conf->logical_block_size * UINT16_MAX) {
This risks overflow. Better would be:
if (conf->min_io_size / conf->logical_block-size > UINT16_MAX)
+error_setg(errp,
+ "min_io_size must not exceed " stringify(UINT16_MAX)
+ " logical blocks");
+return false;
+}
+
if (!QEMU_IS_ALIGNED(conf->opt_io_size, conf->logical_block_size)) {
error_setg(errp,
"opt_io_size must be a multiple of logical_block_size");
+++ b/tests/qemu-iotests/172.out
@@ -24,11 +24,11 @@ Testing:
dev: floppy, id ""
unit = 0 (0x0)
drive = "floppy0"
-logical_block_size = 512 (0x200)
-physical_block_size = 512 (0x200)
-min_io_size = 0 (0x0)
-opt_io_size = 0 (0x0)
-discard_granularity = 4294967295 (0x)
+logical_block_size = 512 (512 B)
+physical_block_size = 512 (512 B)
+min_io_size = 0 (0 B)
+opt_io_size = 0 (0 B)
+discard_granularity = 4294967295 (4 GiB)
Although 4 GiB is not quite the same as 4294967295, the exact byte value
next to the approximate size is not too bad. The mechanical fallout
from the change from int to size is fine to me.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v6 5/5] block: lift blocksize property limit to 2 MiB
On 5/27/20 7:45 AM, Roman Kagan wrote: Logical and physical block sizes in QEMU are limited to 32 KiB. This appears unnecessary tight, and we've seen bigger block sizes handy unnecessarily at times. Lift the limitation up to 2 MiB which appears to be good enough for everybody, and matches the qcow2 cluster size limit. Signed-off-by: Roman Kagan --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v6 4/5] block: make size-related BlockConf properties accept size suffixes
On 5/27/20 3:53 PM, Roman Kagan wrote: --- v5 -> v6: - add prop_size32 instead of going with 64bit Would it be worth adding prop_size32 as its own patch, before using it here? I've no strong opinion on this. Should I better split it out when respinning? Patch splitting is an art-form. But in general, a long series of smaller patches each easy to review is going to get accepted into the tree faster than a single patch that merges multiple changes into one big blob, even if the net diff is identical. It's rare that someone will ask you to merge patches because you split too far, so the real tradeoff is whether it will cost you more time to split than what you will save the next reviewer (including the maintainer that will merge your patches, depending on whether the maintainer also reviews it or just trusts my review), if you decide to go with a v7. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v7 4/8] qdev-properties: add size32 property type
On 5/28/20 4:39 PM, Roman Kagan wrote:
Introduce size32 property type which handles size suffixes (k, m) just
like size property, but is uint32_t rather than uint64_t.
Does it handle 'g' as well? (even though the set of valid 32-bit sizes
with a g suffix is rather small ;)
It's going to
be useful for properties that are byte sizes but are inherently 32bit,
like BlkConf.opt_io_size or .discard_granularity (they are switched to
this new property type in a followup commit).
The getter for size32 is left out for a separate patch as its benefit is
less obvious, and it affects test output; for now the regular uint32
getter is used.
Signed-off-by: Roman Kagan
---
+static void set_size32(Object *obj, Visitor *v, const char *name, void *opaque,
+ Error **errp)
+{
+DeviceState *dev = DEVICE(obj);
+Property *prop = opaque;
+uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
+uint64_t value;
+Error *local_err = NULL;
+
+if (dev->realized) {
+qdev_prop_set_after_realize(dev, name, errp);
+return;
+}
+
+visit_type_size(v, name, &value, &local_err);
Yes, it does.
Whether or not the commit message is tweaked,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v7 5/8] qdev-properties: make blocksize accept size suffixes
On 5/28/20 4:39 PM, Roman Kagan wrote: It appears convenient to be able to specify physical_block_size and logical_block_size using common size suffixes. Teach the blocksize property setter to interpret them. Also express the upper and lower limits in the respective units. Signed-off-by: Roman Kagan --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v7 6/8] block: make BlockConf size props 32bit and accept size suffixes
On 5/28/20 4:39 PM, Roman Kagan wrote:
Convert all size-related properties in BlockConf to 32bit. This will
allow to accomodate bigger block sizes (in a followup patch).
s/allow to accomodate/accommodate/
This also allows to make them all accept size suffixes, either via
DEFINE_PROP_BLOCKSIZE or via DEFINE_PROP_SIZE32.
Also, since min_io_size is exposed to the guest by scsi and virtio-blk
devices as an uint16_t in units of logical blocks, introduce an
additional check in blkconf_blocksizes to prevent its silent truncation.
Signed-off-by: Roman Kagan
---
+if (conf->min_io_size / conf->logical_block_size > UINT16_MAX) {
+error_setg(errp,
+ "min_io_size must not exceed " stringify(UINT16_MAX)
+ " logical blocks");
On my libc, this results in "must not exceed (65535) logical blocks".
Worse, I could envision a platform where it prints something funky like:
"exceed (2 * (32768) + 1) logical", based on however complex the
definition of UINT16_MAX is. You're better off printing this one with
%d than with stringify().
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v7 7/8] qdev-properties: add getter for size32 and blocksize
On 5/28/20 4:39 PM, Roman Kagan wrote: Add getter for size32, and use it for blocksize, too. In its human-readable branch, it reports approximate size in human-readable units next to the exact byte value, like the getter for 64bit size does. Adjust the expected test output accordingly. Signed-off-by: Roman Kagan --- v6 -> v7: - split out into separate patch [Eric] Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v12 0/8] error: auto propagated local_err part I
On 7/7/20 11:50 AM, Markus Armbruster wrote: To speed things up, I'm taking the liberty to respin Vladimir's series with my documentation amendments. After my documentation work, I'm very much inclined to rename ERRP_AUTO_PROPAGATE() to ERRP_GUARD(). The fact that it propagates below the hood is detail. What matters to its users is that it lets them use @errp more freely. Thoughts? I like it - the shorter name is easier to type. (The rename is a mechanical change, so if we agree to it, we should do it up front to minimize the churn to all the functions where we add use of the macro) Based-on: Message-Id: <20200707160613.848843-1-arm...@redhat.com> Available from my public repository https://repo.or.cz/qemu/armbru.git on branch error-auto. v12: (based on "[PATCH v4 00/45] Less clumsy error checking") 01: Comments merged properly with recent commit "error: Document Error API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE() before its helpers, and touch up style. 01-08: Commit messages tweaked -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()
On 7/7/20 11:50 AM, Markus Armbruster wrote:
From: Vladimir Sementsov-Ogievskiy
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
the 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
Reviewed-by: Paul Durrant
Reviewed-by: Greg Kurz
Reviewed-by: Eric Blake
[Comments merged properly with recent commit "error: Document Error
API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
before its helpers, and touch up style. Commit message tweaked.]
Signed-off-by: Markus Armbruster
---
include/qapi/error.h | 160 ++-
1 file changed, 141 insertions(+), 19 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3fed49747d..c865a7d2f1 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -128,18 +122,26 @@
* handle the error...
* }
* when it doesn't, say a void function:
+ * ERRP_AUTO_PROPAGATE();
+ * foo(arg, errp);
+ * if (*errp) {
+ * handle the error...
+ * }
+ * More on ERRP_AUTO_PROPAGATE() below.
+ *
+ * Code predating ERRP_AUTO_PROPAGATE() still exits, and looks like this:
exists
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
- * error_propagate(errp, err);
+ * error_propagate(errp, err); // deprecated
* }
- * Do *not* "optimize" this to
+ * Avoid in new code. Do *not* "optimize" it to
* foo(arg, errp);
* if (*errp) { // WRONG!
* handle the error...
* }
- * because errp may be NULL!
+ * because errp may be NULL! Guard with ERRP_AUTO_PROPAGATE().
maybe:
because errp may be NULL without the ERRP_AUTO_PROPAGATE() guard.
*
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
@@ -158,6 +160,19 @@
* handle the error...
* }
*
+ * Pass an existing error to the caller:
+ * = Converting to ERRP_AUTO_PROPAGATE() =
+ *
+ * To convert a function to use ERRP_AUTO_PROPAGATE():
+ *
+ * 0. If the Error ** parameter is not named @errp, rename it to
+ *@errp.
+ *
+ * 1. Add an ERRP_AUTO_PROPAGATE() invocation, by convention right at
+ *the beginning of the function. This makes @errp safe to use.
+ *
+ * 2. Replace &err by errp, and err by *errp. Delete local variable
+ *@err.
+ *
+ * 3. Delete error_propagate(errp, *errp), replace
+ *error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
+ *
Why a comma here?
+ * 4. Ensure @errp is valid at return: when you destroy *errp, set
+ *errp = NULL.
+ *
+ * Example:
+ *
+ * bool fn(..., Error **errp)
+ * {
+ * Error *err = NULL;
+ *
+ * foo(arg, &err);
+ * if (err) {
+ * handle the error...
+ * error_propagate(errp, err);
+ * return false;
+ * }
+ * ...
+ * }
+ *
+ * becomes
+ *
+ * bool fn(..., Error **errp)
+ * {
+ * ERRP_AUTO_PROPAGATE();
+ *
+ * foo(arg, errp);
+ * if (*errp) {
+ * handle the error...
+ * return false;
+ * }
+ * ...
+ * }
Do we want the example to show the use of error_free and *errp = NULL?
Otherwise, this is looking good to me. It will need a tweak if we go
with the shorter name ERRP_GUARD, but I like that idea.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v12 2/8] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()
On 7/7/20 11:50 AM, Markus Armbruster wrote:
From: Vladimir Sementsov-Ogievskiy
Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
does corresponding changes in code (look for details in
include/qapi/error.h)
Usage example:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
--max-width 80 FILES...
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Markus Armbruster
Signed-off-by: Markus Armbruster
---
scripts/coccinelle/auto-propagated-errp.cocci | 337 ++
include/qapi/error.h | 3 +
MAINTAINERS | 1 +
3 files changed, 341 insertions(+)
create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
Needs a tweak if we go with ERRP_GUARD. But that's easy.
+
+// Convert special case with goto separately.
+// I tried merging this into the following rule the obvious way, but
+// it made Coccinelle hang on block.c
+//
+// Note interesting thing: if we don't do it here, and try to fixup
+// "out: }" things later after all transformations (the rule will be
+// the same, just without error_propagate() call), coccinelle fails to
+// match this "out: }".
"out: }" is not valid C; would referring to "out: ; }" fare any better?
+@ disable optional_qualifier@
+identifier rule1.fn, rule1.local_err, out;
+symbol errp;
+@@
+
+ fn(..., Error ** , ...)
+ {
+ <...
+-goto out;
++return;
+ ...>
+- out:
+-error_propagate(errp, local_err);
+ }
+
+// Convert most of local_err related stuff.
+//
+// Note, that we inherit rule1.fn and rule1.local_err names, not
+// objects themselves. We may match something not related to the
+// pattern matched by rule1. For example, local_err may be defined with
+// the same name in different blocks inside one function, and in one
+// block follow the propagation pattern and in other block doesn't.
+//
+// Note also that errp-cleaning functions
+// error_free_errp
+// error_report_errp
+// error_reportf_errp
+// warn_report_errp
+// warn_reportf_errp
+// are not yet implemented. They must call corresponding Error* -
+// freeing function and then set *errp to NULL, to avoid further
+// propagation to original errp (consider ERRP_AUTO_PROPAGATE in use).
+// For example, error_free_errp may look like this:
+//
+//void error_free_errp(Error **errp)
+//{
+//error_free(*errp);
+//*errp = NULL;
+//}
I guess we can still decide later if we want these additional functions,
or if they will even help after the number of places we have already
improved after applying this script as-is and with Markus' cleanups in
place.
While I won't call myself a Coccinelle expert, it at least looks sane
enough that I'm comfortable if you add:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v12 7/8] nbd: Use ERRP_AUTO_PROPAGATE()
On 7/7/20 11:50 AM, Markus Armbruster wrote:
From: Vladimir Sementsov-Ogievskiy
If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &error_fatal
(the program will exit prior to the error_append_hint() or
error_prepend() call). Fix such cases.
If we want to check error after errp-function call, we need to
introduce local_err and then propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
&error_fatal, this means that we don't break error_abort
(we'll abort on error_set, not on error_propagate)
This commit is generated by command
sed -n '/^Network Block Device (NBD)$/,/^$/{s/^F: //p}' \
MAINTAINERS | \
xargs git ls-files | grep '\.[hc]$' | \
xargs spatch \
--sp-file scripts/coccinelle/auto-propagated-errp.cocci \
--macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80
Reported-by: Kevin Wolf
Reported-by: Greg Kurz
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Markus Armbruster
[Commit message tweaked]
Signed-off-by: Markus Armbruster
---
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 3/3] Use g_new() & friends where that makes obvious sense
On Mon, Mar 14, 2022 at 05:01:08PM +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...
>
> Signed-off-by: Markus Armbruster
> ---
I agree that this is mechanical, but...
> qga/commands-win32.c | 8 ++---
> qga/commands.c | 2 +-
> qom/qom-qmp-cmds.c | 2 +-
> replay/replay-char.c | 4 +--
> replay/replay-events.c | 10 +++---
> scripts/coverity-scan/model.c| 2 +-
...are we sure we want to touch this particular file?
> diff --git a/scripts/coverity-scan/model.c b/scripts/coverity-scan/model.c
> index 9d4fba53d9..30bea672e1 100644
> --- a/scripts/coverity-scan/model.c
> +++ b/scripts/coverity-scan/model.c
> @@ -356,7 +356,7 @@ int g_poll (GPollFD *fds, unsigned nfds, int timeout)
> typedef struct _GIOChannel GIOChannel;
> GIOChannel *g_io_channel_unix_new(int fd)
> {
> -GIOChannel *c = g_malloc0(sizeof(GIOChannel));
> +GIOChannel *c = g_new0(GIOChannel, 1);
> __coverity_escape__(fd);
> return c;
> }
Our model has a definition of g_malloc0(), but I'm not sure whether
Coverity picks up the macro g_new0() in the same manner.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PULL 04/20] migration/dirtyrate: introduce struct and adjust DirtyRateStat
On Mon, Nov 01, 2021 at 11:08:56PM +0100, Juan Quintela wrote:
> From: Hyman Huang(黄勇)
>
> introduce "DirtyRateMeasureMode" to specify what method should be
> used to calculate dirty rate, introduce "DirtyRateVcpu" to store
> dirty rate for each vcpu.
>
> use union to store stat data of specific mode
>
> Signed-off-by: Hyman Huang(黄勇)
> Message-Id:
> <661c98c40f40e163aa58334337af8f3ddf41316a.1624040308.git.huang...@chinatelecom.cn>
> Reviewed-by: Peter Xu
> Reviewed-by: Juan Quintela
> Signed-off-by: Juan Quintela
> ---
> qapi/migration.json | 30 +++
> migration/dirtyrate.h | 21 +++
> migration/dirtyrate.c | 48 +--
> 3 files changed, 75 insertions(+), 24 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 9aa8bc5759..94eece16e1 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1731,6 +1731,21 @@
> { 'event': 'UNPLUG_PRIMARY',
>'data': { 'device-id': 'str' } }
>
> +##
> +# @DirtyRateVcpu:
> +#
> +# Dirty rate of vcpu.
> +#
> +# @id: vcpu index.
> +#
> +# @dirty-rate: dirty rate.
> +#
> +# Since: 6.1
I'm a bit late on the review, since this pull request is already in.
We'll want a followup patch that changes this to mention 6.2, to
correctly match the release that will first have it. Such a followup
is safe during freeze, since it is doc-only.
> +#
> +##
> +{ 'struct': 'DirtyRateVcpu',
> + 'data': { 'id': 'int', 'dirty-rate': 'int64' } }
> +
> ##
> # @DirtyRateStatus:
> #
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 28/28] docs: update Xen-on-KVM documentation
On Wed, Oct 25, 2023 at 03:50:42PM +0100, David Woodhouse wrote:
> From: David Woodhouse
>
> Add notes about console and network support, and how to launch PV guests.
> Clean up the disk configuration examples now that that's simpler, and
> remove the comment about IDE unplug on q35/AHCI now that it's fixed.
>
> Also update stale avocado test filename in MAINTAINERS.
>
> Signed-off-by: David Woodhouse
> ---
> +Xen paravirtual devices
> +---
> +
> +The Xen PCI platform device is enabled automatically for a Xen guest. This
> +allows a guest to unplug all emulated devices, in order to use paravirtual
> +block and network drivers instead.
> +
> +Those paravirtual Xen block, network (and console) devices can be created
> +through the command line, and/or hot-plugged.
> +
> +To provide a Xen console device, define a character device and then a device
> +of type ``xen-console`` to connect to it. For the Xen console equivalent of
> +the handy ``-serial mon:stdio`` option, for example:
> +
> +.. parsed-literal::
> + -chardev -chardev stdio,mux=on,id=char0,signal=off -mon char0 \\
> + -device xen-console,chardev=char0
Is -chardev supposed to appear twice here?
...
> +
> +Booting Xen PV guests
> +-
> +
> +Booting PV guest kernels is possible by using the Xen PV shim (a version of
> Xen
> +itself, designed to run inside a Xen HVM guest and provide memory management
> +services for one guest alone).
> +
> +The Xen binary is provided as the ``-kernel`` and the guest kernel itself (or
> +PV Grub image) as the ``-initrd`` image, which actually just means the first
> +multiboot "module". For example:
> +
> +.. parsed-literal::
> +
> + |qemu_system| --accel kvm,xen-version=0x40011,kernel-irqchip=split \\
> + -chardev stdio,id=char0 -device xen-console,chardev=char0 \\
> + -display none -m 1G -kernel xen -initrd bzImage \\
> + -append "pv-shim console=xen,pv -- console=hvc0 root=/dev/xvda1" \\
> + -drive file=${GUEST_IMAGE},if=xen
Is the space between -- and console= intentionsl?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH] block: get rid of blk->guest_block_size
On Wed, May 18, 2022 at 02:09:45PM +0100, Stefan Hajnoczi wrote:
> Commit 1b7fd729559c ("block: rename buffer_alignment to
> guest_block_size") noted:
>
> At this point, the field is set by the device emulation, but completely
> ignored by the block layer.
>
> The last time the value of buffer_alignment/guest_block_size was
> actually used was before commit 339064d50639 ("block: Don't use guest
> sector size for qemu_blockalign()").
>
> This value has not been used since 2013. Get rid of it.
>
> Cc: Xie Yongji
> Signed-off-by: Stefan Hajnoczi
> ---
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
On Wed, Nov 29, 2023 at 02:55:42PM -0500, Stefan Hajnoczi wrote: > Protect the Task Management Function BH state with a lock. The TMF BH > runs in the main loop thread. An IOThread might process a TMF at the > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > must be protected by a lock. > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > This avoids more locking to protect the virtqueue and SCSI layer state. Are we trying to get this into 8.2? > > Signed-off-by: Stefan Hajnoczi > --- > include/hw/virtio/virtio-scsi.h | 3 +- > hw/scsi/virtio-scsi.c | 62 ++--- > 2 files changed, 43 insertions(+), 22 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 02/12] tests: remove aio_context_acquire() tests
On Wed, Nov 29, 2023 at 02:55:43PM -0500, Stefan Hajnoczi wrote: > The aio_context_acquire() API is being removed. Drop the test case that > calls the API. > > Signed-off-by: Stefan Hajnoczi > --- > tests/unit/test-aio.c | 67 +-- > 1 file changed, 1 insertion(+), 66 deletions(-) The rest of this series should not hold up 8.2. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 03/12] aio: make aio_context_acquire()/aio_context_release() a no-op
On Wed, Nov 29, 2023 at 02:55:44PM -0500, Stefan Hajnoczi wrote:
> aio_context_acquire()/aio_context_release() has been replaced by
> fine-grained locking to protect state shared by multiple threads. The
> AioContext lock still plays the role of balancing locking in
> AIO_WAIT_WHILE() and many functions in QEMU either require that the
> AioContext lock is held or not held for this reason. In other words, the
> AioContext lock is purely there for consistency with itself and serves
> no real purpose anymore.
>
> Stop actually acquiring/releasing the lock in
> aio_context_acquire()/aio_context_release() so that subsequent patches
> can remove callers across the codebase incrementally.
>
> I have performed "make check" and qemu-iotests stress tests across
> x86-64, ppc64le, and aarch64 to confirm that there are no failures as a
> result of eliminating the lock.
>
> Signed-off-by: Stefan Hajnoczi
> ---
> util/async.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/util/async.c b/util/async.c
> index 8f90ddc304..04ee83d220 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -725,12 +725,12 @@ void aio_context_unref(AioContext *ctx)
>
> void aio_context_acquire(AioContext *ctx)
> {
> -qemu_rec_mutex_lock(&ctx->lock);
> +/* TODO remove this function */
> }
>
> void aio_context_release(AioContext *ctx)
> {
> -qemu_rec_mutex_unlock(&ctx->lock);
> + /* TODO remove this function */
> }
>
> QEMU_DEFINE_STATIC_CO_TLS(AioContext *, my_aiocontext)
> --
> 2.42.0
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH 04/12] graph-lock: remove AioContext locking
On Wed, Nov 29, 2023 at 02:55:45PM -0500, Stefan Hajnoczi wrote: > Stop acquiring/releasing the AioContext lock in > bdrv_graph_wrlock()/bdrv_graph_unlock() since the lock no longer has any > effect. > > The distinction between bdrv_graph_wrunlock() and > bdrv_graph_wrunlock_ctx() becomes meaningless and they can be collapsed > into one function. > > Signed-off-by: Stefan Hajnoczi > --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 05/12] block: remove AioContext locking
On Wed, Nov 29, 2023 at 02:55:46PM -0500, Stefan Hajnoczi wrote:
> This is the big patch that removes
> aio_context_acquire()/aio_context_release() from the block layer and
> affected block layer users.
>
> There isn't a clean way to split this patch and the reviewers are likely
> the same group of people, so I decided to do it in one patch.
>
> Signed-off-by: Stefan Hajnoczi
> ---
> +++ b/block.c
> @@ -7585,29 +7433,12 @@ void coroutine_fn bdrv_co_leave(BlockDriverState *bs,
> AioContext *old_ctx)
>
> void coroutine_fn bdrv_co_lock(BlockDriverState *bs)
> {
> -AioContext *ctx = bdrv_get_aio_context(bs);
> -
> -/* In the main thread, bs->aio_context won't change concurrently */
> -assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> -
> -/*
> - * We're in coroutine context, so we already hold the lock of the main
> - * loop AioContext. Don't lock it twice to avoid deadlocks.
> - */
> -assert(qemu_in_coroutine());
Is this assertion worth keeping in the short term?...
> -if (ctx != qemu_get_aio_context()) {
> -aio_context_acquire(ctx);
> -}
> +/* TODO removed in next patch */
> }
...I guess I'll see in the next patch.
>
> void coroutine_fn bdrv_co_unlock(BlockDriverState *bs)
> {
> -AioContext *ctx = bdrv_get_aio_context(bs);
> -
> -assert(qemu_in_coroutine());
> -if (ctx != qemu_get_aio_context()) {
> -aio_context_release(ctx);
> -}
> +/* TODO removed in next patch */
> }
Same comment.
> +++ b/blockdev.c
> @@ -1395,7 +1352,6 @@ static void external_snapshot_action(TransactionAction
> *action,
> /* File name of the new image (for 'blockdev-snapshot-sync') */
> const char *new_image_file;
> ExternalSnapshotState *state = g_new0(ExternalSnapshotState, 1);
> -AioContext *aio_context;
> uint64_t perm, shared;
>
> /* TODO We'll eventually have to take a writer lock in this function */
I'm guessing removal of the locking gets us one step closer to
implementing this TODO at a later time? Or is it now a stale comment?
Either way, it doesn't affect this patch.
> +++ b/migration/block.c
> @@ -270,7 +270,6 @@ static int mig_save_device_bulk(QEMUFile *f,
> BlkMigDevState *bmds)
>
> if (bmds->shared_base) {
> qemu_mutex_lock_iothread();
> -aio_context_acquire(blk_get_aio_context(bb));
...
> @@ -313,22 +311,10 @@ static int mig_save_device_bulk(QEMUFile *f,
> BlkMigDevState *bmds)
> block_mig_state.submitted++;
> blk_mig_unlock();
>
> -/* We do not know if bs is under the main thread (and thus does
> - * not acquire the AioContext when doing AIO) or rather under
> - * dataplane. Thus acquire both the iothread mutex and the
> - * AioContext.
> - *
> - * This is ugly and will disappear when we make bdrv_* thread-safe,
> - * without the need to acquire the AioContext.
> - */
> -qemu_mutex_lock_iothread();
> -aio_context_acquire(blk_get_aio_context(bmds->blk));
Will conflict, but with trivial resolution, with your other thread
renaming things to qemu_bql_lock().
> +++ b/tests/unit/test-blockjob.c
> -static void test_complete_in_standby(void)
> -{
> @@ -531,13 +402,5 @@ int main(int argc, char **argv)
> g_test_add_func("/blockjob/cancel/standby", test_cancel_standby);
> g_test_add_func("/blockjob/cancel/pending", test_cancel_pending);
> g_test_add_func("/blockjob/cancel/concluded", test_cancel_concluded);
> -
> -/*
> - * This test is flaky and sometimes fails in CI and otherwise:
> - * don't run unless user opts in via environment variable.
> - */
> - if (getenv("QEMU_TEST_FLAKY_TESTS")) {
> -g_test_add_func("/blockjob/complete_in_standby",
> test_complete_in_standby);
> -}
Looks like you ripped out this entire test, because it is no longer
viable. I might have mentioned it in the commit message, or squashed
the removal of this test into the earlier 02/12 patch.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH 06/12] scsi: remove AioContext locking
On Wed, Nov 29, 2023 at 02:55:47PM -0500, Stefan Hajnoczi wrote: > The AioContext lock no longer has any effect. Remove it. > > Signed-off-by: Stefan Hajnoczi > --- > include/hw/virtio/virtio-scsi.h | 14 -- > hw/scsi/scsi-bus.c | 2 -- > hw/scsi/scsi-disk.c | 28 > hw/scsi/virtio-scsi.c | 18 -- > 4 files changed, 4 insertions(+), 58 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 07/12] aio-wait: draw equivalence between AIO_WAIT_WHILE() and AIO_WAIT_WHILE_UNLOCKED()
On Wed, Nov 29, 2023 at 02:55:48PM -0500, Stefan Hajnoczi wrote: > Now that the AioContext lock no longer exists, AIO_WAIT_WHILE() and > AIO_WAIT_WHILE_UNLOCKED() are equivalent. > > A future patch will get rid of AIO_WAIT_WHILE_UNLOCKED(). > > Signed-off-by: Stefan Hajnoczi > --- > include/block/aio-wait.h | 16 > 1 file changed, 4 insertions(+), 12 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 08/12] aio: remove aio_context_acquire()/aio_context_release() API
On Wed, Nov 29, 2023 at 02:55:49PM -0500, Stefan Hajnoczi wrote:
> Delete these functions because nothing calls these functions anymore.
>
> I introduced these APIs in commit 98563fc3ec44 ("aio: add
> aio_context_acquire() and aio_context_release()") in 2014. It's with a
> sigh of relief that I delete these APIs almost 10 years later.
>
> Thanks to Paolo Bonzini's vision for multi-queue QEMU, we got an
> understanding of where the code needed to go in order to remove the
> limitations that the original dataplane and the IOThread/AioContext
> approach that followed it.
>
> Emanuele Giuseppe Esposito had the splendid determination to convert
> large parts of the codebase so that they no longer needed the AioContext
> lock. This was a painstaking process, both in the actual code changes
> required and the iterations of code review that Emanuele eeked out of
s/eeked/eked/
> Kevin and me over many months.
>
> Kevin Wolf tackled multitudes of graph locking conversions to protect
> in-flight I/O from run-time changes to the block graph as well as the
> clang Thread Safety Analysis annotations that allow the compiler to
> check whether the graph lock is being used correctly.
>
> And me, well, I'm just here to add some pizzazz to the QEMU multi-queue
> block layer :). Thank you to everyone who helped with this effort,
> including Eric Blake, code reviewer extraordinaire, and others who I've
> forgotten to mention.
>
> Signed-off-by: Stefan Hajnoczi
> ---
> include/block/aio.h | 17 -----
> util/async.c| 10 --
> 2 files changed, 27 deletions(-)
>
Yay!
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH 09/12] docs: remove AioContext lock from IOThread docs
On Wed, Nov 29, 2023 at 02:55:50PM -0500, Stefan Hajnoczi wrote: > Encourage the use of locking primitives and stop mentioning the > AioContext lock since it is being removed. > > Signed-off-by: Stefan Hajnoczi > --- > docs/devel/multiple-iothreads.txt | 45 +++ > 1 file changed, 15 insertions(+), 30 deletions(-) > > diff --git a/docs/devel/multiple-iothreads.txt > b/docs/devel/multiple-iothreads.txt > index a3e949f6b3..4865196bde 100644 > --- a/docs/devel/multiple-iothreads.txt Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 10/12] scsi: remove outdated AioContext lock comment
On Wed, Nov 29, 2023 at 02:55:51PM -0500, Stefan Hajnoczi wrote: > The SCSI subsystem no longer uses the AioContext lock. Request > processing runs exclusively in the BlockBackend's AioContext since > "scsi: only access SCSIDevice->requests from one thread" and hence the > lock is unnecessary. > > Signed-off-by: Stefan Hajnoczi > --- > hw/scsi/scsi-disk.c | 1 - > 1 file changed, 1 deletion(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 11/12] job: remove outdated AioContext locking comments
On Wed, Nov 29, 2023 at 02:55:52PM -0500, Stefan Hajnoczi wrote: > The AioContext lock no longer exists. > > Signed-off-by: Stefan Hajnoczi > --- > include/qemu/job.h | 20 > 1 file changed, 20 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 12/12] block: remove outdated AioContext locking comments
On Wed, Nov 29, 2023 at 02:55:53PM -0500, Stefan Hajnoczi wrote: > The AioContext lock no longer exists. > > There is one noteworthy change: > > - * More specifically, these functions use BDRV_POLL_WHILE(bs), which > - * requires the caller to be either in the main thread and hold > - * the BlockdriverState (bs) AioContext lock, or directly in the > - * home thread that runs the bs AioContext. Calling them from > - * another thread in another AioContext would cause deadlocks. > + * More specifically, these functions use BDRV_POLL_WHILE(bs), which > requires > + * the caller to be either in the main thread or directly in the home > thread > + * that runs the bs AioContext. Calling them from another thread in another > + * AioContext would cause deadlocks. > > I am not sure whether deadlocks are still possible. Maybe they have just > moved to the fine-grained locks that have replaced the AioContext. Since > I am not sure if the deadlocks are gone, I have kept the substance > unchanged and just removed mention of the AioContext. I'd rather text that may be overly conservative than an omission that could lead to a bug; so I'm okay with your action there. > > Signed-off-by: Stefan Hajnoczi > --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [RFC PATCH 01/10] sysemu: Introduce qemu_security_policy_taint() API
On Thu, Sep 09, 2021 at 01:20:15AM +0200, Philippe Mathieu-Daudé wrote:
> Introduce qemu_security_policy_taint() which allows unsafe (read
> "not very maintained") code to 'taint' QEMU security policy.
>
> The "security policy" is the @SecurityPolicy QAPI enum, composed of:
> - "none" (no policy, current behavior)
> - "warn" (display a warning when the policy is tainted, keep going)
> - "strict" (once tainted, exit QEMU before starting the VM)
>
> The qemu_security_policy_is_strict() helper is also provided, which
> will be proved useful once a VM is started (example we do not want
s/be proved/prove/
> to kill a running VM if an unsafe device is hot-added).
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> qapi/run-state.json | 16 +++
> include/qemu-common.h | 19
> softmmu/vl.c | 67 +++
> qemu-options.hx | 17 +++
> 4 files changed, 119 insertions(+)
>
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 43d66d700fc..b15a107fa01 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -638,3 +638,19 @@
> { 'struct': 'MemoryFailureFlags',
>'data': { 'action-required': 'bool',
> 'recursive': 'bool'} }
> +
> +##
> +# @SecurityPolicy:
> +#
> +# An enumeration of the actions taken when the security policy is tainted.
> +#
> +# @none: do nothing.
> +#
> +# @warn: display a warning.
> +#
> +# @strict: prohibit QEMU to start a VM.
s/to start/from starting/
> +#
> +# Since: 6.2
> +##
> +{ 'enum': 'SecurityPolicy',
> + 'data': [ 'none', 'warn', 'strict' ] }
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [RFC PATCH 02/10] accel: Use qemu_security_policy_taint(), mark KVM and Xen as safe
On Thu, Sep 09, 2021 at 01:20:16AM +0200, Philippe Mathieu-Daudé wrote:
> Add the AccelClass::secure_policy_supported field to classify
> safe (within security boundary) vs unsafe accelerators.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/qemu/accel.h | 5 +
> accel/kvm/kvm-all.c | 1 +
> accel/xen/xen-all.c | 1 +
> softmmu/vl.c | 3 +++
> 4 files changed, 10 insertions(+)
>
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 4f4c283f6fc..895e30be0de 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -44,6 +44,11 @@ typedef struct AccelClass {
> hwaddr start_addr, hwaddr size);
> #endif
> bool *allowed;
> +/*
> + * Whether the accelerator is withing QEMU security policy boundary.
within
> + * See: https://www.qemu.org/contribute/security-process/
> + */
> +bool secure_policy_supported;
> /*
> * Array of global properties that would be applied when specific
> * accelerator is chosen. It works like MachineClass.compat_props
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [RFC PATCH 03/10] block: Use qemu_security_policy_taint() API
On Thu, Sep 09, 2021 at 01:20:17AM +0200, Philippe Mathieu-Daudé wrote:
> Add the BlockDriver::bdrv_taints_security_policy() handler.
> Drivers implementing it might taint the global QEMU security
> policy.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/block/block_int.h | 6 +-
> block.c | 6 ++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f1a54db0f8c..0ec0a5c06e9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -169,7 +169,11 @@ struct BlockDriver {
> int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
>Error **errp);
> void (*bdrv_close)(BlockDriverState *bs);
> -
> +/*
> + * Return %true if the driver is withing QEMU security policy boundary,
within
> + * %false otherwise. See:
> https://www.qemu.org/contribute/security-process/
> + */
> +bool (*bdrv_taints_security_policy)(BlockDriverState *bs);
>
> int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
> Error **errp);
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 3/4] virtio: use defer_call() in virtio_irqfd_notify()
On Thu, Aug 17, 2023 at 11:58:46AM -0400, Stefan Hajnoczi wrote: > virtio-blk and virtio-scsi invoke virtio_irqfd_notify() to send Used > Buffer Notifications from an IOThread. This involves an eventfd > write(2) syscall. Calling this repeatedly when completing multiple I/O > requests in a row is wasteful. > > Use the defer_call() API to batch together virtio_irqfd_notify() calls > made during thread pool (aio=threads), Linux AIO (aio=native), and > io_uring (aio=io_uring) completion processing. > > Behavior is unchanged for emulated devices that do not use > defer_call_begin()/defer_call_end() since defer_call() immediately > invokes the callback when called outside a > defer_call_begin()/defer_call_end() region. > > fio rw=randread bs=4k iodepth=64 numjobs=8 IOPS increases by ~9% with a > single IOThread and 8 vCPUs. iodepth=1 decreases by ~1% but this could > be noise. Detailed performance data and configuration specifics are > available here: > https://gitlab.com/stefanha/virt-playbooks/-/tree/blk_io_plug-irqfd > > This duplicates the BH that virtio-blk uses for batching. The next > commit will remove it. > > Signed-off-by: Stefan Hajnoczi > --- > block/io_uring.c | 6 ++ > block/linux-aio.c | 4 > hw/virtio/virtio.c | 11 ++- > util/thread-pool.c | 5 +++++ > 4 files changed, 25 insertions(+), 1 deletion(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v2 4/4] virtio-blk: remove batch notification BH
On Thu, Aug 17, 2023 at 11:58:47AM -0400, Stefan Hajnoczi wrote: > There is a batching mechanism for virtio-blk Used Buffer Notifications > that is no longer needed because the previous commit added batching to > virtio_notify_irqfd(). > > Note that this mechanism was rarely used in practice because it is only > enabled when EVENT_IDX is not negotiated by the driver. Modern drivers > enable EVENT_IDX. > > Signed-off-by: Stefan Hajnoczi > --- > hw/block/dataplane/virtio-blk.c | 48 + > 1 file changed, 1 insertion(+), 47 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 1/7] migration/rdma: Fix save_page method to fail on polling error
On Thu, Aug 31, 2023 at 03:25:40PM +0200, Markus Armbruster wrote: > qemu_rdma_save_page() reports polling error with error_report(), then > succeeds anyway. This is because the variable holding the polling > status *shadows* the variable the function returns. The latter > remains zero. > > Broken since day one, and duplicated more recently. > > Fixes: 2da776db4846 (rdma: core logic) > Fixes: b390afd8c50b (migration/rdma: Fix out of order wrid) Alas, the curse of immutable git history preserving typos in commit subjects ;) The alternative of rewriting history and breaking SHA references is worse. > Signed-off-by: Markus Armbruster > --- > migration/rdma.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH 7/7] qobject atomics osdep: Make a few macros more hygienic
gt; * constant variants, so we have to dumb things down there.
> */
> +#define PASTE(a, b) a##b
> +#define MIN_INTERNAL(a, b, l) \
> +({ \
> +typeof(1 ? (a) : (b)) PASTE(_a, l) = (a), PASTE(_b, l) = (b); \
> +PASTE(_a, l) < PASTE(_b, l) ? PASTE(_a, l) : PASTE(_b, l); \
> +})
And again.
I think you are definitely on the right track to have all internal
variable declarations within a macro definition use multi-layered
expansion with the help of __COUNTER__ to ensure that the macro's
temporary variable is globally unique; so if you leave it as written,
I could live with:
Reviewed-by: Eric Blake
But if you respin it to pick up any of my suggestions, I'll definitely
spend another review cycle on the result.
If it helps, here's what I ended up using in nbdkit:
https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/unique-name.h#L36
/* https://stackoverflow.com/a/1597129
* https://stackoverflow.com/a/12711226
*/
#define XXUNIQUE_NAME(name, counter) name ## counter
#define XUNIQUE_NAME(name, counter) XXUNIQUE_NAME (name, counter)
#define NBDKIT_UNIQUE_NAME(name) XUNIQUE_NAME (name, __COUNTER__)
https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/minmax.h#L47
#include "unique-name.h"
#undef MIN
#define MIN(x, y) \
MIN_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y))
...
#define MIN_1(x, y, _x, _y) ({ \
__auto_type _x = (x);\
__auto_type _y = (y);\
_x < _y ? _x : _y; \
})
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH 7/7] qobject atomics osdep: Make a few macros more hygienic
;
# 12 "foo.c"
({ typeof (o) _obj0 = (o); _obj0 ? container_of(&_obj0->base, QObject, base) :
NULL; )}
# 22 "foo.c"
({ typeof (o) _obj1 = (o); _obj1 ? container_of(&_obj1->base, QObject, base) :
NULL; )}
So the important part was merely ensuring that __COUNTER__ has a
chance to be expanded PRIOR to the point where ## gets its argument,
and one less layer of indirection was still sufficient for that.
>
> Complication: the innermost macro needs to take all its variable names
> as arguments. The macro above has just one. Macros below have two.
>
> Not quite sure which version is easier to understand.
Proper comments may help; once you realize that you are passing in the
unique variable name(s) to be used as the temporary identifier(s),
rather than an integer that still needs to be glued, then separating
the task of generating name(s) (which is done once per name, instead
of repeated 3 times) makes sense to me. I also like minimizing the
number of times I have to spell out __COUNTER__; wrapping unique name
generation behind a well-named helper macro gives a better view of why
it is needed in the first place.
At any rate, this comment still applies:
> >
> > I think you are definitely on the right track to have all internal
> > variable declarations within a macro definition use multi-layered
> > expansion with the help of __COUNTER__ to ensure that the macro's
> > temporary variable is globally unique; so if you leave it as written,
> > I could live with:
> >
> > Reviewed-by: Eric Blake
> >
> > But if you respin it to pick up any of my suggestions, I'll definitely
> > spend another review cycle on the result.
> >
> > If it helps, here's what I ended up using in nbdkit:
> >
> > https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/unique-name.h#L36
> > /* https://stackoverflow.com/a/1597129
> > * https://stackoverflow.com/a/12711226
> > */
> > #define XXUNIQUE_NAME(name, counter) name ## counter
> > #define XUNIQUE_NAME(name, counter) XXUNIQUE_NAME (name, counter)
> > #define NBDKIT_UNIQUE_NAME(name) XUNIQUE_NAME (name, __COUNTER__)
> >
> > https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/minmax.h#L47
> > #include "unique-name.h"
> >
> > #undef MIN
> > #define MIN(x, y) \
> > MIN_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y))
> >
> > ...
> > #define MIN_1(x, y, _x, _y) ({ \
> > __auto_type _x = (x);\
> > __auto_type _y = (y);\
> > _x < _y ? _x : _y; \
> > })
>
> Thanks!
and so I'm fine leaving it up to you if you want to copy the paradigm
I've seen elsewhere, or if you want to leave it as you first proposed.
The end result is the same, and it is more of a judgment call on which
form is easier for you to reason about.
But as other commenters mentioned, we already have a glue() macro, so
use that instead of PASTE(), no matter what else you choose.
Looking forward to v2!
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH v2 7/7] qobject atomics osdep: Make a few macros more hygienic
On Wed, Sep 20, 2023 at 08:31:49PM +0200, Markus Armbruster wrote:
...
> The only reliable way to prevent unintended variable name capture is
> -Wshadow.
>
> One blocker for enabling it is shadowing hiding in function-like
> macros like
>
> qdict_put(dict, "name", qobject_ref(...))
>
> qdict_put() wraps its last argument in QOBJECT(), and the last
> argument here contains another QOBJECT().
>
> Use dark preprocessor sorcery to make the macros that give us this
> problem use different variable names on every call.
>
> Signed-off-by: Markus Armbruster
> Reviewed-by: Eric Blake
It's changed (for the better) since v1, so I'm re-reviewing.
> ---
> include/qapi/qmp/qobject.h | 11 +--
> include/qemu/atomic.h | 17 -
> include/qemu/compiler.h| 3 +++
> include/qemu/osdep.h | 31 +++
> 4 files changed, 47 insertions(+), 15 deletions(-)
>
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 9003b71fd3..d36cc97805 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -45,10 +45,17 @@ struct QObject {
> struct QObjectBase_ base;
> };
>
> -#define QOBJECT(obj) ({ \
> +/*
> + * Preprocessory sorcery ahead: use a different identifier for the
s/Preprocessory/Preprocessor/ (multiple times in the patch)
> + * local variable in each expansion, so we can nest macro calls
> + * without shadowing variables.
> + */
> +#define QOBJECT_INTERNAL(obj, _obj) ({ \
> typeof(obj) _obj = (obj); \
> -_obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
> +_obj\
> +? container_of(&(_obj)->base, QObject, base) : NULL;\
As pointed out before, you can write &_obj->base instead of
&(_obj)->base, now that we know _obj is a single identifier rather
than an arbitrary expression. Not strictly necessary since the extra
() doesn't change semantics...
> })
> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), MAKE_IDENTFIER(_obj))
>
> /* Required for qobject_to() */
> #define QTYPE_CAST_TO_QNull QTYPE_QNULL
> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
> index d95612f7a0..d4cbd01909 100644
> --- a/include/qemu/atomic.h
> +++ b/include/qemu/atomic.h
> @@ -157,13 +157,20 @@
> smp_read_barrier_depends();
> #endif
>
> -#define qatomic_rcu_read(ptr) \
> -({ \
> +/*
> + * Preprocessory sorcery ahead: use a different identifier for the
> + * local variable in each expansion, so we can nest macro calls
> + * without shadowing variables.
> + */
> +#define qatomic_rcu_read_internal(ptr, _val)\
> +({ \
> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
> -typeof_strip_qual(*ptr) _val; \
> -qatomic_rcu_read__nocheck(ptr, &_val); \
> -_val; \
> +typeof_strip_qual(*ptr) _val; \
> +qatomic_rcu_read__nocheck(ptr, &_val); \
...but it looks odd for the patch to not be consistent on that front.
> +_val; \
> })
> +#define qatomic_rcu_read(ptr) \
> +qatomic_rcu_read_internal((ptr), MAKE_IDENTFIER(_val))
>
> #define qatomic_rcu_set(ptr, i) do { \
> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index a309f90c76..03236d830c 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -37,6 +37,9 @@
> #define tostring(s) #s
> #endif
>
> +/* Expands into an identifier stemN, where N is another number each time */
> +#define MAKE_IDENTFIER(stem) glue(stem, __COUNTER__)
I like how this turned out.
With the spelling fix, and optionally with the redundant () dropped,
you can keep my R-b.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH v6 1/5] qapi: net: Add query-netdev command
On 3/2/21 12:02 PM, Alexey Kirillov wrote:
> The query-netdev command is used to get the configuration of the current
> network device backends (netdevs).
> This is the QMP analog of the HMP command "info network" but only for netdevs
> (i.e. excluding NIC and hubports).
>
> The query-netdev command returns an array of objects of the NetdevInfo type,
> which are an extension of Netdev type. It means that response can be used for
> netdev-add after small modification. This can be useful for recreate the same
> netdev configuration.
>
> Information about the network device is filled in when it is created or
> modified and is available through the NetClientState->stored_config.
>
> Signed-off-by: Alexey Kirillov
> Acked-by: Markus Armbruster
> ---
> +++ b/net/net.c
>
> +NetdevInfoList *qmp_query_netdev(Error **errp)
> +{
> +NetdevInfoList *list = NULL;
> +NetClientState *nc;
> +
> +QTAILQ_FOREACH(nc, &net_clients, next) {
> +/*
> + * Only look at netdevs (backend network devices), not for each queue
> + * or NIC / hubport
> + */
> +if (nc->stored_config) {
> +NetdevInfoList *node = g_new0(NetdevInfoList, 1);
Please use QAPI_LIST_PREPEND instead of open-coding it.
> +
> +node->value = QAPI_CLONE(NetdevInfo, nc->stored_config);
> +g_free(node->value->id); /* Need to dealloc default empty id */
> +node->value->id = g_strdup(nc->name);
> +
> +node->value->has_peer_id = nc->peer != NULL;
> +if (node->value->has_peer_id) {
> +node->value->peer_id = g_strdup(nc->peer->name);
> +}
> +
> +node->next = list;
> +list = node;
> +}
> +}
> +
> +++ b/net/slirp.c
> @@ -345,6 +345,14 @@ static SaveVMHandlers savevm_slirp_state = {
> .load_state = net_slirp_state_load,
> };
>
> +#define APPEND_STRINGLIST(tail, new_val) \
> +do { \
> +*(tail) = g_new0(StringList, 1); \
> +(*(tail))->value = g_new0(String, 1); \
> +(*(tail))->value->str = g_strdup((new_val)); \
> +(tail) = &((*(tail))->next); \
> +} while (0)
Please use QAPI_LIST_APPEND instead of re-coding it.
> +++ b/qapi/net.json
> @@ -714,3 +714,83 @@
> ##
> { 'event': 'FAILOVER_NEGOTIATED',
>'data': {'device-id': 'str'} }
> +
> +##
> +# @NetBackend:
> +#
> +# Available netdev backend drivers.
> +#
> +# Since: 6.0
> +##
> +{ 'enum': 'NetBackend',
> + 'data': [ 'user', 'tap', 'l2tpv3', 'socket', 'vde', 'bridge', 'netmap',
> +'vhost-user', 'vhost-vdpa' ] }
Is it worth alphabetizing this list?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 16/16] virtio: make it possible to detach host notifier from any thread
On Wed, Apr 19, 2023 at 01:28:17PM -0400, Stefan Hajnoczi wrote: > virtio_queue_aio_detach_host_notifier() does two things: > 1. It removes the fd handler from the event loop. > 2. It processes the virtqueue one last time. > > The first step can be peformed by any thread and without taking the > AioContext lock. > > The second step may need the AioContext lock (depending on the device > implementation) and runs in the thread where request processing takes > place. virtio-blk and virtio-scsi therefore call > virtio_queue_aio_detach_host_notifier() from a BH that is scheduled in > AioContext > > Scheduling a BH is undesirable for .drained_begin() functions. The next > patch will introduce a .drained_begin() function that needs to call > virtio_queue_aio_detach_host_notifier(). > > Move the virtqueue processing out to the callers of > virtio_queue_aio_detach_host_notifier() so that the function can be > called from any thread. This is in preparation for the next patch. > This mentions a next patch, but is 16/16 in the series. Am I missing something? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 1/6] block: add blk_io_plug_call() API
On Wed, May 17, 2023 at 06:10:17PM -0400, Stefan Hajnoczi wrote: > Introduce a new API for thread-local blk_io_plug() that does not > traverse the block graph. The goal is to make blk_io_plug() multi-queue > friendly. > > Instead of having block drivers track whether or not we're in a plugged > section, provide an API that allows them to defer a function call until > we're unplugged: blk_io_plug_call(fn, opaque). If blk_io_plug_call() is > called multiple times with the same fn/opaque pair, then fn() is only > called once at the end of the function - resulting in batching. > > This patch introduces the API and changes blk_io_plug()/blk_io_unplug(). > blk_io_plug()/blk_io_unplug() no longer require a BlockBackend argument > because the plug state is now thread-local. > > Later patches convert block drivers to blk_io_plug_call() and then we > can finally remove .bdrv_co_io_plug() once all block drivers have been > converted. > > Signed-off-by: Stefan Hajnoczi > --- > +++ b/block/plug.c > + > +/** > + * blk_io_plug_call: > + * @fn: a function pointer to be invoked > + * @opaque: a user-defined argument to @fn() > + * > + * Call @fn(@opaque) immediately if not within a > blk_io_plug()/blk_io_unplug() > + * section. > + * > + * Otherwise defer the call until the end of the outermost > + * blk_io_plug()/blk_io_unplug() section in this thread. If the same > + * @fn/@opaque pair has already been deferred, it will only be called once > upon > + * blk_io_unplug() so that accumulated calls are batched into a single call. > + * > + * The caller must ensure that @opaque is not be freed before @fn() is > invoked. s/be // > + */ > +void blk_io_plug_call(void (*fn)(void *), void *opaque) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/6] block/nvme: convert to blk_io_plug_call() API
On Wed, May 17, 2023 at 06:10:18PM -0400, Stefan Hajnoczi wrote: > Stop using the .bdrv_co_io_plug() API because it is not multi-queue > block layer friendly. Use the new blk_io_plug_call() API to batch I/O > submission instead. > > Signed-off-by: Stefan Hajnoczi > --- > block/nvme.c | 44 > 1 file changed, 12 insertions(+), 32 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 3/6] block/blkio: convert to blk_io_plug_call() API
On Wed, May 17, 2023 at 06:10:19PM -0400, Stefan Hajnoczi wrote: > Stop using the .bdrv_co_io_plug() API because it is not multi-queue > block layer friendly. Use the new blk_io_plug_call() API to batch I/O > submission instead. > > Signed-off-by: Stefan Hajnoczi > --- > block/blkio.c | 40 +--- > 1 file changed, 21 insertions(+), 19 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 4/6] block/io_uring: convert to blk_io_plug_call() API
On Wed, May 17, 2023 at 06:10:20PM -0400, Stefan Hajnoczi wrote: > Stop using the .bdrv_co_io_plug() API because it is not multi-queue > block layer friendly. Use the new blk_io_plug_call() API to batch I/O > submission instead. > > Signed-off-by: Stefan Hajnoczi > --- > include/block/raw-aio.h | 7 --- > block/file-posix.c | 10 - > block/io_uring.c| 45 - > block/trace-events | 5 ++--- > 4 files changed, 19 insertions(+), 48 deletions(-) > > @@ -337,7 +325,6 @@ void luring_io_unplug(void) > * @type: type of request > * > * Fetches sqes from ring, adds to pending queue and preps them > - * > */ > static int luring_do_submit(int fd, LuringAIOCB *luringcb, LuringState *s, > uint64_t offset, int type) > @@ -370,14 +357,16 @@ static int luring_do_submit(int fd, LuringAIOCB > *luringcb, LuringState *s, Looks a bit like a stray hunk, but you are touching the function, so it's okay. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 5/6] block/linux-aio: convert to blk_io_plug_call() API
On Wed, May 17, 2023 at 06:10:21PM -0400, Stefan Hajnoczi wrote: > Stop using the .bdrv_co_io_plug() API because it is not multi-queue > block layer friendly. Use the new blk_io_plug_call() API to batch I/O > submission instead. > > Signed-off-by: Stefan Hajnoczi > --- > include/block/raw-aio.h | 7 --- > block/file-posix.c | 28 > block/linux-aio.c | 41 +++-- > 3 files changed, 11 insertions(+), 65 deletions(-) > Nice to see that not only is it friendlier to multi-queue, it's also fewer lines of code. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 6/6] block: remove bdrv_co_io_plug() API
On Wed, May 17, 2023 at 06:10:22PM -0400, Stefan Hajnoczi wrote: > No block driver implements .bdrv_co_io_plug() anymore. Get rid of the > function pointers. > > Signed-off-by: Stefan Hajnoczi > --- > include/block/block-io.h | 3 --- > include/block/block_int-common.h | 11 -- > block/io.c | 37 > 3 files changed, 51 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
