Re: [PATCH v2 01/13] ui/console-vc: Replace sprintf() by snprintf()
On Thu, Apr 11, 2024 at 2:16 PM Philippe Mathieu-Daudé wrote: > > sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, > resulting in painful developper experience. > > Replace sprintf() by snprintf() in order to avoid: > > [702/1310] Compiling C object libcommon.fa.p/ui_console-vc.c.o > ui/console-vc.c:824:21: warning: 'sprintf' is deprecated: > This function is provided for compatibility reasons only. > Due to security concerns inherent in the design of sprintf(3), > it is highly recommended that you use snprintf(3) instead. > [-Wdeprecated-declarations] > sprintf(response, "\033[%d;%dR", > ^ > 1 warning generated. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Marc-André Lureau > --- > ui/console-vc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ui/console-vc.c b/ui/console-vc.c > index 899fa11c94..847d5fb174 100644 > --- a/ui/console-vc.c > +++ b/ui/console-vc.c > @@ -821,7 +821,7 @@ static void vc_putchar(VCChardev *vc, int ch) > break; > case 6: > /* report cursor position */ > -sprintf(response, "\033[%d;%dR", > +snprintf(response, sizeof(response), "\033[%d;%dR", > (s->y_base + s->y) % s->total_height + 1, > s->x + 1); > vc_respond_str(vc, response); > -- > 2.41.0 >
Re: [PATCH 01/12] ui/console-vc: Replace sprintf() by g_strdup_printf()
On Wed, Apr 10, 2024 at 8:06 PM Philippe Mathieu-Daudé
wrote:
>
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience.
>
> Replace sprintf() by g_strdup_printf() in order to avoid:
>
> [702/1310] Compiling C object libcommon.fa.p/ui_console-vc.c.o
> ui/console-vc.c:824:21: warning: 'sprintf' is deprecated:
> This function is provided for compatibility reasons only.
> Due to security concerns inherent in the design of sprintf(3),
> it is highly recommended that you use snprintf(3) instead.
> [-Wdeprecated-declarations]
> sprintf(response, "\033[%d;%dR",
> ^
> 1 warning generated.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Marc-André Lureau
> ---
> ui/console-vc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ui/console-vc.c b/ui/console-vc.c
> index 899fa11c94..b455db436d 100644
> --- a/ui/console-vc.c
> +++ b/ui/console-vc.c
> @@ -648,7 +648,7 @@ static void vc_putchar(VCChardev *vc, int ch)
> QemuTextConsole *s = vc->console;
> int i;
> int x, y;
> -char response[40];
> +g_autofree char *response = NULL;
>
> switch(vc->state) {
> case TTY_STATE_NORM:
> @@ -821,7 +821,7 @@ static void vc_putchar(VCChardev *vc, int ch)
> break;
> case 6:
> /* report cursor position */
> -sprintf(response, "\033[%d;%dR",
> +response = g_strdup_printf("\033[%d;%dR",
> (s->y_base + s->y) % s->total_height + 1,
> s->x + 1);
> vc_respond_str(vc, response);
> --
> 2.41.0
>
Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
Hi
On Wed, Apr 3, 2024 at 12:31 PM Vladimir Sementsov-Ogievskiy
wrote:
>
> On 03.04.24 11:11, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Apr 2, 2024 at 11:24 PM Vladimir Sementsov-Ogievskiy
> > wrote:
> >>
> >> On 02.04.24 18:34, Eric Blake wrote:
> >>> On Tue, Apr 02, 2024 at 12:58:43PM +0300, Vladimir Sementsov-Ogievskiy
> >>> wrote:
> >>>>>> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
> >>>>>>
> >>>>>> Didn't you try to change WITH_ macros somehow, so that compiler
> >>>>>> believe in our good intentions?
> >>>>>>
> >>>>>
> >>>>>
> >>>>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>>>>for (g_autoptr(QemuLockable) var = \
> >>>>>
> >>>>> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
> >>>>> var; \
> >>>>> qemu_lockable_auto_unlock(var), var = NULL)
> >>>>>
> >>>>> I can't think of a clever way to rewrite this. The compiler probably
> >>>>> thinks the loop may not run, due to the "var" condition. But how to
> >>>>> convince it otherwise? it's hard to introduce another variable too..
> >>>>
> >>>>
> >>>> hmm. maybe like this?
> >>>>
> >>>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>>> for (g_autoptr(QemuLockable) var = \
> >>>>
> >>>> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
> >>>>var2 = (void *)(true); \
> >>>>var2; \
> >>>>qemu_lockable_auto_unlock(var), var2 = NULL)
> >>>>
> >>>>
> >>>> probably, it would be simpler for compiler to understand the logic this
> >>>> way. Could you check?
> >>>
> >>> Wouldn't that attach __attribute__((cleanup(xxx))) to var2, at which
> >>> point we could cause the compiler to call xxx((void*)(true)) if the
> >>> user does an early return inside the lock guard, with disastrous
> >>> consequences? Or is the __attribute__ applied only to the first out
> >>> of two declarations in a list?
> >>>
> >>
> >> Oh, most probably you are right, seems g_autoptr apply it to both
> >> variables. Also, we don't need qemu_lockable_auto_unlock(var) separate
> >> call, if we zero-out another variable. So, me fixing:
> >>
> >> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >> for (QemuLockable *var
> >> __attribute__((cleanup(qemu_lockable_auto_unlock))) =
> >> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
> >>*var2 = (void *)(true); \
> >>var2; \
> >>var2 = NULL)
> >>
> >> (and we'll need to modify qemu_lockable_auto_unlock() to take
> >> "QemuLockable **x" argument)
> >>
> >
> > That's almost good enough. I fixed a few things to generate var2.
> >
> > I applied a similar approach to WITH_GRAPH_RDLOCK_GUARD macro:
> >
> > --- a/include/block/graph-lock.h
> > +++ b/include/block/graph-lock.h
> > @@ -224,13 +224,22 @@ graph_lockable_auto_unlock(GraphLockable *x)
> >
> > G_DEFINE_AUTOPTR_CLEANUP_FUNC(GraphLockable, graph_lockable_auto_unlock)
> >
> > -#define WITH_GRAPH_RDLOCK_GUARD_(var)
> >\
> > -for (g_autoptr(GraphLockable) var =
> > graph_lockable_auto_lock(GML_OBJ_()); \
> > - var;
> >\
> > - graph_lockable_auto_unlock(var), var = NULL)
> > +static inline void TSA_NO_TSA coroutine_fn
> > +graph_lockable_auto_cleanup(GraphLockable **x)
> > +{
> > +graph_lockable_auto_unlock(*x);
> > +}
> > +
> > +#define WITH_GRAPH_RDLOCK_GUARD__(var) \
> > +GraphLockable *var \
> > +__attribute__((cleanup(graph_lockable_auto_cleanup)))
> > G_GNUC_UNUSED = \
> > + graph_lockable_auto_lock(GML_OBJ_())
> > +
> > +#define WITH_GRAPH_RDLOCK_GUARD_(var, var2) \
> > +for (WITH_GRAPH_RDLOCK_GUARD__(var), *var2 = (void *)true; var2;
> > var2 = NULL)
> >
> > #define WITH_GRAPH_RDLOCK_GUARD() \
> > -WITH_GRAPH_RDLOCK_GUARD
Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
Hi
On Tue, Apr 2, 2024 at 11:24 PM Vladimir Sementsov-Ogievskiy
wrote:
>
> On 02.04.24 18:34, Eric Blake wrote:
> > On Tue, Apr 02, 2024 at 12:58:43PM +0300, Vladimir Sementsov-Ogievskiy
> > wrote:
> >>>> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
> >>>>
> >>>> Didn't you try to change WITH_ macros somehow, so that compiler believe
> >>>> in our good intentions?
> >>>>
> >>>
> >>>
> >>> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >>> for (g_autoptr(QemuLockable) var = \
> >>>
> >>> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
> >>>var; \
> >>>qemu_lockable_auto_unlock(var), var = NULL)
> >>>
> >>> I can't think of a clever way to rewrite this. The compiler probably
> >>> thinks the loop may not run, due to the "var" condition. But how to
> >>> convince it otherwise? it's hard to introduce another variable too..
> >>
> >>
> >> hmm. maybe like this?
> >>
> >> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >> for (g_autoptr(QemuLockable) var = \
> >> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))),
> >> \
> >> var2 = (void *)(true); \
> >> var2; \
> >> qemu_lockable_auto_unlock(var), var2 = NULL)
> >>
> >>
> >> probably, it would be simpler for compiler to understand the logic this
> >> way. Could you check?
> >
> > Wouldn't that attach __attribute__((cleanup(xxx))) to var2, at which
> > point we could cause the compiler to call xxx((void*)(true)) if the
> > user does an early return inside the lock guard, with disastrous
> > consequences? Or is the __attribute__ applied only to the first out
> > of two declarations in a list?
> >
>
> Oh, most probably you are right, seems g_autoptr apply it to both variables.
> Also, we don't need qemu_lockable_auto_unlock(var) separate call, if we
> zero-out another variable. So, me fixing:
>
> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> for (QemuLockable *var
> __attribute__((cleanup(qemu_lockable_auto_unlock))) =
> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
> *var2 = (void *)(true); \
> var2; \
> var2 = NULL)
>
> (and we'll need to modify qemu_lockable_auto_unlock() to take "QemuLockable
> **x" argument)
>
That's almost good enough. I fixed a few things to generate var2.
I applied a similar approach to WITH_GRAPH_RDLOCK_GUARD macro:
--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -224,13 +224,22 @@ graph_lockable_auto_unlock(GraphLockable *x)
G_DEFINE_AUTOPTR_CLEANUP_FUNC(GraphLockable, graph_lockable_auto_unlock)
-#define WITH_GRAPH_RDLOCK_GUARD_(var) \
-for (g_autoptr(GraphLockable) var = graph_lockable_auto_lock(GML_OBJ_()); \
- var; \
- graph_lockable_auto_unlock(var), var = NULL)
+static inline void TSA_NO_TSA coroutine_fn
+graph_lockable_auto_cleanup(GraphLockable **x)
+{
+graph_lockable_auto_unlock(*x);
+}
+
+#define WITH_GRAPH_RDLOCK_GUARD__(var) \
+GraphLockable *var \
+__attribute__((cleanup(graph_lockable_auto_cleanup))) G_GNUC_UNUSED = \
+ graph_lockable_auto_lock(GML_OBJ_())
+
+#define WITH_GRAPH_RDLOCK_GUARD_(var, var2) \
+for (WITH_GRAPH_RDLOCK_GUARD__(var), *var2 = (void *)true; var2;
var2 = NULL)
#define WITH_GRAPH_RDLOCK_GUARD() \
-WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__))
+WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__),
glue(graph_lockable_auto, __COUNTER__))
Unfortunately, it doesn't work in all cases. It seems to have issues
with some guards:
../block/stream.c: In function ‘stream_run’:
../block/stream.c:216:12: error: ‘ret’ may be used uninitialized
[-Werror=maybe-uninitialized]
216 | if (ret < 0) {
What should we do? change the macros + cherry-pick the missing
false-positives, or keep this series as is?
--
Marc-André Lureau
Re: [PATCH 09/19] hw/sdhci: fix -Werror=maybe-uninitialized false-positive
Hi
On Thu, Mar 28, 2024 at 3:31 PM Philippe Mathieu-Daudé
wrote:
>
> On 28/3/24 11:20, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > ../hw/sd/sdhci.c:846:16: error: ‘res’ may be used uninitialized
> > [-Werror=maybe-uninitialized]
> >
> > False-positive, because "length" is non-null.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > hw/sd/sdhci.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index c5e0bc018b..da5351d4e5 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -745,7 +745,7 @@ static void sdhci_do_adma(SDHCIState *s)
> > const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
> > const MemTxAttrs attrs = { .memory = true };
> > ADMADescr dscr = {};
> > -MemTxResult res;
> > +MemTxResult res = MEMTX_ERROR;
>
> Something is indeed odd here. Due to the for() loop, the variable
> initialization should be moved after the SDHC_ADMA_ATTR_ACT_TRAN
> switch case.
>
Moving the variable initialization? What do you suggest exactly?
thanks
--
Marc-André Lureau
Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives
Hi
On Fri, Mar 29, 2024 at 12:35 PM Vladimir Sementsov-Ogievskiy
wrote:
>
> On 28.03.24 13:20, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > ../block/stream.c:193:19: error: ‘unfiltered_bs’ may be used uninitialized
> > [-Werror=maybe-uninitialized]
> > ../block/stream.c:176:5: error: ‘len’ may be used uninitialized
> > [-Werror=maybe-uninitialized]
> > trace/trace-block.h:906:9: error: ‘ret’ may be used uninitialized
> > [-Werror=maybe-uninitialized]
> >
> > Signed-off-by: Marc-André Lureau
>
> Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
>
> Didn't you try to change WITH_ macros somehow, so that compiler believe in
> our good intentions?
>
#define WITH_QEMU_LOCK_GUARD_(x, var) \
for (g_autoptr(QemuLockable) var = \
qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
var; \
qemu_lockable_auto_unlock(var), var = NULL)
I can't think of a clever way to rewrite this. The compiler probably
thinks the loop may not run, due to the "var" condition. But how to
convince it otherwise? it's hard to introduce another variable too..
> Actually, "unused variable initialization" is bad thing too.
>
> Anyway, if no better solution for now:
> Acked-by: Vladimir Sementsov-Ogievskiy
>
> > ---
> > block/stream.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/stream.c b/block/stream.c
> > index 7031eef12b..9076203193 100644
> > --- a/block/stream.c
> > +++ b/block/stream.c
> > @@ -155,8 +155,8 @@ static void stream_clean(Job *job)
> > static int coroutine_fn stream_run(Job *job, Error **errp)
> > {
> > StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> > -BlockDriverState *unfiltered_bs;
> > -int64_t len;
> > +BlockDriverState *unfiltered_bs = NULL;
> > +int64_t len = -1;
> > int64_t offset = 0;
> > int error = 0;
> > int64_t n = 0; /* bytes */
> > @@ -177,7 +177,7 @@ static int coroutine_fn stream_run(Job *job, Error
> > **errp)
> >
> > for ( ; offset < len; offset += n) {
> > bool copy;
> > -int ret;
> > +int ret = -1;
> >
> > /* Note that even when no rate limit is applied we need to yield
> >* with no pending I/O here so that bdrv_drain_all() returns.
>
> --
> Best regards,
> Vladimir
>
>
--
Marc-André Lureau
Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled
Hi
On Tue, Nov 21, 2023 at 2:57 PM Marc-André Lureau
wrote:
>
> Hi
>
> On Wed, Nov 15, 2023 at 9:28 PM David Woodhouse wrote:
> >
> > From: David Woodhouse
> >
> > If a Xen console is configured on the command line, do not add a default
> > serial port.
> >
> > Signed-off-by: David Woodhouse
> > ---
> > system/vl.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/system/vl.c b/system/vl.c
> > index 5af7ced2a1..8109231834 100644
> > --- a/system/vl.c
> > +++ b/system/vl.c
> > @@ -198,6 +198,7 @@ static const struct {
> > const char *driver;
> > int *flag;
> > } default_list[] = {
> > +{ .driver = "xen-console", .flag = _serial},
> > { .driver = "isa-serial", .flag = _serial},
> > { .driver = "isa-parallel", .flag = _parallel },
> > { .driver = "isa-fdc", .flag = _floppy},
>
> Consistent with the rest of the lines (no conditional compilation nor
> driver #define..)
> Reviewed-by: Marc-André Lureau
>
> btw, while quickly testing this (do we have any test for xen-console?):
>
> $ qemu --accel kvm,xen-version=0x40011,kernel-irqchip=split -device
> xen-console,chardev=foo -chardev stdio,id=foo
> (and close gtk window)
>
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x55c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> 387if (nc->incoming_queue) {
> (gdb) bt
> #0 0x55c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
> #1 0x55c11a14 in qemu_del_nic (nic=0x58b6f930) at
> ../net/net.c:459
> #2 0x559e398b in xen_netdev_unrealize (xendev=0x58b6b510)
> at ../hw/net/xen_nic.c:550
> #3 0x55b6e22f in xen_device_unrealize (dev=0x58b6b510) at
> ../hw/xen/xen-bus.c:973
> #4 0x55b6e351 in xen_device_exit (n=0x58b6b5e0, data=0x0)
> at ../hw/xen/xen-bus.c:1002
> #5 0x00005555560bc3fc in notifier_list_notify (list=0x570b5fc0
> , data=0x0) at ../util/notify.c:39
> #6 0x55ba1d49 in qemu_run_exit_notifiers () at
> ../system/runstate.c:800
Ok, I found related "[PATCH 1/3] net: do not delete nics in net_cleanup()"
--
Marc-André Lureau
Re: [PATCH 2/3] vl: disable default serial when xen-console is enabled
Hi
On Wed, Nov 15, 2023 at 9:28 PM David Woodhouse wrote:
>
> From: David Woodhouse
>
> If a Xen console is configured on the command line, do not add a default
> serial port.
>
> Signed-off-by: David Woodhouse
> ---
> system/vl.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/system/vl.c b/system/vl.c
> index 5af7ced2a1..8109231834 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -198,6 +198,7 @@ static const struct {
> const char *driver;
> int *flag;
> } default_list[] = {
> +{ .driver = "xen-console", .flag = _serial},
> { .driver = "isa-serial", .flag = _serial},
> { .driver = "isa-parallel", .flag = _parallel },
> { .driver = "isa-fdc", .flag = _floppy},
Consistent with the rest of the lines (no conditional compilation nor
driver #define..)
Reviewed-by: Marc-André Lureau
btw, while quickly testing this (do we have any test for xen-console?):
$ qemu --accel kvm,xen-version=0x40011,kernel-irqchip=split -device
xen-console,chardev=foo -chardev stdio,id=foo
(and close gtk window)
Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
0x55c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
387if (nc->incoming_queue) {
(gdb) bt
#0 0x55c11695 in qemu_free_net_client (nc=0x0) at ../net/net.c:387
#1 0x55c11a14 in qemu_del_nic (nic=0x58b6f930) at ../net/net.c:459
#2 0x559e398b in xen_netdev_unrealize (xendev=0x58b6b510)
at ../hw/net/xen_nic.c:550
#3 0x55b6e22f in xen_device_unrealize (dev=0x58b6b510) at
../hw/xen/xen-bus.c:973
#4 0x55b6e351 in xen_device_exit (n=0x58b6b5e0, data=0x0)
at ../hw/xen/xen-bus.c:1002
#5 0x560bc3fc in notifier_list_notify (list=0x570b5fc0
, data=0x0) at ../util/notify.c:39
#6 0x55ba1d49 in qemu_run_exit_notifiers () at ../system/runstate.c:800
--
Marc-André Lureau
Re: [PATCH v2 9/9] docs/migration: reflect the changes about needed subsections
Hi On Tue, Oct 24, 2023 at 2:47 PM Juan Quintela wrote: > > marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Signed-off-by: Marc-André Lureau > > --- > > docs/devel/migration.rst | 17 - > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst > > index c3e1400c0c..50f313f178 100644 > > --- a/docs/devel/migration.rst > > +++ b/docs/devel/migration.rst > > @@ -240,17 +240,16 @@ a newer form of device, or adding that state that you > > previously > > forgot to migrate. This is best solved using a subsection. > > > > A subsection is "like" a device vmstate, but with a particularity, it > > -has a Boolean function that tells if that values are needed to be sent > > -or not. If this functions returns false, the subsection is not sent. > > -Subsections have a unique name, that is looked for on the receiving > > -side. > > +has a Boolean function that tells if that values are needed or not. If > > +this functions returns false, the subsection is not sent. Subsections > > +have a unique name, that is looked for on the receiving side. > > > > On the receiving side, if we found a subsection for a device that we > > -don't understand, we just fail the migration. If we understand all > > -the subsections, then we load the state with success. There's no check > > -that a subsection is loaded, so a newer QEMU that knows about a subsection > > -can (with care) load a stream from an older QEMU that didn't send > > -the subsection. > > +don't understand, we just fail the migration. If we understand all the > > +subsections, then we load the state with success. There's no check > > +that an optional subsection is loaded, so a newer QEMU that knows > > +about a subsection can (with care) load a stream from an older QEMU > > +that didn't send the subsection. > > Reviewed-by: Juan Quintela > > Just wondering. What device propmted you to write this series? When I worked on ramfb, I did various testing with the subsection handling and was surprised by the default lazy behaviour. I initially thought it was a bug to ignore missing sections (both needed and not-needed), then I realize from the doc that it was partially by design. I thought it was clearer to make "needed' section actually required on load as well. I wonder though of the potential of breakage from old QEMU versions, how do we test cross-version migration? Do you think also "needed" section are actually required? Perhaps we need better wording and documentation instead...
Re: [PATCH 00/16] qapi qga/qapi-schema: Doc fixes
On Tue, Apr 4, 2023 at 4:02 PM Markus Armbruster wrote: > > It's always nice to get doc fixes into the release, but if it's too > late, it's too late. > > Generated code does not change, except for the last patch, which moves > a bit of code without changing it. > > Markus Armbruster (16): > qga/qapi-schema: Tidy up documentation of guest-fsfreeze-status > qga/qapi-schema: Fix a misspelled reference > qapi: Fix misspelled references > qapi: Fix up references to long gone error classes > qapi/block-core: Clean up after removal of dirty bitmap @status > qapi: @foo should be used to reference, not ``foo`` > qapi: Tidy up examples > qapi: Delete largely misleading "Stability Considerations" > qapi: Fix bullet list markup in documentation > qapi: Fix unintended definition lists in documentation > qga/qapi-schema: Fix member documentation markup > qapi: Fix argument documentation markup > qapi: Replace ad hoc "since" documentation by member documentation > qapi: Fix misspelled section tags in doc comments > qapi: Format since information the conventional way: (since X.Y) > qapi storage-daemon/qapi: Fix documentation section structure > > docs/devel/qapi-code-gen.rst | 8 ++- > docs/interop/firmware.json | 6 +- > qapi/block-core.json | 82 ++-- > qapi/block.json | 2 +- > qapi/char.json | 4 +- > qapi/control.json| 2 +- > qapi/cryptodev.json | 4 ++ > qapi/job.json| 4 +- > qapi/machine-target.json | 2 +- > qapi/machine.json| 26 + > qapi/migration.json | 37 - > qapi/misc.json | 6 +- > qapi/net.json| 25 +++-- > qapi/qapi-schema.json| 24 +--- > qapi/qdev.json | 2 +- > qapi/qom.json| 4 +- > qapi/rdma.json | 2 +- > qapi/replay.json | 3 + > qapi/run-state.json | 6 +- > qapi/stats.json | 3 +- > qapi/tpm.json| 3 +- > qapi/trace.json | 1 + > qapi/ui.json | 12 ++-- > qga/qapi-schema.json | 10 ++-- > storage-daemon/qapi/qapi-schema.json | 22 +--- > 25 files changed, 154 insertions(+), 146 deletions(-) > > -- > 2.39.2 > > lgtm, Reviewed-by: Marc-André Lureau -- Marc-André Lureau
Re: [PULL 00/25] Win socket patches
Hi On Mon, Mar 13, 2023 at 3:44 PM wrote: > > From: Marc-André Lureau > > The following changes since commit 29c8a9e31a982874ce4e2c15f2bf82d5f8dc3517: > > Merge tag 'linux-user-for-8.0-pull-request' of > https://gitlab.com/laurent_vivier/qemu into staging (2023-03-12 10:57:00 > +) > > are available in the Git repository at: > > https://gitlab.com/marcandre.lureau/qemu.git tags/win-socket-pull-request > > for you to fetch changes up to 0006c18362cbe1e93587ac1e8ee965c08e6970f2: > > QMP/HMP: only actually implement getfd on CONFIG_POSIX (2023-03-13 15:41:32 > +0400) > > > QMP command to import win32 sockets > > -------- > > Marc-André Lureau (25): > util: drop qemu_fork() > tests: use closesocket() > io: use closesocket() > tests: add test-error-report > error: add global _warn destination > win32/socket: introduce qemu_socket_select() helper > win32/socket: introduce qemu_socket_unselect() helper > aio: make aio_set_fd_poll() static to aio-posix.c > aio/win32: aio_set_fd_handler() only supports SOCKET > main-loop: remove qemu_fd_register(), win32/slirp/socket specific > slirp: unregister the win32 SOCKET > slirp: open-code qemu_socket_(un)select() > win32: avoid mixing SOCKET and file descriptor space > os-posix: remove useless ioctlsocket() define > win32: replace closesocket() with close() wrapper > tests: fix path separator, use g_build_filename() > char: do not double-close fd when failing to add client > tests/docker: fix a win32 error due to portability > osdep: implement qemu_socketpair() for win32 > qmp: 'add_client' actually expects sockets > monitor: release the lock before calling close() > qmp: add 'get-win32-socket' > libqtest: make qtest_qmp_add_client work on win32 > qtest: enable vnc-display test on win32 Would you consider those 3 patches as "features"? Is it too late to include before hard-feature freeze? The patches were ready for the soft-freeze deadline (I was expecting Paolo to send a PR last week, so I could quickly follow up with those patches in a PR) > QMP/HMP: only actually implement getfd on CONFIG_POSIX > > qapi/misc.json | 36 ++- > include/block/aio.h | 8 - > include/qapi/error.h | 6 + > include/qemu/main-loop.h | 2 - > include/qemu/osdep.h | 14 -- > include/qemu/sockets.h | 2 - > include/sysemu/os-posix.h| 3 - > include/sysemu/os-win32.h| 15 +- > tests/qtest/libqtest.h | 5 +- > backends/tpm/tpm_emulator.c | 6 +- > chardev/char.c | 2 - > crypto/afalg.c | 6 +- > hw/hyperv/syndbg.c | 4 +- > io/channel-socket.c | 8 +- > io/channel-watch.c | 10 +- > monitor/fds.c| 77 -- > monitor/hmp-cmds.c | 2 + > monitor/qmp-cmds.c | 7 + > net/dgram.c | 14 +- > net/slirp.c | 16 +- > net/socket.c | 22 +- > tests/qtest/libqtest.c | 26 +- > tests/qtest/microbit-test.c | 2 +- > tests/qtest/netdev-socket.c | 10 +- > tests/qtest/vnc-display-test.c | 12 +- > tests/unit/socket-helpers.c | 2 +- > tests/unit/test-error-report.c | 139 +++ > tests/unit/test-io-channel-command.c | 2 +- > util/aio-posix.c | 6 +- > util/aio-win32.c | 23 +- > util/error.c | 10 +- > util/main-loop.c | 11 - > util/oslib-posix.c | 70 -- > util/oslib-win32.c | 350 --- > util/qemu-sockets.c | 22 +- > hmp-commands.hx | 2 + > tests/docker/docker.py | 6 +- > tests/unit/meson.build | 1 + > 38 files changed, 701 insertions(+), 258 deletions(-) > create mode 100644 tests/unit/test-error-report.c > > -- > 2.39.2 > > -- Marc-André Lureau
Re: [PULL 00/25] Win socket patches
Hi On Mon, Mar 13, 2023 at 3:43 PM wrote: > From: Marc-André Lureau > > The following changes since commit > 29c8a9e31a982874ce4e2c15f2bf82d5f8dc3517: > > Merge tag 'linux-user-for-8.0-pull-request' of > https://gitlab.com/laurent_vivier/qemu into staging (2023-03-12 10:57:00 > +) > > are available in the Git repository at: > > https://gitlab.com/marcandre.lureau/qemu.git > tags/win-socket-pull-request > > for you to fetch changes up to 0006c18362cbe1e93587ac1e8ee965c08e6970f2: > > QMP/HMP: only actually implement getfd on CONFIG_POSIX (2023-03-13 > 15:41:32 +0400) > > > QMP command to import win32 sockets > > -------- > > Marc-André Lureau (25): > util: drop qemu_fork() > tests: use closesocket() > io: use closesocket() > tests: add test-error-report > error: add global _warn destination > win32/socket: introduce qemu_socket_select() helper > win32/socket: introduce qemu_socket_unselect() helper > aio: make aio_set_fd_poll() static to aio-posix.c > aio/win32: aio_set_fd_handler() only supports SOCKET > main-loop: remove qemu_fd_register(), win32/slirp/socket specific > slirp: unregister the win32 SOCKET > slirp: open-code qemu_socket_(un)select() > win32: avoid mixing SOCKET and file descriptor space > os-posix: remove useless ioctlsocket() define > win32: replace closesocket() with close() wrapper > tests: fix path separator, use g_build_filename() > char: do not double-close fd when failing to add client > tests/docker: fix a win32 error due to portability > osdep: implement qemu_socketpair() for win32 > qmp: 'add_client' actually expects sockets > monitor: release the lock before calling close() > qmp: add 'get-win32-socket' > libqtest: make qtest_qmp_add_client work on win32 > qtest: enable vnc-display test on win32 > QMP/HMP: only actually implement getfd on CONFIG_POSIX > My bad, last patch is not the one I intended (with message fix suggested by Markus). Sending v2. > > qapi/misc.json | 36 ++- > include/block/aio.h | 8 - > include/qapi/error.h | 6 + > include/qemu/main-loop.h | 2 - > include/qemu/osdep.h | 14 -- > include/qemu/sockets.h | 2 - > include/sysemu/os-posix.h| 3 - > include/sysemu/os-win32.h| 15 +- > tests/qtest/libqtest.h | 5 +- > backends/tpm/tpm_emulator.c | 6 +- > chardev/char.c | 2 - > crypto/afalg.c | 6 +- > hw/hyperv/syndbg.c | 4 +- > io/channel-socket.c | 8 +- > io/channel-watch.c | 10 +- > monitor/fds.c| 77 -- > monitor/hmp-cmds.c | 2 + > monitor/qmp-cmds.c | 7 + > net/dgram.c | 14 +- > net/slirp.c | 16 +- > net/socket.c | 22 +- > tests/qtest/libqtest.c | 26 +- > tests/qtest/microbit-test.c | 2 +- > tests/qtest/netdev-socket.c | 10 +- > tests/qtest/vnc-display-test.c | 12 +- > tests/unit/socket-helpers.c | 2 +- > tests/unit/test-error-report.c | 139 +++ > tests/unit/test-io-channel-command.c | 2 +- > util/aio-posix.c | 6 +- > util/aio-win32.c | 23 +- > util/error.c | 10 +- > util/main-loop.c | 11 - > util/oslib-posix.c | 70 -- > util/oslib-win32.c | 350 --- > util/qemu-sockets.c | 22 +- > hmp-commands.hx | 2 + > tests/docker/docker.py | 6 +- > tests/unit/meson.build | 1 + > 38 files changed, 701 insertions(+), 258 deletions(-) > create mode 100644 tests/unit/test-error-report.c > > -- > 2.39.2 > >
Re: [PATCH v3 13/16] slirp: open-code qemu_socket_(un)select()
Hi
On Mon, Mar 6, 2023 at 5:59 PM Stefan Berger wrote:
>
>
> On 2/21/23 07:47, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > We are about to make the QEMU socket API use file-descriptor space only,
> > but libslirp gives us SOCKET as fd, still.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > net/slirp.c | 10 +++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/slirp.c b/net/slirp.c
> > index a7c35778a6..c33b3e02e7 100644
> > --- a/net/slirp.c
> > +++ b/net/slirp.c
> > @@ -251,16 +251,20 @@ static void net_slirp_register_poll_fd(int fd,
> void *opaque)
>
> Shouldn't this int fd rather be a SOCKET s instead? Or do you get compiler
> warnings then?
>
>
Yes, you would get compiler warnings, because the "int fd" argument is from
the slirp API, whether it is posix or win32.
> > #ifdef WIN32
> > AioContext *ctxt = qemu_get_aio_context();
> >
> > -qemu_socket_select(fd, event_notifier_get_handle(>notifier),
> > +if (WSAEventSelect(fd, event_notifier_get_handle(>notifier),
> > FD_READ | FD_ACCEPT | FD_CLOSE |
> > - FD_CONNECT | FD_WRITE | FD_OOB, NULL);
> > + FD_CONNECT | FD_WRITE | FD_OOB) != 0) {
> > +error_setg_win32(_warn, WSAGetLastError(), "failed to
> WSAEventSelect()");
> > +}
> > #endif
> > }
> > > static void net_slirp_unregister_poll_fd(int fd, void *opaque)
>
> Same here.
>
> > {
> > #ifdef WIN32
> > -qemu_socket_unselect(fd, NULL);
> > +if (WSAEventSelect(fd, NULL, 0) != 0) {
> > +error_setg_win32(_warn, WSAGetLastError(), "failed to
> WSAEventSelect()");
> > +}
> > #endif
> > }
> >
>
>
Re: [PATCH v3 00/16] win32: do not mix SOCKET and fd space
Hi Paolo On Mon, Mar 6, 2023 at 12:05 PM Paolo Bonzini wrote: > On 3/2/23 15:09, Marc-André Lureau wrote: > > > > > > v3: > > - fix closesocket() to prevent CloseHandle() from close() > > - fix direct closesocket() usage (#undef closesocket before) > > - add a test for _warn > > - add r-b tags > > > > ping (I am missing reviews, thanks) > > I'm going to queue this series today if that's fine for you---thanks for > reminding! > > Great, thanks! (I suppose you'll drop "RFC: build-sys: add slirp.wrap", and perhaps queue the other meson/wrap series instead)
Re: [PATCH v3 12/16] slirp: unregister the win32 SOCKET
Hi
On Thu, Mar 2, 2023 at 10:45 PM Stefan Berger wrote:
>
>
> On 2/21/23 07:47, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > Presumably, this is what should happen when the SOCKET is to be removed.
> > (it probably worked until now because closesocket() does it implicitly,
> > but we never now how the slirp library could use the SOCKET later)
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > net/slirp.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/slirp.c b/net/slirp.c
> > index 0730a935ba..a7c35778a6 100644
> > --- a/net/slirp.c
> > +++ b/net/slirp.c
> > @@ -259,7 +259,9 @@ static void net_slirp_register_poll_fd(int fd, void
> *opaque)
> >
> > static void net_slirp_unregister_poll_fd(int fd, void *opaque)
> > {
> > -/* no qemu_fd_unregister */
> > +#ifdef WIN32
> The majority of code seems to use _WIN32. Not sure what is 'right'.
>
>
Both should be correct. I think I like the "WIN32" version better though
(see also
https://stackoverflow.com/questions/662084/whats-the-difference-between-the-win32-and-win32-defines-in-c
)
> Reviewed-by: Stefan Berger
>
>
thanks
> > +qemu_socket_unselect(fd, NULL);
> > +#endif
> > }
> >
> > static void net_slirp_notify(void *opaque)
>
>
Re: [PATCH v3 14/16] win32: avoid mixing SOCKET and file descriptor space
Hi
On Fri, Mar 3, 2023 at 12:54 AM Stefan Berger wrote:
>
>
>
> On 2/21/23 07:47, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > Until now, a win32 SOCKET handle is often cast to an int file
> > descriptor, as this is what other OS use for sockets. When necessary,
> > QEMU eventually queries whether it's a socket with the help of
> > fd_is_socket(). However, there is no guarantee of conflict between the
> > fd and SOCKET space. Such conflict would have surprising consequences,
> > we shouldn't mix them.
> >
> > Also, it is often forgotten that SOCKET must be closed with
> > closesocket(), and not close().
> >
> > Instead, let's make the win32 socket wrapper functions return and take a
> > file descriptor, and let util/ wrappers do the fd/SOCKET conversion as
> > necessary. A bit of adaptation is necessary in io/ as well.
> >
> > Unfortunately, we can't drop closesocket() usage, despite
> > _open_osfhandle() documentation claiming transfer of ownership, testing
> > shows bad behaviour if you forget to call closesocket().
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > include/sysemu/os-win32.h | 4 +-
> > io/channel-watch.c| 6 +-
> > util/aio-win32.c | 9 +-
> > util/oslib-win32.c| 219 --
> > 4 files changed, 197 insertions(+), 41 deletions(-)
>
> > #undef connect
> > @@ -308,7 +315,13 @@ int qemu_connect_wrap(int sockfd, const struct
> > sockaddr *addr,
> > socklen_t addrlen)
> > {
> > int ret;
> > -ret = connect(sockfd, addr, addrlen);
> > +SOCKET s = _get_osfhandle(sockfd);
> > +
> > +if (s == INVALID_SOCKET) {
> > +return -1;
> > +}
> > +
> > +ret = connect(s, addr, addrlen);
>
>
> Previously you passed int sockfd and now you convert this sockfd to a SOCKET
> s and you can pass this as an alternative? Or was sockfd before this patch a
> SOCKET??
yes, sockfd was in fact a SOCKET.
Previous to this patch, a SOCKET is cast to int, as a fake fd, and
back to SOCKET as necessary. The whole point of this patch is to avoid
mixing SOCKET & fd space, instead a SOCKET is associated with a real
CRT fd.
thanks
--
Marc-André Lureau
Re: [PATCH v3 00/16] win32: do not mix SOCKET and fd space
Hi On Tue, Feb 21, 2023 at 4:48 PM wrote: > From: Marc-André Lureau > > Hi, > > A win32 SOCKET handle is often cast to an int file descriptor, as this is > what > other OS use for sockets. When necessary, QEMU eventually queries whether > it's a > socket with the help of fd_is_socket(). However, there is no guarantee of > conflict between the fd and SOCKET space. Such conflict would have > surprising > consequences. We can fix this by using FDs only. > > After fixing a few missed closesocket(), this patch series makes the win32 > socket API wrappers take FDs. It finally get rid of closesocket() usage by > using > a close() wrapper instead. (note that fdopen/fclose would not be enough > either > to close the underlying socket appropriately) > > v3: > - fix closesocket() to prevent CloseHandle() from close() > - fix direct closesocket() usage (#undef closesocket before) > - add a test for _warn > - add r-b tags > > ping (I am missing reviews, thanks) > v2: > - add clean up patch "util: drop qemu_fork()" > - add a "_warn", to help with basic error reporting > - fix errno handling after _get_osfhandle() > - introduce qemu_socket_(un)select() helpers > - add patch "aio_set_fd_handler() only supports SOCKET" > - add meson slirp.wrap RFC > - various misc cleanups > - add r-b tags > > Marc-André Lureau (16): > util: drop qemu_fork() > tests: use closesocket() > io: use closesocket() > tests: add test-error-report > error: add global _warn destination > win32/socket: introduce qemu_socket_select() helper > win32/socket: introduce qemu_socket_unselect() helper > aio: make aio_set_fd_poll() static to aio-posix.c > aio/win32: aio_set_fd_handler() only supports SOCKET > RFC: build-sys: add slirp.wrap > main-loop: remove qemu_fd_register(), win32/slirp/socket specific > slirp: unregister the win32 SOCKET > slirp: open-code qemu_socket_(un)select() > win32: avoid mixing SOCKET and file descriptor space > os-posix: remove useless ioctlsocket() define > win32: replace closesocket() with close() wrapper > > include/block/aio.h| 8 -- > include/qapi/error.h | 6 + > include/qemu/main-loop.h | 2 - > include/qemu/osdep.h | 14 -- > include/sysemu/os-posix.h | 3 - > include/sysemu/os-win32.h | 15 ++- > backends/tpm/tpm_emulator.c| 6 +- > crypto/afalg.c | 6 +- > hw/hyperv/syndbg.c | 4 +- > io/channel-socket.c| 8 +- > io/channel-watch.c | 10 +- > net/dgram.c| 14 +- > net/slirp.c| 16 ++- > net/socket.c | 22 +-- > tests/qtest/libqtest.c | 8 +- > tests/qtest/microbit-test.c| 2 +- > tests/qtest/netdev-socket.c| 10 +- > tests/unit/socket-helpers.c| 2 +- > tests/unit/test-error-report.c | 139 +++ > util/aio-posix.c | 6 +- > util/aio-win32.c | 23 ++-- > util/error.c | 10 +- > util/main-loop.c | 11 -- > util/oslib-posix.c | 70 -- > util/oslib-win32.c | 240 - > util/qemu-sockets.c| 22 +-- > .gitignore | 2 + > subprojects/slirp.wrap | 6 + > tests/unit/meson.build | 1 + > 29 files changed, 461 insertions(+), 225 deletions(-) > create mode 100644 tests/unit/test-error-report.c > create mode 100644 subprojects/slirp.wrap > > -- > 2.39.2 > >
Re: [PATCH] tests/unit/test-image-locking: Fix handling of temporary files
Hi
On Wed, Oct 12, 2022 at 1:03 PM Thomas Huth wrote:
> test-image-locking leaves some temporary files around - clean
> them up. While we're at it, test-image-locking is a unit test,
> so it should not use "qtest.*" for temporary file names. Give
> them better names instead, so that it clear where the temporary
> files come from.
>
> Signed-off-by: Thomas Huth
> ---
> tests/unit/test-image-locking.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/unit/test-image-locking.c
> b/tests/unit/test-image-locking.c
> index a47299c247..2624cec6a0 100644
> --- a/tests/unit/test-image-locking.c
> +++ b/tests/unit/test-image-locking.c
> @@ -79,7 +79,7 @@ static void test_image_locking_basic(void)
> g_autofree char *img_path = NULL;
> uint64_t perm, shared_perm;
>
> -int fd = g_file_open_tmp("qtest.XX", _path, NULL);
> +int fd = g_file_open_tmp("qemu-tst-img-lock.XX", _path, NULL);
> assert(fd >= 0);
>
> perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
> @@ -120,7 +120,7 @@ static void test_set_perm_abort(void)
> g_autofree char *img_path = NULL;
> uint64_t perm, shared_perm;
> int r;
> -int fd = g_file_open_tmp("qtest.XX", _path, NULL);
> +int fd = g_file_open_tmp("qemu-tst-img-lock.XX", _path, NULL);
> assert(fd >= 0);
>
> perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
> @@ -140,6 +140,8 @@ static void test_set_perm_abort(void)
> check_locked_bytes(fd, perm, ~shared_perm);
> blk_unref(blk1);
> blk_unref(blk2);
> + close(fd);
> +unlink(img_path);
>
Perhaps we should use g_unlink() instead for better portability? although
this is pre-existing.
otherwise, lgtm
Reviewed-by: Marc-André Lureau
--
Marc-André Lureau
Re: [PATCH v4 00/54] tests/qtest: Enable running qtest on Windows
tio-blk-test.c | 4 +- > tests/qtest/virtio-net-failover.c | 9 +- > tests/qtest/virtio-net-test.c | 13 +- > tests/qtest/virtio-scsi-test.c | 4 +- > tests/unit/test-image-locking.c | 8 +- > tests/unit/test-qga.c | 2 +- > tests/vhost-user-bridge.c | 3 +- > util/osdep.c| 33 + > util/qemu-sockets.c | 5 +- > .gitlab-ci.d/windows.yml| 8 +- > accel/meson.build | 1 + > accel/qtest/meson.build | 1 + > tests/qtest/meson.build | 42 +++ > 45 files changed, 452 insertions(+), 240 deletions(-) > > -- > 2.34.1 > > > -- Marc-André Lureau
Re: [PATCH v3 19/54] tests/qtest: virtio-blk-test: Avoid using hardcoded /tmp
On Sun, Sep 25, 2022 at 4:09 PM Bin Meng wrote:
> From: Bin Meng
>
> This case was written to use hardcoded /tmp directory for temporary
> files. Update to use g_file_open_tmp() for a portable implementation.
>
> Signed-off-by: Bin Meng
>
Reviewed-by: Marc-André Lureau
> ---
>
> Changes in v3:
> - Split to a separate patch
>
> tests/qtest/virtio-blk-test.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/virtio-blk-test.c b/tests/qtest/virtio-blk-test.c
> index dc5eed31c8..19c01f808b 100644
> --- a/tests/qtest/virtio-blk-test.c
> +++ b/tests/qtest/virtio-blk-test.c
> @@ -49,10 +49,10 @@ static void drive_destroy(void *path)
> static char *drive_create(void)
> {
> int fd, ret;
> -char *t_path = g_strdup("/tmp/qtest.XX");
> +char *t_path;
>
> /* Create a temporary raw image */
> -fd = mkstemp(t_path);
> +fd = g_file_open_tmp("qtest.XX", _path, NULL);
> g_assert_cmpint(fd, >=, 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> g_assert_cmpint(ret, ==, 0);
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v3 13/54] tests/qtest: ide-test: Avoid using hardcoded /tmp
On Sun, Sep 25, 2022 at 3:56 PM Bin Meng wrote:
> From: Bin Meng
>
> This case was written to use hardcoded /tmp directory for temporary
> files. Update to use g_file_open_tmp() for a portable implementation.
>
> Signed-off-by: Bin Meng
>
Reviewed-by: Marc-André Lureau
> ---
>
> Changes in v3:
> - Split to a separate patch
>
> tests/qtest/ide-test.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index 5bcb75a7e5..25302be6dc 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -121,8 +121,8 @@ enum {
> static QPCIBus *pcibus = NULL;
> static QGuestAllocator guest_malloc;
>
> -static char tmp_path[] = "/tmp/qtest.XX";
> -static char debug_path[] = "/tmp/qtest-blkdebug.XX";
> +static char *tmp_path;
> +static char *debug_path;
>
> static QTestState *ide_test_start(const char *cmdline_fmt, ...)
> {
> @@ -1015,12 +1015,12 @@ int main(int argc, char **argv)
> int ret;
>
> /* Create temporary blkdebug instructions */
> -fd = mkstemp(debug_path);
> +fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
> g_assert(fd >= 0);
> close(fd);
>
> /* Create a temporary raw image */
> -fd = mkstemp(tmp_path);
> +fd = g_file_open_tmp("qtest.XX", _path, NULL);
> g_assert(fd >= 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> g_assert(ret == 0);
> @@ -1049,7 +1049,9 @@ int main(int argc, char **argv)
>
> /* Cleanup */
> unlink(tmp_path);
> +g_free(tmp_path);
> unlink(debug_path);
> +g_free(debug_path);
>
> return ret;
> }
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v3 09/54] tests/qtest: fdc-test: Avoid using hardcoded /tmp
On Sun, Sep 25, 2022 at 3:49 PM Bin Meng wrote:
> From: Bin Meng
>
> This case was written to use hardcoded /tmp directory for temporary
> files. Update to use g_file_open_tmp() for a portable implementation.
>
> Signed-off-by: Bin Meng
>
Reviewed-by: Marc-André Lureau
> ---
>
> Changes in v3:
> - Split to a separate patch
>
> tests/qtest/fdc-test.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> index 52ade90a7d..1f9b99ad6d 100644
> --- a/tests/qtest/fdc-test.c
> +++ b/tests/qtest/fdc-test.c
> @@ -68,7 +68,7 @@ enum {
> DSKCHG = 0x80,
> };
>
> -static char test_image[] = "/tmp/qtest.XX";
> +static char *test_image;
>
> #define assert_bit_set(data, mask) g_assert_cmphex((data) & (mask), ==,
> (mask))
> #define assert_bit_clear(data, mask) g_assert_cmphex((data) & (mask), ==,
> 0)
> @@ -608,7 +608,7 @@ int main(int argc, char **argv)
> int ret;
>
> /* Create a temporary raw image */
> -fd = mkstemp(test_image);
> +fd = g_file_open_tmp("qtest.XX", _image, NULL);
> g_assert(fd >= 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> g_assert(ret == 0);
> @@ -640,6 +640,7 @@ int main(int argc, char **argv)
> /* Cleanup */
> qtest_end();
> unlink(test_image);
> +g_free(test_image);
>
> return ret;
> }
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 20/39] tests/qtest: {ahci, ide}-test: Use relative path for temporary files for win32
Hi
On Tue, Sep 20, 2022 at 1:50 PM Bin Meng wrote:
> From: Bin Meng
>
> These test cases uses "blkdebug:path/to/config:path/to/image" for
> testing. On Windows, absolute file paths contain the delimiter ':'
> which causes the blkdebug filename parser fail to parse filenames.
>
> Signed-off-by: Bin Meng
>
I don't have a much better solution to propose at this point (to actually
use a temp directory), so:
Reviewed-by: Marc-André Lureau
> ---
>
> (no changes since v1)
>
> tests/qtest/ahci-test.c | 21 ++---
> tests/qtest/ide-test.c | 20 ++--
> 2 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index 00524f14c6..c57576b08c 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1833,7 +1833,7 @@ static void create_ahci_io_test(enum IOMode type,
> enum AddrMode addr,
>
> int main(int argc, char **argv)
> {
> -const char *arch;
> +const char *arch, *base;
> int ret;
> int fd;
> int c;
> @@ -1871,8 +1871,22 @@ int main(int argc, char **argv)
> return 0;
> }
>
> +/*
> + * "base" stores the starting point where we create temporary files.
> + *
> + * On Windows, this is set to the relative path of current working
> + * directory, because the absolute path causes the blkdebug filename
> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> + */
> +#ifndef _WIN32
> +base = g_get_tmp_dir();
> +#else
> +base = ".";
> +#endif
> +
> /* Create a temporary image */
> -fd = g_file_open_tmp("qtest.XX", _path, NULL);
> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
> +fd = g_mkstemp(tmp_path);
> g_assert(fd >= 0);
> if (have_qemu_img()) {
> imgfmt = "qcow2";
> @@ -1889,7 +1903,8 @@ int main(int argc, char **argv)
> close(fd);
>
> /* Create temporary blkdebug instructions */
> -fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
> +fd = g_mkstemp(debug_path);
> g_assert(fd >= 0);
> close(fd);
>
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index 25302be6dc..5e3e28aea2 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -1011,16 +1011,32 @@ static void test_cdrom_dma(void)
>
> int main(int argc, char **argv)
> {
> +const char *base;
> int fd;
> int ret;
>
> +/*
> + * "base" stores the starting point where we create temporary files.
> + *
> + * On Windows, this is set to the relative path of current working
> + * directory, because the absolute path causes the blkdebug filename
> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> + */
> +#ifndef _WIN32
> +base = g_get_tmp_dir();
> +#else
> +base = ".";
> +#endif
> +
> /* Create temporary blkdebug instructions */
> -fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
> +fd = g_mkstemp(debug_path);
> g_assert(fd >= 0);
> close(fd);
>
> /* Create a temporary raw image */
> -fd = g_file_open_tmp("qtest.XX", _path, NULL);
> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
> +fd = g_mkstemp(tmp_path);
> g_assert(fd >= 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> g_assert(ret == 0);
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 08/39] block/vvfat: Unify the mkdir() call
On Tue, Sep 20, 2022 at 2:58 PM Bin Meng wrote:
> From: Bin Meng
>
> There is a difference in the mkdir() call for win32 and non-win32
> platforms, and currently is handled in the codes with #ifdefs.
>
> glib provides a portable g_mkdir() API and we can use it to unify
> the codes without #ifdefs.
>
> Signed-off-by: Bin Meng
>
Reviewed-by: Marc-André Lureau
> ---
>
> Changes in v2:
> - Change to use g_mkdir()
>
> block/vvfat.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d6dd919683..723beef025 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -25,6 +25,7 @@
>
> #include "qemu/osdep.h"
> #include
> +#include
> #include "qapi/error.h"
> #include "block/block_int.h"
> #include "block/qdict.h"
> @@ -2726,13 +2727,9 @@ static int
> handle_renames_and_mkdirs(BDRVVVFATState* s)
> mapping_t* mapping;
> int j, parent_path_len;
>
> -#ifdef __MINGW32__
> -if (mkdir(commit->path))
> +if (g_mkdir(commit->path, 0755)) {
> return -5;
> -#else
> -if (mkdir(commit->path, 0755))
> -return -5;
> -#endif
> + }
>
> mapping = insert_mapping(s, commit->param.mkdir.cluster,
> commit->param.mkdir.cluster + 1);
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 07/39] tests: Avoid using hardcoded /tmp in test cases
trerror(errno));
> +g_free(image_path);
> exit(EXIT_FAILURE);
> }
> if (ftruncate(fd, UNIFORM_FLASH_SIZE) < 0) {
> int error_code = errno;
> close(fd);
> -unlink(image_path);
> +cleanup(NULL);
> g_printerr("Failed to truncate file %s to %u MB: %s\n",
> image_path,
> UNIFORM_FLASH_SIZE, strerror(error_code));
> exit(EXIT_FAILURE);
> diff --git a/tests/qtest/qmp-test.c b/tests/qtest/qmp-test.c
> index bf7304c7dc..0fa00c12dc 100644
> --- a/tests/qtest/qmp-test.c
> +++ b/tests/qtest/qmp-test.c
> @@ -161,12 +161,13 @@ static void test_qmp_protocol(void)
>
> /* Out-of-band tests */
>
> -char tmpdir[] = "/tmp/qmp-test-XX";
> +char *tmpdir;
> char *fifo_name;
>
> static void setup_blocking_cmd(void)
> {
> -if (!g_mkdtemp(tmpdir)) {
> +tmpdir = g_dir_make_tmp("qmp-test-XX", NULL);
> +if (!tmpdir) {
> g_error("g_mkdtemp: %s", strerror(errno));
> }
> fifo_name = g_strdup_printf("%s/fifo", tmpdir);
> @@ -179,6 +180,7 @@ static void cleanup_blocking_cmd(void)
> {
> unlink(fifo_name);
> rmdir(tmpdir);
> +g_free(tmpdir);
> }
>
> static void send_cmd_that_blocks(QTestState *s, const char *id)
> diff --git a/tests/qtest/vhost-user-blk-test.c
> b/tests/qtest/vhost-user-blk-test.c
> index a81c2a2715..07a4c2d500 100644
> --- a/tests/qtest/vhost-user-blk-test.c
> +++ b/tests/qtest/vhost-user-blk-test.c
> @@ -841,7 +841,8 @@ static char *create_listen_socket(int *fd)
> char *path;
>
> /* No race because our pid makes the path unique */
> -path = g_strdup_printf("/tmp/qtest-%d-sock.XX", getpid());
> +path = g_strdup_printf("%s/qtest-%d-sock.XX",
> + g_get_tmp_dir(), getpid());
> tmp_fd = mkstemp(path);
> g_assert_cmpint(tmp_fd, >=, 0);
> close(tmp_fd);
> diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> index d7d6cfc9bd..4e1aae1794 100644
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -482,8 +482,7 @@ static TestServer *test_server_new(const gchar *name,
> struct vhost_user_ops *ops)
> {
> TestServer *server = g_new0(TestServer, 1);
> -char template[] = "/tmp/vhost-test-XX";
> -const char *tmpfs;
> +g_autofree const char *tmpfs;
>
> server->context = g_main_context_new();
> server->loop = g_main_loop_new(server->context, FALSE);
> @@ -491,9 +490,10 @@ static TestServer *test_server_new(const gchar *name,
> /* run the main loop thread so the chardev may operate */
> server->thread = g_thread_new(NULL, thread_function, server->loop);
>
> -tmpfs = g_mkdtemp(template);
> +tmpfs = g_dir_make_tmp("vhost-test-XX", NULL);
> if (!tmpfs) {
> -g_test_message("g_mkdtemp on path (%s): %s", template,
> strerror(errno));
> +g_test_message("g_dir_make_tmp on path (%s): %s", tmpfs,
> + strerror(errno));
> }
> g_assert(tmpfs);
>
> diff --git a/tests/qtest/virtio-blk-test.c b/tests/qtest/virtio-blk-test.c
> index dc5eed31c8..19c01f808b 100644
> --- a/tests/qtest/virtio-blk-test.c
> +++ b/tests/qtest/virtio-blk-test.c
> @@ -49,10 +49,10 @@ static void drive_destroy(void *path)
> static char *drive_create(void)
> {
> int fd, ret;
> -char *t_path = g_strdup("/tmp/qtest.XX");
> +char *t_path;
>
> /* Create a temporary raw image */
> -fd = mkstemp(t_path);
> +fd = g_file_open_tmp("qtest.XX", _path, NULL);
> g_assert_cmpint(fd, >=, 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> g_assert_cmpint(ret, ==, 0);
> diff --git a/tests/qtest/virtio-scsi-test.c
> b/tests/qtest/virtio-scsi-test.c
> index 8ceb12aacd..073a89d535 100644
> --- a/tests/qtest/virtio-scsi-test.c
> +++ b/tests/qtest/virtio-scsi-test.c
> @@ -268,7 +268,7 @@ static void test_iothread_attach_node(void *obj, void
> *data,
> QVirtioSCSIPCI *scsi_pci = obj;
> QVirtioSCSI *scsi = _pci->scsi;
> QVirtioSCSIQueues *vs;
> -char tmp_path[] = "/tmp/qtest.XX";
> +g_autofree char *tmp_path;
> int fd;
> int ret;
>
> @@ -282,7 +282,7 @@ static void test_iothread_attach_node(void *obj, void
> *data,
> vs = qvirtio_scsi_init(scsi->vdev);
>
> /* Create a temporary qcow2 overlay*/
> -fd = mkstemp(tmp_path);
> +fd = g_file_open_tmp("qtest.XX", _path, NULL);
> g_assert(fd >= 0);
> close(fd);
>
> diff --git a/tests/unit/test-image-locking.c
> b/tests/unit/test-image-locking.c
> index ba057bd66c..d09ff43fcb 100644
> --- a/tests/unit/test-image-locking.c
> +++ b/tests/unit/test-image-locking.c
> @@ -76,10 +76,10 @@ static void check_locked_bytes(int fd, uint64_t
> perm_locks,
> static void test_image_locking_basic(void)
> {
> BlockBackend *blk1, *blk2, *blk3;
> -char img_path[] = "/tmp/qtest.XX";
> +g_autofree char *img_path;
> uint64_t perm, shared_perm;
>
> -int fd = mkstemp(img_path);
> +int fd = g_file_open_tmp("qtest.XX", _path, NULL);
> assert(fd >= 0);
>
> perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
> @@ -117,10 +117,10 @@ static void test_image_locking_basic(void)
> static void test_set_perm_abort(void)
> {
> BlockBackend *blk1, *blk2;
> -char img_path[] = "/tmp/qtest.XX";
> +g_autofree char *img_path;
> uint64_t perm, shared_perm;
> int r;
> -int fd = mkstemp(img_path);
> +int fd = g_file_open_tmp("qtest.XX", _path, NULL);
> assert(fd >= 0);
>
> perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> index a05a4628ed..b73d231cd5 100644
> --- a/tests/unit/test-qga.c
> +++ b/tests/unit/test-qga.c
> @@ -59,7 +59,7 @@ fixture_setup(TestFixture *fixture, gconstpointer data,
> gchar **envp)
>
> fixture->loop = g_main_loop_new(NULL, FALSE);
>
> -fixture->test_dir = g_strdup("/tmp/qgatest.XX");
> +fixture->test_dir = g_strdup_printf("%s/qgatest.XX",
> g_get_tmp_dir());
> g_assert_nonnull(g_mkdtemp(fixture->test_dir));
>
> path = g_build_filename(fixture->test_dir, "sock", NULL);
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 9b1dab2f28..fecdf915e7 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -631,7 +631,6 @@ static void *notifier_thread(void *arg)
> static void
> vubr_host_notifier_setup(VubrDev *dev)
> {
> -char template[] = "/tmp/vubr-XX";
> pthread_t thread;
> size_t length;
> void *addr;
> @@ -639,7 +638,7 @@ vubr_host_notifier_setup(VubrDev *dev)
>
> length = qemu_real_host_page_size() * VHOST_USER_BRIDGE_MAX_QUEUES;
>
> -fd = mkstemp(template);
> +fd = g_file_open_tmp("vubr-XX", NULL, NULL);
> if (fd < 0) {
> vubr_die("mkstemp()");
> }
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 03/39] block: Unify the get_tmp_filename() implementation
Hi
On Tue, Sep 20, 2022 at 2:17 PM Bin Meng wrote:
> From: Bin Meng
>
> At present get_tmp_filename() has platform specific implementations
> to get the directory to use for temporary files. Switch over to use
> g_get_tmp_dir() which works on all supported platforms.
>
>
As discussed in v1, there are other things to do while touching this code.
And since it is optional for the series goal, please send this as a
different patch/series.
> Signed-off-by: Bin Meng
> ---
>
> (no changes since v1)
>
> block.c | 16 ++--
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/block.c b/block.c
> index bc85f46eed..d06df47f72 100644
> --- a/block.c
> +++ b/block.c
> @@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> HDGeometry *geo)
> */
> int get_tmp_filename(char *filename, int size)
> {
> -#ifdef _WIN32
> -char temp_dir[MAX_PATH];
> -/* GetTempFileName requires that its output buffer (4th param)
> - have length MAX_PATH or greater. */
> -assert(size >= MAX_PATH);
> -return (GetTempPath(MAX_PATH, temp_dir)
> -&& GetTempFileName(temp_dir, "qem", 0, filename)
> -? 0 : -GetLastError());
> -#else
> int fd;
> const char *tmpdir;
> -tmpdir = getenv("TMPDIR");
> -if (!tmpdir) {
> -tmpdir = "/var/tmp";
> -}
> +tmpdir = g_get_tmp_dir();
> +
> if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> return -EOVERFLOW;
> }
> @@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size)
> return -errno;
> }
> return 0;
> -#endif
> }
>
> /*
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
Hi
On Fri, Sep 2, 2022 at 3:16 PM Markus Armbruster wrote:
> Marc-André Lureau writes:
>
> > Hi
> >
> > On Thu, Aug 11, 2022 at 5:35 PM Markus Armbruster
> wrote:
> >
> >> Daniel P. Berrangé writes:
> >>
> >> > On Thu, Aug 11, 2022 at 02:50:01PM +0400, Marc-André Lureau wrote:
> >> >> Hi
> >> >>
> >> >> On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell <
> peter.mayd...@linaro.org
> >> >
> >> >> wrote:
> >>
> >> [...]
> >>
> >> >> > As Markus says, your branch ends up moving most of qobject into
> >> >> > qemu-common/. We are never going to let that out of QEMU proper,
> >> >> > because we are never going to allow ourselves to be tied to API
> >> >> > compatibility with it as an external library. So anything that
> >> >> >
> >> >>
> >> >> Why is that? We do it with a lot of dependencies already, with stable
> >> APIs.
> >> >>
> >> >> Furthermore, we don't "have to" be tied to a specific ABI/API, we can
> >> >> continue to link statically and compile against a specific version.
> like
> >> >> with libvfio-user today.
> >> >>
> >> >> And at this point, I am _not_ proposing to have an extra
> "qemu-common"
> >> >> repository. I don't think there are enough reasons to want that
> either.
> >> >>
> >> >>
> >> >>
> >> >> > needs qobject is never going to leave the QEMU codebase. Which
> >> >> > means that there's not much gain from shoving it into subproject/
> >> >> > IMHO.
> >> >>
> >> >>
> >> >> (just to be extra clear, it's qobject not QOM we are talking about)
> >> >>
> >> >> qobject is fundamental to all the QAPI related generated code. Why
> >> should
> >> >> that remain tight to QEMU proper?
> >> >
> >> > Neither qobject nor QOM have ever been designed to be public APIs.
> >> > Though admittedly qobject is quite a bit simpler as an API, I'm
> >> > not convinced its current design is something we want to consider
> >> > public. As an example, just last month Markus proposed changing
> >> > QDict's implementation
> >> >
> >> > https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
> >> >
> >> >
> >> > If we want external projects to be able to take advantage of QAPI,
> >> > the bare minimum we need to be public is a QAPI parser, from which
> >> > people can then build any code generators desired.
> >>
> >> Basically scripts/qapi/ less the code generators.
> >>
> >>
> > The generated code is used by qemu-ga & storage daemon, at least. They
> are
> > the first potential consumers, after qemu.
> >
> >
> >> Not sure a subproject would be a good fit.
> >>
> >
> > (I won't repeat the arguments of structuring a project)
> >
> >
> >>
> >> Shot from the hip: could the build process spit out something external
> >> projects could consume? It's how "consumables" are commonly delivered.
> >> E.g. .so + a bunch of headers. Sometimes that gets packaged. Sometimes
> >> it gets copied into the consuming tree ("vendored").
> >>
> >>
> > Not sure I follow, but that's just the "install" step isn't it ?
> >
> > Sure, we could have a "qemu-devel", that ships qapi-gen, I guess. But
> then,
> > you would expect stability/versioning. That's not what I am proposing (at
> > least at this point), which is just about the build system and project
> > structure, so we can build and work on subprojects independently. (for
> ex,
> > in my wip branch, qemu-ga meson.build is 115 lines, doesn't need QEMU
> > configure etc)
>
> I'm afraid I'm still wobbly on the benefits of subprojects, or even how
> to work with them.
>
> How exactly would we "build and work independently" on the subprojects
> involving QAPI? git-clone all of QEMU, but build only a subproject (and
> its dependencies)? Am I confused?
>
Yes, QEMU repository would hold some subprojects (like libvhost-user
today), that you can build/develop independently with just meson / ninja.
>
> >> > We don't neccessarily need the current QAPI C code generator. There
> >> >
Re: [PATCH 38/51] tests/qtest: {ahci, ide}-test: Open file in binary mode
Hi On Wed, Aug 24, 2022 at 3:08 PM Bin Meng wrote: > From: Xuzhou Cheng > > By default Windows opens file in text mode, while a POSIX compliant > implementation treats text files and binary files the same. > > The fopen() 'mode' string can include the letter 'b' to indicate > binary mode shall be used. POSIX spec says the character 'b' shall > have no effect, but is allowed for ISO C standard conformance. > Let's add the letter 'b' which works on both POSIX and Windows. > > Similar situation applies to the open() 'flags' where O_BINARY is > used for binary mode. > > Signed-off-by: Xuzhou Cheng > Signed-off-by: Bin Meng > --- > > tests/qtest/ahci-test.c | 2 +- > tests/qtest/ide-test.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c > index bce9ff770c..be11508c75 100644 > --- a/tests/qtest/ahci-test.c > +++ b/tests/qtest/ahci-test.c > @@ -1453,7 +1453,7 @@ static int prepare_iso(size_t size, unsigned char > **buf, char **name) > * Close the file and reopen it. > */ > close(fd); > -fd = open(cdrom_path, O_WRONLY); > +fd = open(cdrom_path, O_WRONLY | O_BINARY); > g_assert(fd != -1); > that should be gone in next iteration, with g_mkstemp() usage. > #endif > > diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c > index c5cad6c0be..ee03dea4fa 100644 > --- a/tests/qtest/ide-test.c > +++ b/tests/qtest/ide-test.c > @@ -892,7 +892,7 @@ static void cdrom_pio_impl(int nblocks) > > /* Prepopulate the CDROM with an interesting pattern */ > generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE); > -fh = fopen(tmp_path, "w+"); > +fh = fopen(tmp_path, "wb+"); > ret = fwrite(pattern, ATAPI_BLOCK_SIZE, patt_blocks, fh); > g_assert_cmpint(ret, ==, patt_blocks); > fclose(fh); > @@ -993,7 +993,7 @@ static void test_cdrom_dma(void) > prdt[0].size = cpu_to_le32(len | PRDT_EOT); > > generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE); > -fh = fopen(tmp_path, "w+"); > + fh = fopen(tmp_path, "wb+"); > ret = fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh); > g_assert_cmpint(ret, ==, 16); > fclose(fh); > -- > 2.34.1 > > > ack this part, Reviewed-by: Marc-André Lureau -- Marc-André Lureau
Re: [PATCH 33/51] tests/qtest: {ahci, ide}-test: Use relative path for temporary files
On Wed, Aug 24, 2022 at 2:55 PM Bin Meng wrote:
> From: Bin Meng
>
> These test cases uses "blkdebug:path/to/config:path/to/image" for
> testing. On Windows, absolute file paths contain the delimiter ':'
> which causes the blkdebug filename parser fail to parse filenames.
>
>
hmm.. maybe it should learn to escape paths..
Signed-off-by: Bin Meng
> ---
>
> tests/qtest/ahci-test.c | 19 ---
> tests/qtest/ide-test.c | 18 --
> 2 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index 0e88cd0eef..bce9ff770c 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1848,7 +1848,7 @@ static void create_ahci_io_test(enum IOMode type,
> enum AddrMode addr,
>
> int main(int argc, char **argv)
> {
> -const char *arch;
> +const char *arch, *base;
> int ret;
> int fd;
> int c;
> @@ -1886,8 +1886,21 @@ int main(int argc, char **argv)
> return 0;
> }
>
> +/*
> + * "base" stores the starting point where we create temporary files.
> + *
> + * On Windows, this is set to the relative path of current working
> + * directory, because the absolute path causes the blkdebug filename
> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> + */
> +#ifndef _WIN32
> +base = g_get_tmp_dir();
> +#else
> +base = ".";
> +#endif
>
Meanwhile, that seems reasonable. Perhaps chdir() to the temporary
directory first? (assuming other paths are absolute)
> +
> /* Create a temporary image */
> -tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
> fd = mkstemp(tmp_path);
> g_assert(fd >= 0);
> if (have_qemu_img()) {
> @@ -1905,7 +1918,7 @@ int main(int argc, char **argv)
> close(fd);
>
> /* Create temporary blkdebug instructions */
> -debug_path = g_strdup_printf("%s/qtest-blkdebug.XX",
> g_get_tmp_dir());
> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
> fd = mkstemp(debug_path);
> g_assert(fd >= 0);
> close(fd);
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index ebbf8e0126..c5cad6c0be 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -1011,17 +1011,31 @@ static void test_cdrom_dma(void)
>
> int main(int argc, char **argv)
> {
> +const char *base;
> int fd;
> int ret;
>
> +/*
> + * "base" stores the starting point where we create temporary files.
> + *
> + * On Windows, this is set to the relative path of current working
> + * directory, because the absolute path causes the blkdebug filename
> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> + */
> +#ifndef _WIN32
> +base = g_get_tmp_dir();
> +#else
> +base = ".";
> +#endif
> +
> /* Create temporary blkdebug instructions */
> -debug_path = g_strdup_printf("%s/qtest-blkdebug.XX",
> g_get_tmp_dir());
> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
> fd = mkstemp(debug_path);
> g_assert(fd >= 0);
> close(fd);
>
> /* Create a temporary raw image */
> -tmp_path = g_strdup_printf("%s/qtest.XX", g_get_tmp_dir());
> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
> fd = mkstemp(tmp_path);
> g_assert(fd >= 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH 32/51] tests/qtest: Fix ERROR_SHARING_VIOLATION for win32
Hi
On Wed, Aug 24, 2022 at 2:03 PM Bin Meng wrote:
> From: Bin Meng
>
> On Windows, the MinGW provided mkstemp() API opens the file with
> exclusive access, denying other processes to read/write the file.
> Such behavior prevents the QEMU executable from opening the file,
> (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
>
g_mkstemp() doesn't have this behaviour (after running a quick test). Use
it?
>
> This can be fixed by closing the file and reopening it.
>
> Signed-off-by: Bin Meng
> ---
>
> tests/qtest/ahci-test.c| 14 ++
> tests/qtest/boot-serial-test.c | 13 +
> 2 files changed, 27 insertions(+)
>
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index f26cd6f86f..0e88cd0eef 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1443,6 +1443,20 @@ static int prepare_iso(size_t size, unsigned char
> **buf, char **name)
> int fd = mkstemp(cdrom_path);
>
> g_assert(fd != -1);
> +#ifdef _WIN32
> +/*
> + * On Windows, the MinGW provided mkstemp() API opens the file with
> + * exclusive access, denying other processes to read/write the file.
> + * Such behavior prevents the QEMU executable from opening the file,
> + * (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
> + *
> + * Close the file and reopen it.
> + */
> +close(fd);
> +fd = open(cdrom_path, O_WRONLY);
> +g_assert(fd != -1);
> +#endif
> +
> g_assert(buf);
> g_assert(name);
> patt = g_malloc(size);
> diff --git a/tests/qtest/boot-serial-test.c
> b/tests/qtest/boot-serial-test.c
> index 404adcfa20..fb6c81bf35 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -235,6 +235,19 @@ static void test_machine(const void *data)
>
> ser_fd = mkstemp(serialtmp);
> g_assert(ser_fd != -1);
> +#ifdef _WIN32
> +/*
> + * On Windows, the MinGW provided mkstemp() API opens the file with
> + * exclusive access, denying other processes to read/write the file.
> + * Such behavior prevents the QEMU executable from opening the file,
> + * (e.g.: CreateFile returns ERROR_SHARING_VIOLATION).
> + *
> + * Close the file and reopen it.
> + */
> + close(ser_fd);
> +ser_fd = open(serialtmp, O_RDONLY);
> +g_assert(ser_fd != -1);
> +#endif
>
> if (test->kernel) {
> code = test->kernel;
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH 08/51] block/vvfat: Unify the mkdir() call
Hi
On Wed, Aug 24, 2022 at 2:20 PM Bin Meng wrote:
> From: Bin Meng
>
> There is a difference in the mkdir() call for win32 and non-win32
> platforms, and currently is handled in the codes with #ifdefs.
>
> glib provides a portable g_mkdir_with_parents() API and we can use
> it to unify the codes without #ifdefs.
>
Why use the _with_parents() version?
You should replace with g_mkdir(), or explain the reasons.
thanks
>
> Signed-off-by: Bin Meng
> ---
>
> block/vvfat.c | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d6dd919683..9c389ce5ea 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2726,13 +2726,9 @@ static int
> handle_renames_and_mkdirs(BDRVVVFATState* s)
> mapping_t* mapping;
> int j, parent_path_len;
>
> -#ifdef __MINGW32__
> -if (mkdir(commit->path))
> +if (g_mkdir_with_parents(commit->path, 0755)) {
> return -5;
> -#else
> -if (mkdir(commit->path, 0755))
> -return -5;
> -#endif
> +}
>
> mapping = insert_mapping(s, commit->param.mkdir.cluster,
> commit->param.mkdir.cluster + 1);
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH 03/51] block: Unify the get_tmp_filename() implementation
Hi Bin
On Wed, Aug 24, 2022 at 1:42 PM Bin Meng wrote:
> From: Bin Meng
>
> At present get_tmp_filename() has platform specific implementations
> to get the directory to use for temporary files. Switch over to use
> g_get_tmp_dir() which works on all supported platforms.
>
>
It "works" quite differently though. Is this patch really necessary here?
If yes, please explain why.
If not, I suggest you drop optional / rfc / "nice to have" patches from the
series. It will help to get it merged faster.
thanks
> Signed-off-by: Bin Meng
> ---
>
> block.c | 16 ++--
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/block.c b/block.c
> index bc85f46eed..d06df47f72 100644
> --- a/block.c
> +++ b/block.c
> @@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> HDGeometry *geo)
> */
> int get_tmp_filename(char *filename, int size)
> {
> -#ifdef _WIN32
> -char temp_dir[MAX_PATH];
> -/* GetTempFileName requires that its output buffer (4th param)
> - have length MAX_PATH or greater. */
> -assert(size >= MAX_PATH);
> -return (GetTempPath(MAX_PATH, temp_dir)
> -&& GetTempFileName(temp_dir, "qem", 0, filename)
> -? 0 : -GetLastError());
> -#else
> int fd;
> const char *tmpdir;
> -tmpdir = getenv("TMPDIR");
> -if (!tmpdir) {
> -tmpdir = "/var/tmp";
> -}
> +tmpdir = g_get_tmp_dir();
> +
> if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> return -EOVERFLOW;
> }
> @@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size)
> return -errno;
> }
> return 0;
> -#endif
> }
>
> /*
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
Hi
On Thu, Aug 11, 2022 at 5:35 PM Markus Armbruster wrote:
> Daniel P. Berrangé writes:
>
> > On Thu, Aug 11, 2022 at 02:50:01PM +0400, Marc-André Lureau wrote:
> >> Hi
> >>
> >> On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell >
> >> wrote:
>
> [...]
>
> >> > As Markus says, your branch ends up moving most of qobject into
> >> > qemu-common/. We are never going to let that out of QEMU proper,
> >> > because we are never going to allow ourselves to be tied to API
> >> > compatibility with it as an external library. So anything that
> >> >
> >>
> >> Why is that? We do it with a lot of dependencies already, with stable
> APIs.
> >>
> >> Furthermore, we don't "have to" be tied to a specific ABI/API, we can
> >> continue to link statically and compile against a specific version. like
> >> with libvfio-user today.
> >>
> >> And at this point, I am _not_ proposing to have an extra "qemu-common"
> >> repository. I don't think there are enough reasons to want that either.
> >>
> >>
> >>
> >> > needs qobject is never going to leave the QEMU codebase. Which
> >> > means that there's not much gain from shoving it into subproject/
> >> > IMHO.
> >>
> >>
> >> (just to be extra clear, it's qobject not QOM we are talking about)
> >>
> >> qobject is fundamental to all the QAPI related generated code. Why
> should
> >> that remain tight to QEMU proper?
> >
> > Neither qobject nor QOM have ever been designed to be public APIs.
> > Though admittedly qobject is quite a bit simpler as an API, I'm
> > not convinced its current design is something we want to consider
> > public. As an example, just last month Markus proposed changing
> > QDict's implementation
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
> >
> >
> > If we want external projects to be able to take advantage of QAPI,
> > the bare minimum we need to be public is a QAPI parser, from which
> > people can then build any code generators desired.
>
> Basically scripts/qapi/ less the code generators.
>
>
The generated code is used by qemu-ga & storage daemon, at least. They are
the first potential consumers, after qemu.
> Not sure a subproject would be a good fit.
>
(I won't repeat the arguments of structuring a project)
>
> Shot from the hip: could the build process spit out something external
> projects could consume? It's how "consumables" are commonly delivered.
> E.g. .so + a bunch of headers. Sometimes that gets packaged. Sometimes
> it gets copied into the consuming tree ("vendored").
>
>
Not sure I follow, but that's just the "install" step isn't it ?
Sure, we could have a "qemu-devel", that ships qapi-gen, I guess. But then,
you would expect stability/versioning. That's not what I am proposing (at
least at this point), which is just about the build system and project
structure, so we can build and work on subprojects independently. (for ex,
in my wip branch, qemu-ga meson.build is 115 lines, doesn't need QEMU
configure etc)
> > We don't neccessarily need the current QAPI C code generator. There
> > could be a new C generator that didn't use qobject, but instead used
> > some standard GLib types like GHashTable/GList instead of QDict/QList.
>
> Yes, that should be possible.
>
>
I can't see much benefit from doing that extra work. It would create two C
APIs, making future bindings efforts more difficult etc.
QObject is very much like GValue though (the naming is better too :), just
like the QOM & GObject story.
thanks
--
Marc-André Lureau
Re: [RFC v2 00/10] Introduce an extensible static analyzer
Hi On Fri, Aug 12, 2022 at 7:49 PM Alberto Faria wrote: > > On Thu, Aug 4, 2022 at 12:44 PM Marc-André Lureau > wrote: > > On fc36, I had several dependencies I needed to install manually (imho > > they should have been pulled by python3-clang), but more annoyingly I > > got: > > clang.cindex.LibclangError: libclang.so: cannot open shared object > > file: No such file or directory. To provide a path to libclang use > > Config.set_library_path() or Config.set_library_file(). > > > > clang-libs doesn't install libclang.so, I wonder why. I made a link > > manually and it works, but it's probably incorrect. I'll try to open > > issues for the clang packaging. > > That's strange. Thanks for looking into this. No that's normal, I just got confused. clang-devel provides it. However, I opened https://bugzilla.redhat.com/show_bug.cgi?id=2115362 "python3-clang depends on libclang.so"
Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
Hi On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell wrote: > On Thu, 11 Aug 2022 at 11:09, Marc-André Lureau > wrote: > > On Thu, Aug 11, 2022 at 1:05 PM Markus Armbruster > wrote: > >> Your moves tear closely related code apart. This is going to be a drag > >> for developers in general and maintainers in particular. > >> > >> Ergonomics suffer when related code is in multiple places. Having to > >> switch between directories and remember where is what will a constant > >> low-level pain. Things that used to be simple & quick, like git-grep > >> qapi/*.c, become more involved. > >> > > > > It's inevitable when a project grows. QEMU is already a very large > project. Over the years, we have structured the project, by moving things > and splitting in subdirectories. Imho, this is actually helpful in many > ways, and we got used to it easily hopefully. > > I agree with this. But generally we've tried to do that by > having the subdirectory splitting and naming match the > structure of the codebase. The exception, which I strongly > dislike, is that contrib/ is a grabbag of random stuff. > subprojects/ is starting to feel like it is also turning > into "place where random stuff ends up"... > Yes, most of contrib/* should probably be standalone projects. If only we had some kind of common library/subproject :) subproject/* is a meson *cough* convention (imposed location for subprojects). If we don't want to depend on external released libraries, that's just the way it is. > > > Do you see fundamental reasons why qemu-ga or (qemu-img, qemu-nbd, > storage-daemon, virtiofsd, vhost-user-*, *helper, ivshmem* etc) need to be > tight to qemu code, release and management process? I am not saying it's > time to move them out of qemu project, but I believe it's helpful to have > code that is structured and can be compiled indepently. > > > > And by having "standalone" subprojects, we can more easily evolve in new > directions, without touching the rest of the projects. > > As Markus says, your branch ends up moving most of qobject into > qemu-common/. We are never going to let that out of QEMU proper, > because we are never going to allow ourselves to be tied to API > compatibility with it as an external library. So anything that > Why is that? We do it with a lot of dependencies already, with stable APIs. Furthermore, we don't "have to" be tied to a specific ABI/API, we can continue to link statically and compile against a specific version. like with libvfio-user today. And at this point, I am _not_ proposing to have an extra "qemu-common" repository. I don't think there are enough reasons to want that either. > needs qobject is never going to leave the QEMU codebase. Which > means that there's not much gain from shoving it into subproject/ > IMHO. (just to be extra clear, it's qobject not QOM we are talking about) qobject is fundamental to all the QAPI related generated code. Why should that remain tight to QEMU proper? thanks -- Marc-André Lureau
Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
On Thu, Aug 11, 2022 at 1:05 PM Markus Armbruster wrote: > Marc-André Lureau writes: > > > Hi > > > > On Thu, Aug 11, 2022 at 10:52 AM Markus Armbruster > > wrote: > > > >> Marc-André Lureau writes: > >> > >> > Hi > >> > > >> > On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster > >> wrote: > >> > > >> >> marcandre.lur...@redhat.com writes: > >> >> > >> >> > From: Marc-André Lureau > >> >> > > >> >> > This is just moving qapi-gen.py and related subdir to qemu-common, > to > >> >> > ease review and proceed step by step. The following patches will > move > >> >> > related necessary code, tests etc. > >> >> > > >> >> > Signed-off-by: Marc-André Lureau > >> >> > >> >> As moved files tend to become low-level annoyances for a long time, > I'd > >> >> like to understand why you want to move them. The commit message > says > >> >> "to ease review", which I suspect isn't the real reason. Perhaps you > >> >> explained all that elsewhere already, but I missed it. > >> >> > >> >> > >> >> > >> > The end goal is to split some projects, such as qemu-ga, to standalone > >> > meson projects/subprojects. We will be able to build them > independently > >> > from the rest of QEMU, and later on perhaps handle them outside of > QEMU > >> > main repository. To achieve this, I first introduce a qemu-common > >> > subproject, where qapi and common units are provided. You can check > >> > https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak > peek at > >> > current end result. > >> > >> I worry this move of the QAPI generator code into > >> subjprojects/common/scripts/qapi/ will be followed by a move into its > >> own subproject. > >> > > > > Do you mean: it could be moved again to another smaller subproject? not > > really, see below > > > > > >> Ignorant question: could we turn the QAPI generator into a subproject in > >> place? > >> > > > > If it's just the generator, probably the target would then be a python > > project (not meson), similar to python-qemu-qmp. > > > > But I don't see much point, since it's not really a standalone python > > module, it generates code, and that code needs most of what is in > > qemu-common (see > > > https://gitlab.com/marcandre.lureau/qemu/-/tree/qga/subprojects/qemu-common > ). > > It's best to have it together imho. Maybe we can consider a different > > naming or to be more careful not to add stuff that is not strictly needed > > by qapi? > > I had a look at subjprojects/qemu-common in your qga branch. Contents: > > * Subproject machinery > > * Some common headers (glib-compat.h), but not others (qemu/osdep.h). I > guess it's whatever subjproject code needs. > > Is subprojects/qemu-common/include/block/nvme.h there by accident? > It's a header shared with qemu-ga (the commit message explains) > > * Most of the QObject subsystem > > qobject/block-qdict.c is left behind. > It's qemu block specific. Not needed to move at this point, it drags other stuff iirc. > * Most of the QAPI subsystem > > Some visitors left behind: opts, forward, string input / output. Hmm, > only the .c, the .h are in the subjproject. Accident? > If they are not shared with qemu-ga, I didn't move them yet. They can stay specific to qemu or specific subprojects, or we can decide to move them (if that doesn't drag too much stuff along). > > A bit of HMP support left behind. > Can you be more specific? > > * Parts of util/ and include/qemu/ > > Error reporting, key-value CLI, some C utilities, but not others > (e.g. qemu/atomic.h, but not qemu/atomic128.h). I guess it's again > whatever subjproject code needs. > > * Parts of the QAPI Schema subsystem > > Aside: MAINTAINERS mostly not updated. > > That needs fixing. > Your moves tear closely related code apart. This is going to be a drag > for developers in general and maintainers in particular. > > Ergonomics suffer when related code is in multiple places. Having to > switch between directories and remember where is what will a constant > low-level pain. Things that used to be simple & quick, like git-grep > qapi/*.c, become more involved. > > It's inevitable when a project grows. QEMU is already a very large proj
Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
Hi On Thu, Aug 11, 2022 at 10:52 AM Markus Armbruster wrote: > Marc-André Lureau writes: > > > Hi > > > > On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster > wrote: > > > >> marcandre.lur...@redhat.com writes: > >> > >> > From: Marc-André Lureau > >> > > >> > This is just moving qapi-gen.py and related subdir to qemu-common, to > >> > ease review and proceed step by step. The following patches will move > >> > related necessary code, tests etc. > >> > > >> > Signed-off-by: Marc-André Lureau > >> > >> As moved files tend to become low-level annoyances for a long time, I'd > >> like to understand why you want to move them. The commit message says > >> "to ease review", which I suspect isn't the real reason. Perhaps you > >> explained all that elsewhere already, but I missed it. > >> > >> > >> > > The end goal is to split some projects, such as qemu-ga, to standalone > > meson projects/subprojects. We will be able to build them independently > > from the rest of QEMU, and later on perhaps handle them outside of QEMU > > main repository. To achieve this, I first introduce a qemu-common > > subproject, where qapi and common units are provided. You can check > > https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak peek > at > > current end result. > > I worry this move of the QAPI generator code into > subjprojects/common/scripts/qapi/ will be followed by a move into its > own subproject. > Do you mean: it could be moved again to another smaller subproject? not really, see below > Ignorant question: could we turn the QAPI generator into a subproject in > place? > If it's just the generator, probably the target would then be a python project (not meson), similar to python-qemu-qmp. But I don't see much point, since it's not really a standalone python module, it generates code, and that code needs most of what is in qemu-common (see https://gitlab.com/marcandre.lureau/qemu/-/tree/qga/subprojects/qemu-common). It's best to have it together imho. Maybe we can consider a different naming or to be more careful not to add stuff that is not strictly needed by qapi? (fwiw, it's a bit of a shame python-qemu-qmp didn't import git history from qemu.. we did better with libslirp. If we ever move code in standalone repositories again, we should be careful to keep history with it) > > I said "to ease review and proceed step by step" simply because there are > > no other changes: I don't move the rest of the qapi code & tests all > > together, it's in the subsequent series. > > I'd recommend to provide a bit more context in the commit message, even > if you copy it to several messages in a row. Our future selves will > likely be grateful. > > -- Marc-André Lureau
Re: [PATCH v2 2/2] util/aio-win32: Correct the event array size in aio_poll()
On Tue, Aug 9, 2022 at 8:43 PM Bin Meng wrote:
> From: Bin Meng
>
> WaitForMultipleObjects() can only wait for MAXIMUM_WAIT_OBJECTS
> object handles. Correct the event array size in aio_poll() and
> add a assert() to ensure it does not cause out of bound access.
>
> Signed-off-by: Bin Meng
> Reviewed-by: Stefan Weil
>
lgtm
Reviewed-by: Marc-André Lureau
> ---
>
> Changes in v2:
> - change 'count' to unsigned
>
> util/aio-win32.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index 44003d645e..80cfe012ad 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -326,9 +326,9 @@ void aio_dispatch(AioContext *ctx)
> bool aio_poll(AioContext *ctx, bool blocking)
> {
> AioHandler *node;
> -HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
> +HANDLE events[MAXIMUM_WAIT_OBJECTS];
> bool progress, have_select_revents, first;
> -int count;
> +unsigned count;
> int timeout;
>
> /*
> @@ -369,6 +369,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
> QLIST_FOREACH_RCU(node, >aio_handlers, node) {
> if (!node->deleted && node->io_notify
> && aio_node_check(ctx, node->is_external)) {
> +assert(count < MAXIMUM_WAIT_OBJECTS);
> events[count++] = event_notifier_get_handle(node->e);
> }
> }
> --
> 2.34.1
>
>
--
Marc-André Lureau
Re: [PATCH v2 14/15] mtest2make.py: teach suite name that are just "PROJECT"
Hi
On Fri, Aug 5, 2022 at 2:39 PM Paolo Bonzini wrote:
> On 7/12/22 11:35, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > A subproject test may be simply in the "PROJECT" suite (such as
> > "qemu-common" with the following patches)
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > scripts/mtest2make.py | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
> > index 0fe81efbbcec..606821ee2732 100644
> > --- a/scripts/mtest2make.py
> > +++ b/scripts/mtest2make.py
> > @@ -51,8 +51,11 @@ def process_tests(test, targets, suites):
> >
> > test_suites = test['suite'] or ['default']
> > for s in test_suites:
> > -# The suite name in the introspection info is "PROJECT:SUITE"
> > -s = s.split(':')[1]
> > +# The suite name in the introspection info is "PROJECT" or
> "PROJECT:SUITE"
> > +try:
> > +s = s.split(':')[1]
> > +except IndexError:
> > +continue
>
> Shouldn't you continue with s begin simply "PROJECT"? That is, just
>
> if ':' in s:
> s = s.split(':')[1]
>
> This way you can do "make check-qemu-common".
>
> > if s == 'slow' or s == 'thorough':
> > continue
> > if s.endswith('-slow'):
>
> and then however these two "ifs" need to be under the case where the
> suite name is "PROJECT:SUITE".
>
>
Thanks for the tips, updated
--
Marc-André Lureau
Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
Hi On Fri, Aug 5, 2022 at 12:12 PM Markus Armbruster wrote: > marcandre.lur...@redhat.com writes: > > > From: Marc-André Lureau > > > > This is just moving qapi-gen.py and related subdir to qemu-common, to > > ease review and proceed step by step. The following patches will move > > related necessary code, tests etc. > > > > Signed-off-by: Marc-André Lureau > > As moved files tend to become low-level annoyances for a long time, I'd > like to understand why you want to move them. The commit message says > "to ease review", which I suspect isn't the real reason. Perhaps you > explained all that elsewhere already, but I missed it. > > > The end goal is to split some projects, such as qemu-ga, to standalone meson projects/subprojects. We will be able to build them independently from the rest of QEMU, and later on perhaps handle them outside of QEMU main repository. To achieve this, I first introduce a qemu-common subproject, where qapi and common units are provided. You can check https://gitlab.com/marcandre.lureau/qemu/-/commits/qga for a sneak peek at current end result. I said "to ease review and proceed step by step" simply because there are no other changes: I don't move the rest of the qapi code & tests all together, it's in the subsequent series. -- Marc-André Lureau
Re: [RFC v2 00/10] Introduce an extensible static analyzer
Hi On Fri, Jul 29, 2022 at 5:01 PM Alberto Faria wrote: > > This series introduces a static analyzer for QEMU. It consists of a > single static-analyzer.py script that relies on libclang's Python > bindings, and provides a common framework on which arbitrary static > analysis checks can be developed and run against QEMU's code base. > > Summary of the series: > > - Patch 1 adds the base static analyzer, along with a simple check > that finds static functions whose return value is never used, and > patch 2 fixes many occurrences of this. > > - Patch 3 introduces support for output-comparison check tests, and > adds some tests to the abovementioned check. > > - Patch 4 makes the analyzer skip checks on a translation unit when it > hasn't been modified since the last time those checks passed. > > - Patch 5 adds a check to ensure that non-coroutine_fn functions don't > perform direct calls to coroutine_fn functions, and patch 6 fixes > some violations of this rule. > > - Patch 7 adds a check to ensure that operations on coroutine_fn > pointers make sense, like assignment and indirect calls, and patch 8 > fixes some problems detected by the check. (Implementing this check > properly is complicated, since AFAICT annotation attributes cannot > be applied directly to types. This part still needs a lot of work.) > > - Patch 9 introduces a no_coroutine_fn marker for functions that > should not be called from coroutines, makes generated_co_wrapper > evaluate to no_coroutine_fn, and adds a check enforcing this rule. > Patch 10 fixes some violations that it finds. > > The current primary motivation for this work is enforcing rules around > block layer coroutines, which is why most of the series focuses on that. > However, the static analyzer is intended to be sufficiently generic to > satisfy other present and future QEMU static analysis needs. > > Performance isn't great, but with some more optimization, the analyzer > should be fast enough to be used iteratively during development, given > that it avoids reanalyzing unmodified translation units, and that users > can restrict the set of translation units under consideration. It should > also be fast enough to run in CI (?). > > Consider a small QEMU configuration and build (all commands were run on > the same 12-thread laptop): > > $ cd build && time ../configure --target-list=x86_64-softmmu && cd .. > [...] > > real0m17.232s > user0m13.261s > sys 0m3.895s > > $ time make -C build -j $(nproc) all > [...] > > real2m39.029s > user14m49.370s > sys 1m57.364s > > $ time make -C build -j $(nproc) check > [...] > > real2m46.349s > user6m4.718s > sys 4m15.660s > > We can run the static analyzer against all translation units enabled in > this configuration: > > $ time ./static-analyzer.py build > util/qemu-coroutine.c:122:23: non-coroutine_fn function calls > coroutine_fn qemu_coroutine_self() > io/channel.c:152:17: non-coroutine_fn function calls coroutine_fn > qio_channel_yield() > [...] > Analyzed 1649 translation units in 520.3 seconds. > > real8m42.342s > user95m51.759s > sys 0m21.576s > > You will need libclang's Python bindings to run this. Try `dnf install > python3-clang` or `apt install python3-clang`. > > It takes around 1 to 2 seconds for the analyzer to load the compilation > database, determine which translation units to analyze, etc. The > durations reported by the analyzer itself don't include those steps, > which is why they differ from what `time` reports. > > We can also analyze only some of the translation units: > > $ time ./static-analyzer.py build block > block/raw-format.c:420:12: non-coroutine_fn function calls coroutine_fn > bdrv_co_ioctl() > block/blkverify.c:266:12: non-coroutine_fn function calls coroutine_fn > bdrv_co_flush() > [...] > Analyzed 21 translation units (58 other were up-to-date) in 5.8 seconds. > > real0m7.031s > user0m40.951s > sys 0m1.299s > > Since the previous command had already analyzed all translation units, > only the ones that had problems were reanalyzed. > > Now skipping all the actual checks, but still parsing and building the > AST for each translation unit, and adding --force to reanalyze all > translation units: > > $ time ./static-analyzer.py build --force --skip-checks > Analyzed 1649 translation units in 41.2 seconds. > > real0m42.296s > user7m14.256s > sys 0m15.803s > > And now running a single check: > > $ time ./static-analyzer.py build --force --check return-value-never-used > Analyzed 1649 translation units in 157.6 seconds. > > real2m38.759s > user29m28.930s > sys 0m17.968s > > TODO: > - Run in GitLab CI (?). > - Finish the "coroutine_fn" check. > - Add check tests where missing. > - Avoid
Re: [PATCH 8/9] scripts/qapi-gen: add -i option
Hi
On Tue, Aug 2, 2022 at 5:28 PM Markus Armbruster wrote:
> Marc-André Lureau writes:
>
> > Hi
> >
> >
> > On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster
> wrote:
> >>
> >> marcandre.lur...@redhat.com writes:
> >>
> >> > From: Marc-André Lureau
> >> >
> >> > Replace hard-coded "qemu/osdep.h" include with a qapi-gen option to
> >> > specify the headers to include. This will allow to substitute QEMU
> >> > osdep.h with glib.h for example, for projects with different
> >> > global headers.
> >> >
> >> > For historical reasons, we can keep the default as "qemu/osdep.h".
> >> >
> >> > Signed-off-by: Marc-André Lureau
> >>
> >> I wish we'd all agree on "config.h" (conventional with autoconf). But
> >> we don't.
> >>
> >> Precedence for tweaking generated code with command line options: -p.
> >>
> >> I suspect that we'll always specify the same -p and -i for a given
> >> schema. To me, that suggests they should both go into the schema
> >> instead. Observation, not demand.
> >>
> >> > ---
> >> > scripts/qapi/commands.py | 15 ++-
> >> > scripts/qapi/events.py | 17 +++--
> >> > scripts/qapi/gen.py| 17 +
> >> > scripts/qapi/introspect.py | 11 +++
> >> > scripts/qapi/main.py | 17 +++--
> >> > scripts/qapi/types.py | 17 +++--
> >> > scripts/qapi/visit.py | 17 +++--
> >> > 7 files changed, 78 insertions(+), 33 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> >> > index 38ca38a7b9dd..781491b6390d 100644
> >> > --- a/scripts/qapi/commands.py
> >> > +++ b/scripts/qapi/commands.py
> >> > @@ -294,9 +294,9 @@ def gen_register_command(name: str,
> >> >
> >> >
> >> > class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> >> > -def __init__(self, prefix: str, gen_tracing: bool):
> >> > +def __init__(self, prefix: str, include: List[str], gen_tracing:
> bool):
> >>
> >> Ignorant question: why ist this List[str], not str? Do multiple options
> >> accumulate into a list?
> >>
> >> Alright, I'm back from further down: looks like they do.
> >>
> >> > super().__init__(
> >> > -prefix, 'qapi-commands',
> >> > +prefix, include, 'qapi-commands',
> >> > ' * Schema-defined QAPI/QMP commands', None, __doc__,
> >> > gen_tracing=gen_tracing)
> >> > self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]]
> = {}
> >> > @@ -308,7 +308,8 @@ def _begin_user_module(self, name: str) -> None:
> >> > types = self._module_basename('qapi-types', name)
> >> > visit = self._module_basename('qapi-visit', name)
> >> > self._genc.add(mcgen('''
> >> > -#include "qemu/osdep.h"
> >> > +%(include)s
> >> > +
> >> > #include "qapi/compat-policy.h"
> >> > #include "qapi/visitor.h"
> >> > #include "qapi/qmp/qdict.h"
> >> > @@ -318,6 +319,7 @@ def _begin_user_module(self, name: str) -> None:
> >> > #include "%(commands)s.h"
> >> >
> >> > ''',
> >> > + include=self.genc_include(),
> >> > commands=commands, visit=visit))
> >> >
> >> > if self._gen_tracing and commands != 'qapi-commands':
> >> > @@ -344,7 +346,8 @@ def visit_begin(self, schema: QAPISchema) -> None:
> >> > ''',
> >> > c_prefix=c_name(self._prefix,
> protect=False)))
> >> > self._genc.add(mcgen('''
> >> > -#include "qemu/osdep.h"
> >> > +%(include)s
> >> > +
> >> > #include "%(prefix)sqapi-commands.h"
> >> > #include "%(prefix)sqapi-init-commands.h"
> >> >
> >> > @@ -353,6 +356,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
> >> > QTAILQ_INIT(cmds);
> >> >
> >> > ''',
> >> > + include=self.genc_include(),
> >
Re: [PATCH v2 06/15] qapi: move QEMU-specific dispatch code in monitor
Hi
On Tue, Aug 2, 2022 at 3:04 PM Markus Armbruster wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > Make QMP-dispatch code free from QEMU-specific OOB dispatch/async
> > coroutine handling. This will allow to move the base code to
> > qemu-common, and clear other users from potential mis-ususe (QGA doesn't
>
misuse :)
> have OOB or coroutine).
>
> I trust the utilty of such a move will become clear later in this
> series.
>
> >
> > To do that, introduce an optional callback QmpDispatchRun called when a
> > QMP command should be run, to allow QEMU to override the default
> > behaviour.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > include/qapi/qmp/dispatch.h | 7 ++--
> > monitor/qmp.c | 68 -
> > qapi/qmp-dispatch.c | 64 +++---
> > qga/main.c | 2 +-
> > tests/unit/test-qmp-cmds.c | 6 ++--
> > 5 files changed, 81 insertions(+), 66 deletions(-)
> >
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index 1e4240fd0dbc..b659da613f2e 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -14,7 +14,6 @@
> > #ifndef QAPI_QMP_DISPATCH_H
> > #define QAPI_QMP_DISPATCH_H
> >
> > -#include "monitor/monitor.h"
> > #include "qemu/queue.h"
> >
> > typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
> > @@ -41,6 +40,10 @@ typedef struct QmpCommand
> >
> > typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
> >
> > +typedef void (QmpDispatchRun)(bool oob, const QmpCommand *cmd,
> > + QDict *args, QObject **ret, Error **errp,
> > + void *run_data);
> > +
> > void qmp_register_command(QmpCommandList *cmds, const char *name,
> >QmpCommandFunc *fn, QmpCommandOptions options,
> >unsigned special_features);
> > @@ -56,7 +59,7 @@ const char *qmp_command_name(const QmpCommand *cmd);
> > bool qmp_has_success_response(const QmpCommand *cmd);
> > QDict *qmp_error_response(Error *err);
> > QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> > -bool allow_oob, Monitor *cur_mon);
> > +bool allow_oob, QmpDispatchRun run_cb, void
> *run_data);
> > bool qmp_is_oob(const QDict *dict);
> >
> > typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void
> *opaque);
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index 092c527b6fc9..f8dec97c96bb 100644
> > --- a/monitor/qmp.c
> > +++ b/monitor/qmp.c
> > @@ -132,6 +132,72 @@ static void monitor_qmp_respond(MonitorQMP *mon,
> QDict *rsp)
> > }
> > }
> >
> > +typedef struct QmpDispatchBH {
> > +const QmpCommand *cmd;
> > +Monitor *cur_mon;
> > +QDict *args;
> > +QObject **ret;
> > +Error **errp;
> > +Coroutine *co;
> > +} QmpDispatchBH;
> > +
> > +static void do_qmp_dispatch_bh(void *opaque)
> > +{
> > +QmpDispatchBH *data = opaque;
> > +
> > +assert(monitor_cur() == NULL);
> > +monitor_set_cur(qemu_coroutine_self(), data->cur_mon);
> > +data->cmd->fn(data->args, data->ret, data->errp);
> > +monitor_set_cur(qemu_coroutine_self(), NULL);
> > +aio_co_wake(data->co);
> > +}
> > +
> > +/*
> > + * Runs outside of coroutine context for OOB commands, but in coroutine
> > + * context for everything else.
> > + */
> > +static void qmp_dispatch_run(bool oob, const QmpCommand *cmd,
> > + QDict *args, QObject **ret, Error **errp,
> > + void *run_data)
> > +{
> > +Monitor *cur_mon = run_data;
> > +
> > +assert(!(oob && qemu_in_coroutine()));
> > +assert(monitor_cur() == NULL);
> > +
> > +if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
> > +monitor_set_cur(qemu_coroutine_self(), cur_mon);
> > +cmd->fn(args, ret, errp);
> > +monitor_set_cur(qemu_coroutine_self(), NULL);
> > +} else {
> > + /*
> > +* Actual context doesn't match the one the command needs.
> > +*
> > +* Case 1: we are in coroutine context, but command does not
> > +* have QCO_COROUTINE. We need to drop out of co
Re: [PATCH v2 10/15] qemu-common: introduce a common subproject
Hi
On Tue, Jul 12, 2022 at 6:58 PM Warner Losh wrote:
>
>
>
> On Tue, Jul 12, 2022 at 3:36 AM wrote:
>>
>> From: Marc-André Lureau
>>
>> Add a new meson subproject to provide common code and scripts for QEMU
>> and tools. Initially, it will offer QAPI/QMP code generation and
>> common utilities.
>>
>> libvhost-user & libvduse will make use of the subproject to avoid having
>> include/ links to common headers.
>>
>> The other targeted user is qemu-ga, which will also be converted to a
>> subproject (so it can be built, moved, released etc independent from QEMU).
>>
>> Other projects such as qemu-storage-daemon could be built standalone
>> eventually in the future.
>>
>> Note that with meson subprojects are "global". Projects will share
>> subprojects
>> (https://mesonbuild.com/Subprojects.html#subprojects-depending-on-other-subprojects).
>> We will add extra subprojects/ links to allow standalone subproject
>> compilation though.
>>
>> This initial commit simply set the stage to build and link against it.
>>
>> Signed-off-by: Marc-André Lureau
>> ---
>> meson.build | 9 -
>> .../qemu-common/include}/qemu/help-texts.h | 0
>> linux-user/meson.build | 4 ++--
>> subprojects/libvduse/meson.build | 2 ++
>> subprojects/libvduse/subprojects/qemu-common | 1 +
>> subprojects/libvhost-user/meson.build| 2 ++
>> subprojects/libvhost-user/subprojects/qemu-common| 1 +
>> subprojects/qemu-common/meson.build | 8
>> 8 files changed, 24 insertions(+), 3 deletions(-)
>> rename {include => subprojects/qemu-common/include}/qemu/help-texts.h (100%)
>> create mode 12 subprojects/libvduse/subprojects/qemu-common
>> create mode 12 subprojects/libvhost-user/subprojects/qemu-common
>> create mode 100644 subprojects/qemu-common/meson.build
>>
>> diff --git a/meson.build b/meson.build
>> index bc5569ace159..254eb1263a66 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -167,6 +167,10 @@ if 'dtrace' in get_option('trace_backends')
>>endif
>> endif
>>
>> +add_project_arguments('-I' + meson.current_source_dir() /
>> 'subprojects/qemu-common/include',
>> + language: ['c', 'cpp', 'objc'],
>> +)
>> +
>> if get_option('iasl') == ''
>>iasl = find_program('iasl', required: false)
>> else
>> @@ -1577,6 +1581,9 @@ if libbpf.found() and not cc.links('''
>>endif
>> endif
>>
>> +qemu_common = subproject('qemu-common')
>> +qemu_common = qemu_common.get_variable('qemu_common_dep')
>> +
>> #
>> # config-host.h #
>> #
>> @@ -3052,7 +3059,7 @@ util_ss.add_all(trace_ss)
>> util_ss = util_ss.apply(config_all, strict: false)
>> libqemuutil = static_library('qemuutil',
>> sources: util_ss.sources() + stub_ss.sources()
>> + genh,
>> - dependencies: [util_ss.dependencies(), libm,
>> threads, glib, socket, malloc, pixman])
>> + dependencies: [util_ss.dependencies(), libm,
>> threads, glib, socket, malloc, pixman, qemu_common])
>> qemuutil = declare_dependency(link_with: libqemuutil,
>>sources: genh + version_res,
>>dependencies: [event_loop_base])
>> diff --git a/include/qemu/help-texts.h
>> b/subprojects/qemu-common/include/qemu/help-texts.h
>> similarity index 100%
>> rename from include/qemu/help-texts.h
>> rename to subprojects/qemu-common/include/qemu/help-texts.h
>> diff --git a/linux-user/meson.build b/linux-user/meson.build
>> index de4320af053c..fc6cdb55d657 100644
>> --- a/linux-user/meson.build
>> +++ b/linux-user/meson.build
>> @@ -7,7 +7,7 @@ linux_user_ss = ss.source_set()
>> common_user_inc += include_directories('include/host/' / host_arch)
>> common_user_inc += include_directories('include')
>>
>> -linux_user_ss.add(files(
>> +linux_user_ss.add([files(
>>'elfload.c',
>>'exit.c',
>>'fd-trans.c',
>> @@ -20,7 +20,7 @@ linux_user_ss.add(files(
>>'thunk.c',
>>'uaccess.c',
>>'uname.c',
>> -))
>> +), qemu_common])
>
>
> Question: Why does linux-user need these, but bsd-user does not?
>
Indeed, it's not needed anymore, thanks!
Re: [PATCH 6/9] error-report: add a callback to overwrite error_vprintf
Hi On Thu, Jul 7, 2022 at 10:05 PM Marc-André Lureau < marcandre.lur...@gmail.com> wrote: > Hi > > On Thu, Jul 7, 2022 at 4:18 PM Markus Armbruster > wrote: > >> marcandre.lur...@redhat.com writes: >> >> > From: Marc-André Lureau >> > >> > error_vprintf() is implemented in monitor.c, which overrides the >> > default implementation from stubs/, while avoiding a direct dependency >> > to the monitor from error-report.c. >> > >> > However, the stub solution isn't working when moving error-report.c and >> > stubs/error-printf.c in a common library. Linking with such library >> > creates conflicts for the error_vprintf() implementations >> >> I'm feeling dense today: how? >> > > If I try to overwrite a symbol in qemu when linking with a static library > from a subproject, the linker fails: > > FAILED: storage-daemon/qemu-storage-daemon > cc -m64 -mcx16 -o storage-daemon/qemu-storage-daemon > storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-introspect.c.o > storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-types.c.o > storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-visit.c.o > storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-commands.c.o > storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-init-commands.c.o > storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-events.c.o > storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-emit-events.c.o > storage-daemon/qemu-storage-daemon.p/qemu-storage-daemon.c.o > -Wl,--as-needed -Wl,--no-undefined -pie -Wl,--whole-archive libblockdev.fa > libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa -Wl,--start-group > libevent-loop-base.a libchardev.fa libqmp.fa -Wl,--no-whole-archive > -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -fstack-protector-strong > -Wl,-rpath,/usr/lib64/iscsi -Wl,-rpath-link,/usr/lib64/iscsi libqemuutil.a > subprojects/libvhost-user/libvhost-user-glib.a > subprojects/libvhost-user/libvhost-user.a > subprojects/qemu-common/libqemu-common.a libblockdev.fa > subprojects/libvduse/libvduse.a libblock.fa libcrypto.fa libauthz.fa > libqom.fa libio.fa libchardev.fa libqmp.fa @block.syms > /usr/lib64/libgnutls.so /usr/lib64/liburing.so /usr/lib64/libgio-2.0.so > /usr/lib64/libgobject-2.0.so /usr/lib64/libglib-2.0.so > -Wl,--export-dynamic -pthread -lgmodule-2.0 -lglib-2.0 -lglib-2.0 -lm > /usr/lib64/libpixman-1.so -lgmodule-2.0 -lglib-2.0 -lglib-2.0 -lgmodule-2.0 > -lglib-2.0 -lglib-2.0 -lgmodule-2.0 -lglib-2.0 -lglib-2.0 > /usr/lib64/libfuse3.so -lpthread -lgmodule-2.0 -lglib-2.0 -lglib-2.0 > /usr/lib64/libzstd.so /usr/lib64/libz.so /usr/lib64/iscsi/libiscsi.so -laio > -lpam -lutil -Wl,--end-group > /usr/bin/ld: > subprojects/qemu-common/libqemu-common.a.p/src_error-report.c.o: in > function `error_init': > /home/elmarco/src/qemu/build/../subprojects/qemu-common/src/error-report.c:413: > multiple definition of `error_init'; > libqmp.fa.p/monitor_qmp.c.o:/home/elmarco/src/qemu/build/../monitor/qmp.c:40: > first defined here > > But I can't really explain why it works when overwriting symbols from > libqemuutil.a, I am a bit puzzled here. Am I missing something obvious? > (this is a bit of dark linker magic going on) > > After playing with this for a few hours ... I managed to get the symbol override work, by having a single overridable function per unit. No idea why in qemutil.a/stubs we manage to have several. That's a mystery. Anyway, I will send a new version where I also introduce the subproject, to demonstrate it works. thanks > >> Why would the linker pull in error-printf.o when the symbols it defines >> are already provided by .o the linker processed before the library >> containing error-printf.o? >> >> > (and weak >> > symbols aren't great either). >> >> Weak symbols are great, except Windows isn't. >> >> > Instead, use the "traditional" approach to >> > provide overidable callbacks. >> > >> > Signed-off-by: Marc-André Lureau >> >> >> > > -- > Marc-André Lureau > -- Marc-André Lureau
Re: [PATCH v4 01/12] qga: treat get-guest-fsinfo as "best effort"
On Fri, Jul 8, 2022 at 8:16 PM John Snow wrote:
> In some container environments, there may be references to block devices
> witnessable from a container through /proc/self/mountinfo that reference
> devices we simply don't have access to in the container, and cannot
> provide information about.
>
> Instead of failing the entire fsinfo command, return stub information
> for these failed lookups.
>
> This allows test-qga to pass under docker tests, which are in turn used
> by the CentOS VM tests.
>
> Signed-off-by: John Snow
>
Reviewed-by: Marc-André Lureau
> ---
> qga/commands-posix.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0469dc409d4..355de050a1c 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1207,7 +1207,15 @@ static void build_guest_fsinfo_for_device(char
> const *devpath,
>
> syspath = realpath(devpath, NULL);
> if (!syspath) {
> -error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> +if (errno != ENOENT) {
> +error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> +return;
> +}
> +
> +/* ENOENT: This devpath may not exist because of container config
> */
> +if (!fs->name) {
> +fs->name = g_path_get_basename(devpath);
> +}
> return;
> }
>
> --
> 2.34.3
>
>
>
--
Marc-André Lureau
Re: [PATCH 1/9] monitor: make error_vprintf_unless_qmp() static
Hi
On Fri, Jul 8, 2022 at 5:56 PM Markus Armbruster wrote:
> Marc-André Lureau writes:
>
> > Hi
> >
> > On Thu, Jul 7, 2022 at 4:25 PM Markus Armbruster
> wrote:
> >
> >> marcandre.lur...@redhat.com writes:
> >>
> >> > From: Marc-André Lureau
> >> >
> >> > Not needed outside monitor.c. Remove the needless stub.
> >> >
> >> > Signed-off-by: Marc-André Lureau
> >> > ---
> >> > include/monitor/monitor.h | 1 -
> >> > monitor/monitor.c | 3 ++-
> >> > stubs/error-printf.c | 5 -
> >> > 3 files changed, 2 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> >> > index a4b40e8391db..44653e195b45 100644
> >> > --- a/include/monitor/monitor.h
> >> > +++ b/include/monitor/monitor.h
> >> > @@ -56,7 +56,6 @@ void monitor_register_hmp(const char *name, bool
> info,
> >> > void monitor_register_hmp_info_hrt(const char *name,
> >> > HumanReadableText
> *(*handler)(Error **errp));
> >> >
> >> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> G_GNUC_PRINTF(1, 0);
> >> > int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1,
> 2);
> >> >
> >> > #endif /* MONITOR_H */
> >> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> >> > index 86949024f643..ba4c1716a48a 100644
> >> > --- a/monitor/monitor.c
> >> > +++ b/monitor/monitor.c
> >> > @@ -273,7 +273,8 @@ int error_vprintf(const char *fmt, va_list ap)
> >> > return vfprintf(stderr, fmt, ap);
> >> > }
> >> >
> >> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> >> > +G_GNUC_PRINTF(1, 0)
> >> > +static int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> >> > {
> >> > Monitor *cur_mon = monitor_cur();
> >> >
> >> > diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> >> > index 0e326d801059..1afa0f62ca26 100644
> >> > --- a/stubs/error-printf.c
> >> > +++ b/stubs/error-printf.c
> >> > @@ -16,8 +16,3 @@ int error_vprintf(const char *fmt, va_list ap)
> >> > }
> >> > return vfprintf(stderr, fmt, ap);
> >> > }
> >> > -
> >> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> >> > -{
> >> > -return error_vprintf(fmt, ap);
> >> > -}
> >>
> >> When I write a printf-like utility function, I habitually throw in a
> >> vprintf-like function.
> >>
> >> Any particular reason for hiding this one? To avoid misunderstandings:
> >> I'm fine with hiding it if it's causing you trouble.
> >
> > I don't think I had an issue with it, only that I wrote tests for the
> > error-report.h API, and didn't see the need to cover a function that
> isn't
> > used outside the unit.
>
> I'd keep it and not worry about missing tests; the tests of
> error_printf_unless_qmp() exercise it fine.
>
ok
>
> >> Except I think we'd better delete than hide then: inline into
> >> error_printf_unless_qmp(). Makes sense?
> >
> > It can't be easily inlined because of the surrounding va_start/va_end
>
> Easily enough, I think:
>
ah yes indeed! :)
>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 86949024f6..201a672ac6 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -273,27 +273,22 @@ int error_vprintf(const char *fmt, va_list ap)
> return vfprintf(stderr, fmt, ap);
> }
>
> -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> -{
> -Monitor *cur_mon = monitor_cur();
> -
> -if (!cur_mon) {
> -return vfprintf(stderr, fmt, ap);
> -}
> -if (!monitor_cur_is_qmp()) {
> -return monitor_vprintf(cur_mon, fmt, ap);
> -}
> -return -1;
> -}
> -
> int error_printf_unless_qmp(const char *fmt, ...)
> {
> +Monitor *cur_mon = monitor_cur();
> va_list ap;
> int ret;
>
> va_start(ap, fmt);
> -ret = error_vprintf_unless_qmp(fmt, ap);
> +if (!cur_mon) {
> +ret = vfprintf(stderr, fmt, ap);
> +} else if (!monitor_cur_is_qmp()) {
> +ret = monitor_vprintf(cur_mon, fmt, ap);
> +} else {
> +ret = -1;
> +}
> va_end(ap);
> +
> return ret;
> }
>
>
>
--
Marc-André Lureau
Re: [PATCH 5/9] error-report: introduce ErrorReportDetailedFunc
Hi
On Thu, Jul 7, 2022 at 4:13 PM Markus Armbruster wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > Remove monitor dependency from error printing code, by allowing programs
> > to set a callback for when to use "detailed" reporting or not.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > include/qemu/error-report.h | 4 +++-
> > bsd-user/main.c | 2 +-
> > linux-user/main.c| 2 +-
> > qemu-img.c | 2 +-
> > qemu-io.c| 2 +-
> > qemu-nbd.c | 2 +-
> > scsi/qemu-pr-helper.c| 2 +-
> > softmmu/vl.c | 7 ++-
> > storage-daemon/qemu-storage-daemon.c | 7 ++-
> > util/error-report.c | 8 +---
> > 10 files changed, 26 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> > index 3ae2357fda54..e2e630f207f0 100644
> > --- a/include/qemu/error-report.h
> > +++ b/include/qemu/error-report.h
> > @@ -13,6 +13,8 @@
> > #ifndef QEMU_ERROR_REPORT_H
> > #define QEMU_ERROR_REPORT_H
> >
> > +typedef bool (*ErrorReportDetailedFunc)(void);
> > +
> > typedef struct Location {
> > /* all members are private to qemu-error.c */
> > enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind;
> > @@ -46,7 +48,7 @@ bool error_report_once_cond(bool *printed, const char
> *fmt, ...)
> > bool warn_report_once_cond(bool *printed, const char *fmt, ...)
> > G_GNUC_PRINTF(2, 3);
> >
> > -void error_init(const char *argv0);
> > +void error_init(const char *argv0, ErrorReportDetailedFunc detailed_fn);
> >
> > /*
> > * Similar to error_report(), except it prints the message just once.
> > diff --git a/bsd-user/main.c b/bsd-user/main.c
> > index 6f09180d6541..d5f8fca863d7 100644
> > --- a/bsd-user/main.c
> > +++ b/bsd-user/main.c
> > @@ -292,7 +292,7 @@ int main(int argc, char **argv)
> >
> > save_proc_pathname(argv[0]);
> >
> > -error_init(argv[0]);
> > +error_init(argv[0], NULL);
> > module_call_init(MODULE_INIT_TRACE);
> > qemu_init_cpu_list();
> > module_call_init(MODULE_INIT_QOM);
> [More such calls of error_init()...]
> > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > index 54e920ada1a1..3b46fc9c1fc5 100644
> > --- a/softmmu/vl.c
> > +++ b/softmmu/vl.c
> > @@ -2590,6 +2590,11 @@ void qmp_x_exit_preconfig(Error **errp)
> > }
> > }
> >
> > +static bool error_is_detailed(void)
> > +{
> > +return !monitor_cur();
> > +}
> > +
> > void qemu_init(int argc, char **argv, char **envp)
> > {
> > QemuOpts *opts;
> > @@ -2634,7 +2639,7 @@ void qemu_init(int argc, char **argv, char **envp)
> > qemu_add_opts(_action_opts);
> > module_call_init(MODULE_INIT_OPTS);
> >
> > -error_init(argv[0]);
> > +error_init(argv[0], error_is_detailed);
> > qemu_init_exec_dir(argv[0]);
> >
> > qemu_init_arch_modules();
> > diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> > index c104817cdddc..7e4d5030a045 100644
> > --- a/storage-daemon/qemu-storage-daemon.c
> > +++ b/storage-daemon/qemu-storage-daemon.c
> > @@ -368,13 +368,18 @@ static void pid_file_init(void)
> > atexit(pid_file_cleanup);
> > }
> >
> > +static bool error_is_detailed(void)
> > +{
> > +return !monitor_cur();
> > +}
> > +
> > int main(int argc, char *argv[])
> > {
> > #ifdef CONFIG_POSIX
> > signal(SIGPIPE, SIG_IGN);
> > #endif
> >
> > -error_init(argv[0]);
> > +error_init(argv[0], error_is_detailed);
> > qemu_init_exec_dir(argv[0]);
> > os_setup_signal_handling();
> >
> > diff --git a/util/error-report.c b/util/error-report.c
> > index c43227a975e2..c2181f80a83d 100644
> > --- a/util/error-report.c
> > +++ b/util/error-report.c
> > @@ -11,7 +11,6 @@
> > */
> >
> > #include "qemu/osdep.h"
> > -#include "monitor/monitor.h"
> > #include "qemu/error-report.h"
> >
> > /*
> > @@ -28,6 +27,7 @@ typedef enum {
> > bool message_with_timestamp;
> > bool error_with_guestname;
> > const char *error_guest_name;
> > +ErrorReportDetailedFunc detailed_fn = NULL;
> >
> > int error_
Re: [PATCH 6/9] error-report: add a callback to overwrite error_vprintf
Hi On Thu, Jul 7, 2022 at 4:18 PM Markus Armbruster wrote: > marcandre.lur...@redhat.com writes: > > > From: Marc-André Lureau > > > > error_vprintf() is implemented in monitor.c, which overrides the > > default implementation from stubs/, while avoiding a direct dependency > > to the monitor from error-report.c. > > > > However, the stub solution isn't working when moving error-report.c and > > stubs/error-printf.c in a common library. Linking with such library > > creates conflicts for the error_vprintf() implementations > > I'm feeling dense today: how? > If I try to overwrite a symbol in qemu when linking with a static library from a subproject, the linker fails: FAILED: storage-daemon/qemu-storage-daemon cc -m64 -mcx16 -o storage-daemon/qemu-storage-daemon storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-introspect.c.o storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-types.c.o storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-visit.c.o storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-commands.c.o storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-init-commands.c.o storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-events.c.o storage-daemon/qemu-storage-daemon.p/meson-generated_.._qapi_qapi-emit-events.c.o storage-daemon/qemu-storage-daemon.p/qemu-storage-daemon.c.o -Wl,--as-needed -Wl,--no-undefined -pie -Wl,--whole-archive libblockdev.fa libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa -Wl,--start-group libevent-loop-base.a libchardev.fa libqmp.fa -Wl,--no-whole-archive -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -fstack-protector-strong -Wl,-rpath,/usr/lib64/iscsi -Wl,-rpath-link,/usr/lib64/iscsi libqemuutil.a subprojects/libvhost-user/libvhost-user-glib.a subprojects/libvhost-user/libvhost-user.a subprojects/qemu-common/libqemu-common.a libblockdev.fa subprojects/libvduse/libvduse.a libblock.fa libcrypto.fa libauthz.fa libqom.fa libio.fa libchardev.fa libqmp.fa @block.syms /usr/lib64/libgnutls.so /usr/lib64/liburing.so /usr/lib64/libgio-2.0.so /usr/lib64/libgobject-2.0.so /usr/lib64/libglib-2.0.so -Wl,--export-dynamic -pthread -lgmodule-2.0 -lglib-2.0 -lglib-2.0 -lm /usr/lib64/libpixman-1.so -lgmodule-2.0 -lglib-2.0 -lglib-2.0 -lgmodule-2.0 -lglib-2.0 -lglib-2.0 -lgmodule-2.0 -lglib-2.0 -lglib-2.0 /usr/lib64/libfuse3.so -lpthread -lgmodule-2.0 -lglib-2.0 -lglib-2.0 /usr/lib64/libzstd.so /usr/lib64/libz.so /usr/lib64/iscsi/libiscsi.so -laio -lpam -lutil -Wl,--end-group /usr/bin/ld: subprojects/qemu-common/libqemu-common.a.p/src_error-report.c.o: in function `error_init': /home/elmarco/src/qemu/build/../subprojects/qemu-common/src/error-report.c:413: multiple definition of `error_init'; libqmp.fa.p/monitor_qmp.c.o:/home/elmarco/src/qemu/build/../monitor/qmp.c:40: first defined here But I can't really explain why it works when overwriting symbols from libqemuutil.a, I am a bit puzzled here. Am I missing something obvious? (this is a bit of dark linker magic going on) > Why would the linker pull in error-printf.o when the symbols it defines > are already provided by .o the linker processed before the library > containing error-printf.o? > > > (and weak > > symbols aren't great either). > > Weak symbols are great, except Windows isn't. > > > Instead, use the "traditional" approach to > > provide overidable callbacks. > > > > Signed-off-by: Marc-André Lureau > > > -- Marc-André Lureau
Re: [PATCH 1/9] monitor: make error_vprintf_unless_qmp() static
Hi
On Thu, Jul 7, 2022 at 4:25 PM Markus Armbruster wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > Not needed outside monitor.c. Remove the needless stub.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > include/monitor/monitor.h | 1 -
> > monitor/monitor.c | 3 ++-
> > stubs/error-printf.c | 5 -
> > 3 files changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index a4b40e8391db..44653e195b45 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -56,7 +56,6 @@ void monitor_register_hmp(const char *name, bool info,
> > void monitor_register_hmp_info_hrt(const char *name,
> > HumanReadableText *(*handler)(Error
> **errp));
> >
> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> G_GNUC_PRINTF(1, 0);
> > int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
> >
> > #endif /* MONITOR_H */
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 86949024f643..ba4c1716a48a 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -273,7 +273,8 @@ int error_vprintf(const char *fmt, va_list ap)
> > return vfprintf(stderr, fmt, ap);
> > }
> >
> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> > +G_GNUC_PRINTF(1, 0)
> > +static int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> > {
> > Monitor *cur_mon = monitor_cur();
> >
> > diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> > index 0e326d801059..1afa0f62ca26 100644
> > --- a/stubs/error-printf.c
> > +++ b/stubs/error-printf.c
> > @@ -16,8 +16,3 @@ int error_vprintf(const char *fmt, va_list ap)
> > }
> > return vfprintf(stderr, fmt, ap);
> > }
> > -
> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> > -{
> > -return error_vprintf(fmt, ap);
> > -}
>
> When I write a printf-like utility function, I habitually throw in a
> vprintf-like function.
>
> Any particular reason for hiding this one? To avoid misunderstandings:
> I'm fine with hiding it if it's causing you trouble.
>
I don't think I had an issue with it, only that I wrote tests for the
error-report.h API, and didn't see the need to cover a function that isn't
used outside the unit.
>
> Except I think we'd better delete than hide then: inline into
> error_printf_unless_qmp(). Makes sense?
>
It can't be easily inlined because of the surrounding va_start/va_end
--
Marc-André Lureau
Re: [PATCH v3 01/13] qga: treat get-guest-fsinfo as "best effort"
Hi
On Thu, Jul 7, 2022 at 8:10 AM John Snow wrote:
> In some container environments, there may be references to block devices
> witnessable from a container through /proc/self/mountinfo that reference
> devices we simply don't have access to in the container, and cannot
> provide information about.
>
> Instead of failing the entire fsinfo command, return stub information
> for these failed lookups.
>
> This allows test-qga to pass under docker tests, which are in turn used
> by the CentOS VM tests.
>
> Signed-off-by: John Snow
> ---
> qga/commands-posix.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0469dc409d4..950c9d72fe7 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1207,7 +1207,12 @@ static void build_guest_fsinfo_for_device(char
> const *devpath,
>
> syspath = realpath(devpath, NULL);
> if (!syspath) {
> -error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> +if (errno == ENOENT) {
> +/* This devpath may not exist because of container config,
> etc. */
> +fs->name = g_path_get_basename(devpath);
> +} else {
> +error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> +}
>
It looks like this function is called recursively with the same "fs"
argument. That's probably why there is a if (!fs->name) check next. You may
want to check it too to avoid leaks and incorrect info.
> return;
> }
>
> --
> 2.34.3
>
>
>
--
Marc-André Lureau
Re: [PATCH 9/9] scripts/qapi: add required system includes to visitor
Hi
On Thu, Jun 23, 2022 at 5:43 PM Markus Armbruster wrote:
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > The generated visitor code includes abort() & assert(), we shouldn't
> > rely on the global "-i" headers to include the necessary system headers.
>
> Suggest ", even though the default qemu/osdep.h always does.
>
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > scripts/qapi/visit.py | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 1ff464c0360f..d686df17f4b6 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -326,6 +326,8 @@ def __init__(self, prefix: str, include: List[str]):
> > def _begin_builtin_module(self) -> None:
> > self._genc.preamble_add(mcgen('''
> > %(include)s
> > +#include
> > +#include
> >
> > #include "qapi/error.h"
> > #include "qapi/qapi-builtin-visit.h"
> > @@ -342,6 +344,8 @@ def _begin_user_module(self, name: str) -> None:
> > visit = self._module_basename('qapi-visit', name)
> > self._genc.preamble_add(mcgen('''
> > %(include)s
> > +#include
> > +#include
> >
> > #include "qapi/error.h"
> > #include "qapi/qmp/qerror.h"
>
> Mildly irritating, because we normally kill such includes as redundant
> on sight.
>
> The builtin module (qapi-builtin-visit.c) doesn't actually need these
> headers. I guess you include them just in case that changes.
True, at least not directly. I will drop it for now, we can add it
back when/if I figure out it is necessary.
Re: [PATCH 8/9] scripts/qapi-gen: add -i option
Hi
On Tue, Jun 21, 2022 at 6:14 PM Markus Armbruster wrote:
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > Replace hard-coded "qemu/osdep.h" include with a qapi-gen option to
> > specify the headers to include. This will allow to substitute QEMU
> > osdep.h with glib.h for example, for projects with different
> > global headers.
> >
> > For historical reasons, we can keep the default as "qemu/osdep.h".
> >
> > Signed-off-by: Marc-André Lureau
>
> I wish we'd all agree on "config.h" (conventional with autoconf). But
> we don't.
>
> Precedence for tweaking generated code with command line options: -p.
>
> I suspect that we'll always specify the same -p and -i for a given
> schema. To me, that suggests they should both go into the schema
> instead. Observation, not demand.
>
> > ---
> > scripts/qapi/commands.py | 15 ++-
> > scripts/qapi/events.py | 17 +++--
> > scripts/qapi/gen.py| 17 +
> > scripts/qapi/introspect.py | 11 +++
> > scripts/qapi/main.py | 17 +++--
> > scripts/qapi/types.py | 17 +++--
> > scripts/qapi/visit.py | 17 +++--
> > 7 files changed, 78 insertions(+), 33 deletions(-)
> >
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index 38ca38a7b9dd..781491b6390d 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -294,9 +294,9 @@ def gen_register_command(name: str,
> >
> >
> > class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
> > -def __init__(self, prefix: str, gen_tracing: bool):
> > +def __init__(self, prefix: str, include: List[str], gen_tracing: bool):
>
> Ignorant question: why ist this List[str], not str? Do multiple options
> accumulate into a list?
>
> Alright, I'm back from further down: looks like they do.
>
> > super().__init__(
> > -prefix, 'qapi-commands',
> > +prefix, include, 'qapi-commands',
> > ' * Schema-defined QAPI/QMP commands', None, __doc__,
> > gen_tracing=gen_tracing)
> > self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
> > @@ -308,7 +308,8 @@ def _begin_user_module(self, name: str) -> None:
> > types = self._module_basename('qapi-types', name)
> > visit = self._module_basename('qapi-visit', name)
> > self._genc.add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "qapi/compat-policy.h"
> > #include "qapi/visitor.h"
> > #include "qapi/qmp/qdict.h"
> > @@ -318,6 +319,7 @@ def _begin_user_module(self, name: str) -> None:
> > #include "%(commands)s.h"
> >
> > ''',
> > + include=self.genc_include(),
> > commands=commands, visit=visit))
> >
> > if self._gen_tracing and commands != 'qapi-commands':
> > @@ -344,7 +346,8 @@ def visit_begin(self, schema: QAPISchema) -> None:
> > ''',
> > c_prefix=c_name(self._prefix, protect=False)))
> > self._genc.add(mcgen('''
> > -#include "qemu/osdep.h"
> > +%(include)s
> > +
> > #include "%(prefix)sqapi-commands.h"
> > #include "%(prefix)sqapi-init-commands.h"
> >
> > @@ -353,6 +356,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
> > QTAILQ_INIT(cmds);
> >
> > ''',
> > + include=self.genc_include(),
> > prefix=self._prefix,
> > c_prefix=c_name(self._prefix, protect=False)))
> >
> > @@ -404,7 +408,8 @@ def visit_command(self,
> > def gen_commands(schema: QAPISchema,
> > output_dir: str,
> > prefix: str,
> > + include: List[str],
> > gen_tracing: bool) -> None:
> > -vis = QAPISchemaGenCommandVisitor(prefix, gen_tracing)
> > +vis = QAPISchemaGenCommandVisitor(prefix, include, gen_tracing)
> > schema.visit(vis)
> > vis.write(output_dir)
> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > index 27b44c49f5e9..6e677d11d2e0 100644
> > --- a/scripts/qapi/events.py
> > +++ b/scripts/qapi/events.py
> > @@ -175,9 +175,9 @@ def gen_event_send(name: str,
> >
&
Re: [PATCH v2 03/10] qga: treat get-guest-fsinfo as "best effort"
Hi
On Thu, Jun 16, 2022 at 6:27 PM John Snow wrote:
> In some container environments, there may be references to block devices
> witnessable from a container through /proc/self/mountinfo that reference
> devices we simply don't have access to in the container, and could not
> provide information about.
>
> Instead of failing the entire fsinfo command, return stub information
> for these failed lookups.
>
> This allows test-qga to pass under docker tests, which are in turn used
> by the CentOS VM tests.
>
> Signed-off-by: John Snow
> ---
> qga/commands-posix.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0469dc409d4..5989d4dca9d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1207,7 +1207,13 @@ static void build_guest_fsinfo_for_device(char
> const *devpath,
>
> syspath = realpath(devpath, NULL);
> if (!syspath) {
> -error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> +if (errno == ENOENT) {
> +/* This devpath may not exist because of container config,
> etc. */
> +fprintf(stderr, "realpath(%s) returned NULL/ENOENT\n",
> devpath);
>
qga uses g_critical() (except for some win32 code paths atm)
> +fs->name = g_strdup("??\?-ENOENT");
>
Hmm, maybe we should make the field optional instead.
> +} else {
> + error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> +}
> return;
> }
>
> --
> 2.34.3
>
>
>
--
Marc-André Lureau
Re: [PATCH 0/9] Preliminary patches for subproject split
Hi On Thu, Jun 16, 2022 at 5:12 PM Paolo Bonzini wrote: > > On 6/16/22 14:40, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Hi, > > > > Here is another subset of the large "subproject(qga)"" series I intend to > > send > > soon after (https://gitlab.com/marcandre.lureau/qemu/-/commits/qga). > > Hi, > > I took a peek there and I have a question. While the configure script > has had enough of a diet that it should be possible to compile the > subprojects as standalone with relatively little effort, how do you plan > to handle things such as compiler flags that are detected by meson.build > (especially enabling/disabling warnings)? > If you run top-level qemu configure, then I think flags should be shared with add_global_arguments() (to be verified). qemu-ga (and qemu-common) in the branch do compile standalone. There are still small quirks to fix the CI though with the last patches. Also the last patch could probably deserve a little bit more splitting (for ex, switching from QEMU_FULL_VERSION to VCS_TAG etc), that branch is still WIP (after 2 months!). > Paolo > > > Thanks > > > > Marc-André Lureau (9): > >monitor: make error_vprintf_unless_qmp() static > >error-report: misc comment fix > >error-report: introduce "detailed" variable > >error-report: simplify print_loc() > >error-report: introduce ErrorReportDetailedFunc > >error-report: add a callback to overwrite error_vprintf > >qapi: move QEMU-specific dispatch code in monitor > >scripts/qapi-gen: add -i option > >scripts/qapi: add required system includes to visitor > > > > include/monitor/monitor.h| 2 +- > > include/qapi/qmp/dispatch.h | 7 ++- > > include/qemu/error-report.h | 8 +++- > > bsd-user/main.c | 2 +- > > linux-user/main.c| 2 +- > > monitor/monitor.c| 5 +- > > monitor/qmp.c| 68 +++- > > qapi/qmp-dispatch.c | 64 ++ > > qemu-img.c | 2 +- > > qemu-io.c| 2 +- > > qemu-nbd.c | 2 +- > > qga/main.c | 2 +- > > scsi/qemu-pr-helper.c| 2 +- > > softmmu/vl.c | 7 ++- > > storage-daemon/qemu-storage-daemon.c | 7 ++- > > stubs/error-printf.c | 23 -- > > tests/unit/test-qmp-cmds.c | 6 +-- > > util/error-report.c | 46 ++- > > scripts/qapi/commands.py | 15 -- > > scripts/qapi/events.py | 17 --- > > scripts/qapi/gen.py | 17 +++ > > scripts/qapi/introspect.py | 11 +++-- > > scripts/qapi/main.py | 17 --- > > scripts/qapi/types.py| 17 --- > > scripts/qapi/visit.py| 21 ++--- > > stubs/meson.build| 1 - > > 26 files changed, 226 insertions(+), 147 deletions(-) > > delete mode 100644 stubs/error-printf.c > > >
Re: [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create()
Hi
On Thu, May 5, 2022 at 1:33 PM Markus Armbruster wrote:
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > The function takes care of setting CLOEXEC, and reporting error.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > qga/commands-posix.c | 11 +++
> > 1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 0ef049650e31..8ebc327c5e02 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -370,21 +370,16 @@ safe_open_or_create(const char *path, const char
> > *mode, Error **errp)
> > * open() is decisive and its third argument is ignored, and the second
> > * open() and the fchmod() are never called.
> > */
> > -fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
> > +fd = qemu_open_cloexec(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0),
> > 0, errp);
> > if (fd == -1 && errno == EEXIST) {
> > +g_clear_pointer(errp, error_free);
>
> Aha, this is where you really need ERRP_GUARD().
>
> Elsewhere, we use
>
>error_free(errp);
>errp = NULL;
>
More like:
error_free(*errp);
*errp = NULL;
compared to:
g_clear_pointer(errp, error_free);
I have a preference ;) but I will switch to the former for now.
> If one of these two ways is superior, we should use the superior one
> everywhere.
>
> If it's a wash, we should stick to the prevalent way.
>
> > oflag &= ~(unsigned)O_CREAT;
> > -fd = open(path, oflag);
> > +fd = qemu_open_cloexec(path, oflag, 0, errp);
> > }
> > if (fd == -1) {
> > -error_setg_errno(errp, errno,
> > - "failed to open file '%s' "
> > - "(mode: '%s')",
> > - path, mode);
>
> This changes the error message, doesn't it?
>
> Not an objection, just something that might be worth noting in the
> commit message.
>
ok
> > goto end;
> > }
> >
> > -qemu_set_cloexec(fd);
> > -
> > if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
> > error_setg_errno(errp, errno,
> > "failed to set permission 0%03o on new file '%s'
> > (mode: '%s')",
>
Re: [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec()
Hi
On Thu, May 5, 2022 at 3:43 PM Markus Armbruster wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > qemu_open_old() uses qemu_open_internal() which handles special
> > "/dev/fdset/" path for monitor fd sets, set CLOEXEC, and uses Error
> > reporting (and some O_DIRECT special error casing).
> >
> > The monitor fdset handling is unnecessary for qga, use
> > qemu_open_cloexec() instead.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > qga/channel-posix.c | 14 +-
> > qga/commands-posix.c | 24
> > 2 files changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> > index 0ce594bc36c2..a1262b130145 100644
> > --- a/qga/channel-posix.c
> > +++ b/qga/channel-posix.c
> > @@ -1,4 +1,5 @@
> > #include "qemu/osdep.h"
> > +#include "qemu/cutils.h"
> > #include
> > #include "qapi/error.h"
> > #include "qemu/sockets.h"
> > @@ -128,11 +129,15 @@ static gboolean ga_channel_open(GAChannel *c,
> const gchar *path,
> > switch (c->method) {
> > case GA_CHANNEL_VIRTIO_SERIAL: {
> > assert(fd < 0);
> > -fd = qemu_open_old(path, O_RDWR | O_NONBLOCK
> > +fd = qemu_open_cloexec(
> > +path,
> > #ifndef CONFIG_SOLARIS
> > - | O_ASYNC
> > +O_ASYNC |
> > #endif
> > - );
> > +O_RDWR | O_NONBLOCK,
> > +0,
> > +errp
> > +);
> > if (fd == -1) {
> > error_setg_errno(errp, errno, "error opening channel");
> > return false;
> > @@ -157,9 +162,8 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> > struct termios tio;
> >
> > assert(fd < 0);
> > -fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
> > +fd = qemu_open_cloexec(path, O_RDWR | O_NOCTTY | O_NONBLOCK, 0,
> errp);
> > if (fd == -1) {
> > -error_setg_errno(errp, errno, "error opening channel");
> > return false;
> > }
> > tcgetattr(fd, );
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 8ebc327c5e02..f82205e25813 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -1392,6 +1392,7 @@ static GuestDiskInfoList *get_disk_partitions(
> >
> > static void get_nvme_smart(GuestDiskInfo *disk)
> > {
> > +Error *err = NULL;
> > int fd;
> > GuestNVMeSmart *smart;
> > NvmeSmartLog log = {0};
> > @@ -1404,9 +1405,10 @@ static void get_nvme_smart(GuestDiskInfo *disk)
> > | (((sizeof(log) >> 2) - 1) << 16)
> > };
> >
> > -fd = qemu_open_old(disk->name, O_RDONLY);
> > +fd = qemu_open_cloexec(disk->name, O_RDONLY, 0, );
> > if (fd == -1) {
> > -g_debug("Failed to open device: %s: %s", disk->name,
> g_strerror(errno));
> > +g_debug("Failed to open device: %s: %s", disk->name,
> error_get_pretty(err));
> > +error_free(err);
> > return;
> > }
> >
> > @@ -1737,9 +1739,8 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
> has_mountpoints,
> > }
> > }
> >
> > -fd = qemu_open_old(mount->dirname, O_RDONLY);
> > +fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
> > if (fd == -1) {
> > -error_setg_errno(errp, errno, "failed to open %s",
> mount->dirname);
> > goto error;
> > }
> >
> > @@ -1804,7 +1805,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
> >
> > QTAILQ_FOREACH(mount, , next) {
> > logged = false;
> > -fd = qemu_open_old(mount->dirname, O_RDONLY);
> > +fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, NULL);
> > if (fd == -1) {
> > continue;
> > }
> > @@ -1864,21 +1865,20 @@ static void guest_fsfreeze_cleanup(void)
> > GuestFilesystemTrimResponse *
> > qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> > {
> > +ERRP_GUARD();
> > GuestFilesystemTrimResponse *response;
> > GuestFilesystemTrimResult *result;
> > int ret = 0;
>
Re: [PATCH v2 15/15] test/qga: use g_auto wherever sensible
Hi
On Thu, May 5, 2022 at 3:47 PM Markus Armbruster wrote:
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > tests/unit/test-qga.c | 125 +++---
> > 1 file changed, 45 insertions(+), 80 deletions(-)
> >
> > diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> > index ab0b12a2dd16..53cefc2c2649 100644
> > --- a/tests/unit/test-qga.c
> > +++ b/tests/unit/test-qga.c
> > @@ -51,8 +51,11 @@ static void
> > fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
> > {
> > const gchar *extra_arg = data;
> > -GError *error = NULL;
> > -gchar *cwd, *path, *cmd, **argv = NULL;
> > +g_autoptr(GError) error = NULL;
> > +g_autofree char *cwd = NULL;
> > +g_autofree char *path = NULL;
> > +g_autofree char *cmd = NULL;
> > +g_auto(GStrv) argv = NULL;
>
> Arranges five variables to be auto-freed, where ...
>
> >
> > fixture->loop = g_main_loop_new(NULL, FALSE);
> >
> > @@ -78,17 +81,12 @@ fixture_setup(TestFixture *fixture, gconstpointer data,
> > gchar **envp)
> >
> > fixture->fd = connect_qga(path);
> > g_assert_cmpint(fixture->fd, !=, -1);
> > -
> > -g_strfreev(argv);
> > -g_free(cmd);
> > -g_free(cwd);
> > -g_free(path);
>
> ... only four were freed before. The extra one is @error. Which can
> only be null here, thanks to g_assert_no_error(), can't it?
Right, but for consistency we can have it. I can drop it too.
Re: [PATCH v2 08/15] qga: throw an Error in ga_channel_open()
Hi
On Thu, May 5, 2022 at 3:41 PM Markus Armbruster wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > Allow for a single point of error reporting, and further refactoring.
>
> This sounds like there is no behavioral change intended. But it looks
> like there is change; see below.
>
> > Signed-off-by: Marc-André Lureau
> > ---
> > qga/channel-posix.c | 42 +-
> > 1 file changed, 17 insertions(+), 25 deletions(-)
> >
> > diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> > index a996858e2492..0ce594bc36c2 100644
> > --- a/qga/channel-posix.c
> > +++ b/qga/channel-posix.c
> > @@ -119,8 +119,9 @@ static int ga_channel_client_add(GAChannel *c, int
> fd)
> > }
> >
> > static gboolean ga_channel_open(GAChannel *c, const gchar *path,
> > -GAChannelMethod method, int fd)
> > +GAChannelMethod method, int fd, Error
> **errp)
> > {
> > +ERRP_GUARD();
> > int ret;
> > c->method = method;
> >
> > @@ -133,21 +134,20 @@ static gboolean ga_channel_open(GAChannel *c,
> const gchar *path,
> > #endif
> > );
> > if (fd == -1) {
> > -g_critical("error opening channel: %s", strerror(errno));
> > +error_setg_errno(errp, errno, "error opening channel");
> > return false;
> > }
> > #ifdef CONFIG_SOLARIS
> > ret = ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI);
> > if (ret == -1) {
> > -g_critical("error setting event mask for channel: %s",
> > - strerror(errno));
> > +error_setg_errno(errp, errno, "error setting event mask for
> channel");
> > close(fd);
> > return false;
> > }
> > #endif
> > ret = ga_channel_client_add(c, fd);
> > if (ret) {
> > -g_critical("error adding channel to main loop");
> > +error_setg(errp, "error adding channel to main loop");
> > close(fd);
> > return false;
> > }
> > @@ -159,7 +159,7 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> > assert(fd < 0);
> > fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
> > if (fd == -1) {
> > -g_critical("error opening channel: %s", strerror(errno));
> > +error_setg_errno(errp, errno, "error opening channel");
> > return false;
> > }
> > tcgetattr(fd, );
> > @@ -180,7 +180,7 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> > tcsetattr(fd, TCSANOW, );
> > ret = ga_channel_client_add(c, fd);
> > if (ret) {
> > -g_critical("error adding channel to main loop");
> > +error_setg(errp, "error adding channel to main loop");
> > close(fd);
> > return false;
> > }
> > @@ -188,12 +188,8 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> > }
> > case GA_CHANNEL_UNIX_LISTEN: {
> > if (fd < 0) {
> > -Error *local_err = NULL;
> > -
> > -fd = unix_listen(path, _err);
> > -if (local_err != NULL) {
> > -g_critical("%s", error_get_pretty(local_err));
> > -error_free(local_err);
> > +fd = unix_listen(path, errp);
> > +if (fd < 0) {
> > return false;
> > }
> > }
> > @@ -202,24 +198,19 @@ static gboolean ga_channel_open(GAChannel *c,
> const gchar *path,
> > }
> > case GA_CHANNEL_VSOCK_LISTEN: {
> > if (fd < 0) {
> > -Error *local_err = NULL;
> > SocketAddress *addr;
> > char *addr_str;
> >
> > addr_str = g_strdup_printf("vsock:%s", path);
> > -addr = socket_parse(addr_str, _err);
> > +addr = socket_parse(addr_str, errp);
> > g_free(addr_str);
> > -if (local_err != NULL) {
> > -g_critical("%s", error_get_pretty(local_err));
> > -error_free(local_err);
> > +if (
Re: [PATCH v2 05/15] qga: flatten safe_open_or_create()
Hi
On Thu, May 5, 2022 at 3:20 PM Markus Armbruster wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > There is a bit too much branching in the function, this can be
> > simplified a bit, and have a common exit point thanks to ERRP_PROPAGATE.
>
> Do you mean ERRP_GUARD()?
>
yes
>
> I'm not sure the commit reduces "branching", but it certainly reduces
> nesting, which I find improves readability.
>
ok
>
> > This also helps with the following error handling changes.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > qga/commands-posix.c | 124 ++-
> > 1 file changed, 63 insertions(+), 61 deletions(-)
>
> I think the diff is easier to review with space change ignored:
>
> | diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> | index 78f2f21001..b4b19d3eb4 100644
> | --- a/qga/commands-posix.c
> | +++ b/qga/commands-posix.c
> | @@ -315,12 +315,14 @@ find_open_flag(const char *mode_str, Error **errp)
> | static FILE *
> | safe_open_or_create(const char *path, const char *mode, Error **errp)
> | {
> | -Error *local_err = NULL;
> | -int oflag;
> | +ERRP_GUARD();
> | +int oflag, fd = -1;
>
> I'd prefer
>
> +ERRP_GUARD();
>int oflag;
> +int fd = -1;
>
>
ok
> because it's slightly less churn, and I dislike variables with and
> without initializer in the same declaration. Matter of taste.
>
> | +FILE *f = NULL;
> |
> | -oflag = find_open_flag(mode, _err);
> | -if (local_err == NULL) {
> | -int fd;
> | +oflag = find_open_flag(mode, errp);
> | +if (*errp) {
>
> I'd prefer
>
>if (oflag < 0) {
>
> No need for ERRP_GUARD() then, as far as I can tell.
>
>
"qga: use qemu_open_cloexec() for safe_open_or_create()" expects it to be
non-null though, I can move it there.
> | +goto end;
> | +}
> |
> | /* If the caller wants / allows creation of a new file, we
> implement it
> | * with a two step process: open() + (open() / fchmod()).
> | @@ -349,39 +351,39 @@ safe_open_or_create(const char *path, const char
> *mode, Error **errp)
> | oflag &= ~(unsigned)O_CREAT;
> | fd = open(path, oflag);
> | }
> | -
> | if (fd == -1) {
> | -error_setg_errno(_err, errno, "failed to open file
> '%s' "
> | - "(mode: '%s')", path, mode);
> | -} else {
> | +error_setg_errno(errp, errno,
> | + "failed to open file '%s' "
> | + "(mode: '%s')",
> | + path, mode);
> | +goto end;
> | +}
> | +
> | qemu_set_cloexec(fd);
> |
> | if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
> | -error_setg_errno(_err, errno, "failed to set
> permission "
> | - "0%03o on new file '%s' (mode: '%s')",
> | +error_setg_errno(errp, errno,
> | + "failed to set permission 0%03o on new file
> '%s' (mode: '%s')",
> | (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
> | -} else {
> | -FILE *f;
> | +goto end;
> | +}
> |
> | f = fdopen(fd, mode);
> | if (f == NULL) {
> | -error_setg_errno(_err, errno, "failed to
> associate "
> | - "stdio stream with file descriptor
> %d, "
> | - "file '%s' (mode: '%s')", fd,
> path, mode);
> | -} else {
> | -return f;
> | -}
> | +error_setg_errno(errp, errno,
> | + "failed to associate stdio stream with file
> descriptor %d, "
> | + "file '%s' (mode: '%s')",
> | + fd, path, mode);
> | }
> |
> | +end:
> | +if (f == NULL && fd != -1) {
> | close(fd);
> | if (oflag & O_CREAT) {
> | unlink(path);
> | }
> | }
> | -}
> | -
> | -error_propagate(errp, local_err);
> | -return NULL;
> | +return f;
> | }
> |
> | int64_t qmp_guest_file_open(const char *path, bool has_mode, const char
> *mode,
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 03/15] tests: make libqmp buildable for win32
Hi
On Thu, May 5, 2022 at 2:52 PM Markus Armbruster wrote:
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau
> >
> > Signed-off-by: Marc-André Lureau
> > Reviewed-by: Thomas Huth
> > ---
> > tests/qtest/libqmp.h | 2 ++
> > tests/qtest/libqmp.c | 35 +--
> > 2 files changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/qtest/libqmp.h b/tests/qtest/libqmp.h
> > index 94aa97328a17..772f18b73ba3 100644
> > --- a/tests/qtest/libqmp.h
> > +++ b/tests/qtest/libqmp.h
> > @@ -20,8 +20,10 @@
> > #include "qapi/qmp/qdict.h"
> >
> > QDict *qmp_fd_receive(int fd);
> > +#ifndef G_OS_WIN32
>
> What's the difference between G_OS_WIN32 and _WIN32?
>
> We have 10 of the former, but >250 of the latter. If they are
> effectively the same, we should pick one and stick to it.
There are some subtle differences when compiling for cygwin, in which
case G_OS_WIN32 is not defined.
I usually pick G_OS_{UNIX,WIN32} defines, mostly for consistency, but
in many situation _WIN32/WIN32 is fine.
(and we also have CONFIG_WIN32)
Re: [PATCH 06/10] Replace qemu_pipe() with g_unix_open_pipe()
Hi On Fri, Apr 22, 2022 at 1:00 PM Daniel P. Berrangé wrote: > > On Fri, Apr 22, 2022 at 12:36:35PM +0400, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > GLib g_unix_open_pipe() is essentially like qemu_pipe(), available since > > 2.30. > > > > Signed-off-by: Marc-André Lureau > > --- > > include/qemu/osdep.h| 4 > > qemu-nbd.c | 5 +++-- > > util/event_notifier-posix.c | 2 +- > > util/oslib-posix.c | 22 -- > > 4 files changed, 4 insertions(+), 29 deletions(-) > > There are a bunch of places still using 'pipe'instead of 'qemu_pipe' > that should be switched also. > Ok, that would be a different patch though. Can you ack this one for now? > > With regards, > Daniel > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| >
Re: [PATCH 06/41] include: rename qemu-common.h qemu/copyright.h
Hi
On Wed, Apr 20, 2022 at 7:17 PM Daniel P. Berrangé
wrote:
> On Wed, Apr 20, 2022 at 05:25:49PM +0400, marcandre.lur...@redhat.com
> wrote:
> > From: Marc-André Lureau
>
> Could use a commit message explaining why this is a good
> idea.
>
> I see it contains QEMU_COPYRIGHT macro, but it also then
> contains QEMU_HELP_BOTTOM which is about bug reporting
> not copyright.
>
> IMHO something like 'qemu-cli.h' could be a better match
>
That was Peter's suggestion:
https://patchew.org/QEMU/20220323155743.1585078-1-marcandre.lur...@redhat.com/20220323155743.1585078-33-marcandre.lur...@redhat.com/#CAFEAcA9kYweS2zMHjWDuV_y2AxKbgJ5UYNHLK3sASCLVD=y...@mail.gmail.com
I don't mind qemu-cli.h or elsewhere
Peter?
> >
> > Suggested-by: Peter Maydell
> > Signed-off-by: Marc-André Lureau
> > ---
> > include/{qemu-common.h => qemu/copyright.h} | 0
> > bsd-user/main.c | 2 +-
> > linux-user/main.c | 2 +-
> > qemu-img.c | 2 +-
> > qemu-io.c | 2 +-
> > qemu-nbd.c | 2 +-
> > qga/main.c | 2 +-
> > scsi/qemu-pr-helper.c | 2 +-
> > softmmu/vl.c| 2 +-
> > storage-daemon/qemu-storage-daemon.c| 2 +-
> > tools/virtiofsd/passthrough_ll.c| 2 +-
> > ui/cocoa.m | 2 +-
> > 12 files changed, 11 insertions(+), 11 deletions(-)
> > rename include/{qemu-common.h => qemu/copyright.h} (100%)
> >
> > diff --git a/include/qemu-common.h b/include/qemu/copyright.h
> > similarity index 100%
> > rename from include/qemu-common.h
> > rename to include/qemu/copyright.h
> > diff --git a/bsd-user/main.c b/bsd-user/main.c
> > index 88d347d05ebf..aaab3f278534 100644
> > --- a/bsd-user/main.c
> > +++ b/bsd-user/main.c
> > @@ -24,7 +24,7 @@
> > #include
> >
> > #include "qemu/osdep.h"
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qemu/units.h"
> > #include "qemu/accel.h"
> > #include "sysemu/tcg.h"
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index fbc9bcfd5f5f..744d216b1e8e 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -18,7 +18,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qemu/units.h"
> > #include "qemu/accel.h"
> > #include "sysemu/tcg.h"
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 116e05867558..a2b1d3653a1e 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -25,7 +25,7 @@
> > #include "qemu/osdep.h"
> > #include
> >
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qemu/qemu-progress.h"
> > #include "qemu-version.h"
> > #include "qapi/error.h"
> > diff --git a/qemu-io.c b/qemu-io.c
> > index eb8afc8b413b..952a36643b0c 100644
> > --- a/qemu-io.c
> > +++ b/qemu-io.c
> > @@ -15,7 +15,7 @@
> > #include
> > #endif
> >
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qapi/error.h"
> > #include "qemu-io.h"
> > #include "qemu/error-report.h"
> > diff --git a/qemu-nbd.c b/qemu-nbd.c
> > index 713e7557a9eb..f4d121c0c40e 100644
> > --- a/qemu-nbd.c
> > +++ b/qemu-nbd.c
> > @@ -21,7 +21,7 @@
> > #include
> > #include
> >
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qapi/error.h"
> > #include "qemu/cutils.h"
> > #include "sysemu/block-backend.h"
> > diff --git a/qga/main.c b/qga/main.c
> > index ac63d8e47802..8994f73e4735 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -18,7 +18,7 @@
> > #include
> > #include
> > #endif
> > -#include "qemu-common.h"
> > +#include "qemu/copyright.h"
> > #include "qapi/qmp/json-parser.h"
> > #include "qapi/qmp/qdict.h"
> > #include "qapi/qmp/qjson.h"
> > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> > index f281daeced8d..e7549ffb3bc9 100644
Re: [PATCH 28/41] Use QEMU_SANITIZE_ADDRESS
On Wed, Apr 20, 2022 at 6:20 PM Thomas Huth wrote:
>
> On 20/04/2022 15.26, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > tests/qtest/fdc-test.c| 2 +-
> > util/coroutine-ucontext.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> > index 4aa72f36431f..0b3c2c0d523f 100644
> > --- a/tests/qtest/fdc-test.c
> > +++ b/tests/qtest/fdc-test.c
> > @@ -550,7 +550,7 @@ static void fuzz_registers(void)
> >
> > static bool qtest_check_clang_sanitizer(void)
> > {
> > -#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
> > +#ifdef QEMU_SANITIZE_ADDRESS
> > return true;
> > #else
> > g_test_skip("QEMU not configured using --enable-sanitizers");
> > diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
> > index 904b375192ca..52725f5344fb 100644
> > --- a/util/coroutine-ucontext.c
> > +++ b/util/coroutine-ucontext.c
> > @@ -30,7 +30,7 @@
> > #include
> > #endif
> >
> > -#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
> > +#ifdef QEMU_SANITIZE_THREAD
>
> Shouldn't that be QEMU_SANITIZE_ADDRESS instead?
>
oops, thanks
Re: [PATCH 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
Hi On Wed, Mar 16, 2022 at 5:28 PM Thomas Huth wrote: > > On 16/03/2022 14.16, Philippe Mathieu-Daudé wrote: > > On 16/3/22 10:52, marcandre.lur...@redhat.com wrote: > >> From: Marc-André Lureau > >> > >> One less qemu-specific macro. It also helps to make some headers/units > >> only depend on glib, and thus moved in standalone projects eventually. > >> > >> Signed-off-by: Marc-André Lureau > >> --- > >> audio/audio.h | 4 +-- > >> block/qcow2.h | 2 +- > >> bsd-user/qemu.h | 2 +- > >> hw/display/qxl.h| 2 +- > >> hw/net/rocker/rocker.h | 2 +- > >> hw/xen/xen_pt.h | 2 +- > >> include/chardev/char-fe.h | 2 +- > >> include/disas/dis-asm.h | 2 +- > >> include/hw/acpi/aml-build.h | 12 +++ > >> include/hw/core/cpu.h | 2 +- > >> include/hw/hw.h | 2 +- > >> include/hw/virtio/virtio.h | 2 +- > >> include/hw/xen/xen-bus-helper.h | 4 +-- > >> include/hw/xen/xen-bus.h| 4 +-- > >> include/hw/xen/xen_common.h | 2 +- > >> include/hw/xen/xen_pvdev.h | 2 +- > >> include/monitor/monitor.h | 4 +-- > >> include/qapi/error.h| 20 ++-- > >> include/qapi/qmp/qjson.h| 8 ++--- > >> include/qemu/buffer.h | 2 +- > >> include/qemu/compiler.h | 11 ++- > >> include/qemu/error-report.h | 24 +++--- > >> include/qemu/log-for-trace.h| 2 +- > >> include/qemu/log.h | 2 +- > >> include/qemu/qemu-print.h | 8 ++--- > >> include/qemu/readline.h | 2 +- > >> qga/guest-agent-core.h | 2 +- > >> qga/vss-win32/requester.h | 2 +- > >> scripts/cocci-macro-file.h | 2 +- > >> tests/qtest/libqos/libqtest.h | 42 - > >> tests/qtest/libqtest-single.h | 2 +- > >> tests/qtest/migration-helpers.h | 6 ++-- > >> audio/alsaaudio.c | 4 +-- > >> audio/dsoundaudio.c | 4 +-- > >> audio/ossaudio.c| 4 +-- > >> audio/paaudio.c | 2 +- > >> audio/sdlaudio.c| 2 +- > >> block/blkverify.c | 2 +- > >> block/ssh.c | 4 +-- > >> fsdev/9p-marshal.c | 2 +- > >> fsdev/virtfs-proxy-helper.c | 2 +- > >> hw/9pfs/9p.c| 2 +- > >> hw/acpi/aml-build.c | 4 +-- > >> hw/mips/fuloong2e.c | 2 +- > >> hw/mips/malta.c | 2 +- > >> hw/net/rtl8139.c| 2 +- > >> hw/virtio/virtio.c | 2 +- > >> io/channel-websock.c| 2 +- > >> monitor/hmp.c | 4 +-- > >> nbd/server.c| 10 +++--- > >> qemu-img.c | 4 +-- > >> qemu-io.c | 2 +- > >> qobject/json-parser.c | 2 +- > >> softmmu/qtest.c | 4 +-- > >> tests/qtest/libqtest.c | 2 +- > >> tests/unit/test-qobject-input-visitor.c | 4 +-- > >> audio/coreaudio.m | 4 +-- > >> scripts/checkpatch.pl | 2 +- > >> 58 files changed, 130 insertions(+), 137 deletions(-) > > > >> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > >> index 3baa5e3790f7..f2bd050e3b9a 100644 > >> --- a/include/qemu/compiler.h > >> +++ b/include/qemu/compiler.h > >> @@ -79,19 +79,12 @@ > >> #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - > >> \ > >> sizeof(QEMU_BUILD_BUG_ON_STRUCT(x))) > >> -#if defined(__clang__) > >> -/* clang doesn't support gnu_printf, so use printf. */ > >> -# define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m))) >
Re: [PATCH 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
Hi On Wed, Mar 16, 2022 at 5:30 PM Daniel P. Berrangé wrote: > > On Wed, Mar 16, 2022 at 01:52:48PM +0400, marcandre.lur...@redhat.com wrote: > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > > index 3baa5e3790f7..f2bd050e3b9a 100644 > > --- a/include/qemu/compiler.h > > +++ b/include/qemu/compiler.h > > @@ -79,19 +79,12 @@ > > #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \ > > sizeof(QEMU_BUILD_BUG_ON_STRUCT(x))) > > > > -#if defined(__clang__) > > -/* clang doesn't support gnu_printf, so use printf. */ > > -# define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m))) > > -#else > > -/* Use gnu_printf (qemu uses standard format strings). */ > > -# define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m))) > > -# if defined(_WIN32) > > +#if !defined(__clang__) && defined(_WIN32) > > /* > > * Map __printf__ to __gnu_printf__ because we want standard format > > strings even > > * when MinGW or GLib include files use __printf__. > > */ > > -# define __printf__ __gnu_printf__ > > -# endif > > +# define __printf__ __gnu_printf__ > > #endif > > I'm not convinced we shold have this remaining define, even > before your patch. > > For code we've implemented, we should have used __gnu_printf__ > already if we know it uses GNU format on Windows. > > For code in GLib, its header file uses __gnu_printf__ for anything > that relies on its portable printf replacement, which is basically > everything in GLib. > > For anything else we should honour whatever they declare, and not > assume their impl is the GNU one. > > > I guess it is easy enough to validate by deleting this and seeing > if we get any warnings from the mingw CI jobs about printf/gnu_printf > mismatch. Please comment on that thread: https://patchew.org/QEMU/20220224183701.608720-1-marcandre.lur...@redhat.com/20220224183701.608720-6-marcandre.lur...@redhat.com/ > > With regards, > Daniel > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| >
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
On Wed, Mar 16, 2022 at 5:04 PM Philippe Mathieu-Daudé wrote: > > On 16/3/22 10:53, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Replace a config-time define with a compile time condition > > define (compatible with clang and gcc) that must be declared prior to > > its usage. This avoids having a global configure time define, but also > > prevents from bad usage, if the config header wasn't included before. > > > > This can help to make some code independent from qemu too. > > > > gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. > > > > Signed-off-by: Marc-André Lureau > > --- > > meson.build | 1 - > > accel/tcg/atomic_template.h | 4 +- > > audio/audio.h | 2 +- > > hw/display/pl110_template.h | 6 +-- > > hw/net/can/ctucan_core.h| 2 +- > > hw/net/vmxnet3.h| 4 +- > > include/exec/cpu-all.h | 4 +- > > include/exec/cpu-common.h | 2 +- > > include/exec/memop.h| 2 +- > > include/exec/memory.h | 2 +- > > include/fpu/softfloat-types.h | 2 +- > > include/hw/core/cpu.h | 2 +- > > include/hw/i386/intel_iommu.h | 6 +-- > > include/hw/i386/x86-iommu.h | 4 +- > > include/hw/virtio/virtio-access.h | 6 +-- > > include/hw/virtio/virtio-gpu-bswap.h| 2 +- > > include/libdecnumber/dconfig.h | 2 +- > > include/net/eth.h | 2 +- > > include/qemu/bswap.h| 8 ++-- > > include/qemu/compiler.h | 2 + > > include/qemu/host-utils.h | 2 +- > > include/qemu/int128.h | 2 +- > > include/ui/qemu-pixman.h| 2 +- > > net/util.h | 2 +- > > target/arm/cpu.h| 8 ++-- > > target/arm/translate-a64.h | 2 +- > > target/arm/vec_internal.h | 2 +- > > target/i386/cpu.h | 2 +- > > target/mips/cpu.h | 2 +- > > target/ppc/cpu.h| 2 +- > > target/s390x/tcg/vec.h | 2 +- > > target/xtensa/cpu.h | 2 +- > > tests/fp/platform.h | 4 +- > > accel/kvm/kvm-all.c | 4 +- > > audio/dbusaudio.c | 2 +- > > disas.c | 2 +- > > hw/core/loader.c| 4 +- > > hw/display/artist.c | 6 +-- > > hw/display/pxa2xx_lcd.c | 2 +- > > hw/display/vga.c| 12 +++--- > > hw/display/virtio-gpu-gl.c | 2 +- > > hw/s390x/event-facility.c | 2 +- > > hw/virtio/vhost.c | 2 +- > > linux-user/arm/nwfpe/double_cpdo.c | 4 +- > > linux-user/arm/nwfpe/fpa11_cpdt.c | 4 +- > > linux-user/ppc/signal.c | 3 +- > > linux-user/syscall.c| 6 +-- > > net/net.c | 4 +- > > target/alpha/translate.c| 2 +- > > target/arm/crypto_helper.c | 2 +- > > target/arm/helper.c | 2 +- > > target/arm/kvm64.c | 4 +- > > target/arm/neon_helper.c| 2 +- > > target/arm/sve_helper.c | 4 +- > > target/arm/translate-sve.c | 6 +-- > > target/arm/translate-vfp.c | 2 +- > > target/arm/translate.c | 2 +- > > target/hppa/translate.c | 2 +- > > target/i386/tcg/translate.c | 2 +- > > target/mips/tcg/lmmi_helper.c | 2 +- > > target/mips/tcg/msa_helper.c| 54 - > > target/ppc/arch_dump.c | 2 +- > > target/ppc/int_helper.c | 22 +- > > target/ppc/kvm.c| 4 +- > > target/ppc/mem_helper.c | 2 +- > > target/riscv/vector_helper.c| 2 +- > > target/s390x/tcg/translate.c| 2 +- > > target/sparc/vis_helper.c | 4 +- > > tcg/tcg-op.c| 4 +- > > tcg/tcg.c | 12 +++--- > > tests/qtest/vhost-us
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
Hi
On Wed, Mar 16, 2022 at 3:16 PM Halil Pasic wrote:
>
> On Wed, 16 Mar 2022 11:28:59 +0100
> Thomas Huth wrote:
>
> > On 16/03/2022 10.53, marcandre.lur...@redhat.com wrote:
> > > From: Marc-André Lureau
> > >
> > > Replace a config-time define with a compile time condition
> > > define (compatible with clang and gcc) that must be declared prior to
> > > its usage. This avoids having a global configure time define, but also
> > > prevents from bad usage, if the config header wasn't included before.
> > >
> > > This can help to make some code independent from qemu too.
> > >
> > > gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2.
> > >
> > > Signed-off-by: Marc-André Lureau
> > > ---
> > [...]
> > > @@ -188,7 +188,7 @@ CPU_CONVERT(le, 64, uint64_t)
> > >* a compile-time constant if you pass in a constant. So this can be
> > >* used to initialize static variables.
> > >*/
> > > -#if defined(HOST_WORDS_BIGENDIAN)
> > > +#if HOST_BIG_ENDIAN
> > > # define const_le32(_x) \
> > > _x) & 0x00ffU) << 24) | \
> > >(((_x) & 0xff00U) << 8) | \
> > > @@ -211,7 +211,7 @@ typedef union {
> > >
> > > typedef union {
> > > float64 d;
> > > -#if defined(HOST_WORDS_BIGENDIAN)
> > > +#if HOST_BIG_ENDIAN
> > > struct {
> > > uint32_t upper;
> > > uint32_t lower;
> > > @@ -235,7 +235,7 @@ typedef union {
> > >
> > > typedef union {
> > > float128 q;
> > > -#if defined(HOST_WORDS_BIGENDIAN)
> > > +#if HOST_BIG_ENDIAN
> > > struct {
> > > uint32_t upmost;
> > > uint32_t upper;
> > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> > > index 0a5e67fb970e..7fdd88adb368 100644
> > > --- a/include/qemu/compiler.h
> > > +++ b/include/qemu/compiler.h
> > > @@ -7,6 +7,8 @@
> > > #ifndef COMPILER_H
> > > #define COMPILER_H
> > >
> > > +#define HOST_BIG_ENDIAN (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)
> >
> > Why don't you do it this way instead:
> >
> > #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > #define HOST_WORDS_BIGENDIAN 1
> > #endif
> >
> > ... that way you could avoid the churn in all the other files?
> >
>
> I guess "prevents from bad usage, if the config header wasn't included
> before" from the commit message is the answer to that question. I agree
> that it is more robust. If we keep the #if defined we really can't
> differentiate between "not defined because not big-endian" and "not
> defined because the appropriate header was not included."
That's right, thanks
Re: [PATCH v2 4/5] Replace qemu_gettimeofday() with g_get_real_time()
Hi
On Mon, Mar 7, 2022 at 2:13 PM Laurent Vivier wrote:
> Le 05/03/2022 à 20:17, Marc-André Lureau a écrit :
> > On Sat, Mar 5, 2022 at 1:18 AM wrote:
> >>
> >> From: Marc-André Lureau
> >>
> >> GLib g_get_real_time() is an alternative to gettimeofday() which allows
> >> to simplify our code.
> >>
> >> For semihosting, a few bits are lost on POSIX host, but this shouldn't
> >> be a big concern.
> >>
> >> Signed-off-by: Marc-André Lureau
> >> Reviewed-by: Laurent Vivier
> >> ---
> >> blockdev.c | 8
> >> hw/rtc/m41t80.c| 6 +++---
> >> hw/virtio/virtio-balloon.c | 9 +
> >> qapi/qmp-event.c | 12 +---
> >> qemu-img.c | 8
> >> target/m68k/m68k-semi.c| 22 ++
> >> target/nios2/nios2-semi.c | 23 ++-
> >> 7 files changed, 37 insertions(+), 51 deletions(-)
> >>
> ...
> >> index 19d3cd003833..025716b3ec37 100644
> >> --- a/qapi/qmp-event.c
> >> +++ b/qapi/qmp-event.c
> >> @@ -20,15 +20,13 @@
> >>
> >> static void timestamp_put(QDict *qdict)
> >> {
> >> -int err;
> >> QDict *ts;
> >> -qemu_timeval tv;
> >> +int64_t rt = g_get_real_time();
> >>
> >> -err = qemu_gettimeofday();
> >> -/* Put -1 to indicate failure of getting host time */
> >> -ts = qdict_from_jsonf_nofail("{ 'seconds': %lld, 'microseconds':
> %lld }",
> >> - err < 0 ? -1LL : (long long)tv.tv_sec,
> >> - err < 0 ? -1LL : (long
> long)tv.tv_usec);
> >> +ts = qdict_from_jsonf_nofail("{ 'seconds': %" G_GINT64_FORMAT
> >> + ", 'microseconds': %" G_GINT64_FORMAT
> "}",
> >> + rt / G_USEC_PER_SEC,
> >> + rt % G_USEC_PER_SEC);
> >
> > NACK this, fixed in v3
>
> Why keeping the %lld is better than moving to %G_GINT64_FORMAT?
>
>
It's not supported by json-parser.c parse_interpolation(). We could address
this in a different patch.
--
Marc-André Lureau
Re: [PATCH v3 2/5] qtest: replace gettimeofday with GTimer
Hi
On Mon, Mar 7, 2022 at 11:46 AM Thomas Huth wrote:
> On 07/03/2022 08.03, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > glib provides a convenience helper to measure elapsed time. It isn't
> > subject to wall-clock time changes.
> >
> > Note that this changes the initial OPENED time, which used to print the
> > current time.
> [...]
> > @@ -846,21 +828,20 @@ static void qtest_event(void *opaque, QEMUChrEvent
> event)
> > for (i = 0; i < ARRAY_SIZE(irq_levels); i++) {
> > irq_levels[i] = 0;
> > }
> > -qemu_gettimeofday(_time);
> > +
> > +g_clear_pointer(, g_timer_destroy);
> > +timer = g_timer_new();
> > qtest_opened = true;
> > if (qtest_log_fp) {
> > -fprintf(qtest_log_fp, "[I " FMT_timeval "] OPENED\n",
> > -(long) start_time.tv_sec, (long)
> start_time.tv_usec);
> > +fprintf(qtest_log_fp, "[I " FMT_timeval "] OPENED\n",
> g_timer_elapsed(timer, NULL));
> > }
> > break;
>
> The new timestamp here is quite unuseful now, of course ... could you
> replace it with g_get_current_time() instead?
>
Eventually, but I wonder why this (and only this) particular timestamp
should be the current time.
--
Marc-André Lureau
Re: [PATCH v2 4/5] Replace qemu_gettimeofday() with g_get_real_time()
On Sat, Mar 5, 2022 at 1:18 AM wrote:
>
> From: Marc-André Lureau
>
> GLib g_get_real_time() is an alternative to gettimeofday() which allows
> to simplify our code.
>
> For semihosting, a few bits are lost on POSIX host, but this shouldn't
> be a big concern.
>
> Signed-off-by: Marc-André Lureau
> Reviewed-by: Laurent Vivier
> ---
> blockdev.c | 8
> hw/rtc/m41t80.c| 6 +++---
> hw/virtio/virtio-balloon.c | 9 +
> qapi/qmp-event.c | 12 +---
> qemu-img.c | 8
> target/m68k/m68k-semi.c| 22 ++
> target/nios2/nios2-semi.c | 23 ++-
> 7 files changed, 37 insertions(+), 51 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 42e098b458b1..4b07dbfbdefc 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1235,7 +1235,7 @@ static void internal_snapshot_prepare(BlkActionState
> *common,
> BlockDriverState *bs;
> QEMUSnapshotInfo old_sn, *sn;
> bool ret;
> -qemu_timeval tv;
> +int64_t rt;
> BlockdevSnapshotInternal *internal;
> InternalSnapshotState *state;
> AioContext *aio_context;
> @@ -1305,9 +1305,9 @@ static void internal_snapshot_prepare(BlkActionState
> *common,
> /* 3. take the snapshot */
> sn = >sn;
> pstrcpy(sn->name, sizeof(sn->name), name);
> -qemu_gettimeofday();
> -sn->date_sec = tv.tv_sec;
> -sn->date_nsec = tv.tv_usec * 1000;
> +rt = g_get_real_time();
> +sn->date_sec = rt / G_USEC_PER_SEC;
> +sn->date_nsec = (rt % G_USEC_PER_SEC) * 1000;
> sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> if (replay_mode != REPLAY_MODE_NONE) {
> sn->icount = replay_get_current_icount();
> diff --git a/hw/rtc/m41t80.c b/hw/rtc/m41t80.c
> index a00971a67e1c..e045c864bb44 100644
> --- a/hw/rtc/m41t80.c
> +++ b/hw/rtc/m41t80.c
> @@ -47,7 +47,7 @@ static uint8_t m41t80_recv(I2CSlave *i2c)
> {
> M41t80State *s = M41T80(i2c);
> struct tm now;
> -qemu_timeval tv;
> +int64_t rt;
>
> if (s->addr < 0) {
> s->addr = 0;
> @@ -57,8 +57,8 @@ static uint8_t m41t80_recv(I2CSlave *i2c)
> }
> switch (s->addr++) {
> case 0:
> -qemu_gettimeofday();
> -return to_bcd(tv.tv_usec / 1);
> +rt = g_get_real_time();
> +return to_bcd((rt % G_USEC_PER_SEC) / 1);
> case 1:
> return to_bcd(now.tm_sec);
> case 2:
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index e6c1b0aa46fe..b1bada84cecc 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -452,7 +452,6 @@ static void virtio_balloon_receive_stats(VirtIODevice
> *vdev, VirtQueue *vq)
> VirtQueueElement *elem;
> VirtIOBalloonStat stat;
> size_t offset = 0;
> -qemu_timeval tv;
>
> elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> if (!elem) {
> @@ -484,13 +483,7 @@ static void virtio_balloon_receive_stats(VirtIODevice
> *vdev, VirtQueue *vq)
> s->stats[tag] = val;
> }
> s->stats_vq_offset = offset;
> -
> -if (qemu_gettimeofday() < 0) {
> -warn_report("%s: failed to get time of day", __func__);
> -goto out;
> -}
> -
> -s->stats_last_update = tv.tv_sec;
> +s->stats_last_update = g_get_real_time() / G_USEC_PER_SEC;
>
> out:
> if (balloon_stats_enabled(s)) {
> diff --git a/qapi/qmp-event.c b/qapi/qmp-event.c
> index 19d3cd003833..025716b3ec37 100644
> --- a/qapi/qmp-event.c
> +++ b/qapi/qmp-event.c
> @@ -20,15 +20,13 @@
>
> static void timestamp_put(QDict *qdict)
> {
> -int err;
> QDict *ts;
> -qemu_timeval tv;
> +int64_t rt = g_get_real_time();
>
> -err = qemu_gettimeofday();
> -/* Put -1 to indicate failure of getting host time */
> -ts = qdict_from_jsonf_nofail("{ 'seconds': %lld, 'microseconds': %lld }",
> - err < 0 ? -1LL : (long long)tv.tv_sec,
> - err < 0 ? -1LL : (long long)tv.tv_usec);
> +ts = qdict_from_jsonf_nofail("{ 'seconds': %" G_GINT64_FORMAT
> + ", 'microseconds': %" G_GINT64_FORMAT "}",
> + rt / G_USEC_PER_SEC,
> + rt % G_USEC_PER_SEC);
NACK this, fixed in v3
> qdict_put(qdict, "timestamp", ts);
> }
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 6fe2466032f9..e26773909684 10
Re: [PATCH 3/4] Replace qemu_gettimeofday() with g_get_real_time()
Hi
On Sat, Mar 5, 2022 at 12:40 AM BALATON Zoltan wrote:
> On Fri, 4 Mar 2022, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau
> >
> > GLib g_get_real_time() is an alternative to gettimeofday().
> >
> > For semihosting, a few bits are lost on POSIX host, but this shouldn't
> > be a big concern.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > blockdev.c | 8
> > hw/rtc/m41t80.c| 6 +++---
> > hw/virtio/virtio-balloon.c | 9 +
> > qapi/qmp-event.c | 12 +---
> > qemu-img.c | 8
> > qga/commands-posix.c | 11 +--
> > target/m68k/m68k-semi.c| 22 ++
> > target/nios2/nios2-semi.c | 24 +++-
> > 8 files changed, 39 insertions(+), 61 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 42e098b458b1..4b07dbfbdefc 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1235,7 +1235,7 @@ static void
> internal_snapshot_prepare(BlkActionState *common,
> > BlockDriverState *bs;
> > QEMUSnapshotInfo old_sn, *sn;
> > bool ret;
> > -qemu_timeval tv;
> > +int64_t rt;
> > BlockdevSnapshotInternal *internal;
> > InternalSnapshotState *state;
> > AioContext *aio_context;
> > @@ -1305,9 +1305,9 @@ static void
> internal_snapshot_prepare(BlkActionState *common,
> > /* 3. take the snapshot */
> > sn = >sn;
> > pstrcpy(sn->name, sizeof(sn->name), name);
> > -qemu_gettimeofday();
> > -sn->date_sec = tv.tv_sec;
> > -sn->date_nsec = tv.tv_usec * 1000;
> > +rt = g_get_real_time();
> > +sn->date_sec = rt / G_USEC_PER_SEC;
> > +sn->date_nsec = (rt % G_USEC_PER_SEC) * 1000;
> > sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > if (replay_mode != REPLAY_MODE_NONE) {
> > sn->icount = replay_get_current_icount();
> > diff --git a/hw/rtc/m41t80.c b/hw/rtc/m41t80.c
> > index a00971a67e1c..e045c864bb44 100644
> > --- a/hw/rtc/m41t80.c
> > +++ b/hw/rtc/m41t80.c
>
> This part
>
> Acked-by: BALATON Zoltan
>
> but why don't you use g_get_current_time() instead which seems to be a
> more direct replacement for gettimeofday, then you don't have to do maths
> with G_USEC_PER_SEC.>
>
>
As per glib docs, "g_get_current_time has been deprecated since version
2.62 and should not be used in newly-written code. GTimeVal is not
year-2038-safe. Use g_get_real_time() instead."
thanks
> Regards.
> BALATON Zoltan
>
> > @@ -47,7 +47,7 @@ static uint8_t m41t80_recv(I2CSlave *i2c)
> > {
> > M41t80State *s = M41T80(i2c);
> > struct tm now;
> > -qemu_timeval tv;
> > +int64_t rt;
> >
> > if (s->addr < 0) {
> > s->addr = 0;
> > @@ -57,8 +57,8 @@ static uint8_t m41t80_recv(I2CSlave *i2c)
> > }
> > switch (s->addr++) {
> > case 0:
> > -qemu_gettimeofday();
> > -return to_bcd(tv.tv_usec / 1);
> > +rt = g_get_real_time();
> > +return to_bcd((rt % G_USEC_PER_SEC) / 1);
> > case 1:
> > return to_bcd(now.tm_sec);
> > case 2:
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index e6c1b0aa46fe..b1bada84cecc 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -452,7 +452,6 @@ static void
> virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> > VirtQueueElement *elem;
> > VirtIOBalloonStat stat;
> > size_t offset = 0;
> > -qemu_timeval tv;
> >
> > elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > if (!elem) {
> > @@ -484,13 +483,7 @@ static void
> virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> > s->stats[tag] = val;
> > }
> > s->stats_vq_offset = offset;
> > -
> > -if (qemu_gettimeofday() < 0) {
> > -warn_report("%s: failed to get time of day", __func__);
> > -goto out;
> > -}
> > -
> > -s->stats_last_update = tv.tv_sec;
> > +s->stats_last_update = g_get_real_time() / G_USEC_PER_SEC;
> >
> > out:
> > if (balloon_stats_enabled(s)) {
> > diff --git a/qapi/qmp-event.c b/qapi/qmp-event.c
> > index 19d3cd003833..025716b3ec37 100644
> > --- a/qapi/qmp-event.c
> > +++ b/qapi/qmp-event.c
> >
Re: [PATCH 3/4] Replace qemu_gettimeofday() with g_get_real_time()
Hi
On Fri, Mar 4, 2022 at 8:10 PM Laurent Vivier wrote:
> Le 04/03/2022 à 16:27, marcandre.lur...@redhat.com a écrit :
> > From: Marc-André Lureau
> >
> > GLib g_get_real_time() is an alternative to gettimeofday().
> >
> > For semihosting, a few bits are lost on POSIX host, but this shouldn't
> > be a big concern.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > blockdev.c | 8
> > hw/rtc/m41t80.c| 6 +++---
> > hw/virtio/virtio-balloon.c | 9 +
> > qapi/qmp-event.c | 12 +---
> > qemu-img.c | 8
> > qga/commands-posix.c | 11 +--
> > target/m68k/m68k-semi.c| 22 ++
> > target/nios2/nios2-semi.c | 24 +++-
> > 8 files changed, 39 insertions(+), 61 deletions(-)
> >
> ...
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 75dbaab68ea9..3311a4ca0167 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -138,16 +138,7 @@ void qmp_guest_shutdown(bool has_mode, const char
> *mode, Error **errp)
> >
> > int64_t qmp_guest_get_time(Error **errp)
> > {
> > - int ret;
> > - qemu_timeval tq;
> > -
> > - ret = qemu_gettimeofday();
> > - if (ret < 0) {
> > - error_setg_errno(errp, errno, "Failed to get time");
> > - return -1;
> > - }
> > -
> > - return tq.tv_sec * 10LL + tq.tv_usec * 1000;
> > +return g_get_real_time();
>
> now you return microseconds, before it was nanoseconds.
>
>
my bad, will fix it in v2, thanks for noticing!
> qga/qapi-schema.json:
>
> ##
> # @guest-get-time:
> #
> # Get the information about guest's System Time relative to
> # the Epoch of 1970-01-01 in UTC.
> #
> # Returns: Time in nanoseconds.
> #
> # Since: 1.5
> ##
> { 'command': 'guest-get-time',
>'returns': 'int' }
>
> Except this problem:
>
> Reviewed-by: Laurent Vivier
>
>
--
Marc-André Lureau
Re: [PATCH 02/10] chardev/char-socket: tcp_chr_recv: don't clobber errno
On Thu, Nov 11, 2021 at 7:38 PM Roman Kagan wrote:
> tcp_chr_recv communicates the specific error condition to the caller via
> errno. However, after setting it, it may call into some system calls or
> library functions which can clobber the errno.
>
> Avoid this by moving the errno assignment to the end of the function.
>
> Signed-off-by: Roman Kagan
>
Reviewed-by: Marc-André Lureau
---
> chardev/char-socket.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 836cfa0bc2..90054ce58c 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -346,13 +346,6 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf,
> size_t len)
> NULL);
> }
>
> -if (ret == QIO_CHANNEL_ERR_BLOCK) {
> -errno = EAGAIN;
> -ret = -1;
> -} else if (ret == -1) {
> -errno = EIO;
> -}
> -
> if (msgfds_num) {
> /* close and clean read_msgfds */
> for (i = 0; i < s->read_msgfds_num; i++) {
> @@ -381,6 +374,13 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf,
> size_t len)
> #endif
> }
>
> +if (ret == QIO_CHANNEL_ERR_BLOCK) {
> +errno = EAGAIN;
> +ret = -1;
> + } else if (ret == -1) {
> +errno = EIO;
> +}
> +
> return ret;
> }
>
> --
> 2.33.1
>
>
>
--
Marc-André Lureau
Re: [PATCH 03/10] chardev/char-socket: tcp_chr_sync_read: don't clobber errno
On Thu, Nov 11, 2021 at 7:36 PM Roman Kagan wrote:
> After the return from tcp_chr_recv, tcp_chr_sync_read calls into a
> function which eventually makes a system call and may clobber errno.
>
> Make a copy of errno right after tcp_chr_recv and restore the errno on
> return from tcp_chr_sync_read.
>
> Signed-off-by: Roman Kagan
>
Reviewed-by: Marc-André Lureau
---
> chardev/char-socket.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 90054ce58c..cf7f2ba65a 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -581,6 +581,7 @@ static int tcp_chr_sync_read(Chardev *chr, const
> uint8_t *buf, int len)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
> int size;
> +int saved_errno;
>
> if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
> return 0;
> @@ -588,6 +589,7 @@ static int tcp_chr_sync_read(Chardev *chr, const
> uint8_t *buf, int len)
>
> qio_channel_set_blocking(s->ioc, true, NULL);
> size = tcp_chr_recv(chr, (void *) buf, len);
> +saved_errno = errno;
> if (s->state != TCP_CHARDEV_STATE_DISCONNECTED) {
> qio_channel_set_blocking(s->ioc, false, NULL);
> }
> @@ -596,6 +598,7 @@ static int tcp_chr_sync_read(Chardev *chr, const
> uint8_t *buf, int len)
> tcp_chr_disconnect(chr);
> }
>
> +errno = saved_errno;
> return size;
> }
>
> --
> 2.33.1
>
>
>
--
Marc-André Lureau
Re: [PATCH 04/10] chardev/char-fe: don't allow EAGAIN from blocking read
Hi
On Thu, Nov 11, 2021 at 7:44 PM Roman Kagan wrote:
> As its name suggests, ChardevClass.chr_sync_read is supposed to do a
> blocking read. The only implementation of it, tcp_chr_sync_read, does
> set the underlying io channel to the blocking mode indeed.
>
> Therefore a failure return with EAGAIN is not expected from this call.
>
> So do not retry it in qemu_chr_fe_read_all; instead place an assertion
> that it doesn't fail with EAGAIN.
>
The code was introduced in :
commit 7b0bfdf52d694c9a3a96505aa42ce3f8d63acd35
Author: Nikolay Nikolaev
Date: Tue May 27 15:03:48 2014 +0300
Add chardev API qemu_chr_fe_read_all
Also touched later by Daniel in:
commit 53628efbc8aa7a7ab5354d24b971f4d69452151d
Author: Daniel P. Berrangé
Date: Thu Mar 31 16:29:27 2016 +0100
char: fix broken EAGAIN retry on OS-X due to errno clobbering
> Signed-off-by: Roman Kagan
> ---
> chardev/char-fe.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index 7789f7be9c..f94efe928e 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -68,13 +68,10 @@ int qemu_chr_fe_read_all(CharBackend *be, uint8_t
> *buf, int len)
> }
>
> while (offset < len) {
> -retry:
> res = CHARDEV_GET_CLASS(s)->chr_sync_read(s, buf + offset,
>len - offset);
> -if (res == -1 && errno == EAGAIN) {
> -g_usleep(100);
> -goto retry;
> -}
> +/* ->chr_sync_read should block */
> +assert(!(res < 0 && errno == EAGAIN));
>
>
While I agree with the rationale to clean this code a bit, I am not so sure
about replacing it with an assert(). In the past, when we did such things
we had unexpected regressions :)
A slightly better approach perhaps is g_warn_if_fail(), although it's not
very popular in qemu.
> if (res == 0) {
> break;
> --
> 2.33.1
>
>
>
--
Marc-André Lureau
Re: [PATCH 04/23] chardev/baum: Avoid dynamic stack allocation
On Thu, May 6, 2021 at 1:15 AM Philippe Mathieu-Daudé
wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
>
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Marc-André Lureau
---
> chardev/baum.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/baum.c b/chardev/baum.c
> index 0822e9ed5f3..bc09cda3471 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -299,7 +299,8 @@ static void baum_chr_accept_input(struct Chardev *chr)
> static void baum_write_packet(BaumChardev *baum, const uint8_t *buf, int
> len)
> {
> Chardev *chr = CHARDEV(baum);
> -uint8_t io_buf[1 + 2 * len], *cur = io_buf;
> +g_autofree uint8_t *io_buf = g_malloc(1 + 2 * len);
>
fwiw, for non-bottleneck code, I would simply use g_malloc0() everywhere,
ymmv
+uint8_t *cur = io_buf;
> int room;
> *cur++ = ESC;
> while (len--)
> --
> 2.26.3
>
>
>
--
Marc-André Lureau
Re: [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions
On Thu, May 6, 2021 at 1:13 AM Philippe Mathieu-Daudé
wrote:
> Replace '84' magic value by the X_MAX definition, and '1' by Y_MAX.
>
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Marc-André Lureau
> ---
> chardev/baum.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/chardev/baum.c b/chardev/baum.c
> index 5deca778bc4..adc3d7b3b56 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -87,6 +87,9 @@
>
> #define BUF_SIZE 256
>
> +#define X_MAX 84
> +#define Y_MAX 1
> +
> struct BaumChardev {
> Chardev parent;
>
> @@ -244,11 +247,11 @@ static int baum_deferred_init(BaumChardev *baum)
> brlapi_perror("baum: brlapi__getDisplaySize");
> return 0;
> }
> -if (baum->y > 1) {
> -baum->y = 1;
> +if (baum->y > Y_MAX) {
> +baum->y = Y_MAX;
> }
> -if (baum->x > 84) {
> -baum->x = 84;
> + if (baum->x > X_MAX) {
> +baum->x = X_MAX;
> }
>
> con = qemu_console_lookup_by_index(0);
> --
> 2.26.3
>
>
>
--
Marc-André Lureau
Re: [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation
On Thu, May 6, 2021 at 1:14 AM Philippe Mathieu-Daudé
wrote:
> We know 'x * y' will be at most 'X_MAX * Y_MAX' (which is not
> a big value, it is actually 84). Instead of having the compiler
> use variable-length array, declare an array able to hold the
> maximum 'x * y'.
>
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Marc-André Lureau
---
> chardev/baum.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/chardev/baum.c b/chardev/baum.c
> index adc3d7b3b56..0822e9ed5f3 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -383,9 +383,9 @@ static int baum_eat_packet(BaumChardev *baum, const
> uint8_t *buf, int len)
> switch (req) {
> case BAUM_REQ_DisplayData:
> {
> -uint8_t cells[baum->x * baum->y], c;
> -uint8_t text[baum->x * baum->y];
> -uint8_t zero[baum->x * baum->y];
> +uint8_t cells[X_MAX * Y_MAX], c;
> +uint8_t text[X_MAX * Y_MAX];
> +uint8_t zero[X_MAX * Y_MAX];
> int cursor = BRLAPI_CURSOR_OFF;
> int i;
>
> @@ -408,7 +408,7 @@ static int baum_eat_packet(BaumChardev *baum, const
> uint8_t *buf, int len)
> }
> timer_del(baum->cellCount_timer);
>
> -memset(zero, 0, sizeof(zero));
> +memset(zero, 0, baum->x * baum->y);
>
eh, I would have left the sizeof(zero)..
> brlapi_writeArguments_t wa = {
> .displayNumber = BRLAPI_DISPLAY_DEFAULT,
> --
> 2.26.3
>
>
>
--
Marc-André Lureau
Re: [PATCH 08/14] chardev: reject use of 'wait' flag for socket client chardevs
On Wed, Feb 24, 2021 at 5:15 PM Daniel P. Berrangé
wrote:
> This only makes sense conceptually when used with listener chardevs.
>
> Signed-off-by: Daniel P. Berrangé
>
Reviewed-by: Marc-André Lureau
---
> chardev/char-socket.c| 12
> docs/system/deprecated.rst | 6 --
> docs/system/removed-features.rst | 6 ++
> 3 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 9061981f6d..b24618b581 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1336,14 +1336,10 @@ static bool
> qmp_chardev_validate_socket(ChardevSocket *sock,
> return false;
> }
> if (sock->has_wait) {
> -warn_report("'wait' option is deprecated with "
> -"socket in client connect mode");
> -if (sock->wait) {
> -error_setg(errp, "%s",
> - "'wait' option is incompatible with "
> - "socket in client connect mode");
> -return false;
> -}
> +error_setg(errp, "%s",
> + "'wait' option is incompatible with "
> + "socket in client connect mode");
> +return false;
> }
> }
>
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 78474f0845..c69887dca8 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -192,12 +192,6 @@ Since the ``dirty-bitmaps`` field is optionally
> present in both the old and
> new locations, clients must use introspection to learn where to anticipate
> the field if/when it does appear in command output.
>
> -chardev client socket with ``wait`` option (since 4.0)
> -''
> -
> -Character devices creating sockets in client mode should not specify
> -the 'wait' field, which is only applicable to sockets in server mode
> -
> ``nbd-server-add`` and ``nbd-server-remove`` (since 5.2)
>
>
> diff --git a/docs/system/removed-features.rst
> b/docs/system/removed-features.rst
> index 7942c2e513..870a222062 100644
> --- a/docs/system/removed-features.rst
> +++ b/docs/system/removed-features.rst
> @@ -106,6 +106,12 @@ The ``query-cpus`` command is replaced by the
> ``query-cpus-fast`` command.
> The ``arch`` output member of the ``query-cpus-fast`` command is
> replaced by the ``target`` output member.
>
> +chardev client socket with ``wait`` option (removed in 6.0)
> +'''
> +
> +Character devices creating sockets in client mode should not specify
> +the 'wait' field, which is only applicable to sockets in server mode
> +
> Human Monitor Protocol (HMP) commands
> -
>
> --
> 2.29.2
>
>
Re: [PATCH v14 3/7] chardev/char-socket.c: Add yank feature
On Mon, Dec 28, 2020 at 7:08 PM Lukas Straub wrote:
> Register a yank function to shutdown the socket on yank.
>
> Signed-off-by: Lukas Straub
> Acked-by: Stefan Hajnoczi
>
Acked-by: Marc-André Lureau
---
> chardev/char-socket.c | 34 ++
> 1 file changed, 34 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 213a4c8dd0..8a707d766c 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -34,6 +34,7 @@
> #include "qapi/error.h"
> #include "qapi/clone-visitor.h"
> #include "qapi/qapi-visit-sockets.h"
> +#include "qemu/yank.h"
>
> #include "chardev/char-io.h"
> #include "qom/object.h"
> @@ -70,6 +71,7 @@ struct SocketChardev {
> size_t read_msgfds_num;
> int *write_msgfds;
> size_t write_msgfds_num;
> +bool registered_yank;
>
> SocketAddress *addr;
> bool is_listen;
> @@ -415,6 +417,12 @@ static void tcp_chr_free_connection(Chardev *chr)
>
> tcp_set_msgfds(chr, NULL, 0);
> remove_fd_in_watch(chr);
> +if (s->state == TCP_CHARDEV_STATE_CONNECTING
> +|| s->state == TCP_CHARDEV_STATE_CONNECTED) {
> +yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
> + yank_generic_iochannel,
> + QIO_CHANNEL(s->sioc));
> +}
> object_unref(OBJECT(s->sioc));
> s->sioc = NULL;
> object_unref(OBJECT(s->ioc));
> @@ -932,6 +940,9 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
> }
> tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
> tcp_chr_set_client_ioc_name(chr, sioc);
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> + yank_generic_iochannel,
> + QIO_CHANNEL(sioc));
> ret = tcp_chr_new_client(chr, sioc);
> object_unref(OBJECT(sioc));
> return ret;
> @@ -946,6 +957,9 @@ static void tcp_chr_accept(QIONetListener *listener,
>
> tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
> tcp_chr_set_client_ioc_name(chr, cioc);
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> + yank_generic_iochannel,
> + QIO_CHANNEL(cioc));
> tcp_chr_new_client(chr, cioc);
> }
>
> @@ -961,6 +975,9 @@ static int tcp_chr_connect_client_sync(Chardev *chr,
> Error **errp)
> object_unref(OBJECT(sioc));
> return -1;
> }
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> + yank_generic_iochannel,
> + QIO_CHANNEL(sioc));
> tcp_chr_new_client(chr, sioc);
> object_unref(OBJECT(sioc));
> return 0;
> @@ -976,6 +993,9 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
> tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
> sioc = qio_net_listener_wait_client(s->listener);
> tcp_chr_set_client_ioc_name(chr, sioc);
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> + yank_generic_iochannel,
> + QIO_CHANNEL(sioc));
> tcp_chr_new_client(chr, sioc);
> object_unref(OBJECT(sioc));
> }
> @@ -1086,6 +1106,9 @@ static void char_socket_finalize(Object *obj)
> object_unref(OBJECT(s->tls_creds));
> }
> g_free(s->tls_authz);
> +if (s->registered_yank) {
> +yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
> +}
>
> qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> }
> @@ -1101,6 +1124,9 @@ static void qemu_chr_socket_connected(QIOTask *task,
> void *opaque)
>
> if (qio_task_propagate_error(task, )) {
> tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
> +yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
> + yank_generic_iochannel,
> + QIO_CHANNEL(sioc));
> check_report_connect_error(chr, err);
> goto cleanup;
> }
> @@ -1134,6 +1160,9 @@ static void tcp_chr_connect_client_async(Chardev
> *chr)
> tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
> sioc = qio_channel_socket_new();
> tcp_chr_set_client_ioc_name(chr, sioc);
> +yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
> + yank_generic_iochannel,
> + QIO_CHANNEL(sioc));
> /*
> * Normally code would use the qio_channel_socket_connect_async
> * method which uses a QIOTask + qio_task_set_error internally
> @@ -1376,6 +1405,11 @@ static void qmp_chardev_open_socket(Chardev *chr,
> qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
> }
>
> +if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp))
> {
> +return;
> +}
> +s->registered_yank = true;
> +
> /* be isn't opened until we get a connection */
> *be_opened = false;
>
> --
> 2.29.2
>
>
--
Marc-André Lureau
Re: [PATCH v14 1/7] Introduce yank feature
On Tue, Jan 12, 2021 at 6:18 PM Markus Armbruster wrote: > Marc-André Lureau writes: > > > On Mon, Jan 11, 2021 at 5:34 PM Markus Armbruster > wrote: > > > >> Marc-André Lureau writes: > >> > >> > Hi > >> > > >> > On Mon, Dec 28, 2020 at 7:08 PM Lukas Straub > >> wrote: > >> > > >> >> The yank feature allows to recover from hanging qemu by "yanking" > >> >> at various parts. Other qemu systems can register themselves and > >> >> multiple yank functions. Then all yank functions for selected > >> >> instances can be called by the 'yank' out-of-band qmp command. > >> >> Available instances can be queried by a 'query-yank' oob command. > >> >> > >> > > >> > Looking at the changes and API usage, I wonder if it wouldn't have > been > >> > simpler to associate the yank function directly with the YankInstance > >> > (removing the need for register/unregister functions - tracking the > state > >> > left to the callback). Have you tried that approach? If not, I could > >> check > >> > if this idea would work. > >> > >> Considering we're at v14... would it make sense to commit the current > >> approach, then explore the alternative approach on top? > >> > >> > > works for me > > > >> If yes, is v14 committable as is? > >> > >> > > Acked-by: Marc-André Lureau > > Thanks! Does your ACK apply to the series, or just to PATCH 1? > I didn't look much at the rest of the series, but PATCH 3 too, I'll reply there. -- Marc-André Lureau
Re: [PATCH v14 1/7] Introduce yank feature
On Mon, Jan 11, 2021 at 5:34 PM Markus Armbruster wrote: > Marc-André Lureau writes: > > > Hi > > > > On Mon, Dec 28, 2020 at 7:08 PM Lukas Straub > wrote: > > > >> The yank feature allows to recover from hanging qemu by "yanking" > >> at various parts. Other qemu systems can register themselves and > >> multiple yank functions. Then all yank functions for selected > >> instances can be called by the 'yank' out-of-band qmp command. > >> Available instances can be queried by a 'query-yank' oob command. > >> > > > > Looking at the changes and API usage, I wonder if it wouldn't have been > > simpler to associate the yank function directly with the YankInstance > > (removing the need for register/unregister functions - tracking the state > > left to the callback). Have you tried that approach? If not, I could > check > > if this idea would work. > > Considering we're at v14... would it make sense to commit the current > approach, then explore the alternative approach on top? > > works for me If yes, is v14 committable as is? > > Acked-by: Marc-André Lureau -- Marc-André Lureau
Re: [PATCH v14 1/7] Introduce yank feature
l_ss.add(when: 'CONFIG_GIO', if_true: [files('dbus.c'), gio])
> + util_ss.add(files('yank.c'))
> endif
>
> if have_block
> diff --git a/util/yank.c b/util/yank.c
> new file mode 100644
> index 00..fc08f65209
> --- /dev/null
> +++ b/util/yank.c
> @@ -0,0 +1,207 @@
> +/*
> + * QEMU yank feature
> + *
> + * Copyright (c) Lukas Straub
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/thread.h"
> +#include "qemu/queue.h"
> +#include "qemu/lockable.h"
> +#include "qapi/qapi-commands-yank.h"
> +#include "qapi/qapi-visit-yank.h"
> +#include "qapi/clone-visitor.h"
> +#include "io/channel.h"
> +#include "qemu/yank.h"
> +
> +struct YankFuncAndParam {
> +YankFn *func;
> +void *opaque;
> +QLIST_ENTRY(YankFuncAndParam) next;
> +};
> +
> +struct YankInstanceEntry {
> +YankInstance *instance;
> +QLIST_HEAD(, YankFuncAndParam) yankfns;
> +QLIST_ENTRY(YankInstanceEntry) next;
> +};
> +
> +typedef struct YankFuncAndParam YankFuncAndParam;
> +typedef struct YankInstanceEntry YankInstanceEntry;
> +
> +/*
> + * This lock protects the yank_instance_list below. Because it's taken by
> + * OOB-capable commands, it must be "fast", i.e. it may only be held for a
> + * bounded, short time. See docs/devel/qapi-code-gen.txt for additional
> + * information.
> + */
> +static QemuMutex yank_lock;
> +
> +static QLIST_HEAD(, YankInstanceEntry) yank_instance_list
> += QLIST_HEAD_INITIALIZER(yank_instance_list);
> +
> +static bool yank_instance_equal(const YankInstance *a, const YankInstance
> *b)
> +{
> +if (a->type != b->type) {
> +return false;
> +}
> +
> +switch (a->type) {
> +case YANK_INSTANCE_TYPE_BLOCK_NODE:
> +return g_str_equal(a->u.block_node.node_name,
> + b->u.block_node.node_name);
> +
> +case YANK_INSTANCE_TYPE_CHARDEV:
> +return g_str_equal(a->u.chardev.id, b->u.chardev.id);
> +
> +case YANK_INSTANCE_TYPE_MIGRATION:
> +return true;
> +
> +default:
> +abort();
> +}
> +}
> +
> +static YankInstanceEntry *yank_find_entry(const YankInstance *instance)
> +{
> +YankInstanceEntry *entry;
> +
> +QLIST_FOREACH(entry, _instance_list, next) {
> +if (yank_instance_equal(entry->instance, instance)) {
> +return entry;
> +}
> +}
> +return NULL;
> +}
> +
> +bool yank_register_instance(const YankInstance *instance, Error **errp)
> +{
> +YankInstanceEntry *entry;
> +
> +QEMU_LOCK_GUARD(_lock);
> +
> +if (yank_find_entry(instance)) {
> +error_setg(errp, "duplicate yank instance");
> +return false;
> +}
> +
> +entry = g_new0(YankInstanceEntry, 1);
> +entry->instance = QAPI_CLONE(YankInstance, instance);
> +QLIST_INIT(>yankfns);
> +QLIST_INSERT_HEAD(_instance_list, entry, next);
> +
> +return true;
> +}
> +
> +void yank_unregister_instance(const YankInstance *instance)
> +{
> +YankInstanceEntry *entry;
> +
> +QEMU_LOCK_GUARD(_lock);
> +entry = yank_find_entry(instance);
> +assert(entry);
> +
> +assert(QLIST_EMPTY(>yankfns));
> +QLIST_REMOVE(entry, next);
> +qapi_free_YankInstance(entry->instance);
> +g_free(entry);
> +}
> +
> +void yank_register_function(const YankInstance *instance,
> +YankFn *func,
> +void *opaque)
> +{
> +YankInstanceEntry *entry;
> +YankFuncAndParam *func_entry;
> +
> +QEMU_LOCK_GUARD(_lock);
> +entry = yank_find_entry(instance);
> +assert(entry);
> +
> +func_entry = g_new0(YankFuncAndParam, 1);
> +func_entry->func = func;
> +func_entry->opaque = opaque;
> +
> +QLIST_INSERT_HEAD(>yankfns, func_entry, next);
> +}
> +
> +void yank_unregister_function(const YankInstance *instance,
> + YankFn *func,
> + void *opaque)
> +{
> +YankInstanceEntry *entry;
> +YankFuncAndParam *func_entry;
> +
> +QEMU_LOCK_GUARD(_lock);
> +entry = yank_find_entry(instance);
> +assert(entry);
> +
> +QLIST_FOREACH(func_entry, >yankfns, next) {
> + if (func_entry->func == func && func_entry->opaque == opaque) {
> +QLIST_REMOVE(func_entry, next);
> +g_free(func_entry);
> +return;
> +}
> +}
> +
> +abort();
> +}
> +
> +void yank_generic_iochannel(void *opaque)
> +{
> +QIOChannel *ioc = QIO_CHANNEL(opaque);
> +
> +qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +}
> +
> +void qmp_yank(YankInstanceList *instances,
> + Error **errp)
> +{
> +YankInstanceList *tail;
> +YankInstanceEntry *entry;
> +YankFuncAndParam *func_entry;
> +
> +QEMU_LOCK_GUARD(_lock);
> +for (tail = instances; tail; tail = tail->next) {
> +entry = yank_find_entry(tail->value);
> +if (!entry) {
> +error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, "Instance not
> found");
> +return;
> +}
> +}
> +for (tail = instances; tail; tail = tail->next) {
> +entry = yank_find_entry(tail->value);
> +assert(entry);
> +QLIST_FOREACH(func_entry, >yankfns, next) {
> +func_entry->func(func_entry->opaque);
> +}
> +}
> +}
> +
> +YankInstanceList *qmp_query_yank(Error **errp)
> +{
> +YankInstanceEntry *entry;
> +YankInstanceList *ret;
> +
> +ret = NULL;
> +
> +QEMU_LOCK_GUARD(_lock);
> +QLIST_FOREACH(entry, _instance_list, next) {
> +YankInstanceList *new_entry;
> +new_entry = g_new0(YankInstanceList, 1);
> +new_entry->value = QAPI_CLONE(YankInstance, entry->instance);
> +new_entry->next = ret;
> +ret = new_entry;
> +}
> +
> +return ret;
> +}
> +
> +static void __attribute__((__constructor__)) yank_init(void)
> +{
> +qemu_mutex_init(_lock);
> +}
> --
> 2.29.2
>
>
--
Marc-André Lureau
Re: [PATCH v12 1/7] Introduce yank feature
gt; +func_entry->func = func;
> +func_entry->opaque = opaque;
> +
> +QLIST_INSERT_HEAD(>yankfns, func_entry, next);
> +qemu_mutex_unlock(_lock);
> +}
> +
> +void yank_unregister_function(const YankInstance *instance,
> + YankFn *func,
> + void *opaque)
> +{
> +YankInstanceEntry *entry;
> +YankFuncAndParam *func_entry;
> +
> +qemu_mutex_lock(_lock);
>
+entry = yank_find_entry(instance);
> +assert(entry);
> +
> +QLIST_FOREACH(func_entry, >yankfns, next) {
> +if (func_entry->func == func && func_entry->opaque == opaque) {
> +QLIST_REMOVE(func_entry, next);
> +g_slice_free(YankFuncAndParam, func_entry);
> +qemu_mutex_unlock(_lock);
> +return;
> +}
> +}
> +
> +abort();
> +}
> +
> +void yank_generic_iochannel(void *opaque)
> +{
> +QIOChannel *ioc = QIO_CHANNEL(opaque);
> +
> +qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +}
> +
> +void qmp_yank(YankInstanceList *instances,
> + Error **errp)
> +{
> +YankInstanceList *tail;
> +YankInstanceEntry *entry;
> +YankFuncAndParam *func_entry;
> +
> +qemu_mutex_lock(_lock);
>
+for (tail = instances; tail; tail = tail->next) {
> +entry = yank_find_entry(tail->value);
> +if (!entry) {
> +error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, "Instance not
> found");
> +qemu_mutex_unlock(_lock);
> +return;
> +}
> +}
> +for (tail = instances; tail; tail = tail->next) {
> +entry = yank_find_entry(tail->value);
> +assert(entry);
> +QLIST_FOREACH(func_entry, >yankfns, next) {
> +func_entry->func(func_entry->opaque);
> +}
> +}
> +qemu_mutex_unlock(_lock);
> +}
> +
> +YankInstanceList *qmp_query_yank(Error **errp)
> +{
> +YankInstanceEntry *entry;
> +YankInstanceList *ret;
> +
> +ret = NULL;
> +
> +qemu_mutex_lock(_lock);
>
+QLIST_FOREACH(entry, _instance_list, next) {
> +YankInstanceList *new_entry;
> +new_entry = g_new0(YankInstanceList, 1);
> +new_entry->value = QAPI_CLONE(YankInstance, entry->instance);
> +new_entry->next = ret;
> +ret = new_entry;
> +}
> +qemu_mutex_unlock(_lock);
> +
> +return ret;
> +}
> +
> +static void __attribute__((__constructor__)) yank_init(void)
> +{
> +qemu_mutex_init(_lock);
> +}
> --
> 2.20.1
>
>
--
Marc-André Lureau
Re: [PATCH v2 0/4] vhost-user: avoid g_return_val_if() in get/set_config()
Hi Michael, On Thu, Dec 3, 2020 at 5:41 PM Stefano Garzarella wrote: > On Wed, Dec 02, 2020 at 03:26:07PM +, Stefan Hajnoczi wrote: > >v2: > > * Print errors [Marc-André] > > > >Markus Armbruster pointed out that g_return_val_if() is meant for > programming > >errors. It must not be used for input validation since it can be compiled > out. > >Use explicit if statements instead. > > > >This patch series converts vhost-user device backends that use > >g_return_val_if() in get/set_config(). > > > >Stefan Hajnoczi (4): > > contrib/vhost-user-blk: avoid g_return_val_if() input validation > > contrib/vhost-user-gpu: avoid g_return_val_if() input validation > > contrib/vhost-user-input: avoid g_return_val_if() input validation > > block/export: avoid g_return_val_if() input validation > > > > block/export/vhost-user-blk-server.c| 6 +- > > contrib/vhost-user-blk/vhost-user-blk.c | 6 +- > > contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +- > > contrib/vhost-user-input/main.c | 6 +- > > 4 files changed, 20 insertions(+), 4 deletions(-) > > > >-- > >2.28.0 > > > > Reviewed-by: Stefano Garzarella > > You didn't collect the v2 patch series, with the received reviewed-by. Not a big deal here, but please be more careful. thanks -- Marc-André Lureau
Re: [PATCH v2 2/4] contrib/vhost-user-gpu: avoid g_return_val_if() input validation
On Thu, Dec 3, 2020 at 1:52 PM Stefan Hajnoczi wrote:
> On Wed, Dec 02, 2020 at 07:50:51PM +0400, Marc-André Lureau wrote:
> > On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi
> wrote:
> >
> > > Do not validate input with g_return_val_if(). This API is intended for
> > > checking programming errors and is compiled out with
> -DG_DISABLE_CHECKS.
> > >
> > > Use an explicit if statement for input validation so it cannot
> > > accidentally be compiled out.
> > >
> > > Suggested-by: Markus Armbruster
> > > Signed-off-by: Stefan Hajnoczi
> > > ---
> > > contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > b/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > index a019d0a9ac..534bad24d1 100644
> > > --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> > > @@ -1044,7 +1044,11 @@ vg_get_config(VuDev *dev, uint8_t *config,
> uint32_t
> > > len)
> > > {
> > > VuGpu *g = container_of(dev, VuGpu, dev.parent);
> > >
> > > -g_return_val_if_fail(len <= sizeof(struct virtio_gpu_config), -1);
> > > +if (len > sizeof(struct virtio_gpu_config)) {
> > > +g_critical("%s: len %u is larger than %zu",
> > > + __func__, len, sizeof(struct virtio_gpu_config));
> > >
> >
> > g_critical() already has __FILE__ __LINE__ and G_STRFUNC.
>
> I did this for consistency with the logging in this source file. The
> other g_critical() calls in the file also print __func__.
>
>
>
I see, nevermind then. I gave rb anyway
--
Marc-André Lureau
Re: [PATCH v2 4/4] block/export: avoid g_return_val_if() input validation
On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi wrote:
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster
> Signed-off-by: Stefan Hajnoczi
>
Reviewed-by: Marc-André Lureau
---
> block/export/vhost-user-blk-server.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/block/export/vhost-user-blk-server.c
> b/block/export/vhost-user-blk-server.c
> index 62672d1cb9..bccbc98d57 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -267,7 +267,11 @@ vu_blk_get_config(VuDev *vu_dev, uint8_t *config,
> uint32_t len)
> VuServer *server = container_of(vu_dev, VuServer, vu_dev);
> VuBlkExport *vexp = container_of(server, VuBlkExport, vu_server);
>
> -g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
> +if (len > sizeof(struct virtio_blk_config)) {
> +error_report("Invalid get_config len %u, expected <= %zu",
> + len, sizeof(struct virtio_blk_config));
> + return -1;
> +}
>
> memcpy(config, >blkcfg, len);
> return 0;
> --
> 2.28.0
>
>
--
Marc-André Lureau
Re: [PATCH v2 3/4] contrib/vhost-user-input: avoid g_return_val_if() input validation
On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi wrote:
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster
> Signed-off-by: Stefan Hajnoczi
> ---
> contrib/vhost-user-input/main.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-input/main.c
> b/contrib/vhost-user-input/main.c
> index 6020c6f33a..1d79c61200 100644
> --- a/contrib/vhost-user-input/main.c
> +++ b/contrib/vhost-user-input/main.c
> @@ -212,7 +212,11 @@ static int vi_get_config(VuDev *dev, uint8_t *config,
> uint32_t len)
> {
> VuInput *vi = container_of(dev, VuInput, dev.parent);
>
> -g_return_val_if_fail(len <= sizeof(*vi->sel_config), -1);
> +if (len > sizeof(*vi->sel_config)) {
> +g_critical("%s: len %u is larger than %zu",
> + __func__, len, sizeof(*vi->sel_config));
> +return -1;
>
g_critical() already has __FILE__ __LINE__ and G_STRFUNC.
otherwise looks good:
Reviewed-by: Marc-André Lureau
> + }
>
> if (vi->sel_config) {
> memcpy(config, vi->sel_config, len);
> --
> 2.28.0
>
>
--
Marc-André Lureau
Re: [PATCH v2 2/4] contrib/vhost-user-gpu: avoid g_return_val_if() input validation
On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi wrote:
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster
> Signed-off-by: Stefan Hajnoczi
> ---
> contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> b/contrib/vhost-user-gpu/vhost-user-gpu.c
> index a019d0a9ac..534bad24d1 100644
> --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> @@ -1044,7 +1044,11 @@ vg_get_config(VuDev *dev, uint8_t *config, uint32_t
> len)
> {
> VuGpu *g = container_of(dev, VuGpu, dev.parent);
>
> -g_return_val_if_fail(len <= sizeof(struct virtio_gpu_config), -1);
> +if (len > sizeof(struct virtio_gpu_config)) {
> +g_critical("%s: len %u is larger than %zu",
> + __func__, len, sizeof(struct virtio_gpu_config));
>
g_critical() already has __FILE__ __LINE__ and G_STRFUNC.
otherwise looks good:
Reviewed-by: Marc-André Lureau
+return -1;
> +}
>
> if (opt_virgl) {
> g->virtio_config.num_capsets = vg_virgl_get_num_capsets();
> --
> 2.28.0
>
>
--
Marc-André Lureau
Re: [PATCH v2 1/4] contrib/vhost-user-blk: avoid g_return_val_if() input validation
On Wed, Dec 2, 2020 at 7:26 PM Stefan Hajnoczi wrote:
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster
> Signed-off-by: Stefan Hajnoczi
>
Reviewed-by: Marc-André Lureau
---
> contrib/vhost-user-blk/vhost-user-blk.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c
> b/contrib/vhost-user-blk/vhost-user-blk.c
> index dc981bf945..60e3c9ed37 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -404,7 +404,11 @@ vub_get_config(VuDev *vu_dev, uint8_t *config,
> uint32_t len)
> VugDev *gdev;
> VubDev *vdev_blk;
>
> -g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
> +if (len > sizeof(struct virtio_blk_config)) {
> +fprintf(stderr, "Invalid get_config len %u, expected <= %zu\n",
> +len, sizeof(struct virtio_blk_config));
> +return -1;
> +}
>
> gdev = container_of(vu_dev, VugDev, parent);
> vdev_blk = container_of(gdev, VubDev, parent);
> --
> 2.28.0
>
>
--
Marc-André Lureau
Re: [PATCH 0/4] vhost-user: avoid g_return_val_if() in get/set_config()
Hi On Wed, Nov 18, 2020 at 1:17 PM Stefan Hajnoczi wrote: > Markus Armbruster pointed out that g_return_val_if() is meant for > programming > errors. It must not be used for input validation since it can be compiled > out. > Use explicit if statements instead. > > This patch series converts vhost-user device backends that use > g_return_val_if() in get/set_config(). > > Stefan Hajnoczi (4): > contrib/vhost-user-blk: avoid g_return_val_if() input validation > contrib/vhost-user-gpu: avoid g_return_val_if() input validation > contrib/vhost-user-input: avoid g_return_val_if() input validation > block/export: avoid g_return_val_if() input validation > > The condition is the same for all the patches, checking the message config payload is large enough. Afaict, the value is set by the client, so it could be a runtime error, and thus explicit checking is required. Nevertheless, one nice thing about g_return* macros, is that it provides an error message when the condition fails, which helps. Could you replace it? (fwiw, I think g_return* macros are so convenient, I would simply make G_DISABLE_CHECKS forbidden and call it a day) -- Marc-André Lureau
Re: [PATCH 14/36] qdev: Move dev->realized check to qdev_property_set()
On Fri, Oct 30, 2020 at 2:10 AM Eduardo Habkost wrote: > Every single qdev property setter function manually checks > dev->realized. We can just check dev->realized inside > qdev_property_set() instead. > > The check is being added as a separate function > (qdev_prop_allow_set()) because it will become a callback later. > > Signed-off-by: Eduardo Habkost > nice Reviewed-by: Marc-André Lureau -- Marc-André Lureau
Re: [PATCH 09/36] qdev: Make qdev_get_prop_ptr() get Object* arg
On Fri, Oct 30, 2020 at 11:29 AM Marc-André Lureau <
marcandre.lur...@gmail.com> wrote:
>
>
> On Fri, Oct 30, 2020 at 2:07 AM Eduardo Habkost
> wrote:
>
>> Make the code more generic and not specific to TYPE_DEVICE.
>>
>> Signed-off-by: Eduardo Habkost
>>
>
> Nice cleanup!, but fails to build atm
>
> ../hw/block/xen-block.c:403:9: error: ‘dev’ undeclared (first use in this
> function); did you mean ‘vdev’?
> 403 | if (dev->realized) {
>
>
That seems to be the only issue though, so with that fixed:
Reviewed-by: Marc-André Lureau
--
Marc-André Lureau
Re: [PATCH 09/36] qdev: Make qdev_get_prop_ptr() get Object* arg
On Fri, Oct 30, 2020 at 2:07 AM Eduardo Habkost wrote:
> Make the code more generic and not specific to TYPE_DEVICE.
>
> Signed-off-by: Eduardo Habkost
>
Nice cleanup!, but fails to build atm
../hw/block/xen-block.c:403:9: error: ‘dev’ undeclared (first use in this
function); did you mean ‘vdev’?
403 | if (dev->realized) {
--
Marc-André Lureau
Re: [PATCH 3/6] qapi: Remove wrapper struct for simple unions
Hi On Fri, Oct 23, 2020 at 2:40 PM Marc-André Lureau < marcandre.lur...@gmail.com> wrote: > Hi > > On Fri, Oct 23, 2020 at 2:14 PM Kevin Wolf wrote: > >> Variants of simple unions are always contained in a wrapper object >> called 'data' that serves no real use. When mapping a QAPI object to the >> command line, this becomes very visible to users because they have to >> add one or more useless 'data.' to many option names. >> >> As a first step towards avoiding this painful CLI experience, this gets >> rid of the 'data' indirection internally: The QAPI parser doesn't use a >> wrapper object as the variant type any more, so the indirection is >> removed from the generated C types. As a result, the C type looks the >> same for flat and simple unions now. A large part of this patch is >> mechanical conversion of C code to remove the 'data' indirection. >> >> Conceptually, the important change is that variants can now have flat >> and wrapped representations. For a transitioning period, we'll allow >> variants to support both representations in a later patch. Eventually, >> the plan is to deprecate and remove wrapped representations entirely, >> unifying simple and flat unions. >> >> The externally visible interfaces stay unchanged: Visitors still expect >> the 'data' wrappers, and introspection still shows it. >> >> Signed-off-by: Kevin Wolf >> > > breaks the build for me: > > Other than that, I like the change, and it looks quite straightforward. I hope it's acceptable and we are not missing the reasons that extra data indirection was necessary. Having to support both flat and wrapped versions in the externally visible interfaces might be tricky though. -- Marc-André Lureau
Re: [PATCH 2/6] char: Factor out qemu_chr_print_types()
On Fri, Oct 23, 2020 at 2:14 PM Kevin Wolf wrote:
> We'll want to call the same from a non-QemuOpts code path.
>
> Signed-off-by: Kevin Wolf
> ---
> include/chardev/char.h | 1 +
> chardev/char.c | 17 +++--
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index db42f0a8c6..3b91645081 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -212,6 +212,7 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
> */
> void qemu_chr_be_event(Chardev *s, QEMUChrEvent event);
>
> +void qemu_chr_print_types(void);
> int qemu_chr_add_client(Chardev *s, int fd);
> Chardev *qemu_chr_find(const char *name);
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 78553125d3..028612c333 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -633,6 +633,16 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> Error **errp)
> return backend;
> }
>
> +void qemu_chr_print_types(void)
> +{
> +GString *str = g_string_new("");
>
Suggest g_auto here
+
> +chardev_name_foreach(help_string_append, str);
> +
> +qemu_printf("Available chardev backend types: %s\n", str->str);
> +g_string_free(str, true);
> +}
> +
> Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
> Error **errp)
> {
> @@ -644,12 +654,7 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
> GMainContext *context,
> char *bid = NULL;
>
> if (name && is_help_option(name)) {
> -GString *str = g_string_new("");
> -
> -chardev_name_foreach(help_string_append, str);
> -
> -qemu_printf("Available chardev backend types: %s\n", str->str);
> -g_string_free(str, true);
> +qemu_chr_print_types();
> return NULL;
> }
>
> --
> 2.28.0
>
>
>
anyway
Reviewed-by: Marc-André Lureau
--
Marc-André Lureau
Re: [PATCH 1/6] char/stdio: Fix QMP default for 'signal'
On Fri, Oct 23, 2020 at 2:14 PM Kevin Wolf wrote:
> Commit 02c4bdf1 tried to make signal=on the default for stdio chardevs
> except for '-serial mon:stdio', but it forgot about QMP and accidentally
> switched the QMP default from true (except for -nographic) to false
> (always). The documentation was kept unchanged and still describes the
> opposite of the old behaviour (which is an even older documentation
> bug).
>
> Fix all of this by making signal=true the default in ChardevStdio and
> documenting it as such.
>
> Fixes: 02c4bdf1d2ca8c02a9bae16398f260b5c08d08bf
> Signed-off-by: Kevin Wolf
>
Reviewed-by: Marc-André Lureau
---
> qapi/char.json | 3 +--
> chardev/char-stdio.c | 4 +---
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/qapi/char.json b/qapi/char.json
> index b4d66ec90b..43486d1daa 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -321,8 +321,7 @@
> # Configuration info for stdio chardevs.
> #
> # @signal: Allow signals (such as SIGINT triggered by ^C)
> -# be delivered to qemu. Default: true in -nographic mode,
> -# false otherwise.
> +# be delivered to qemu. Default: true.
> #
> # Since: 1.5
> ##
> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
> index 82eaebc1db..403da308c9 100644
> --- a/chardev/char-stdio.c
> +++ b/chardev/char-stdio.c
> @@ -112,9 +112,7 @@ static void qemu_chr_open_stdio(Chardev *chr,
>
> qemu_chr_open_fd(chr, 0, 1);
>
> -if (opts->has_signal) {
> -stdio_allow_signal = opts->signal;
> -}
> +stdio_allow_signal = !opts->has_signal || opts->signal;
> qemu_chr_set_echo_stdio(chr, false);
> }
> #endif
> --
> 2.28.0
>
>
>
--
Marc-André Lureau
Re: [PATCH 13/13] ssh: add GUri-based URI parsing
Hi
On Thu, Jul 23, 2020 at 4:33 PM Richard W.M. Jones
wrote:
> On Thu, Jul 09, 2020 at 11:42:34PM +0400, Marc-André Lureau wrote:
> > Signed-off-by: Marc-André Lureau
> > ---
> > block/ssh.c | 75 +
> > 1 file changed, 58 insertions(+), 17 deletions(-)
> >
> > diff --git a/block/ssh.c b/block/ssh.c
> > index c8f6ad79e3c..d2bc6277613 100644
> > --- a/block/ssh.c
> > +++ b/block/ssh.c
> > @@ -180,9 +180,37 @@ static void sftp_error_trace(BDRVSSHState *s, const
> char *op)
> >
> > static int parse_uri(const char *filename, QDict *options, Error **errp)
> > {
> > +g_autofree char *port_str = NULL;
> > +const char *scheme, *server, *path, *user, *key, *value;
> > +gint port;
> > +
> > +#ifdef HAVE_GLIB_GURI
> > +g_autoptr(GUri) uri = NULL;
> > +g_autoptr(GHashTable) params = NULL;
> > +g_autoptr(GError) err = NULL;
> > +GHashTableIter iter;
> > +
> > +uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, );
> > +if (!uri) {
> > +error_setg(errp, "Failed to parse SSH URI: %s", err->message);
> > +return -EINVAL;
> > +}
> > +
> > +params = g_uri_parse_params(g_uri_get_query(uri), -1,
> > +"&;", G_URI_PARAMS_NONE, );
> > +if (err) {
> > +error_report("Failed to parse SSH URI query: %s", err->message);
> > +return -EINVAL;
> > +}
> > +
> > +scheme = g_uri_get_scheme(uri);
> > +user = g_uri_get_user(uri);
> > +server = g_uri_get_host(uri);
> > +path = g_uri_get_path(uri);
> > +port = g_uri_get_port(uri);
> > +#else
> > g_autoptr(URI) uri = NULL;
> > g_autoptr(QueryParams) qp = NULL;
> > -g_autofree char *port_str = NULL;
> > int i;
>
> As Dan said already, this conditional code looks horrible and is going
> to be a maintenance burden. Was there a later version of this patch
> series that resolved this? I don't think I saw one.
>
The patch is quite experimental. glib didn't even yet receive a release
with GUri! But since I am working on the glib side, I wanted to make sure
it covers qemu needs.
I will revisit the series once GUri is frozen & released (in
mid-september),and use a copy version fallback.
Although, as I said in the cover, this is a bit riskier than having a
transition period with both the old libxml-based parser and glib-based one
for very recent distro.
Tbh, I think having both is not a big burden, because there is very low
activity around those functions. Iow, we are not spreading qemu with a lot
of extra conditionals, but only in very limited mostly static places.
> Rich.
>
>
> > uri = uri_parse(filename);
> > @@ -190,44 +218,57 @@ static int parse_uri(const char *filename, QDict
> *options, Error **errp)
> > return -EINVAL;
> > }
> >
> > -if (g_strcmp0(uri->scheme, "ssh") != 0) {
> > -error_setg(errp, "URI scheme must be 'ssh'");
> > +qp = query_params_parse(uri->query);
> > +if (!qp) {
> > +error_setg(errp, "could not parse query parameters");
> > return -EINVAL;
> > }
> >
> > -if (!uri->server || strcmp(uri->server, "") == 0) {
> > -error_setg(errp, "missing hostname in URI");
> > +scheme = uri->scheme;
> > +user = uri->user;
> > +server = uri->server;
> > +path = uri->path;
> > +port = uri->port;
> > +#endif
> > +if (g_strcmp0(scheme, "ssh") != 0) {
> > +error_setg(errp, "URI scheme must be 'ssh'");
> > return -EINVAL;
> > }
> >
> > -if (!uri->path || strcmp(uri->path, "") == 0) {
> > -error_setg(errp, "missing remote path in URI");
> > +if (!server || strcmp(server, "") == 0) {
> > +error_setg(errp, "missing hostname in URI");
> > return -EINVAL;
> > }
> >
> > -qp = query_params_parse(uri->query);
> > -if (!qp) {
> > -error_setg(errp, "could not parse query parameters");
> > +if (!path || strcmp(path, "") == 0) {
> > +error_setg(errp, "missing remote path in URI");
> > return -EINVAL;
> > }
> >
> > -if(uri->user && strcmp(uri->user, "") != 0) {
> > -qdict_put_str(options, &
[PATCH 11/13] nfs: add GUri-based URI parsing
Signed-off-by: Marc-André Lureau
---
block/nfs.c | 96 -
1 file changed, 65 insertions(+), 31 deletions(-)
diff --git a/block/nfs.c b/block/nfs.c
index 93d719551d2..0b24044535d 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -77,6 +77,31 @@ typedef struct NFSRPC {
static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
{
+const char *scheme, *server, *path, *key, *value;
+
+#ifdef HAVE_GLIB_GURI
+g_autoptr(GUri) uri = NULL;
+g_autoptr(GHashTable) params = NULL;
+g_autoptr(GError) err = NULL;
+GHashTableIter iter;
+
+uri = g_uri_parse(filename, G_URI_FLAGS_ENCODED_QUERY, );
+if (!uri) {
+error_setg(errp, "Failed to parse NFS URI: %s", err->message);
+return -EINVAL;
+}
+
+params = g_uri_parse_params(g_uri_get_query(uri), -1,
+"&;", G_URI_PARAMS_NONE, );
+if (err) {
+error_report("Failed to parse NFS URI query: %s", err->message);
+return -EINVAL;
+}
+
+scheme = g_uri_get_scheme(uri);
+server = g_uri_get_host(uri);
+path = g_uri_get_path(uri);
+#else
g_autoptr(URI) uri = NULL;
g_autoptr(QueryParams) qp = NULL;
int i;
@@ -86,58 +111,67 @@ static int nfs_parse_uri(const char *filename, QDict
*options, Error **errp)
error_setg(errp, "Invalid URI specified");
return -EINVAL;
}
-if (g_strcmp0(uri->scheme, "nfs") != 0) {
-error_setg(errp, "URI scheme must be 'nfs'");
+
+qp = query_params_parse(uri->query);
+if (!qp) {
+error_setg(errp, "could not parse query parameters");
return -EINVAL;
}
-if (!uri->server) {
-error_setg(errp, "missing hostname in URI");
+scheme = uri->scheme;
+server = uri->server;
+path = uri->path;
+#endif
+if (g_strcmp0(scheme, "nfs") != 0) {
+error_setg(errp, "URI scheme must be 'nfs'");
return -EINVAL;
}
-if (!uri->path) {
-error_setg(errp, "missing file path in URI");
+if (!server) {
+error_setg(errp, "missing hostname in URI");
return -EINVAL;
}
-qp = query_params_parse(uri->query);
-if (!qp) {
-error_setg(errp, "could not parse query parameters");
+if (!path) {
+error_setg(errp, "missing file path in URI");
return -EINVAL;
}
-qdict_put_str(options, "server.host", uri->server);
+qdict_put_str(options, "server.host", server);
qdict_put_str(options, "server.type", "inet");
-qdict_put_str(options, "path", uri->path);
+qdict_put_str(options, "path", path);
+#ifdef HAVE_GLIB_GURI
+g_hash_table_iter_init(, params);
+while (g_hash_table_iter_next(, (void **), (void **))) {
+#else
for (i = 0; i < qp->n; i++) {
+key = qp->p[i].name;
+value = qp->p[i].value;
+#endif
unsigned long long val;
-if (!qp->p[i].value) {
-error_setg(errp, "Value for NFS parameter expected: %s",
- qp->p[i].name);
+if (!value) {
+error_setg(errp, "Value for NFS parameter expected: %s", key);
return -EINVAL;
}
-if (parse_uint_full(qp->p[i].value, , 0)) {
-error_setg(errp, "Illegal value for NFS parameter: %s",
- qp->p[i].name);
+if (parse_uint_full(value, , 0)) {
+error_setg(errp, "Illegal value for NFS parameter: %s", key);
return -EINVAL;
}
-if (!strcmp(qp->p[i].name, "uid")) {
-qdict_put_str(options, "user", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "gid")) {
-qdict_put_str(options, "group", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
-qdict_put_str(options, "tcp-syn-count", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "readahead")) {
-qdict_put_str(options, "readahead-size", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "pagecache")) {
-qdict_put_str(options, "page-cache-size", qp->p[i].value);
-} else if (!strcmp(qp->p[i].name, "debug")) {
-qdict_put_str(options, "debug", qp->p[i].value);
+if (!strcmp(key, "uid")) {
+qdict_put_str(options, "user", value);
+} else if (!strcmp(key, "gid")) {
+qdict_put_str(options, "group", value);
+} else if (!strcmp(key, "tcp-syncnt&q
