* Re: [Qemu-devel] [PATCH 1.3] qapi: handle visitor->type_size() in QapiDeallocVisitor
2012-11-26 12:10 [Qemu-devel] [PATCH 1.3] qapi: handle visitor->type_size() in QapiDeallocVisitor Stefan Hajnoczi
@ 2012-11-26 12:43 ` Andreas Färber
2012-11-26 13:22 ` Stefan Hajnoczi
2012-11-26 15:44 ` mdroth
2012-11-26 21:47 ` Anthony Liguori
2 siblings, 1 reply; 6+ messages in thread
From: Andreas Färber @ 2012-11-26 12:43 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: 1079713, Anthony Liguori, qemu-devel, Michael Roth
Am 26.11.2012 13:10, schrieb Stefan Hajnoczi:
> visit_type_size() requires either visitor->type_size() or
> visitor_uint64() to be implemented, otherwise a NULL function pointer is
> invoked.
>
> It is possible to trigger this crash as follows:
>
> $ qemu-system-x86_64 -netdev tap,sndbuf=0,id=netdev0 \
> -device virtio-blk-pci,netdev=netdev0
>
> The 'sndbuf' option has type "size".
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> This patch ensures that -netdev tap,sndbuf=X works in QEMU 1.3.
Reviewed-by: Andreas Färber <afaerber@suse.de>
Did you check whether any other types were unhandled?
Should a comment be added somewhere along the lines of "If you add a
hook here you also need to implement one there" to avoid such
inconsistency for the future?
Andreas
>
> qapi/qapi-dealloc-visitor.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index a154523..a07b171 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -132,6 +132,11 @@ static void qapi_dealloc_type_number(Visitor *v, double *obj, const char *name,
> {
> }
>
> +static void qapi_dealloc_type_size(Visitor *v, size_t *obj, const char *name,
> + Error **errp)
> +{
> +}
> +
> static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[],
> const char *kind, const char *name,
> Error **errp)
> @@ -164,6 +169,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
> v->visitor.type_bool = qapi_dealloc_type_bool;
> v->visitor.type_str = qapi_dealloc_type_str;
> v->visitor.type_number = qapi_dealloc_type_number;
> + v->visitor.type_size = qapi_dealloc_type_size;
>
> QTAILQ_INIT(&v->stack);
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1.3] qapi: handle visitor->type_size() in QapiDeallocVisitor
2012-11-26 12:43 ` Andreas Färber
@ 2012-11-26 13:22 ` Stefan Hajnoczi
2012-11-26 15:43 ` mdroth
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2012-11-26 13:22 UTC (permalink / raw)
To: Andreas Färber
Cc: 1079713, Anthony Liguori, qemu-devel, Stefan Hajnoczi,
Michael Roth
On Mon, Nov 26, 2012 at 1:43 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 26.11.2012 13:10, schrieb Stefan Hajnoczi:
>> visit_type_size() requires either visitor->type_size() or
>> visitor_uint64() to be implemented, otherwise a NULL function pointer is
>> invoked.
>>
>> It is possible to trigger this crash as follows:
>>
>> $ qemu-system-x86_64 -netdev tap,sndbuf=0,id=netdev0 \
>> -device virtio-blk-pci,netdev=netdev0
>>
>> The 'sndbuf' option has type "size".
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>> This patch ensures that -netdev tap,sndbuf=X works in QEMU 1.3.
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> Did you check whether any other types were unhandled?
The visitors do not handle all types. Only the opts visitor and now
the dealloc visitor handle ->type_size().
This will not cause a problem yet because only the netdev options
include fields with the 'size' type. That code path is now covered.
In the longer term we should clean up the int, number, uint64, size
type proliferation and handle them consistently.
> Should a comment be added somewhere along the lines of "If you add a
> hook here you also need to implement one there" to avoid such
> inconsistency for the future?
There is no single point like register_visitor() where we could check
that callbacks are set up. That would have been a nice way to prevent
incomplete visitors.
The issue is that qapi/qapi-visit-core.h says type_uint64 and
type_size may be NULL, but it documents that visit_type_size() falls
back to type_uint64() if type_size() is NULL. The case we hit was
that type_uint64() is also NULL. Should it fall back to type_int()
(int64_t)?
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1.3] qapi: handle visitor->type_size() in QapiDeallocVisitor
2012-11-26 13:22 ` Stefan Hajnoczi
@ 2012-11-26 15:43 ` mdroth
0 siblings, 0 replies; 6+ messages in thread
From: mdroth @ 2012-11-26 15:43 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: 1079713, Anthony Liguori, Andreas Färber, Stefan Hajnoczi,
qemu-devel
On Mon, Nov 26, 2012 at 02:22:58PM +0100, Stefan Hajnoczi wrote:
> On Mon, Nov 26, 2012 at 1:43 PM, Andreas Färber <afaerber@suse.de> wrote:
> > Am 26.11.2012 13:10, schrieb Stefan Hajnoczi:
> >> visit_type_size() requires either visitor->type_size() or
> >> visitor_uint64() to be implemented, otherwise a NULL function pointer is
> >> invoked.
> >>
> >> It is possible to trigger this crash as follows:
> >>
> >> $ qemu-system-x86_64 -netdev tap,sndbuf=0,id=netdev0 \
> >> -device virtio-blk-pci,netdev=netdev0
> >>
> >> The 'sndbuf' option has type "size".
> >>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >> This patch ensures that -netdev tap,sndbuf=X works in QEMU 1.3.
> >
> > Reviewed-by: Andreas Färber <afaerber@suse.de>
> >
> > Did you check whether any other types were unhandled?
>
> The visitors do not handle all types. Only the opts visitor and now
> the dealloc visitor handle ->type_size().
>
> This will not cause a problem yet because only the netdev options
> include fields with the 'size' type. That code path is now covered.
>
> In the longer term we should clean up the int, number, uint64, size
> type proliferation and handle them consistently.
Dealloc visitor is somewhat of a special case, as it only cares about
the underlying C type and not about the visitor-specific representation.
In the case of generated types like these, it only needs to match the
type that QAPI generates (uint64_t in the case of type_size).
For input/output visitors, new types should either have a compatible
fallback, or abort if there's no compatible fallback and we attempt to
use a visitor that doesn't support the type. AFAIK we don't have one
that falls into the latter category atm (there is one in the QIDL series
for native C arrays, which has no generic fallback and will abort in
cases where we use a visitor that doesn't implement that type
explicitly).
Dealloc shouldn't use compatibility fallbacks though, which is probably
something we need to clean up. It should have explicit implementations
for all types we introduce, and those implementations should match the
type use for QAPI-generated types.
>
> > Should a comment be added somewhere along the lines of "If you add a
> > hook here you also need to implement one there" to avoid such
> > inconsistency for the future?
>
> There is no single point like register_visitor() where we could check
> that callbacks are set up. That would have been a nice way to prevent
> incomplete visitors.
>
> The issue is that qapi/qapi-visit-core.h says type_uint64 and
> type_size may be NULL, but it documents that visit_type_size() falls
> back to type_uint64() if type_size() is NULL. The case we hit was
> that type_uint64() is also NULL. Should it fall back to type_int()
> (int64_t)?
I hit this issue implementing a test case for QIDL that introduces
the usage of type_size() for QmpOutputVisitor. The problem is that it
references ->type_uint64() directly instead of using visit_type_uint64()
which has the fallback handling (->type_int()) for visitors that don't
have a specific implementation for type_uint64.
I have a patch in the QIDL series that does this, and this would also fix the
issue you're hitting with the dealloc visitor, but as noted above I
think relying on fallbacks for the dealloc visitor is the wrong
approach, so I think we should treat these as 2 seperate issues and take
your patch for 1.3. The error case I hit isn't reachable in 1.3 atm so I
think that should be sufficient for now.
>
> Stefan
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1.3] qapi: handle visitor->type_size() in QapiDeallocVisitor
2012-11-26 12:10 [Qemu-devel] [PATCH 1.3] qapi: handle visitor->type_size() in QapiDeallocVisitor Stefan Hajnoczi
2012-11-26 12:43 ` Andreas Färber
@ 2012-11-26 15:44 ` mdroth
2012-11-26 21:47 ` Anthony Liguori
2 siblings, 0 replies; 6+ messages in thread
From: mdroth @ 2012-11-26 15:44 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: 1079713, Anthony Liguori, qemu-devel
On Mon, Nov 26, 2012 at 01:10:12PM +0100, Stefan Hajnoczi wrote:
> visit_type_size() requires either visitor->type_size() or
> visitor_uint64() to be implemented, otherwise a NULL function pointer is
> invoked.
>
> It is possible to trigger this crash as follows:
>
> $ qemu-system-x86_64 -netdev tap,sndbuf=0,id=netdev0 \
> -device virtio-blk-pci,netdev=netdev0
>
> The 'sndbuf' option has type "size".
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> This patch ensures that -netdev tap,sndbuf=X works in QEMU 1.3.
>
> qapi/qapi-dealloc-visitor.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index a154523..a07b171 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -132,6 +132,11 @@ static void qapi_dealloc_type_number(Visitor *v, double *obj, const char *name,
> {
> }
>
> +static void qapi_dealloc_type_size(Visitor *v, size_t *obj, const char *name,
> + Error **errp)
> +{
> +}
> +
> static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[],
> const char *kind, const char *name,
> Error **errp)
> @@ -164,6 +169,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
> v->visitor.type_bool = qapi_dealloc_type_bool;
> v->visitor.type_str = qapi_dealloc_type_str;
> v->visitor.type_number = qapi_dealloc_type_number;
> + v->visitor.type_size = qapi_dealloc_type_size;
>
> QTAILQ_INIT(&v->stack);
>
> --
> 1.8.0
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1.3] qapi: handle visitor->type_size() in QapiDeallocVisitor
2012-11-26 12:10 [Qemu-devel] [PATCH 1.3] qapi: handle visitor->type_size() in QapiDeallocVisitor Stefan Hajnoczi
2012-11-26 12:43 ` Andreas Färber
2012-11-26 15:44 ` mdroth
@ 2012-11-26 21:47 ` Anthony Liguori
2 siblings, 0 replies; 6+ messages in thread
From: Anthony Liguori @ 2012-11-26 21:47 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: 1079713, Michael Roth
Stefan Hajnoczi <stefanha@redhat.com> writes:
> visit_type_size() requires either visitor->type_size() or
> visitor_uint64() to be implemented, otherwise a NULL function pointer is
> invoked.
>
> It is possible to trigger this crash as follows:
>
> $ qemu-system-x86_64 -netdev tap,sndbuf=0,id=netdev0 \
> -device virtio-blk-pci,netdev=netdev0
>
> The 'sndbuf' option has type "size".
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> This patch ensures that -netdev tap,sndbuf=X works in QEMU 1.3.
>
Applied. Thanks.
Regards,
Anthony Liguori
> qapi/qapi-dealloc-visitor.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index a154523..a07b171 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -132,6 +132,11 @@ static void qapi_dealloc_type_number(Visitor *v, double *obj, const char *name,
> {
> }
>
> +static void qapi_dealloc_type_size(Visitor *v, size_t *obj, const char *name,
> + Error **errp)
> +{
> +}
> +
> static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[],
> const char *kind, const char *name,
> Error **errp)
> @@ -164,6 +169,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
> v->visitor.type_bool = qapi_dealloc_type_bool;
> v->visitor.type_str = qapi_dealloc_type_str;
> v->visitor.type_number = qapi_dealloc_type_number;
> + v->visitor.type_size = qapi_dealloc_type_size;
>
> QTAILQ_INIT(&v->stack);
>
> --
> 1.8.0
^ permalink raw reply [flat|nested] 6+ messages in thread