qapi/range/memory-device: fixes and cleanups

The qemu api claims to be easier to use, and the resulting code seems to agree.

Signed-off-by David Hildenbrand <[email protected]>

---
 qapi/string-input-visitor.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

qapi/string-input-visitor.c +6 -11

index b3fdd0827d..c1454f999f 100644
@@ -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);

The qemu api claims to be easier to use, and the resulting code seems to agree.

Signed-off-by David Hildenbrand <[email protected]>

---
 qapi/string-input-visitor.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

qapi/string-input-visitor.c +1 -0

index b3fdd0827d..c1454f999f 100644
@@ -20,6 +20,7 @@
 #include "qemu/option.h"
 #include "qemu/queue.h"
 #include "qemu/range.h"
+#include "qemu/cutils.h"
 
 
 struct StringInputVisitor

qapi/string-input-visitor.c +3 -4

+    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);

qapi/string-input-visitor.c +0 -0

                     if (*endptr == '\0') {
                         cur = g_malloc0(sizeof(*cur));
                         range_set_bounds(cur, start, end);

Quoted 2 lines · David Hildenbrand

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() ..."

Quoted 20 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 qapi/string-input-visitor.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

qapi/string-input-visitor.c +1 -0

index b3fdd0827d..c1454f999f 100644
@@ -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)
 

qapi/string-input-visitor.c static void free_range(void *range, void *dummy) · line 50

Quoted 5 lines

qapi/string-input-visitor.c +1 -2

 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.

Quoted 24 lines

qapi/string-input-visitor.c +3 -5

+    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;

qapi/string-input-visitor.c static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

qapi/string-input-visitor.c +1 -4

-                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...

Quoted 4 lines

qapi/string-input-visitor.c +0 -0

                     if (*endptr == '\0') {
                         cur = g_malloc0(sizeof(*cur));
                         range_set_bounds(cur, start, end);

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · David Hildenbrand

The qemu api claims to be easier to use, and the resulting code seems to agree.

Quoted 16 lines

Quoted 16 lines

qapi/string-input-visitor.c +1 -4

@@ -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;

Quoted 8 lines

qapi/string-input-visitor.c static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

qapi/string-input-visitor.c +1 -4

-                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.

Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · David Hildenbrand

The qemu api claims to be easier to use, and the resulting code seems to agree.

[...]

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

qapi/string-input-visitor.c +1 -4

@@ -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;

Quoted 8 lines

qapi/string-input-visitor.c static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

qapi/string-input-visitor.c +1 -4

-                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...

Quoted 4 lines

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.

Quoted 62 lines · Markus Armbruster

Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · David Hildenbrand

The qemu api claims to be easier to use, and the resulting code seems to agree.

[...]

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

qapi/string-input-visitor.c +1 -4

@@ -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;

Quoted 8 lines

qapi/string-input-visitor.c static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

qapi/string-input-visitor.c +1 -4

-                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...

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 73 lines · David Hildenbrand

Quoted 62 lines · Markus Armbruster

Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · David Hildenbrand

The qemu api claims to be easier to use, and the resulting code seems to agree.

[...]

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

qapi/string-input-visitor.c +1 -4

@@ -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;

Quoted 8 lines

qapi/string-input-visitor.c static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

qapi/string-input-visitor.c +1 -4

-                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...

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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?)

Quoted 17 lines · David Hildenbrand

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 83 lines · Markus Armbruster

Quoted 73 lines · David Hildenbrand

Quoted 62 lines · Markus Armbruster

Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · David Hildenbrand

The qemu api claims to be easier to use, and the resulting code seems to agree.

[...]

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

(quoted diff) +1 -4

@@ -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;

Quoted 8 lines

Review static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

(quoted diff) +1 -4

-                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...

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

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?)

Quoted 95 lines · David Hildenbrand

Quoted 83 lines · Markus Armbruster

Quoted 73 lines · David Hildenbrand

Quoted 62 lines · Markus Armbruster

Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · David Hildenbrand

The qemu api claims to be easier to use, and the resulting code seems to agree.

[...]

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

(quoted diff) +1 -4

@@ -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;

Quoted 8 lines

Review static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

(quoted diff) +1 -4

-                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...

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

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*.

Quoted 3 lines

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).

Quoted 127 lines · Markus Armbruster

Quoted 95 lines · David Hildenbrand

Quoted 83 lines · Markus Armbruster

Quoted 73 lines · David Hildenbrand

Quoted 62 lines · Markus Armbruster

Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · David Hildenbrand

The qemu api claims to be easier to use, and the resulting code seems to agree.

[...]

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

(quoted diff) +1 -4

@@ -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;

Quoted 8 lines

Review static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

(quoted diff) +1 -4

-                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...

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

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*.

Quoted 3 lines

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.

Quoted 8 lines

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?

Quoted 140 lines · David Hildenbrand

Quoted 127 lines · Markus Armbruster

Quoted 95 lines · David Hildenbrand

Quoted 83 lines · Markus Armbruster

Quoted 73 lines · David Hildenbrand

Quoted 62 lines · Markus Armbruster

Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · David Hildenbrand

The qemu api claims to be easier to use, and the resulting code seems to agree.

[...]

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

(quoted diff) +1 -4

@@ -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;

Quoted 8 lines

Review static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

(quoted diff) +1 -4

-                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...

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

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*.

Quoted 3 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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()

Quoted 155 lines · Markus Armbruster

Quoted 140 lines · David Hildenbrand

Quoted 127 lines · Markus Armbruster

Quoted 95 lines · David Hildenbrand

Quoted 83 lines · Markus Armbruster

Quoted 73 lines · David Hildenbrand

Quoted 62 lines · Markus Armbruster

Quoted 5 lines · David Hildenbrand

Quoted 4 lines · Markus Armbruster

Quoted 2 lines · David Hildenbrand

The qemu api claims to be easier to use, and the resulting code seems to agree.

[...]

Quoted 25 lines

Quoted 16 lines

Quoted 16 lines

(quoted diff) +1 -4

@@ -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;

Quoted 8 lines

Review static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) · line 73

Quoted 5 lines

(quoted diff) +1 -4

-                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...

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

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*.

Quoted 3 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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

Quoted 12 lines

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()?

Quoted 8 lines

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.

Quoted 7 lines

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().

Quoted 3 lines

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.

Quoted 11 lines

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?

Quoted 27 lines

Quoted 8 lines

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:

Quoted 7 lines

* @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 :)

Quoted 48 lines

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.

Quoted 3 lines

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 :)

Quoted 64 lines

* 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.

Quoted 7 lines

Or mixing types

visit_start_list();
visit_next_list();
visit_type_int64();
visit_next_list();
visit_type_uint64();

Quoted 6 lines

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.

Quoted 4 lines

QAPI type 'any' is the top type, but visit_type_anyList() still calls visit_type_any() for all list members.

Quoted 5 lines

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.

Quoted 24 lines

Quoted 3 lines

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.

Quoted 11 lines

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().

Quoted 4 lines

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 :)

Quoted 85 lines · David Hildenbrand

Quoted 27 lines

Quoted 8 lines

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:

Quoted 7 lines

* @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 :)

Quoted 48 lines

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.

Quoted 6 lines

Quoted 3 lines

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 :)

Quoted 78 lines

Quoted 64 lines

* 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.

Quoted 7 lines

Or mixing types

visit_start_list();
visit_next_list();
visit_type_int64();
visit_next_list();
visit_type_uint64();

Quoted 6 lines

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.

Quoted 4 lines

QAPI type 'any' is the top type, but visit_type_anyList() still calls visit_type_any() for all list members.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 29 lines

Quoted 28 lines

Quoted 3 lines

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.

Quoted 11 lines

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?

Quoted 2 lines

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?

Quoted 5 lines · David Hildenbrand

I found some more ugliness, looking at the tests. I am not sure the test is correct here.

test_visitor_in_intList():

Quoted 7 lines

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.

Quoted 7 lines

Quoted 2 lines

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?

Quoted 13 lines · David Hildenbrand

Quoted 7 lines

Quoted 2 lines

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():

Quoted 7 lines

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.

Right now, we parse uint64_t values just like int64_t values, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that an int64_t is expected.

Parse uin64_t separately. We don't have to worry about ranges.

E.g. we can now also specify

    -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
Instead of only going via negative values
    -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000

Resulting in the same values

(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr: 0xffffffffc0000000
slot: 0
node: 0

Signed-off-by David Hildenbrand <[email protected]>

---
 qapi/string-input-visitor.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

qapi/string-input-visitor.c +1 -8

index c1454f999f..f2df027325 100644
@@ -247,15 +247,16 @@ error:
 static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                               Error **errp)
 {
-    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
-    int64_t i;
-    Error *err = NULL;
-    parse_type_int64(v, name, &i, &err);
-    if (err) {
-        error_propagate(errp, err);
-    } else {
-        *obj = i;
+    StringInputVisitor *siv = to_siv(v);

qapi/string-input-visitor.c +0 -0

 static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,

It's not obvious to me why this looks so different from the code in parse_type_int64(). Should we be using qemu_strtoi64() in the pre-existing function, instead of what's there now?

Quoted 3 lines

static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,

Right now, we parse uint64_t values just like int64_t values, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that an int64_t is expected.

Parse uin64_t separately. We don't have to worry about ranges.

E.g. we can now also specify

    -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
Instead of only going via negative values
    -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000

Resulting in the same values

(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr: 0xffffffffc0000000
slot: 0
node: 0

Signed-off-by David Hildenbrand <[email protected]>

---
 qapi/string-input-visitor.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

qapi/string-input-visitor.c +0 -2

index c1454f999f..f2df027325 100644
@@ -247,15 +247,16 @@ error:
 static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                               Error **errp)
 {
-    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
-    int64_t i;

Quoted 6 lines · David Hildenbrand

Right now, we parse uint64_t values just like int64_t values, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that an int64_t is expected.

Parse uin64_t separately. We don't have to worry about ranges.

The commit message should mention *why* we don't we have to worry about ranges.

Quoted 6 lines

E.g. we can now also specify

    -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
Instead of only going via negative values
    -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000

Quoted 8 lines

Resulting in the same values

(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr: 0xffffffffc0000000
slot: 0
node: 0

Suggest to mention this makes the string-input-visitor catch up with the qobject-input-visitor, which got changed similarly in commit 5923f85fb82.

Quoted 27 lines · Markus Armbruster

Quoted 6 lines · David Hildenbrand

Right now, we parse uint64_t values just like int64_t values, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that an int64_t is expected.

Parse uin64_t separately. We don't have to worry about ranges.

The commit message should mention *why* we don't we have to worry about ranges.

Quoted 14 lines

E.g. we can now also specify

    -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
Instead of only going via negative values
    -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000

Resulting in the same values

(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr: 0xffffffffc0000000
slot: 0
node: 0

Quoted 3 lines

Suggest to mention this makes the string-input-visitor catch up with the qobject-input-visitor, which got changed similarly in commit 5923f85fb82.

One more thing: the qobject-input-visitor change also updated the corresponding output visitor. Shouldn't we do the same here?

Quoted 6 lines · David Hildenbrand

Right now, we parse uint64_t values just like int64_t values, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that an int64_t is expected.

Parse uin64_t separately. We don't have to worry about ranges.

The commit message should mention *why* we don't we have to worry about ranges.

Quoted 7 lines

E.g. we can now also specify

    -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
Instead of only going via negative values
    -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000

Quoted 8 lines

Resulting in the same values

(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr: 0xffffffffc0000000
slot: 0
node: 0

Suggest to mention this makes the string-input-visitor catch up with the qobject-input-visitor, which got changed similarly in commit 5923f85fb82.

Quoted 21 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 qapi/string-input-visitor.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

qapi/string-input-visitor.c +0 -7

index c1454f999f..f2df027325 100644
@@ -247,15 +247,16 @@ error:
 static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                               Error **errp)
 {
-    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
-    int64_t i;
-    Error *err = NULL;
-    parse_type_int64(v, name, &i, &err);
-    if (err) {
-        error_propagate(errp, err);
-    } else {

qapi/string-input-visitor.c error: · line 253

Quoted 5 lines

qapi/string-input-visitor.c +4 -1

-        *obj = i;
+    StringInputVisitor *siv = to_siv(v);
+    uint64_t val;
+
+    if (qemu_strtou64(siv->string, NULL, 0, &val)) {

Works because qemu_strtou64() accepts negative numbers and interprets them modulo 2^64.

qapi/string-input-visitor.c error: · line 255

Quoted 3 lines

qapi/string-input-visitor.c +2 -0

+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                   "an uint64 value");

I think this should be "a uint64 value".

qapi/string-input-visitor.c error: · line 262

Quoted 8 lines

qapi/string-input-visitor.c +3 -0

+        return;
     }
+
+    *obj = val;
 }
 
 static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,

Patch looks good to me otherwise.

Quoted 11 lines · Markus Armbruster

Quoted 6 lines · David Hildenbrand

Right now, we parse uint64_t values just like int64_t values, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that an int64_t is expected.

Parse uin64_t separately. We don't have to worry about ranges.

The commit message should mention *why* we don't we have to worry about ranges.

"Parse uin64_t separately. We don't have to worry about ranges as far as I can see. Ranges are parsed and processed via start_list()/next_list() and friends. parse_type_int64() only has to deal with ranges as it reuses the function parse_str(). E.g. parse_type_size() also does not have to handle ranges. (I assume that we could easily reimplement parse_type_int64() in a similar fashion, too).

The only thing that will change is that uint64_t properties that didn't expect a range will now actually bail out if a range is supplied."

I'll do some more testing.

Quoted 16 lines

Quoted 7 lines

E.g. we can now also specify

    -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
Instead of only going via negative values
    -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000

Quoted 8 lines

Resulting in the same values

(qemu) info memory-devices
Memory device [nvdimm]: "nv1"
addr: 0xffffffffc0000000
slot: 0
node: 0

Quoted 3 lines

Suggest to mention this makes the string-input-visitor catch up with the qobject-input-visitor, which got changed similarly in commit 5923f85fb82.

Yes, I will add that!

Quoted 21 lines

Quoted 21 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 qapi/string-input-visitor.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

qapi/string-input-visitor.c +0 -7

index c1454f999f..f2df027325 100644
@@ -247,15 +247,16 @@ error:
 static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                               Error **errp)
 {
-    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
-    int64_t i;
-    Error *err = NULL;
-    parse_type_int64(v, name, &i, &err);
-    if (err) {
-        error_propagate(errp, err);
-    } else {

Quoted 8 lines

qapi/string-input-visitor.c error: · line 253

Quoted 5 lines

qapi/string-input-visitor.c +4 -1

-        *obj = i;
+    StringInputVisitor *siv = to_siv(v);
+    uint64_t val;
+
+    if (qemu_strtou64(siv->string, NULL, 0, &val)) {

Works because qemu_strtou64() accepts negative numbers and interprets them modulo 2^64.

I will also add a comment to the description that negative numbers will continue to work.

Quoted 1 line

Quoted 4 lines

qapi/string-input-visitor.c error: · line 255

Quoted 2 lines

qapi/string-input-visitor.c +2 -0

+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                   "an uint64 value");

I think this should be "a uint64 value".

As I am not a native speaker, I will stick to your suggestion unless somebody else speaks up.

If there are no changes, let's use a const pointer.

Reviewed-by Dr. David Alan Gilbert <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 include/qemu/range.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

include/qemu/range.h +3 -3

index f28f0c1825..7e75f4e655 100644
@@ -39,7 +39,7 @@ struct Range {
     uint64_t upb;        /* inclusive upper bound */
 };
 
-static inline void range_invariant(Range *range)
+static inline void range_invariant(const Range *range)
 {
     assert(range->lob <= range->upb || range->lob == range->upb + 1);
 }

@@ -48,14 +48,14 @@ static inline void range_invariant(Range *range)
 #define range_empty ((Range){ .lob = 1, .upb = 0 })
 
 /* Is @range empty? */
-static inline bool range_is_empty(Range *range)
+static inline bool range_is_empty(const Range *range)
 {
     range_invariant(range);
     return range->lob > range->upb;
 }
 
 /* Does @range contain @val? */
-static inline bool range_contains(Range *range, uint64_t val)
+static inline bool range_contains(const Range *range, uint64_t val)
 {
     return val >= range->lob && val <= range->upb;
 }

Add some more functions that will be used in memory-device context.

range_init(): Init using lower bound and size, check for validity range_init_nofail(): Init using lower bound and size, validity asserted range_size(): Extract the size of a range range_overlaps_range(): Check for overlaps of two ranges range_contains_range(): Check if one range is contained in the other

Signed-off-by David Hildenbrand <[email protected]>

---
 include/qemu/range.h | 62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

include/qemu/range.h +34 -0

index 7e75f4e655..ba606c6bc0 100644
@@ -112,6 +112,68 @@ static inline uint64_t range_upb(Range *range)
     return range->upb;
 }
 
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. If the range would overflow, returns -ERANGE, otherwise
+ * 0.
+ */
+static inline int QEMU_WARN_UNUSED_RESULT range_init(Range *range, uint64_t lob,
+                                                     uint64_t size)
+{
+    if (lob + size < lob) {
+        return -ERANGE;
+    }
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+    return 0;
+}
+
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. Range must not overflow.
+ */
+static inline void range_init_nofail(Range *range, uint64_t lob, uint64_t size)
+{
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+}
+
+/*
+ * Get the size of @range.
+ */
+static inline uint64_t range_size(const Range *range)
+{
+    return range->upb - range->lob + 1;

include/qemu/range.h +18 -0

+        return false;
+    }
+    return !(range2->upb < range1->lob || range1->upb < range2->lob);
+}
+
+/*
+ * Check if @range1 contains @range2. If one of the ranges is empty,
+ * the result is always "false".
+ */
+static inline bool range_contains_range(const Range *range1,
+                                        const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {
+        return false;
+    }
+    return range1->lob <= range2->lob && range1->upb >= range2->upb;
+}
+
 /*
  * Extend @range to the smallest interval that includes @extend_by, too.
  */

Quoted 55 lines · On Tue, 23 Oct 2018 17:23:03 +0200 David Hildenbrand <[email protected]> wrote:

Add some more functions that will be used in memory-device context.

range_init(): Init using lower bound and size, check for validity range_init_nofail(): Init using lower bound and size, validity asserted range_size(): Extract the size of a range range_overlaps_range(): Check for overlaps of two ranges range_contains_range(): Check if one range is contained in the other

Signed-off-by David Hildenbrand <[email protected]>

---
 include/qemu/range.h | 62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

include/qemu/range.h +34 -0

index 7e75f4e655..ba606c6bc0 100644
@@ -112,6 +112,68 @@ static inline uint64_t range_upb(Range *range)
     return range->upb;
 }
 
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. If the range would overflow, returns -ERANGE, otherwise
+ * 0.
+ */
+static inline int QEMU_WARN_UNUSED_RESULT range_init(Range *range, uint64_t lob,
+                                                     uint64_t size)
+{
+    if (lob + size < lob) {
+        return -ERANGE;
+    }
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+    return 0;
+}
+
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. Range must not overflow.
+ */
+static inline void range_init_nofail(Range *range, uint64_t lob, uint64_t size)
+{
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+}
+
+/*
+ * Get the size of @range.
+ */
+static inline uint64_t range_size(const Range *range)
+{
+    return range->upb - range->lob + 1;

include/qemu/range.h static inline uint64_t range_upb(Range *range) · line 158

Quoted 10 lines

include/qemu/range.h +10 -0

+}
+
+/*
+ * Check if @range1 overlaps with @range2. If one of the ranges is empty,
+ * the result is always "false".
+ */
+static inline bool range_overlaps_range(const Range *range1,
+                                        const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {

compilation fails with:
"error: passing argument 1 of ‘range_is_empty’ discards ‘const’ qualifier from pointer target type [-Werror]"

the same for range_invariant,
following should fix issues:

Quoted 71 lines · Igor Mammedov

Quoted 55 lines · On Tue, 23 Oct 2018 17:23:03 +0200 David Hildenbrand <[email protected]> wrote:

Add some more functions that will be used in memory-device context.

range_init(): Init using lower bound and size, check for validity range_init_nofail(): Init using lower bound and size, validity asserted range_size(): Extract the size of a range range_overlaps_range(): Check for overlaps of two ranges range_contains_range(): Check if one range is contained in the other

Signed-off-by David Hildenbrand <[email protected]>

---
 include/qemu/range.h | 62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

include/qemu/range.h +34 -0

index 7e75f4e655..ba606c6bc0 100644
@@ -112,6 +112,68 @@ static inline uint64_t range_upb(Range *range)
     return range->upb;
 }
 
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. If the range would overflow, returns -ERANGE, otherwise
+ * 0.
+ */
+static inline int QEMU_WARN_UNUSED_RESULT range_init(Range *range, uint64_t lob,
+                                                     uint64_t size)
+{
+    if (lob + size < lob) {
+        return -ERANGE;
+    }
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+    return 0;
+}
+
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. Range must not overflow.
+ */
+static inline void range_init_nofail(Range *range, uint64_t lob, uint64_t size)
+{
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+}
+
+/*
+ * Get the size of @range.
+ */
+static inline uint64_t range_size(const Range *range)
+{
+    return range->upb - range->lob + 1;

include/qemu/range.h static inline uint64_t range_upb(Range *range) · line 158

Quoted 10 lines

include/qemu/range.h +10 -0

+}
+
+/*
+ * Check if @range1 overlaps with @range2. If one of the ranges is empty,
+ * the result is always "false".
+ */
+static inline bool range_overlaps_range(const Range *range1,
+                                        const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {  

compilation fails with:
"error: passing argument 1 of ‘range_is_empty’ discards ‘const’ qualifier from pointer target type [-Werror]"

Quoted 2 lines

the same for range_invariant,
following should fix issues:

I guess you missed patch #3.

Quoted 72 lines · On Thu, 1 Nov 2018 11:29:51 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 71 lines · Igor Mammedov

Quoted 55 lines · On Tue, 23 Oct 2018 17:23:03 +0200 David Hildenbrand <[email protected]> wrote:

Add some more functions that will be used in memory-device context.

range_init(): Init using lower bound and size, check for validity range_init_nofail(): Init using lower bound and size, validity asserted range_size(): Extract the size of a range range_overlaps_range(): Check for overlaps of two ranges range_contains_range(): Check if one range is contained in the other

Signed-off-by David Hildenbrand <[email protected]>

---
 include/qemu/range.h | 62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

include/qemu/range.h +34 -0

index 7e75f4e655..ba606c6bc0 100644
@@ -112,6 +112,68 @@ static inline uint64_t range_upb(Range *range)
     return range->upb;
 }
 
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. If the range would overflow, returns -ERANGE, otherwise
+ * 0.
+ */
+static inline int QEMU_WARN_UNUSED_RESULT range_init(Range *range, uint64_t lob,
+                                                     uint64_t size)
+{
+    if (lob + size < lob) {
+        return -ERANGE;
+    }
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+    return 0;
+}
+
+/*
+ * Initialize @range to span the interval [@lob,@lob + @size - 1].
+ * @size may be 0. Range must not overflow.
+ */
+static inline void range_init_nofail(Range *range, uint64_t lob, uint64_t size)
+{
+    range->lob = lob;
+    range->upb = lob + size - 1;
+    range_invariant(range);
+}
+
+/*
+ * Get the size of @range.
+ */
+static inline uint64_t range_size(const Range *range)
+{
+    return range->upb - range->lob + 1;

include/qemu/range.h static inline uint64_t range_upb(Range *range) · line 158

Quoted 10 lines

include/qemu/range.h +10 -0

+}
+
+/*
+ * Check if @range1 overlaps with @range2. If one of the ranges is empty,
+ * the result is always "false".
+ */
+static inline bool range_overlaps_range(const Range *range1,
+                                        const Range *range2)
+{
+    if (range_is_empty(range1) || range_is_empty(range2)) {  

compilation fails with:
"error: passing argument 1 of ‘range_is_empty’ discards ‘const’ qualifier from pointer target type [-Werror]"

Quoted 4 lines

Quoted 2 lines

the same for range_invariant,
following should fix issues:

I guess you missed patch #3.

Yep, I was applying it randomly. I'd squash 3 and 4 but considering 3 it already queued there is no point in doing so.

Reviewed-by Igor Mammedov <[email protected]>

Shorter and easier to read.

Reviewed-by Dr. David Alan Gilbert <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/mem/memory-device.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

hw/mem/memory-device.c +3 -3

index 7de1ccd497..996ad1490f 100644
@@ -120,7 +120,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     g_assert(address_space_end >= address_space_start);
 
     /* address_space_start indicates the maximum alignment we expect */
-    if (QEMU_ALIGN_UP(address_space_start, align) != address_space_start) {
+    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -131,13 +131,13 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
         return 0;
     }
 
-    if (hint && QEMU_ALIGN_UP(*hint, align) != *hint) {
+    if (hint && !QEMU_IS_ALIGNED(*hint, align)) {
         error_setg(errp, "address must be aligned to 0x%" PRIx64 " bytes",
                    align);
         return 0;
     }
 
-    if (QEMU_ALIGN_UP(size, align) != size) {
+    if (!QEMU_IS_ALIGNED(size, align)) {
         error_setg(errp, "backend memory size must be multiple of 0x%"
                    PRIx64, align);
         return 0;

Should not be a problem right now, but it could theoretically happen in the future.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/mem/memory-device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

hw/mem/memory-device.c +2 -1

index 996ad1490f..8be63c8032 100644
@@ -85,7 +85,8 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size,
 
     /* will we exceed the total amount of memory specified */
     memory_device_used_region_size(OBJECT(ms), &used_region_size);
-    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
+    if (used_region_size + size < used_region_size ||
+        used_region_size + size > ms->maxram_size - ms->ram_size) {
         error_setg(errp, "not enough space, currently 0x%" PRIx64
                    " in use of total space for memory devices 0x" RAM_ADDR_FMT,
                    used_region_size, ms->maxram_size - ms->ram_size);

Should not be a problem right now, but it could theoretically happen in the future.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/mem/memory-device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

hw/mem/memory-device.c +2 -1

index 996ad1490f..8be63c8032 100644
@@ -85,7 +85,8 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size,
 
     /* will we exceed the total amount of memory specified */
     memory_device_used_region_size(OBJECT(ms), &used_region_size);
-    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
+    if (used_region_size + size < used_region_size ||
+        used_region_size + size > ms->maxram_size - ms->ram_size) {
         error_setg(errp, "not enough space, currently 0x%" PRIx64
                    " in use of total space for memory devices 0x" RAM_ADDR_FMT,
                    used_region_size, ms->maxram_size - ms->ram_size);

Let's rewrite it properly using ranges. This fixes certain overflows that are right now possible. E.g.

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly reports an error instead of succeeding.

"can't add memory device [0xffffffffc0000000:0x80000000], range overflow"

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/mem/memory-device.c | 53 ++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

hw/mem/memory-device.c +29 -22

index 8be63c8032..2fb6fc2145 100644
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -145,20 +142,24 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }
 
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], range overflow", *hint, size);
             return 0;
-        } else if ((new_addr + size) > address_space_end) {
+        }
+        if (!range_contains_range(&as, &new)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] beyond 0x%" PRIx64, new_addr, size,
-                       address_space_end);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", range_lob(&new), range_size(&new),
+                       range_lob(&as), range_size(&as));
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        if (range_init(&new, range_lob(&as), size)) {
+            error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
+                       "], range overflow", *hint, size);
+            return 0;
+        }
     }
 
     /* find address range that will fit new memory device */

@@ -166,30 +167,36 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     for (item = list; item; item = g_slist_next(item)) {
         const MemoryDeviceState *md = item->data;
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
-        uint64_t md_size, md_addr;
+        uint64_t next_addr;
+        Range tmp;
 
-        md_addr = mdc->get_addr(md);
-        md_size = memory_device_get_region_size(md, &error_abort);
+        range_init_nofail(&tmp, mdc->get_addr(md),
+                          memory_device_get_region_size(md, &error_abort));
 
-        if (ranges_overlap(md_addr, md_size, new_addr, size)) {
+        if (range_overlaps_range(&tmp, &new)) {
             if (hint) {
                 const DeviceState *d = DEVICE(md);
                 error_setg(errp, "address range conflicts with memory device"
                            " id='%s'", d->id ? d->id : "(unnamed)");
                 goto out;
             }
-            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+
+            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);
+            if (range_init(&new, next_addr, range_size(&new))) {
+                range_make_empty(&new);
+                break;
+            }
         }
     }
 
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "

Let's rewrite it properly using ranges. This fixes certain overflows that are right now possible. E.g.

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly reports an error instead of succeeding.

"can't add memory device [0xffffffffc0000000:0x80000000], range overflow"

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/mem/memory-device.c | 53 ++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

hw/mem/memory-device.c +9 -15

index 8be63c8032..2fb6fc2145 100644
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -145,20 +142,24 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }
 
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], range overflow", *hint, size);
             return 0;
-        } else if ((new_addr + size) > address_space_end) {
+        }
+        if (!range_contains_range(&as, &new)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] beyond 0x%" PRIx64, new_addr, size,
-                       address_space_end);

hw/mem/memory-device.c +6 -4

+            return 0;
+        }
     }
 
     /* find address range that will fit new memory device */

@@ -166,30 +167,36 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     for (item = list; item; item = g_slist_next(item)) {
         const MemoryDeviceState *md = item->data;
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
-        uint64_t md_size, md_addr;
+        uint64_t next_addr;
+        Range tmp;
 
-        md_addr = mdc->get_addr(md);
-        md_size = memory_device_get_region_size(md, &error_abort);
+        range_init_nofail(&tmp, mdc->get_addr(md),
+                          memory_device_get_region_size(md, &error_abort));
 
-        if (ranges_overlap(md_addr, md_size, new_addr, size)) {

hw/mem/memory-device.c +5 -1

+            if (range_init(&new, next_addr, range_size(&new))) {
+                range_make_empty(&new);
+                break;
+            }
         }
     }
 
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "

Quoted 3 lines · On Tue, 23 Oct 2018 17:23:06 +0200 David Hildenbrand <[email protected]> wrote:

Let's rewrite it properly using ranges. This fixes certain overflows that are right now possible. E.g.

Quoted 5 lines

qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \
    -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G
    -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000

Now properly reports an error instead of succeeding.

s/error/error out/

Quoted 63 lines

"can't add memory device [0xffffffffc0000000:0x80000000], range overflow"

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/mem/memory-device.c | 53 ++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

hw/mem/memory-device.c +12 -15

index 8be63c8032..2fb6fc2145 100644
@@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
-    uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
-    uint64_t new_addr = 0;
+    Range as, new = range_empty;
 
     if (!ms->device_memory) {
         error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

@@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                          "enabled, please specify the maxmem option");
         return 0;
     }
-    address_space_start = ms->device_memory->base;
-    address_space_end = address_space_start +
-                        memory_region_size(&ms->device_memory->mr);
-    g_assert(address_space_end >= address_space_start);
+    range_init_nofail(&as, ms->device_memory->base,
+                      memory_region_size(&ms->device_memory->mr));
 
-    /* address_space_start indicates the maximum alignment we expect */
-    if (!QEMU_IS_ALIGNED(address_space_start, align)) {
+    /* start of address space indicates the maximum alignment we expect */
+    if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
                    align);
         return 0;

@@ -145,20 +142,24 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     }
 
     if (hint) {
-        new_addr = *hint;
-        if (new_addr < address_space_start) {
+        if (range_init(&new, *hint, size)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] before 0x%" PRIx64, new_addr, size,
-                       address_space_start);
+                       "], range overflow", *hint, size);
             return 0;
-        } else if ((new_addr + size) > address_space_end) {
+        }
+        if (!range_contains_range(&as, &new)) {
             error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
-                       "] beyond 0x%" PRIx64, new_addr, size,
-                       address_space_end);
+                       "], usable range for memory devices [0x%" PRIx64 ":0x%"
+                       PRIx64 "]", range_lob(&new), range_size(&new),
+                       range_lob(&as), range_size(&as));

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 160

Quoted 7 lines

hw/mem/memory-device.c +3 -1

             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        if (range_init(&new, range_lob(&as), size)) {
+            error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64
+                       "], range overflow", *hint, size);

maybe replace "range overflow" with "too big" or something else more user friendly

Quoted 20 lines

hw/mem/memory-device.c +6 -4

+            return 0;
+        }
     }
 
     /* find address range that will fit new memory device */

@@ -166,30 +167,36 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
     for (item = list; item; item = g_slist_next(item)) {
         const MemoryDeviceState *md = item->data;
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
-        uint64_t md_size, md_addr;
+        uint64_t next_addr;
+        Range tmp;
 
-        md_addr = mdc->get_addr(md);
-        md_size = memory_device_get_region_size(md, &error_abort);
+        range_init_nofail(&tmp, mdc->get_addr(md),
+                          memory_device_get_region_size(md, &error_abort));
 
-        if (ranges_overlap(md_addr, md_size, new_addr, size)) {

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 184

Quoted 10 lines

hw/mem/memory-device.c +3 -1

+        if (range_overlaps_range(&tmp, &new)) {
             if (hint) {
                 const DeviceState *d = DEVICE(md);
                 error_setg(errp, "address range conflicts with memory device"
                            " id='%s'", d->id ? d->id : "(unnamed)");
                 goto out;
             }
-            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+
+            next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align);

                                         ^^^^^^^^^^^^^^^^^^^

this theoretically could overflow and already past 'as' check so it would return an invalid address without erroring out.

But in practice we don't have memory device container ending right on 64bit limit, so it's not really an issue.

Quoted 21 lines

hw/mem/memory-device.c +6 -2

+            if (range_init(&new, next_addr, range_size(&new))) {
+                range_make_empty(&new);
+                break;
+            }
         }
     }
 
-    if (new_addr + size > address_space_end) {
+    if (!range_contains_range(&as, &new)) {
         error_setg(errp, "could not find position in guest address space for "
                    "memory device - memory fragmented due to alignments");
         goto out;
     }
 out:
     g_slist_free(list);
-    return new_addr;
+    return range_lob(&new);
 }
 

hw/mem/memory-device.c static uint64_t memory_device_get_free_addr(MachineState *ms, · line 202

Quoted 1 line

hw/mem/memory-device.c +0 -0

 MemoryDeviceInfoList *qmp_memory_device_list(void)

beside minor notes patch looks good

While working on memory device code, I noticed that specifiying an uint64_t on command line does not work in all cases as we always parse an int64_t. So I fix that and also cleanup the old int64_t parser.

To be able to fix some overflows in memory-device code in a clean way, I am reusing the range implementation of qemu, for which I need some more helpers.

This series is based on
"[PATCH v5 00/16] memory-device: complete refactoring"
which should get pulled soon.

v2 -> v3:
- "qapi: correctly parse uint64_t values from strings" -- don't parse range
-- don't rename "parse_str"

v1 -> v2:
- "range: add some more functions"
-- Reduce number of functions
-- make range_init() return an error in case of overflow -- provide range_init_nofail()
- "memory-device: rewrite address assignment using ranges" -- Use new functions range_init/range_init_nofail
-- Use range_contains_range instead of starts_before/ends_after

David Hildenbrand (7):
qapi: use qemu_strtoi64() in parse_str
qapi: correctly parse uint64_t values from strings
range: pass const pointer where possible
range: add some more functions
memory-device: use QEMU_IS_ALIGNED
memory-device: avoid overflows on very huge devices
memory-device: rewrite address assignment using ranges

 hw/mem/memory-device.c      | 60 ++++++++++++++++++--------------
 include/qemu/range.h        | 68 +++++++++++++++++++++++++++++++++++--
 qapi/string-input-visitor.c | 34 ++++++++-----------
 3 files changed, 114 insertions(+), 48 deletions(-)

Quoted 36 lines · David Hildenbrand

While working on memory device code, I noticed that specifiying an uint64_t on command line does not work in all cases as we always parse an int64_t. So I fix that and also cleanup the old int64_t parser.

To be able to fix some overflows in memory-device code in a clean way, I am reusing the range implementation of qemu, for which I need some more helpers.

This series is based on
"[PATCH v5 00/16] memory-device: complete refactoring"
which should get pulled soon.

v2 -> v3:
- "qapi: correctly parse uint64_t values from strings" -- don't parse range
-- don't rename "parse_str"

v1 -> v2:
- "range: add some more functions"
-- Reduce number of functions
-- make range_init() return an error in case of overflow -- provide range_init_nofail()
- "memory-device: rewrite address assignment using ranges" -- Use new functions range_init/range_init_nofail
-- Use range_contains_range instead of starts_before/ends_after

David Hildenbrand (7):
qapi: use qemu_strtoi64() in parse_str
qapi: correctly parse uint64_t values from strings
range: pass const pointer where possible
range: add some more functions
memory-device: use QEMU_IS_ALIGNED
memory-device: avoid overflows on very huge devices
memory-device: rewrite address assignment using ranges

Quoted 5 lines

 hw/mem/memory-device.c      | 60 ++++++++++++++++++--------------
 include/qemu/range.h        | 68 +++++++++++++++++++++++++++++++++++--
 qapi/string-input-visitor.c | 34 ++++++++-----------
 3 files changed, 114 insertions(+), 48 deletions(-)

Any more comments? If not, I think this is good to go.