The qemu api claims to be easier to use, and the resulting code seems to
agree.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
qapi/string-input-visitor.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index b3fdd0827d..c1454f999f 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -20,6 +20,7 @@
#include "qemu/option.h"
#include "qemu/queue.h"
#include "qemu/range.h"
+#include "qemu/cutils.h"
struct StringInputVisitor
@@ -46,10 +47,10 @@ static void free_range(void *range, void *dummy)
static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
{
- char *str = (char *) siv->string;
- long long start, end;
+ const char *str = (char *) siv->string;
+ const char *endptr;
+ int64_t start, end;
Range *cur;
- char *endptr;
if (siv->ranges) {
return 0;
@@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
}
do {
- errno = 0;
- start = strtoll(str, &endptr, 0);
- if (errno == 0 && endptr > str) {
+ if (!qemu_strtoi64(str, &endptr, 0, &start)) {
if (*endptr == '\0') {
cur = g_malloc0(sizeof(*cur));
range_set_bounds(cur, start, start);
@@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
str = NULL;
} else if (*endptr == '-') {
str = endptr + 1;
- errno = 0;
- end = strtoll(str, &endptr, 0);
- if (errno == 0 && endptr > str && start <= end &&
- (start > INT64_MAX - 65536 ||
- end < start + 65536)) {
+ if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
if (*endptr == '\0') {
cur = g_malloc0(sizeof(*cur));
range_set_bounds(cur, start, end);
--
2.17.1
On Tue, Oct 23, 2018 at 05:23:00PM +0200, David Hildenbrand wrote:
> The qemu api claims to be easier to use, and the resulting code seems to
> agree.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> qapi/string-input-visitor.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index b3fdd0827d..c1454f999f 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -20,6 +20,7 @@
> #include "qemu/option.h"
> #include "qemu/queue.h"
> #include "qemu/range.h"
> +#include "qemu/cutils.h"
>
>
> struct StringInputVisitor
> @@ -46,10 +47,10 @@ static void free_range(void *range, void *dummy)
>
> static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
> {
> - char *str = (char *) siv->string;
> - long long start, end;
> + const char *str = (char *) siv->string;
> + const char *endptr;
> + int64_t start, end;
> Range *cur;
> - char *endptr;
>
> if (siv->ranges) {
> return 0;
> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
> }
>
> do {
> - errno = 0;
> - start = strtoll(str, &endptr, 0);
> - if (errno == 0 && endptr > str) {
> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
> if (*endptr == '\0') {
> cur = g_malloc0(sizeof(*cur));
> range_set_bounds(cur, start, start);
> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
> str = NULL;
> } else if (*endptr == '-') {
> str = endptr + 1;
> - errno = 0;
> - end = strtoll(str, &endptr, 0);
> - if (errno == 0 && endptr > str && start <= end &&
> - (start > INT64_MAX - 65536 ||
> - end < start + 65536)) {
> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
> if (*endptr == '\0') {
> cur = g_malloc0(sizeof(*cur));
> range_set_bounds(cur, start, end);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
David Hildenbrand <david@redhat.com> writes:
> The qemu api claims to be easier to use, and the resulting code seems to
> agree.
Ah, an opportunity to nitpick spelling! "The QEMU API", and "qapi: Use
qemu_strtoi64() ..."
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> qapi/string-input-visitor.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index b3fdd0827d..c1454f999f 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -20,6 +20,7 @@
> #include "qemu/option.h"
> #include "qemu/queue.h"
> #include "qemu/range.h"
> +#include "qemu/cutils.h"
>
>
> struct StringInputVisitor
> @@ -46,10 +47,10 @@ static void free_range(void *range, void *dummy)
>
> static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
> {
> - char *str = (char *) siv->string;
> - long long start, end;
> + const char *str = (char *) siv->string;
And an opportunity to nitpick whitespace! Drop the space between (char
*) and siv->string while there.
> + const char *endptr;
> + int64_t start, end;
> Range *cur;
> - char *endptr;
>
> if (siv->ranges) {
> return 0;
> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
> }
>
> do {
> - errno = 0;
> - start = strtoll(str, &endptr, 0);
> - if (errno == 0 && endptr > str) {
> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
> if (*endptr == '\0') {
> cur = g_malloc0(sizeof(*cur));
> range_set_bounds(cur, start, start);
> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
> str = NULL;
> } else if (*endptr == '-') {
> str = endptr + 1;
> - errno = 0;
> - end = strtoll(str, &endptr, 0);
> - if (errno == 0 && endptr > str && start <= end &&
> - (start > INT64_MAX - 65536 ||
> - end < start + 65536)) {
> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
explain that to me? I'm feeling particularly dense today...
> if (*endptr == '\0') {
> cur = g_malloc0(sizeof(*cur));
> range_set_bounds(cur, start, end);
On 31.10.18 15:40, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> The qemu api claims to be easier to use, and the resulting code seems to
>> agree.
>
> Ah, an opportunity to nitpick spelling! "The QEMU API", and "qapi: Use
> qemu_strtoi64() ..."
Whatever floats your boat ;) Will change.
>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> qapi/string-input-visitor.c | 17 ++++++-----------
>> 1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index b3fdd0827d..c1454f999f 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -20,6 +20,7 @@
>> #include "qemu/option.h"
>> #include "qemu/queue.h"
>> #include "qemu/range.h"
>> +#include "qemu/cutils.h"
>>
>>
>> struct StringInputVisitor
>> @@ -46,10 +47,10 @@ static void free_range(void *range, void *dummy)
>>
>> static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>> {
>> - char *str = (char *) siv->string;
>> - long long start, end;
>> + const char *str = (char *) siv->string;
>
> And an opportunity to nitpick whitespace! Drop the space between (char
> *) and siv->string while there.
Shouldn't checkpatch complain, too? Will fix.
>
>> + const char *endptr;
>> + int64_t start, end;
>> Range *cur;
>> - char *endptr;
>>
>> if (siv->ranges) {
>> return 0;
>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>> }
>>
>> do {
>> - errno = 0;
>> - start = strtoll(str, &endptr, 0);
>> - if (errno == 0 && endptr > str) {
>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>> if (*endptr == '\0') {
>> cur = g_malloc0(sizeof(*cur));
>> range_set_bounds(cur, start, start);
>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>> str = NULL;
>> } else if (*endptr == '-') {
>> str = endptr + 1;
>> - errno = 0;
>> - end = strtoll(str, &endptr, 0);
>> - if (errno == 0 && endptr > str && start <= end &&
>> - (start > INT64_MAX - 65536 ||
>> - end < start + 65536)) {
>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>
> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
> explain that to me? I'm feeling particularly dense today...
qemu_strtoi64 performs all different kinds of error handling completely
internally. This old code here was an attempt to filter out -EWHATEVER
from the response. No longer needed as errors and the actual value are
reported via different ways.
Thanks!
>
>> if (*endptr == '\0') {
>> cur = g_malloc0(sizeof(*cur));
>> range_set_bounds(cur, start, end);
--
Thanks,
David / dhildenb
David Hildenbrand <david@redhat.com> writes:
> On 31.10.18 15:40, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> The qemu api claims to be easier to use, and the resulting code seems to
>>> agree.
[...]
>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>> }
>>>
>>> do {
>>> - errno = 0;
>>> - start = strtoll(str, &endptr, 0);
>>> - if (errno == 0 && endptr > str) {
>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>> if (*endptr == '\0') {
>>> cur = g_malloc0(sizeof(*cur));
>>> range_set_bounds(cur, start, start);
>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>> str = NULL;
>>> } else if (*endptr == '-') {
>>> str = endptr + 1;
>>> - errno = 0;
>>> - end = strtoll(str, &endptr, 0);
>>> - if (errno == 0 && endptr > str && start <= end &&
>>> - (start > INT64_MAX - 65536 ||
>>> - end < start + 65536)) {
>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>>
>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
>> explain that to me? I'm feeling particularly dense today...
>
> qemu_strtoi64 performs all different kinds of error handling completely
> internally. This old code here was an attempt to filter out -EWHATEVER
> from the response. No longer needed as errors and the actual value are
> reported via different ways.
I understand why errno == 0 && endptr > str go away. They also do in
the previous hunk.
The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
unobvious. What does it do before the patch?
The condition goes back to commit 659268ffbff, which predates my watch
as maintainer. Its commit message is of no particular help. Its code
is... allright, the less I say about that, the better.
We're parsing a range here. We already parsed its lower bound into
@start (and guarded against errors), and its upper bound into @end (and
guarded against errors).
If the condition you delete is false, we goto error. So the condition
is about range validity. I figure it's an attempt to require valid
ranges to be no "wider" than 65535. The second part end < start + 65536
checks exactly that, except shit happens when start + 65536 overflows.
The first part attempts to guard against that, but
(1) INT64_MAX is *wrong*, because we compute in long long, and
(2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
WTF?!?
Unless I'm mistaken, the condition is not about handling any of the
errors that qemu_strtoi64() handles for us.
The easiest way for you out of this morass is probably to keep the
condition exactly as it was, then use the "my patch doesn't make things
any worse" get-out-of-jail-free card.
On 31.10.18 18:55, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> On 31.10.18 15:40, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>> agree.
> [...]
>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>> }
>>>>
>>>> do {
>>>> - errno = 0;
>>>> - start = strtoll(str, &endptr, 0);
>>>> - if (errno == 0 && endptr > str) {
>>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>>> if (*endptr == '\0') {
>>>> cur = g_malloc0(sizeof(*cur));
>>>> range_set_bounds(cur, start, start);
>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>> str = NULL;
>>>> } else if (*endptr == '-') {
>>>> str = endptr + 1;
>>>> - errno = 0;
>>>> - end = strtoll(str, &endptr, 0);
>>>> - if (errno == 0 && endptr > str && start <= end &&
>>>> - (start > INT64_MAX - 65536 ||
>>>> - end < start + 65536)) {
>>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>>>
>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
>>> explain that to me? I'm feeling particularly dense today...
>>
>> qemu_strtoi64 performs all different kinds of error handling completely
>> internally. This old code here was an attempt to filter out -EWHATEVER
>> from the response. No longer needed as errors and the actual value are
>> reported via different ways.
>
> I understand why errno == 0 && endptr > str go away. They also do in
> the previous hunk.
>
> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
> unobvious. What does it do before the patch?
>
> The condition goes back to commit 659268ffbff, which predates my watch
> as maintainer. Its commit message is of no particular help. Its code
> is... allright, the less I say about that, the better.
>
> We're parsing a range here. We already parsed its lower bound into
> @start (and guarded against errors), and its upper bound into @end (and
> guarded against errors).
>
> If the condition you delete is false, we goto error. So the condition
> is about range validity. I figure it's an attempt to require valid
> ranges to be no "wider" than 65535. The second part end < start + 65536
> checks exactly that, except shit happens when start + 65536 overflows.
> The first part attempts to guard against that, but
>
> (1) INT64_MAX is *wrong*, because we compute in long long, and
>
> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>
> WTF?!?
>
> Unless I'm mistaken, the condition is not about handling any of the
> errors that qemu_strtoi64() handles for us.
>
> The easiest way for you out of this morass is probably to keep the
> condition exactly as it was, then use the "my patch doesn't make things
> any worse" get-out-of-jail-free card.
>
Looking at the code in qapi/string-output-visitor.c related to range and
list handling I feel like using the get-out-of-jail-free card to get out
of qapi code now :) Too much magic in that code and too little time for
me to understand it all.
Thanks for your time and review anyway. My time is better invested in
other parts of QEMU. I will drop both patches from this series.
--
Thanks,
David / dhildenb
David Hildenbrand <david@redhat.com> writes:
> On 31.10.18 18:55, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>> agree.
>> [...]
>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>> }
>>>>>
>>>>> do {
>>>>> - errno = 0;
>>>>> - start = strtoll(str, &endptr, 0);
>>>>> - if (errno == 0 && endptr > str) {
>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>>>> if (*endptr == '\0') {
>>>>> cur = g_malloc0(sizeof(*cur));
>>>>> range_set_bounds(cur, start, start);
>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>> str = NULL;
>>>>> } else if (*endptr == '-') {
>>>>> str = endptr + 1;
>>>>> - errno = 0;
>>>>> - end = strtoll(str, &endptr, 0);
>>>>> - if (errno == 0 && endptr > str && start <= end &&
>>>>> - (start > INT64_MAX - 65536 ||
>>>>> - end < start + 65536)) {
>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>>>>
>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
>>>> explain that to me? I'm feeling particularly dense today...
>>>
>>> qemu_strtoi64 performs all different kinds of error handling completely
>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>> from the response. No longer needed as errors and the actual value are
>>> reported via different ways.
>>
>> I understand why errno == 0 && endptr > str go away. They also do in
>> the previous hunk.
>>
>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>> unobvious. What does it do before the patch?
>>
>> The condition goes back to commit 659268ffbff, which predates my watch
>> as maintainer. Its commit message is of no particular help. Its code
>> is... allright, the less I say about that, the better.
>>
>> We're parsing a range here. We already parsed its lower bound into
>> @start (and guarded against errors), and its upper bound into @end (and
>> guarded against errors).
>>
>> If the condition you delete is false, we goto error. So the condition
>> is about range validity. I figure it's an attempt to require valid
>> ranges to be no "wider" than 65535. The second part end < start + 65536
>> checks exactly that, except shit happens when start + 65536 overflows.
>> The first part attempts to guard against that, but
>>
>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>
>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>
>> WTF?!?
>>
>> Unless I'm mistaken, the condition is not about handling any of the
>> errors that qemu_strtoi64() handles for us.
>>
>> The easiest way for you out of this morass is probably to keep the
>> condition exactly as it was, then use the "my patch doesn't make things
>> any worse" get-out-of-jail-free card.
>>
>
> Looking at the code in qapi/string-output-visitor.c related to range and
> list handling I feel like using the get-out-of-jail-free card to get out
> of qapi code now :) Too much magic in that code and too little time for
> me to understand it all.
>
> Thanks for your time and review anyway. My time is better invested in
> other parts of QEMU. I will drop both patches from this series.
Understand.
When I first looked at the ranges stuff in the string input visitor, I
felt the urge to clean it up, then sat on my hands until it passed.
The rest is reasonable once you understand how it works. The learning
curve is less than pleasant, though.
On 05.11.18 16:37, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> On 31.10.18 18:55, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>
>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>> agree.
>>> [...]
>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>> }
>>>>>>
>>>>>> do {
>>>>>> - errno = 0;
>>>>>> - start = strtoll(str, &endptr, 0);
>>>>>> - if (errno == 0 && endptr > str) {
>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>>>>> if (*endptr == '\0') {
>>>>>> cur = g_malloc0(sizeof(*cur));
>>>>>> range_set_bounds(cur, start, start);
>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>> str = NULL;
>>>>>> } else if (*endptr == '-') {
>>>>>> str = endptr + 1;
>>>>>> - errno = 0;
>>>>>> - end = strtoll(str, &endptr, 0);
>>>>>> - if (errno == 0 && endptr > str && start <= end &&
>>>>>> - (start > INT64_MAX - 65536 ||
>>>>>> - end < start + 65536)) {
>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>>>>>
>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
>>>>> explain that to me? I'm feeling particularly dense today...
>>>>
>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>> from the response. No longer needed as errors and the actual value are
>>>> reported via different ways.
>>>
>>> I understand why errno == 0 && endptr > str go away. They also do in
>>> the previous hunk.
>>>
>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>> unobvious. What does it do before the patch?
>>>
>>> The condition goes back to commit 659268ffbff, which predates my watch
>>> as maintainer. Its commit message is of no particular help. Its code
>>> is... allright, the less I say about that, the better.
>>>
>>> We're parsing a range here. We already parsed its lower bound into
>>> @start (and guarded against errors), and its upper bound into @end (and
>>> guarded against errors).
>>>
>>> If the condition you delete is false, we goto error. So the condition
>>> is about range validity. I figure it's an attempt to require valid
>>> ranges to be no "wider" than 65535. The second part end < start + 65536
>>> checks exactly that, except shit happens when start + 65536 overflows.
>>> The first part attempts to guard against that, but
>>>
>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>
>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>
>>> WTF?!?
>>>
>>> Unless I'm mistaken, the condition is not about handling any of the
>>> errors that qemu_strtoi64() handles for us.
>>>
>>> The easiest way for you out of this morass is probably to keep the
>>> condition exactly as it was, then use the "my patch doesn't make things
>>> any worse" get-out-of-jail-free card.
>>>
>>
>> Looking at the code in qapi/string-output-visitor.c related to range and
>> list handling I feel like using the get-out-of-jail-free card to get out
>> of qapi code now :) Too much magic in that code and too little time for
>> me to understand it all.
>>
>> Thanks for your time and review anyway. My time is better invested in
>> other parts of QEMU. I will drop both patches from this series.
>
> Understand.
>
> When I first looked at the ranges stuff in the string input visitor, I
> felt the urge to clean it up, then sat on my hands until it passed.
>
> The rest is reasonable once you understand how it works. The learning
> curve is less than pleasant, though.
>
Maybe I'll pick this up again when I have more time to invest.
The general concept
1. of having an input visitor that is able to parse different types
(expected by e.g. a property) sounds sane to me.
2. of having a list of *something*, assuming it is int64_t, and assuming
it is to be parsed into a list of ranges sounds completely broken to me.
I was not even able to find an example QEMU comand line for 2. Is this
maybe some very old code that nobody actually uses anymore? (who uses
list of ranges?)
--
Thanks,
David / dhildenb
On 11/5/18 9:53 AM, David Hildenbrand wrote: >> When I first looked at the ranges stuff in the string input visitor, I >> felt the urge to clean it up, then sat on my hands until it passed. >> >> The rest is reasonable once you understand how it works. The learning >> curve is less than pleasant, though. >> > > Maybe I'll pick this up again when I have more time to invest. > > The general concept > > 1. of having an input visitor that is able to parse different types > (expected by e.g. a property) sounds sane to me. > > 2. of having a list of *something*, assuming it is int64_t, and assuming > it is to be parsed into a list of ranges sounds completely broken to me. > > I was not even able to find an example QEMU comand line for 2. Is this > maybe some very old code that nobody actually uses anymore? (who uses > list of ranges?) I believe that the -numa node,cpus=first-last code is an example use of a list of ranges. At any rate, tests/test-string-input-visitor.c has test_visitor_in_intList() to ensure we don't break back-compat with the existing clients that use ranges, as awkward as they may be. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 05.11.18 17:48, Eric Blake wrote: > On 11/5/18 9:53 AM, David Hildenbrand wrote: > >>> When I first looked at the ranges stuff in the string input visitor, I >>> felt the urge to clean it up, then sat on my hands until it passed. >>> >>> The rest is reasonable once you understand how it works. The learning >>> curve is less than pleasant, though. >>> >> >> Maybe I'll pick this up again when I have more time to invest. >> >> The general concept >> >> 1. of having an input visitor that is able to parse different types >> (expected by e.g. a property) sounds sane to me. >> >> 2. of having a list of *something*, assuming it is int64_t, and assuming >> it is to be parsed into a list of ranges sounds completely broken to me. >> >> I was not even able to find an example QEMU comand line for 2. Is this >> maybe some very old code that nobody actually uses anymore? (who uses >> list of ranges?) > > I believe that the -numa node,cpus=first-last code is an example use of > a list of ranges. At any rate, tests/test-string-input-visitor.c has > test_visitor_in_intList() to ensure we don't break back-compat with the > existing clients that use ranges, as awkward as they may be. > Indeed, thanks for the pointer. Ranges should indeed be supported. But maybe we can refactor this ugly "pre parse all uint64_t ranges" thingy. -- Thanks, David / dhildenb
David Hildenbrand <david@redhat.com> writes:
> On 05.11.18 16:37, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>
>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>> agree.
>>>> [...]
>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>> }
>>>>>>>
>>>>>>> do {
>>>>>>> - errno = 0;
>>>>>>> - start = strtoll(str, &endptr, 0);
>>>>>>> - if (errno == 0 && endptr > str) {
>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>>>>>> if (*endptr == '\0') {
>>>>>>> cur = g_malloc0(sizeof(*cur));
>>>>>>> range_set_bounds(cur, start, start);
>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>> str = NULL;
>>>>>>> } else if (*endptr == '-') {
>>>>>>> str = endptr + 1;
>>>>>>> - errno = 0;
>>>>>>> - end = strtoll(str, &endptr, 0);
>>>>>>> - if (errno == 0 && endptr > str && start <= end &&
>>>>>>> - (start > INT64_MAX - 65536 ||
>>>>>>> - end < start + 65536)) {
>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>>>>>>
>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
>>>>>> explain that to me? I'm feeling particularly dense today...
>>>>>
>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>> from the response. No longer needed as errors and the actual value are
>>>>> reported via different ways.
>>>>
>>>> I understand why errno == 0 && endptr > str go away. They also do in
>>>> the previous hunk.
>>>>
>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>> unobvious. What does it do before the patch?
>>>>
>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>> as maintainer. Its commit message is of no particular help. Its code
>>>> is... allright, the less I say about that, the better.
>>>>
>>>> We're parsing a range here. We already parsed its lower bound into
>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>> guarded against errors).
>>>>
>>>> If the condition you delete is false, we goto error. So the condition
>>>> is about range validity. I figure it's an attempt to require valid
>>>> ranges to be no "wider" than 65535. The second part end < start + 65536
>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>> The first part attempts to guard against that, but
>>>>
>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>
>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>
>>>> WTF?!?
>>>>
>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>> errors that qemu_strtoi64() handles for us.
>>>>
>>>> The easiest way for you out of this morass is probably to keep the
>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>> any worse" get-out-of-jail-free card.
>>>>
>>>
>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>> list handling I feel like using the get-out-of-jail-free card to get out
>>> of qapi code now :) Too much magic in that code and too little time for
>>> me to understand it all.
>>>
>>> Thanks for your time and review anyway. My time is better invested in
>>> other parts of QEMU. I will drop both patches from this series.
>>
>> Understand.
>>
>> When I first looked at the ranges stuff in the string input visitor, I
>> felt the urge to clean it up, then sat on my hands until it passed.
>>
>> The rest is reasonable once you understand how it works. The learning
>> curve is less than pleasant, though.
>>
>
> Maybe I'll pick this up again when I have more time to invest.
>
> The general concept
>
> 1. of having an input visitor that is able to parse different types
> (expected by e.g. a property) sounds sane to me.
>
> 2. of having a list of *something*, assuming it is int64_t, and assuming
> it is to be parsed into a list of ranges sounds completely broken to me.
Starting point: the string visitors can only do scalars. We have a need
for lists of integers (see below). The general solution would be
generalizing these visitors to lists (and maybe objects while we're at
it). YAGNI. So we put in a quick hack that can do just lists of
integers.
Except applying YAGNI to stable interfaces is *bonkers*.
> I was not even able to find an example QEMU comand line for 2. Is this
> maybe some very old code that nobody actually uses anymore? (who uses
> list of ranges?)
The one I remember offhand is -numa node,cpus=..., but that one's
actually parsed with the options visitor. Which is even hairier, but at
least competently coded.
To find uses, we need to follow the uses of the string visitors.
Of the callers of string_input_visitor_new(),
object_property_get_uint16List() is the only one that deals with lists.
It's used by query_memdev() for property host-nodes.
The callers of string_output_visitor_new() lead to MigrationInfo member
postcopy-vcpu-blocktime, and Memdev member host-nodes again.
Searching the QAPI schema for lists of integers coughs up a few more
candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
(likewise), block latency histogram stuff (likewise).
On 05.11.18 21:43, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> On 05.11.18 16:37, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>
>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>
>>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>>> agree.
>>>>> [...]
>>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>> }
>>>>>>>>
>>>>>>>> do {
>>>>>>>> - errno = 0;
>>>>>>>> - start = strtoll(str, &endptr, 0);
>>>>>>>> - if (errno == 0 && endptr > str) {
>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>>>>>>> if (*endptr == '\0') {
>>>>>>>> cur = g_malloc0(sizeof(*cur));
>>>>>>>> range_set_bounds(cur, start, start);
>>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>> str = NULL;
>>>>>>>> } else if (*endptr == '-') {
>>>>>>>> str = endptr + 1;
>>>>>>>> - errno = 0;
>>>>>>>> - end = strtoll(str, &endptr, 0);
>>>>>>>> - if (errno == 0 && endptr > str && start <= end &&
>>>>>>>> - (start > INT64_MAX - 65536 ||
>>>>>>>> - end < start + 65536)) {
>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>>>>>>>
>>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
>>>>>>> explain that to me? I'm feeling particularly dense today...
>>>>>>
>>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>>> from the response. No longer needed as errors and the actual value are
>>>>>> reported via different ways.
>>>>>
>>>>> I understand why errno == 0 && endptr > str go away. They also do in
>>>>> the previous hunk.
>>>>>
>>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>>> unobvious. What does it do before the patch?
>>>>>
>>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>>> as maintainer. Its commit message is of no particular help. Its code
>>>>> is... allright, the less I say about that, the better.
>>>>>
>>>>> We're parsing a range here. We already parsed its lower bound into
>>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>>> guarded against errors).
>>>>>
>>>>> If the condition you delete is false, we goto error. So the condition
>>>>> is about range validity. I figure it's an attempt to require valid
>>>>> ranges to be no "wider" than 65535. The second part end < start + 65536
>>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>>> The first part attempts to guard against that, but
>>>>>
>>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>>
>>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>>
>>>>> WTF?!?
>>>>>
>>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>>> errors that qemu_strtoi64() handles for us.
>>>>>
>>>>> The easiest way for you out of this morass is probably to keep the
>>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>>> any worse" get-out-of-jail-free card.
>>>>>
>>>>
>>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>>> list handling I feel like using the get-out-of-jail-free card to get out
>>>> of qapi code now :) Too much magic in that code and too little time for
>>>> me to understand it all.
>>>>
>>>> Thanks for your time and review anyway. My time is better invested in
>>>> other parts of QEMU. I will drop both patches from this series.
>>>
>>> Understand.
>>>
>>> When I first looked at the ranges stuff in the string input visitor, I
>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>
>>> The rest is reasonable once you understand how it works. The learning
>>> curve is less than pleasant, though.
>>>
>>
>> Maybe I'll pick this up again when I have more time to invest.
>>
>> The general concept
>>
>> 1. of having an input visitor that is able to parse different types
>> (expected by e.g. a property) sounds sane to me.
>>
>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>> it is to be parsed into a list of ranges sounds completely broken to me.
>
> Starting point: the string visitors can only do scalars. We have a need
> for lists of integers (see below). The general solution would be
> generalizing these visitors to lists (and maybe objects while we're at
> it). YAGNI. So we put in a quick hack that can do just lists of
> integers.
>
> Except applying YAGNI to stable interfaces is *bonkers*.
>
>> I was not even able to find an example QEMU comand line for 2. Is this
>> maybe some very old code that nobody actually uses anymore? (who uses
>> list of ranges?)
>
> The one I remember offhand is -numa node,cpus=..., but that one's
> actually parsed with the options visitor. Which is even hairier, but at
> least competently coded.
>
> To find uses, we need to follow the uses of the string visitors.
>
> Of the callers of string_input_visitor_new(),
> object_property_get_uint16List() is the only one that deals with lists.
> It's used by query_memdev() for property host-nodes.
>
> The callers of string_output_visitor_new() lead to MigrationInfo member
> postcopy-vcpu-blocktime, and Memdev member host-nodes again.
>
> Searching the QAPI schema for lists of integers coughs up a few more
> candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
> member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
> (likewise), block latency histogram stuff (likewise).
>
As Eric pointed out, tests/test-string-input-visitor.c actually tests
for range support in test_visitor_in_intList.
I might be completely wrong, but actually the string input visitor
should not pre-parse stuff into a list of ranges, but instead parse on
request (parse_type_...) and advance in the logical list on "next_list".
And we should parse ranges *only* if we are expecting a list. Because a
range is simply a short variant of a list. A straight parse_type_uint64
should bail out if we haven't started a list.
I guess I am starting to understand how this magic is supposed to work.
Always parsing and processing one list token at a time
("size"/"uint64_t" or "range of such") should be the way to go. And if
nobody requested to parse a list (start_list()), also ranges should not
be allowed. This pre-parsing of the whole list and unconditional use of
ranges should go.
Ranges are still ugly but needed as far as I can understand (as a
shortcut for lengthy lists).
Am I on the right track?
--
Thanks,
David / dhildenb
David Hildenbrand <david@redhat.com> writes:
> On 05.11.18 21:43, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> On 05.11.18 16:37, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>
>>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>>
>>>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>>>> agree.
>>>>>> [...]
>>>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> do {
>>>>>>>>> - errno = 0;
>>>>>>>>> - start = strtoll(str, &endptr, 0);
>>>>>>>>> - if (errno == 0 && endptr > str) {
>>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>>>>>>>> if (*endptr == '\0') {
>>>>>>>>> cur = g_malloc0(sizeof(*cur));
>>>>>>>>> range_set_bounds(cur, start, start);
>>>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>> str = NULL;
>>>>>>>>> } else if (*endptr == '-') {
>>>>>>>>> str = endptr + 1;
>>>>>>>>> - errno = 0;
>>>>>>>>> - end = strtoll(str, &endptr, 0);
>>>>>>>>> - if (errno == 0 && endptr > str && start <= end &&
>>>>>>>>> - (start > INT64_MAX - 65536 ||
>>>>>>>>> - end < start + 65536)) {
>>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>>>>>>>>
>>>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
>>>>>>>> explain that to me? I'm feeling particularly dense today...
>>>>>>>
>>>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>>>> from the response. No longer needed as errors and the actual value are
>>>>>>> reported via different ways.
>>>>>>
>>>>>> I understand why errno == 0 && endptr > str go away. They also do in
>>>>>> the previous hunk.
>>>>>>
>>>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>>>> unobvious. What does it do before the patch?
>>>>>>
>>>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>>>> as maintainer. Its commit message is of no particular help. Its code
>>>>>> is... allright, the less I say about that, the better.
>>>>>>
>>>>>> We're parsing a range here. We already parsed its lower bound into
>>>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>>>> guarded against errors).
>>>>>>
>>>>>> If the condition you delete is false, we goto error. So the condition
>>>>>> is about range validity. I figure it's an attempt to require valid
>>>>>> ranges to be no "wider" than 65535. The second part end < start + 65536
>>>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>>>> The first part attempts to guard against that, but
>>>>>>
>>>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>>>
>>>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>>>
>>>>>> WTF?!?
>>>>>>
>>>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>>>> errors that qemu_strtoi64() handles for us.
>>>>>>
>>>>>> The easiest way for you out of this morass is probably to keep the
>>>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>>>> any worse" get-out-of-jail-free card.
>>>>>>
>>>>>
>>>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>>>> list handling I feel like using the get-out-of-jail-free card to get out
>>>>> of qapi code now :) Too much magic in that code and too little time for
>>>>> me to understand it all.
>>>>>
>>>>> Thanks for your time and review anyway. My time is better invested in
>>>>> other parts of QEMU. I will drop both patches from this series.
>>>>
>>>> Understand.
>>>>
>>>> When I first looked at the ranges stuff in the string input visitor, I
>>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>>
>>>> The rest is reasonable once you understand how it works. The learning
>>>> curve is less than pleasant, though.
>>>>
>>>
>>> Maybe I'll pick this up again when I have more time to invest.
>>>
>>> The general concept
>>>
>>> 1. of having an input visitor that is able to parse different types
>>> (expected by e.g. a property) sounds sane to me.
>>>
>>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>>> it is to be parsed into a list of ranges sounds completely broken to me.
>>
>> Starting point: the string visitors can only do scalars. We have a need
>> for lists of integers (see below). The general solution would be
>> generalizing these visitors to lists (and maybe objects while we're at
>> it). YAGNI. So we put in a quick hack that can do just lists of
>> integers.
>>
>> Except applying YAGNI to stable interfaces is *bonkers*.
>>
>>> I was not even able to find an example QEMU comand line for 2. Is this
>>> maybe some very old code that nobody actually uses anymore? (who uses
>>> list of ranges?)
>>
>> The one I remember offhand is -numa node,cpus=..., but that one's
>> actually parsed with the options visitor. Which is even hairier, but at
>> least competently coded.
>>
>> To find uses, we need to follow the uses of the string visitors.
>>
>> Of the callers of string_input_visitor_new(),
>> object_property_get_uint16List() is the only one that deals with lists.
>> It's used by query_memdev() for property host-nodes.
>>
>> The callers of string_output_visitor_new() lead to MigrationInfo member
>> postcopy-vcpu-blocktime, and Memdev member host-nodes again.
>>
>> Searching the QAPI schema for lists of integers coughs up a few more
>> candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
>> member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
>> (likewise), block latency histogram stuff (likewise).
>>
>
> As Eric pointed out, tests/test-string-input-visitor.c actually tests
> for range support in test_visitor_in_intList.
>
> I might be completely wrong, but actually the string input visitor
> should not pre-parse stuff into a list of ranges, but instead parse on
> request (parse_type_...) and advance in the logical list on "next_list".
> And we should parse ranges *only* if we are expecting a list. Because a
> range is simply a short variant of a list. A straight parse_type_uint64
> should bail out if we haven't started a list.
Yes, parse_type_int64() & friends should simply parse the appropriate
integer, *except* when we're working on a list. Then they should return
the next integer, which may or may not require parsing.
Say, input is "1-3,5", and the visitor is called like
visit_start_list()
visit_next_list() more input, returns "there's more"
visit_type_int() parses "1-3,", buffers 2-3, returns 1
visit_next_list() buffer not empty, returns "there's more"
visit_type_int() unbuffers and returns 2
visit_next_list() buffer not empty, returns "there's more"
visit_type_int() unbuffers and returns 3
visit_next_list() more input, returns "there's more"
visit_type_int() parses "5", returns 5
visit_next_list() buffer empty and no more input, returns "done"
visit_end_list()
Alternatively, parse and buffer the whole list at once.
> I guess I am starting to understand how this magic is supposed to work.
> Always parsing and processing one list token at a time
> ("size"/"uint64_t" or "range of such") should be the way to go. And if
> nobody requested to parse a list (start_list()), also ranges should not
> be allowed. This pre-parsing of the whole list and unconditional use of
> ranges should go.
>
> Ranges are still ugly but needed as far as I can understand (as a
> shortcut for lengthy lists).
>
> Am I on the right track?
I believe you are.
The overall problem is to convert between text and (QAPI-generated) C
data structures.
We have a "low magic" solution for JSON text: we split the problem like
JSON text --> QObject --> C data --> QObject --> JSON text
| | | |
JSON parser | | JSON formatter
QObject input visitor |
QObject output visitor
The JSON parser is slightly magical around numbers. Everything else is
straightforward.
We have a "moderate magic" solution for key-value text (used for option
arguments):
key-value text --> QObject --> C data
| |
keyval_parse() |
QObject input visitor
keyval variant
Key-value text is less expressive than JSON: it can't distinguish the
scalar types (everthing's a string), and it can't do empty arrays or
empty non-root objects. The visitor magically converts strings to
whatever type is expected.
We have a "bad magic" solution for "strings":
string --> C data --> string
| |
string input visitor |
string output visitor
Initially, this visitor was simple: only scalars. Adding ranges was a
misguided idea. The way they were coded should never have passed
review.
We have a "more bad magic" solution for certain option arguments:
key-value text --> QemuOpts --> C data
| |
qemu_opts_parse() |
opts visitor
Predates the other key-value solution. Less general (by design), and
nevertheless more complex. I hope the other one can replace it one day.
On 07.11.18 16:29, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> On 05.11.18 21:43, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 05.11.18 16:37, Markus Armbruster wrote:
>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>
>>>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>
>>>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>>>
>>>>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>>>>> agree.
>>>>>>> [...]
>>>>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> do {
>>>>>>>>>> - errno = 0;
>>>>>>>>>> - start = strtoll(str, &endptr, 0);
>>>>>>>>>> - if (errno == 0 && endptr > str) {
>>>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>>>>>>>>> if (*endptr == '\0') {
>>>>>>>>>> cur = g_malloc0(sizeof(*cur));
>>>>>>>>>> range_set_bounds(cur, start, start);
>>>>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>> str = NULL;
>>>>>>>>>> } else if (*endptr == '-') {
>>>>>>>>>> str = endptr + 1;
>>>>>>>>>> - errno = 0;
>>>>>>>>>> - end = strtoll(str, &endptr, 0);
>>>>>>>>>> - if (errno == 0 && endptr > str && start <= end &&
>>>>>>>>>> - (start > INT64_MAX - 65536 ||
>>>>>>>>>> - end < start + 65536)) {
>>>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>>>>>>>>>
>>>>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
>>>>>>>>> explain that to me? I'm feeling particularly dense today...
>>>>>>>>
>>>>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>>>>> from the response. No longer needed as errors and the actual value are
>>>>>>>> reported via different ways.
>>>>>>>
>>>>>>> I understand why errno == 0 && endptr > str go away. They also do in
>>>>>>> the previous hunk.
>>>>>>>
>>>>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>>>>> unobvious. What does it do before the patch?
>>>>>>>
>>>>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>>>>> as maintainer. Its commit message is of no particular help. Its code
>>>>>>> is... allright, the less I say about that, the better.
>>>>>>>
>>>>>>> We're parsing a range here. We already parsed its lower bound into
>>>>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>>>>> guarded against errors).
>>>>>>>
>>>>>>> If the condition you delete is false, we goto error. So the condition
>>>>>>> is about range validity. I figure it's an attempt to require valid
>>>>>>> ranges to be no "wider" than 65535. The second part end < start + 65536
>>>>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>>>>> The first part attempts to guard against that, but
>>>>>>>
>>>>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>>>>
>>>>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>>>>
>>>>>>> WTF?!?
>>>>>>>
>>>>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>>>>> errors that qemu_strtoi64() handles for us.
>>>>>>>
>>>>>>> The easiest way for you out of this morass is probably to keep the
>>>>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>>>>> any worse" get-out-of-jail-free card.
>>>>>>>
>>>>>>
>>>>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>>>>> list handling I feel like using the get-out-of-jail-free card to get out
>>>>>> of qapi code now :) Too much magic in that code and too little time for
>>>>>> me to understand it all.
>>>>>>
>>>>>> Thanks for your time and review anyway. My time is better invested in
>>>>>> other parts of QEMU. I will drop both patches from this series.
>>>>>
>>>>> Understand.
>>>>>
>>>>> When I first looked at the ranges stuff in the string input visitor, I
>>>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>>>
>>>>> The rest is reasonable once you understand how it works. The learning
>>>>> curve is less than pleasant, though.
>>>>>
>>>>
>>>> Maybe I'll pick this up again when I have more time to invest.
>>>>
>>>> The general concept
>>>>
>>>> 1. of having an input visitor that is able to parse different types
>>>> (expected by e.g. a property) sounds sane to me.
>>>>
>>>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>>>> it is to be parsed into a list of ranges sounds completely broken to me.
>>>
>>> Starting point: the string visitors can only do scalars. We have a need
>>> for lists of integers (see below). The general solution would be
>>> generalizing these visitors to lists (and maybe objects while we're at
>>> it). YAGNI. So we put in a quick hack that can do just lists of
>>> integers.
>>>
>>> Except applying YAGNI to stable interfaces is *bonkers*.
>>>
>>>> I was not even able to find an example QEMU comand line for 2. Is this
>>>> maybe some very old code that nobody actually uses anymore? (who uses
>>>> list of ranges?)
>>>
>>> The one I remember offhand is -numa node,cpus=..., but that one's
>>> actually parsed with the options visitor. Which is even hairier, but at
>>> least competently coded.
>>>
>>> To find uses, we need to follow the uses of the string visitors.
>>>
>>> Of the callers of string_input_visitor_new(),
>>> object_property_get_uint16List() is the only one that deals with lists.
>>> It's used by query_memdev() for property host-nodes.
>>>
>>> The callers of string_output_visitor_new() lead to MigrationInfo member
>>> postcopy-vcpu-blocktime, and Memdev member host-nodes again.
>>>
>>> Searching the QAPI schema for lists of integers coughs up a few more
>>> candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
>>> member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
>>> (likewise), block latency histogram stuff (likewise).
>>>
>>
>> As Eric pointed out, tests/test-string-input-visitor.c actually tests
>> for range support in test_visitor_in_intList.
>>
>> I might be completely wrong, but actually the string input visitor
>> should not pre-parse stuff into a list of ranges, but instead parse on
>> request (parse_type_...) and advance in the logical list on "next_list".
>> And we should parse ranges *only* if we are expecting a list. Because a
>> range is simply a short variant of a list. A straight parse_type_uint64
>> should bail out if we haven't started a list.
>
> Yes, parse_type_int64() & friends should simply parse the appropriate
> integer, *except* when we're working on a list. Then they should return
> the next integer, which may or may not require parsing.
>
> Say, input is "1-3,5", and the visitor is called like
>
> visit_start_list()
> visit_next_list() more input, returns "there's more"
> visit_type_int() parses "1-3,", buffers 2-3, returns 1
> visit_next_list() buffer not empty, returns "there's more"
> visit_type_int() unbuffers and returns 2
> visit_next_list() buffer not empty, returns "there's more"
> visit_type_int() unbuffers and returns 3
> visit_next_list() more input, returns "there's more"
> visit_type_int() parses "5", returns 5
> visit_next_list() buffer empty and no more input, returns "done"
> visit_end_list()
>
Would it be valid to do something like this (skipping elements without a
proper visit_type_int)
visit_start_list();
visit_next_list(); more input, returns "there's more"
visit_next_list(); parses "1-3,", buffers 2-3, skips over 1
visit_type_int(); returns 2
...
Or mixing types
visit_start_list();
visit_next_list();
visit_type_int64();
visit_next_list();
visit_type_uint64();
IOW, can I assume that after every visit_next_list(), visit_type_X is
called, and that X remains the same for one pass over the list?
Also, I assume it is supposed to work like this
visit_start_list();
visit_next_list(); more input, returns "there's more"
visit_type_int(); returns 1 (parses 1-3, buffers 1-3)
visit_type_int(); returns 1
visit_type_int(); returns 1
visit_next_list(); more input, unbuffers 1
visit_type_int(); returns 2
So unbuffering actually happens on visit_next_list()?
--
Thanks,
David / dhildenb
David Hildenbrand <david@redhat.com> writes:
> On 07.11.18 16:29, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> On 05.11.18 21:43, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> On 05.11.18 16:37, Markus Armbruster wrote:
>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>
>>>>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>>
>>>>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>>>>> David Hildenbrand <david@redhat.com> writes:
>>>>>>>>>>
>>>>>>>>>>> The qemu api claims to be easier to use, and the resulting code seems to
>>>>>>>>>>> agree.
>>>>>>>> [...]
>>>>>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> do {
>>>>>>>>>>> - errno = 0;
>>>>>>>>>>> - start = strtoll(str, &endptr, 0);
>>>>>>>>>>> - if (errno == 0 && endptr > str) {
>>>>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>>>>>>>>>> if (*endptr == '\0') {
>>>>>>>>>>> cur = g_malloc0(sizeof(*cur));
>>>>>>>>>>> range_set_bounds(cur, start, start);
>>>>>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>>>>>>>>>> str = NULL;
>>>>>>>>>>> } else if (*endptr == '-') {
>>>>>>>>>>> str = endptr + 1;
>>>>>>>>>>> - errno = 0;
>>>>>>>>>>> - end = strtoll(str, &endptr, 0);
>>>>>>>>>>> - if (errno == 0 && endptr > str && start <= end &&
>>>>>>>>>>> - (start > INT64_MAX - 65536 ||
>>>>>>>>>>> - end < start + 65536)) {
>>>>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
>>>>>>>>>>
>>>>>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can you
>>>>>>>>>> explain that to me? I'm feeling particularly dense today...
>>>>>>>>>
>>>>>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>>>>>> from the response. No longer needed as errors and the actual value are
>>>>>>>>> reported via different ways.
>>>>>>>>
>>>>>>>> I understand why errno == 0 && endptr > str go away. They also do in
>>>>>>>> the previous hunk.
>>>>>>>>
>>>>>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>>>>>> unobvious. What does it do before the patch?
>>>>>>>>
>>>>>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>>>>>> as maintainer. Its commit message is of no particular help. Its code
>>>>>>>> is... allright, the less I say about that, the better.
>>>>>>>>
>>>>>>>> We're parsing a range here. We already parsed its lower bound into
>>>>>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>>>>>> guarded against errors).
>>>>>>>>
>>>>>>>> If the condition you delete is false, we goto error. So the condition
>>>>>>>> is about range validity. I figure it's an attempt to require valid
>>>>>>>> ranges to be no "wider" than 65535. The second part end < start + 65536
>>>>>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>>>>>> The first part attempts to guard against that, but
>>>>>>>>
>>>>>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>>>>>
>>>>>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>>>>>
>>>>>>>> WTF?!?
>>>>>>>>
>>>>>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>>>>>> errors that qemu_strtoi64() handles for us.
>>>>>>>>
>>>>>>>> The easiest way for you out of this morass is probably to keep the
>>>>>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>>>>>> any worse" get-out-of-jail-free card.
>>>>>>>>
>>>>>>>
>>>>>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>>>>>> list handling I feel like using the get-out-of-jail-free card to get out
>>>>>>> of qapi code now :) Too much magic in that code and too little time for
>>>>>>> me to understand it all.
>>>>>>>
>>>>>>> Thanks for your time and review anyway. My time is better invested in
>>>>>>> other parts of QEMU. I will drop both patches from this series.
>>>>>>
>>>>>> Understand.
>>>>>>
>>>>>> When I first looked at the ranges stuff in the string input visitor, I
>>>>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>>>>
>>>>>> The rest is reasonable once you understand how it works. The learning
>>>>>> curve is less than pleasant, though.
>>>>>>
>>>>>
>>>>> Maybe I'll pick this up again when I have more time to invest.
>>>>>
>>>>> The general concept
>>>>>
>>>>> 1. of having an input visitor that is able to parse different types
>>>>> (expected by e.g. a property) sounds sane to me.
>>>>>
>>>>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>>>>> it is to be parsed into a list of ranges sounds completely broken to me.
>>>>
>>>> Starting point: the string visitors can only do scalars. We have a need
>>>> for lists of integers (see below). The general solution would be
>>>> generalizing these visitors to lists (and maybe objects while we're at
>>>> it). YAGNI. So we put in a quick hack that can do just lists of
>>>> integers.
>>>>
>>>> Except applying YAGNI to stable interfaces is *bonkers*.
>>>>
>>>>> I was not even able to find an example QEMU comand line for 2. Is this
>>>>> maybe some very old code that nobody actually uses anymore? (who uses
>>>>> list of ranges?)
>>>>
>>>> The one I remember offhand is -numa node,cpus=..., but that one's
>>>> actually parsed with the options visitor. Which is even hairier, but at
>>>> least competently coded.
>>>>
>>>> To find uses, we need to follow the uses of the string visitors.
>>>>
>>>> Of the callers of string_input_visitor_new(),
>>>> object_property_get_uint16List() is the only one that deals with lists.
>>>> It's used by query_memdev() for property host-nodes.
>>>>
>>>> The callers of string_output_visitor_new() lead to MigrationInfo member
>>>> postcopy-vcpu-blocktime, and Memdev member host-nodes again.
>>>>
>>>> Searching the QAPI schema for lists of integers coughs up a few more
>>>> candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
>>>> member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
>>>> (likewise), block latency histogram stuff (likewise).
>>>>
>>>
>>> As Eric pointed out, tests/test-string-input-visitor.c actually tests
>>> for range support in test_visitor_in_intList.
>>>
>>> I might be completely wrong, but actually the string input visitor
>>> should not pre-parse stuff into a list of ranges, but instead parse on
>>> request (parse_type_...) and advance in the logical list on "next_list".
>>> And we should parse ranges *only* if we are expecting a list. Because a
>>> range is simply a short variant of a list. A straight parse_type_uint64
>>> should bail out if we haven't started a list.
>>
>> Yes, parse_type_int64() & friends should simply parse the appropriate
>> integer, *except* when we're working on a list. Then they should return
>> the next integer, which may or may not require parsing.
>>
>> Say, input is "1-3,5", and the visitor is called like
>>
>> visit_start_list()
>> visit_next_list() more input, returns "there's more"
>> visit_type_int() parses "1-3,", buffers 2-3, returns 1
>> visit_next_list() buffer not empty, returns "there's more"
>> visit_type_int() unbuffers and returns 2
>> visit_next_list() buffer not empty, returns "there's more"
>> visit_type_int() unbuffers and returns 3
>> visit_next_list() more input, returns "there's more"
>> visit_type_int() parses "5", returns 5
>> visit_next_list() buffer empty and no more input, returns "done"
>> visit_end_list()
>>
>
> Would it be valid to do something like this (skipping elements without a
> proper visit_type_int)
>
> visit_start_list();
> visit_next_list(); more input, returns "there's more"
> visit_next_list(); parses "1-3,", buffers 2-3, skips over 1
> visit_type_int(); returns 2
> ...
Excellent question!
Here's the relevant part of visit_start_list()'s contract in visitor.h:
* After visit_start_list() succeeds, the caller may visit its members
* one after the other. A real visit (where @obj is non-NULL) uses
* visit_next_list() for traversing the linked list, while a virtual
* visit (where @obj is NULL) uses other means. For each list
* element, call the appropriate visit_type_FOO() with name set to
* NULL and obj set to the address of the value member of the list
* element. Finally, visit_end_list() needs to be called with the
* same @list to clean up, even if intermediate visits fail. See the
* examples above.
So, you *may* visit members, and you *must* call visit_end_list().
But what are "real" and "virtual" visits? Again, the contract:
* @list must be non-NULL for a real walk, in which case @size
* determines how much memory an input or clone visitor will allocate
* into *@list (at least sizeof(GenericList)). Some visitors also
* allow @list to be NULL for a virtual walk, in which case @size is
* ignored.
I'm not sure whether I just decreased or increased global confusion ;)
The file comment explains:
* The QAPI schema defines both a set of C data types, and a QMP wire
* format. QAPI objects can contain references to other QAPI objects,
* resulting in a directed acyclic graph. QAPI also generates visitor
* functions to walk these graphs. This file represents the interface
* for doing work at each node of a QAPI graph; it can also be used
* for a virtual walk, where there is no actual QAPI C struct.
A real walk with an output visitor works like this (error handling
omitted for now):
Error *err = NULL;
intList *tail;
size_t size = sizeof(**obj);
// fetch list's head into *obj, start the list in output
visit_start_list(v, name, (GenericList **)obj, size, &err);
// iterate over list's tails
// below the hood, visit_next_list() iterates over the GenericList
for (tail = *obj; tail;
tail = (intList *)visit_next_list(v, (GenericList *)tail, size)) {
// visit current tail's first member, add it to output
visit_type_int(v, NULL, &tail->value, &err);
}
// end the list in output
visit_end_list(v, (void **)obj);
The exact same code works for a real walk with an input visitor, where
visit_next_list() iterates over the *input* and builds up the
GenericList. Compare qobject_input_next_list() and
qobject_output_next_list().
The code above is a straight copy of generated visit_type_intList() with
error handling cut and comments added.
A real walk works on a QAPI-generated C type. QAPI generates a real
walk for each such type. Open-coding a real walk would senselessly
duplicate the generated one, so we don't. Thus, a real walk always
visits each member.
Okay, I lied: it visits each member until it runs into an error and
bails out. See generated visit_type_intList() in
qapi/qapi-builtin-visit.c.
A virtual walk doesn't work with a GenericList *. Calling
visit_next_list() makes no sense then. visitor.h gives this example of
a virtual walk:
* Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
* like:
*
* <example>
* Visitor *v;
* Error *err = NULL;
* int value;
*
* v = FOO_visitor_new(...);
* visit_start_struct(v, NULL, NULL, 0, &err);
* if (err) {
* goto out;
* }
* visit_start_list(v, "list", NULL, 0, &err);
* if (err) {
* goto outobj;
* }
* value = 1;
* visit_type_int(v, NULL, &value, &err);
* if (err) {
* goto outlist;
* }
* value = 2;
* visit_type_int(v, NULL, &value, &err);
* if (err) {
* goto outlist;
* }
* outlist:
* visit_end_list(v, NULL);
* if (!err) {
* visit_check_struct(v, &err);
* }
* outobj:
* visit_end_struct(v, NULL);
* out:
* error_propagate(errp, err);
* visit_free(v);
Why could this be useful?
1. With the QObject input visitor, it's an alternative way to
destructure a QObject into a bunch of C variables. The "obvious" way
would be calling the QObject accessors. By using the visitors you
get much the error checking code for free. YMMV.
2. With the QObject output visitor, it's an alternative way to build up
a QObject. The "obvious" way would be calling the constructors
directly.
3. With the string input / output visitors, it's a way to parse / format
string visitor syntax without having to construct the C type first.
Right now, we have no virtual list walks outside tests. We do have
virtual struct walks outside tests.
> Or mixing types
>
> visit_start_list();
> visit_next_list();
> visit_type_int64();
> visit_next_list();
> visit_type_uint64();
Another excellent question.
QAPI can only express homogeneous lists, i.e. lists of some type T.
The generated visit_type_TList call the same visit_type_T() for all list
members.
QAPI type 'any' is the top type, but visit_type_anyList() still calls
visit_type_any() for all list members.
Virtual walks could of course do anything. Since they don't exist
outside tests, we can outlaw doing things that cause us trouble.
The virtual walks we currently have in tests/ seem to walk only
homogeneous lists, i.e. always call the same visit_type_T().
> IOW, can I assume that after every visit_next_list(), visit_type_X is
> called, and that X remains the same for one pass over the list?
As far as I can tell, existing code is just fine with that assumption.
We'd have to write it into the contract, though.
> Also, I assume it is supposed to work like this
>
> visit_start_list();
> visit_next_list(); more input, returns "there's more"
> visit_type_int(); returns 1 (parses 1-3, buffers 1-3)
> visit_type_int(); returns 1
> visit_type_int(); returns 1
> visit_next_list(); more input, unbuffers 1
> visit_type_int(); returns 2
>
> So unbuffering actually happens on visit_next_list()?
I believe this violates the contract.
If it's a real walk, the contract wants you to call visit_next_list()
between subsequent visit_type_int().
If it's a virtual walk, calling visit_next_list() makes no sense.
More questions?
>> Would it be valid to do something like this (skipping elements without a
>> proper visit_type_int)
>>
>> visit_start_list();
>> visit_next_list(); more input, returns "there's more"
>> visit_next_list(); parses "1-3,", buffers 2-3, skips over 1
>> visit_type_int(); returns 2
>> ...
>
> Excellent question!
>
> Here's the relevant part of visit_start_list()'s contract in visitor.h:
>
> * After visit_start_list() succeeds, the caller may visit its members
> * one after the other. A real visit (where @obj is non-NULL) uses
> * visit_next_list() for traversing the linked list, while a virtual
> * visit (where @obj is NULL) uses other means. For each list
> * element, call the appropriate visit_type_FOO() with name set to
> * NULL and obj set to the address of the value member of the list
> * element. Finally, visit_end_list() needs to be called with the
> * same @list to clean up, even if intermediate visits fail. See the
> * examples above.
>
> So, you *may* visit members, and you *must* call visit_end_list().
>
> But what are "real" and "virtual" visits? Again, the contract:
>
> * @list must be non-NULL for a real walk, in which case @size
> * determines how much memory an input or clone visitor will allocate
> * into *@list (at least sizeof(GenericList)). Some visitors also
> * allow @list to be NULL for a virtual walk, in which case @size is
> * ignored.
>
> I'm not sure whether I just decreased or increased global confusion ;)
I was reading over these comments and got slightly confused :)
>
> The file comment explains:
>
> * The QAPI schema defines both a set of C data types, and a QMP wire
> * format. QAPI objects can contain references to other QAPI objects,
> * resulting in a directed acyclic graph. QAPI also generates visitor
> * functions to walk these graphs. This file represents the interface
> * for doing work at each node of a QAPI graph; it can also be used
> * for a virtual walk, where there is no actual QAPI C struct.
>
> A real walk with an output visitor works like this (error handling
> omitted for now):
>
> Error *err = NULL;
> intList *tail;
> size_t size = sizeof(**obj);
>
> // fetch list's head into *obj, start the list in output
> visit_start_list(v, name, (GenericList **)obj, size, &err);
>
> // iterate over list's tails
> // below the hood, visit_next_list() iterates over the GenericList
> for (tail = *obj; tail;
> tail = (intList *)visit_next_list(v, (GenericList *)tail, size)) {
> // visit current tail's first member, add it to output
> visit_type_int(v, NULL, &tail->value, &err);
> }
>
> // end the list in output
> visit_end_list(v, (void **)obj);
>
> The exact same code works for a real walk with an input visitor, where
> visit_next_list() iterates over the *input* and builds up the
> GenericList. Compare qobject_input_next_list() and
> qobject_output_next_list().
>
> The code above is a straight copy of generated visit_type_intList() with
> error handling cut and comments added.
>
> A real walk works on a QAPI-generated C type. QAPI generates a real
> walk for each such type. Open-coding a real walk would senselessly
> duplicate the generated one, so we don't. Thus, a real walk always
> visits each member.
>
> Okay, I lied: it visits each member until it runs into an error and
> bails out. See generated visit_type_intList() in
> qapi/qapi-builtin-visit.c.
>
> A virtual walk doesn't work with a GenericList *. Calling
> visit_next_list() makes no sense then. visitor.h gives this example of
> a virtual walk:
Alright, so we must not support virtual walks. But supporting it would
not harm :)
>
> * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
> * like:
> *
> * <example>
> * Visitor *v;
> * Error *err = NULL;
> * int value;
> *
> * v = FOO_visitor_new(...);
> * visit_start_struct(v, NULL, NULL, 0, &err);
> * if (err) {
> * goto out;
> * }
> * visit_start_list(v, "list", NULL, 0, &err);
> * if (err) {
> * goto outobj;
> * }
> * value = 1;
> * visit_type_int(v, NULL, &value, &err);
> * if (err) {
> * goto outlist;
> * }
> * value = 2;
> * visit_type_int(v, NULL, &value, &err);
> * if (err) {
> * goto outlist;
> * }
> * outlist:
> * visit_end_list(v, NULL);
> * if (!err) {
> * visit_check_struct(v, &err);
> * }
> * outobj:
> * visit_end_struct(v, NULL);
> * out:
> * error_propagate(errp, err);
> * visit_free(v);
>
> Why could this be useful?
>
> 1. With the QObject input visitor, it's an alternative way to
> destructure a QObject into a bunch of C variables. The "obvious" way
> would be calling the QObject accessors. By using the visitors you
> get much the error checking code for free. YMMV.
>
> 2. With the QObject output visitor, it's an alternative way to build up
> a QObject. The "obvious" way would be calling the constructors
> directly.
>
> 3. With the string input / output visitors, it's a way to parse / format
> string visitor syntax without having to construct the C type first.
>
> Right now, we have no virtual list walks outside tests. We do have
> virtual struct walks outside tests.
>
>> Or mixing types
>>
>> visit_start_list();
>> visit_next_list();
>> visit_type_int64();
>> visit_next_list();
>> visit_type_uint64();
>
> Another excellent question.
>
> QAPI can only express homogeneous lists, i.e. lists of some type T.
>
> The generated visit_type_TList call the same visit_type_T() for all list
> members.
Okay, my point would be: Somebody coding its own visit code should not
assume this to work.
>
> QAPI type 'any' is the top type, but visit_type_anyList() still calls
> visit_type_any() for all list members.
>
> Virtual walks could of course do anything. Since they don't exist
> outside tests, we can outlaw doing things that cause us trouble.
>
> The virtual walks we currently have in tests/ seem to walk only
> homogeneous lists, i.e. always call the same visit_type_T().
Okay, so bailing out if types are switched (e.g. just about to report a
range of uin64_t and somebody asks for a int64_t) is valid.
>
>> IOW, can I assume that after every visit_next_list(), visit_type_X is
>> called, and that X remains the same for one pass over the list?
>
> As far as I can tell, existing code is just fine with that assumption.
> We'd have to write it into the contract, though.
>
>> Also, I assume it is supposed to work like this
>>
>> visit_start_list();
>> visit_next_list(); more input, returns "there's more"
>> visit_type_int(); returns 1 (parses 1-3, buffers 1-3)
>> visit_type_int(); returns 1
>> visit_type_int(); returns 1
>> visit_next_list(); more input, unbuffers 1
>> visit_type_int(); returns 2
>>
>> So unbuffering actually happens on visit_next_list()?
>
> I believe this violates the contract.
>
> If it's a real walk, the contract wants you to call visit_next_list()
> between subsequent visit_type_int().
>
> If it's a virtual walk, calling visit_next_list() makes no sense.
>
> More questions?
>
Thanks for the excessive answer! I think that should be enough to come
up with a RFC of a *rewrite* of the string input visitor :)
--
Thanks,
David / dhildenb
David Hildenbrand <david@redhat.com> writes:
>>> Would it be valid to do something like this (skipping elements without a
>>> proper visit_type_int)
>>>
>>> visit_start_list();
>>> visit_next_list(); more input, returns "there's more"
>>> visit_next_list(); parses "1-3,", buffers 2-3, skips over 1
>>> visit_type_int(); returns 2
>>> ...
>>
>> Excellent question!
>>
>> Here's the relevant part of visit_start_list()'s contract in visitor.h:
>>
>> * After visit_start_list() succeeds, the caller may visit its members
>> * one after the other. A real visit (where @obj is non-NULL) uses
>> * visit_next_list() for traversing the linked list, while a virtual
>> * visit (where @obj is NULL) uses other means. For each list
>> * element, call the appropriate visit_type_FOO() with name set to
>> * NULL and obj set to the address of the value member of the list
>> * element. Finally, visit_end_list() needs to be called with the
>> * same @list to clean up, even if intermediate visits fail. See the
>> * examples above.
>>
>> So, you *may* visit members, and you *must* call visit_end_list().
>>
>> But what are "real" and "virtual" visits? Again, the contract:
>>
>> * @list must be non-NULL for a real walk, in which case @size
>> * determines how much memory an input or clone visitor will allocate
>> * into *@list (at least sizeof(GenericList)). Some visitors also
>> * allow @list to be NULL for a virtual walk, in which case @size is
>> * ignored.
>>
>> I'm not sure whether I just decreased or increased global confusion ;)
>
> I was reading over these comments and got slightly confused :)
>
>>
>> The file comment explains:
>>
>> * The QAPI schema defines both a set of C data types, and a QMP wire
>> * format. QAPI objects can contain references to other QAPI objects,
>> * resulting in a directed acyclic graph. QAPI also generates visitor
>> * functions to walk these graphs. This file represents the interface
>> * for doing work at each node of a QAPI graph; it can also be used
>> * for a virtual walk, where there is no actual QAPI C struct.
>>
>> A real walk with an output visitor works like this (error handling
>> omitted for now):
>>
>> Error *err = NULL;
>> intList *tail;
>> size_t size = sizeof(**obj);
>>
>> // fetch list's head into *obj, start the list in output
>> visit_start_list(v, name, (GenericList **)obj, size, &err);
>>
>> // iterate over list's tails
>> // below the hood, visit_next_list() iterates over the GenericList
>> for (tail = *obj; tail;
>> tail = (intList *)visit_next_list(v, (GenericList *)tail, size)) {
>> // visit current tail's first member, add it to output
>> visit_type_int(v, NULL, &tail->value, &err);
>> }
>>
>> // end the list in output
>> visit_end_list(v, (void **)obj);
>>
>> The exact same code works for a real walk with an input visitor, where
>> visit_next_list() iterates over the *input* and builds up the
>> GenericList. Compare qobject_input_next_list() and
>> qobject_output_next_list().
>>
>> The code above is a straight copy of generated visit_type_intList() with
>> error handling cut and comments added.
>>
>> A real walk works on a QAPI-generated C type. QAPI generates a real
>> walk for each such type. Open-coding a real walk would senselessly
>> duplicate the generated one, so we don't. Thus, a real walk always
>> visits each member.
>>
>> Okay, I lied: it visits each member until it runs into an error and
>> bails out. See generated visit_type_intList() in
>> qapi/qapi-builtin-visit.c.
>>
>> A virtual walk doesn't work with a GenericList *. Calling
>> visit_next_list() makes no sense then. visitor.h gives this example of
>> a virtual walk:
>
> Alright, so we must not support virtual walks. But supporting it would
> not harm :)
Hmm, let me check something... aha! Both string-input-visitor.h and
string-output-visitor.h have this comment:
* The string input visitor does not implement support for visiting
* QAPI structs, alternates, null, or arbitrary QTypes. It also
* requires a non-null list argument to visit_start_list().
The last sentence means virtual walks are not supported. We owe thanks
to Eric Blake for his commit d9f62dde130 :)
>>
>> * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
>> * like:
>> *
>> * <example>
>> * Visitor *v;
>> * Error *err = NULL;
>> * int value;
>> *
>> * v = FOO_visitor_new(...);
>> * visit_start_struct(v, NULL, NULL, 0, &err);
>> * if (err) {
>> * goto out;
>> * }
>> * visit_start_list(v, "list", NULL, 0, &err);
>> * if (err) {
>> * goto outobj;
>> * }
>> * value = 1;
>> * visit_type_int(v, NULL, &value, &err);
>> * if (err) {
>> * goto outlist;
>> * }
>> * value = 2;
>> * visit_type_int(v, NULL, &value, &err);
>> * if (err) {
>> * goto outlist;
>> * }
>> * outlist:
>> * visit_end_list(v, NULL);
>> * if (!err) {
>> * visit_check_struct(v, &err);
>> * }
>> * outobj:
>> * visit_end_struct(v, NULL);
>> * out:
>> * error_propagate(errp, err);
>> * visit_free(v);
>>
>> Why could this be useful?
>>
>> 1. With the QObject input visitor, it's an alternative way to
>> destructure a QObject into a bunch of C variables. The "obvious" way
>> would be calling the QObject accessors. By using the visitors you
>> get much the error checking code for free. YMMV.
>>
>> 2. With the QObject output visitor, it's an alternative way to build up
>> a QObject. The "obvious" way would be calling the constructors
>> directly.
>>
>> 3. With the string input / output visitors, it's a way to parse / format
>> string visitor syntax without having to construct the C type first.
>>
>> Right now, we have no virtual list walks outside tests. We do have
>> virtual struct walks outside tests.
>>
>>> Or mixing types
>>>
>>> visit_start_list();
>>> visit_next_list();
>>> visit_type_int64();
>>> visit_next_list();
>>> visit_type_uint64();
>>
>> Another excellent question.
>>
>> QAPI can only express homogeneous lists, i.e. lists of some type T.
>>
>> The generated visit_type_TList call the same visit_type_T() for all list
>> members.
>
> Okay, my point would be: Somebody coding its own visit code should not
> assume this to work.
>
>>
>> QAPI type 'any' is the top type, but visit_type_anyList() still calls
>> visit_type_any() for all list members.
>>
>> Virtual walks could of course do anything. Since they don't exist
>> outside tests, we can outlaw doing things that cause us trouble.
>>
>> The virtual walks we currently have in tests/ seem to walk only
>> homogeneous lists, i.e. always call the same visit_type_T().
>
> Okay, so bailing out if types are switched (e.g. just about to report a
> range of uin64_t and somebody asks for a int64_t) is valid.
I think that would be okay.
>>
>>> IOW, can I assume that after every visit_next_list(), visit_type_X is
>>> called, and that X remains the same for one pass over the list?
>>
>> As far as I can tell, existing code is just fine with that assumption.
>> We'd have to write it into the contract, though.
>>
>>> Also, I assume it is supposed to work like this
>>>
>>> visit_start_list();
>>> visit_next_list(); more input, returns "there's more"
>>> visit_type_int(); returns 1 (parses 1-3, buffers 1-3)
>>> visit_type_int(); returns 1
>>> visit_type_int(); returns 1
>>> visit_next_list(); more input, unbuffers 1
>>> visit_type_int(); returns 2
>>>
>>> So unbuffering actually happens on visit_next_list()?
>>
>> I believe this violates the contract.
>>
>> If it's a real walk, the contract wants you to call visit_next_list()
>> between subsequent visit_type_int().
>>
>> If it's a virtual walk, calling visit_next_list() makes no sense.
>>
>> More questions?
>>
>
> Thanks for the excessive answer! I think that should be enough to come
> up with a RFC of a *rewrite* of the string input visitor :)
You're welcome! I love great questions, they make me *think*.
Besides, if something's worth doing, it's probably worth overdoing ;)
On 08.11.18 10:13, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>>>> Would it be valid to do something like this (skipping elements without a
>>>> proper visit_type_int)
>>>>
>>>> visit_start_list();
>>>> visit_next_list(); more input, returns "there's more"
>>>> visit_next_list(); parses "1-3,", buffers 2-3, skips over 1
>>>> visit_type_int(); returns 2
>>>> ...
>>>
>>> Excellent question!
>>>
>>> Here's the relevant part of visit_start_list()'s contract in visitor.h:
>>>
>>> * After visit_start_list() succeeds, the caller may visit its members
>>> * one after the other. A real visit (where @obj is non-NULL) uses
>>> * visit_next_list() for traversing the linked list, while a virtual
>>> * visit (where @obj is NULL) uses other means. For each list
>>> * element, call the appropriate visit_type_FOO() with name set to
>>> * NULL and obj set to the address of the value member of the list
>>> * element. Finally, visit_end_list() needs to be called with the
>>> * same @list to clean up, even if intermediate visits fail. See the
>>> * examples above.
>>>
>>> So, you *may* visit members, and you *must* call visit_end_list().
>>>
>>> But what are "real" and "virtual" visits? Again, the contract:
>>>
>>> * @list must be non-NULL for a real walk, in which case @size
>>> * determines how much memory an input or clone visitor will allocate
>>> * into *@list (at least sizeof(GenericList)). Some visitors also
>>> * allow @list to be NULL for a virtual walk, in which case @size is
>>> * ignored.
>>>
>>> I'm not sure whether I just decreased or increased global confusion ;)
>>
>> I was reading over these comments and got slightly confused :)
>>
>>>
>>> The file comment explains:
>>>
>>> * The QAPI schema defines both a set of C data types, and a QMP wire
>>> * format. QAPI objects can contain references to other QAPI objects,
>>> * resulting in a directed acyclic graph. QAPI also generates visitor
>>> * functions to walk these graphs. This file represents the interface
>>> * for doing work at each node of a QAPI graph; it can also be used
>>> * for a virtual walk, where there is no actual QAPI C struct.
>>>
>>> A real walk with an output visitor works like this (error handling
>>> omitted for now):
>>>
>>> Error *err = NULL;
>>> intList *tail;
>>> size_t size = sizeof(**obj);
>>>
>>> // fetch list's head into *obj, start the list in output
>>> visit_start_list(v, name, (GenericList **)obj, size, &err);
>>>
>>> // iterate over list's tails
>>> // below the hood, visit_next_list() iterates over the GenericList
>>> for (tail = *obj; tail;
>>> tail = (intList *)visit_next_list(v, (GenericList *)tail, size)) {
>>> // visit current tail's first member, add it to output
>>> visit_type_int(v, NULL, &tail->value, &err);
>>> }
>>>
>>> // end the list in output
>>> visit_end_list(v, (void **)obj);
>>>
>>> The exact same code works for a real walk with an input visitor, where
>>> visit_next_list() iterates over the *input* and builds up the
>>> GenericList. Compare qobject_input_next_list() and
>>> qobject_output_next_list().
>>>
>>> The code above is a straight copy of generated visit_type_intList() with
>>> error handling cut and comments added.
>>>
>>> A real walk works on a QAPI-generated C type. QAPI generates a real
>>> walk for each such type. Open-coding a real walk would senselessly
>>> duplicate the generated one, so we don't. Thus, a real walk always
>>> visits each member.
>>>
>>> Okay, I lied: it visits each member until it runs into an error and
>>> bails out. See generated visit_type_intList() in
>>> qapi/qapi-builtin-visit.c.
>>>
>>> A virtual walk doesn't work with a GenericList *. Calling
>>> visit_next_list() makes no sense then. visitor.h gives this example of
>>> a virtual walk:
>>
>> Alright, so we must not support virtual walks. But supporting it would
>> not harm :)
>
> Hmm, let me check something... aha! Both string-input-visitor.h and
> string-output-visitor.h have this comment:
>
> * The string input visitor does not implement support for visiting
> * QAPI structs, alternates, null, or arbitrary QTypes. It also
> * requires a non-null list argument to visit_start_list().
>
> The last sentence means virtual walks are not supported. We owe thanks
> to Eric Blake for his commit d9f62dde130 :)
>
>>>
>>> * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
>>> * like:
>>> *
>>> * <example>
>>> * Visitor *v;
>>> * Error *err = NULL;
>>> * int value;
>>> *
>>> * v = FOO_visitor_new(...);
>>> * visit_start_struct(v, NULL, NULL, 0, &err);
>>> * if (err) {
>>> * goto out;
>>> * }
>>> * visit_start_list(v, "list", NULL, 0, &err);
>>> * if (err) {
>>> * goto outobj;
>>> * }
>>> * value = 1;
>>> * visit_type_int(v, NULL, &value, &err);
>>> * if (err) {
>>> * goto outlist;
>>> * }
>>> * value = 2;
>>> * visit_type_int(v, NULL, &value, &err);
>>> * if (err) {
>>> * goto outlist;
>>> * }
>>> * outlist:
>>> * visit_end_list(v, NULL);
>>> * if (!err) {
>>> * visit_check_struct(v, &err);
>>> * }
>>> * outobj:
>>> * visit_end_struct(v, NULL);
>>> * out:
>>> * error_propagate(errp, err);
>>> * visit_free(v);
>>>
>>> Why could this be useful?
>>>
>>> 1. With the QObject input visitor, it's an alternative way to
>>> destructure a QObject into a bunch of C variables. The "obvious" way
>>> would be calling the QObject accessors. By using the visitors you
>>> get much the error checking code for free. YMMV.
>>>
>>> 2. With the QObject output visitor, it's an alternative way to build up
>>> a QObject. The "obvious" way would be calling the constructors
>>> directly.
>>>
>>> 3. With the string input / output visitors, it's a way to parse / format
>>> string visitor syntax without having to construct the C type first.
>>>
>>> Right now, we have no virtual list walks outside tests. We do have
>>> virtual struct walks outside tests.
>>>
>>>> Or mixing types
>>>>
>>>> visit_start_list();
>>>> visit_next_list();
>>>> visit_type_int64();
>>>> visit_next_list();
>>>> visit_type_uint64();
>>>
>>> Another excellent question.
>>>
>>> QAPI can only express homogeneous lists, i.e. lists of some type T.
>>>
>>> The generated visit_type_TList call the same visit_type_T() for all list
>>> members.
>>
>> Okay, my point would be: Somebody coding its own visit code should not
>> assume this to work.
>>
>>>
>>> QAPI type 'any' is the top type, but visit_type_anyList() still calls
>>> visit_type_any() for all list members.
>>>
>>> Virtual walks could of course do anything. Since they don't exist
>>> outside tests, we can outlaw doing things that cause us trouble.
>>>
>>> The virtual walks we currently have in tests/ seem to walk only
>>> homogeneous lists, i.e. always call the same visit_type_T().
>>
>> Okay, so bailing out if types are switched (e.g. just about to report a
>> range of uin64_t and somebody asks for a int64_t) is valid.
>
> I think that would be okay.
>
>>>
>>>> IOW, can I assume that after every visit_next_list(), visit_type_X is
>>>> called, and that X remains the same for one pass over the list?
>>>
>>> As far as I can tell, existing code is just fine with that assumption.
>>> We'd have to write it into the contract, though.
>>>
>>>> Also, I assume it is supposed to work like this
>>>>
>>>> visit_start_list();
>>>> visit_next_list(); more input, returns "there's more"
>>>> visit_type_int(); returns 1 (parses 1-3, buffers 1-3)
>>>> visit_type_int(); returns 1
>>>> visit_type_int(); returns 1
>>>> visit_next_list(); more input, unbuffers 1
>>>> visit_type_int(); returns 2
>>>>
>>>> So unbuffering actually happens on visit_next_list()?
>>>
>>> I believe this violates the contract.
>>>
>>> If it's a real walk, the contract wants you to call visit_next_list()
>>> between subsequent visit_type_int().
>>>
>>> If it's a virtual walk, calling visit_next_list() makes no sense.
>>>
>>> More questions?
>>>
>>
>> Thanks for the excessive answer! I think that should be enough to come
>> up with a RFC of a *rewrite* of the string input visitor :)
>
> You're welcome! I love great questions, they make me *think*.
>
> Besides, if something's worth doing, it's probably worth overdoing ;)
>
I found some more ugliness, looking at the tests. I am not sure the test
is correct here.
test_visitor_in_intList():
v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
-> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
visitor actually does sorting + duplicate elimination. Now I consider
this is wrong. We are parsing a list of integers, not an ordered set.
What's your take on this?
--
Thanks,
David / dhildenb
David Hildenbrand <david@redhat.com> writes:
> I found some more ugliness, looking at the tests. I am not sure the test
> is correct here.
>
> test_visitor_in_intList():
>
> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>
> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
> visitor actually does sorting + duplicate elimination. Now I consider
> this is wrong. We are parsing a list of integers, not an ordered set.
>
> What's your take on this?
I think you're right. Visitors are tied to QAPI, and QAPI does *lists*,
not sets. Lists are more general than sets.
I figure what drove development of this code was a need for sets, so
sets got implemented. Review fail.
If the visitor does lists, whatever needs sets can convert the lists to
sets.
I'd advise against evolving the current code towards sanity. Instead,
rewrite, and rely on tests to avoid unwanted changes in behavior.
On 08.11.18 15:36, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> I found some more ugliness, looking at the tests. I am not sure the test
>> is correct here.
>>
>> test_visitor_in_intList():
>>
>> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>>
>> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
>> visitor actually does sorting + duplicate elimination. Now I consider
>> this is wrong. We are parsing a list of integers, not an ordered set.
>>
>> What's your take on this?
>
> I think you're right. Visitors are tied to QAPI, and QAPI does *lists*,
> not sets. Lists are more general than sets.
>
> I figure what drove development of this code was a need for sets, so
> sets got implemented. Review fail.
>
> If the visitor does lists, whatever needs sets can convert the lists to
> sets.
>
> I'd advise against evolving the current code towards sanity. Instead,
> rewrite, and rely on tests to avoid unwanted changes in behavior.
>
With the current rewrite I have, I can parse uint64 and int64 lists. The
other types will bail out if lists are used right now.
The changes that have to be done to the tests are:
diff --git a/tests/test-string-input-visitor.c
b/tests/test-string-input-visitor.c
index 88e0e1aa9a..5d2d707e80 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -92,16 +92,6 @@ static void check_ulist(Visitor *v, uint64_t
*expected, size_t n)
uint64List *tail;
int i;
- /* BUG: unsigned numbers above INT64_MAX don't work */
- for (i = 0; i < n; i++) {
- if (expected[i] > INT64_MAX) {
- Error *err = NULL;
- visit_type_uint64List(v, NULL, &res, &err);
- error_free_or_abort(&err);
- return;
- }
- }
-
visit_type_uint64List(v, NULL, &res, &error_abort);
tail = res;
for (i = 0; i < n; i++) {
@@ -118,9 +108,9 @@ static void
test_visitor_in_intList(TestInputVisitorData *data,
const void *unused)
{
- /* Note: the visitor *sorts* ranges *unsigned* */
- int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
+ int64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3,
4, 5, 6, 7, 8 };
int64_t expect2[] = { 32767, -32768, -32767 };
- int64_t expect3[] = { INT64_MAX, INT64_MIN };
+ int64_t expect3[] = { INT64_MIN, INT64_MAX };
uint64_t expect4[] = { UINT64_MAX };
Error *err = NULL;
int64List *res = NULL;
@@ -189,7 +179,7 @@ static void
test_visitor_in_intList(TestInputVisitorData *data,
visit_type_int64(v, NULL, &tail->value, &err);
g_assert_cmpint(tail->value, ==, 0);
visit_type_int64(v, NULL, &val, &err);
- g_assert_cmpint(val, ==, 1); /* BUG */
+ error_free_or_abort(&err);
visit_check_list(v, &error_abort);
visit_end_list(v, (void **)&res);
Basically fixing two bugs (yey, let's make our tests pass by hardcoding
BUG behavior, so the actually fixed code will break them)
And converting two assumptions about ordered sets into unordered lists.
(using unsigned ranges for handling signed ranges is completely broken,
as can also be seen via the removed "Note:")
--
Thanks,
David / dhildenb
On 11/8/18 7:05 AM, David Hildenbrand wrote:
>>> Thanks for the excessive answer! I think that should be enough to come
>>> up with a RFC of a *rewrite* of the string input visitor :)
>>
>> You're welcome! I love great questions, they make me *think*.
>>
>> Besides, if something's worth doing, it's probably worth overdoing ;)
>>
>
> I found some more ugliness, looking at the tests. I am not sure the test
> is correct here.
>
> test_visitor_in_intList():
>
> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>
> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
> visitor actually does sorting + duplicate elimination. Now I consider
> this is wrong. We are parsing a list of integers, not an ordered set.
>
> What's your take on this?
The test matches the current reality (yes, the string input visitor
currently sorts and deduplicates) - but you are also right that in
general JSON [] lists are supposed to preserve order. I suspect our use
of -numa parsing relies on the result being sorted - but I would not be
opposed to a rewrite that dumbs down the string visitor to provide an
unsorted list, and move the sorting into a post-pass belonging to the
-numa code. Updating the tests to match as part of a rewrite is thus
okay, if we have the justification that we audited all other users to
see that they either don't care or that we can move the sorting out of
the list walker and into the callers that do care.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 08.11.18 15:42, Eric Blake wrote:
> On 11/8/18 7:05 AM, David Hildenbrand wrote:
>
>>>> Thanks for the excessive answer! I think that should be enough to come
>>>> up with a RFC of a *rewrite* of the string input visitor :)
>>>
>>> You're welcome! I love great questions, they make me *think*.
>>>
>>> Besides, if something's worth doing, it's probably worth overdoing ;)
>>>
>>
>> I found some more ugliness, looking at the tests. I am not sure the test
>> is correct here.
>>
>> test_visitor_in_intList():
>>
>> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>>
>> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
>> visitor actually does sorting + duplicate elimination. Now I consider
>> this is wrong. We are parsing a list of integers, not an ordered set.
>>
>> What's your take on this?
>
> The test matches the current reality (yes, the string input visitor
> currently sorts and deduplicates) - but you are also right that in
> general JSON [] lists are supposed to preserve order. I suspect our use
> of -numa parsing relies on the result being sorted - but I would not be
> opposed to a rewrite that dumbs down the string visitor to provide an
> unsorted list, and move the sorting into a post-pass belonging to the
> -numa code. Updating the tests to match as part of a rewrite is thus
> okay, if we have the justification that we audited all other users to
> see that they either don't care or that we can move the sorting out of
> the list walker and into the callers that do care.
>
-numa is parsed via the object parser and not the string input parser as
far as I understood Markus. I think only "host-nodes" for memdev would
be relevant right now, and as far as I can see, that shouldn't break
(have to take a closer look).
--
Thanks,
David / dhildenb
© 2016 - 2025 Red Hat, Inc.