migration: don't use uninitialized variables

The qmp_migrate method uses the 'blk' and 'inc' parameter without checking if they're valid or not (they may be uninitialized if command is received via QMP)

Signed-off-by Pawit Pornkitprasan <[email protected]>

---
 migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

migration.c +2 -2

index 9f5a423..f3d1ff7 100644
@@ -385,8 +385,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationParams params;
     const char *p;
 
-    params.blk = blk;
-    params.shared = inc;
+    params.blk = has_blk && blk;
+    params.shared = has_inc && inc;
 
     if (s->state == MIG_STATE_ACTIVE) {
         error_set(errp, QERR_MIGRATION_ACTIVE);

However, wouldn't it be nice if we improved the qapi generator to guarantee a sane default value for optional parameters, even when has_value is false?

Quoted 5 lines · Eric Blake

However, wouldn't it be nice if we improved the qapi generator to guarantee a sane default value for optional parameters, even when has_value is false?

I'm not a qemu expert or anything, but wouldn't that destroy the point of has_value in the first place?

Quoted 8 lines · Pawit Pornkitprasan

The qmp_migrate method uses the 'blk' and 'inc' parameter without checking if they're valid or not (they may be uninitialized if command is received via QMP)

Signed-off-by Pawit Pornkitprasan <[email protected]>

---
 migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by Eric Blake <[email protected]>

However, wouldn't it be nice if we improved the qapi generator to guarantee a sane default value for optional parameters, even when has_value is false?

Quoted 12 lines · On Fri, 19 Jul 2013 04:57:51 -0600 Eric Blake <[email protected]> wrote:

Quoted 8 lines · Pawit Pornkitprasan

The qmp_migrate method uses the 'blk' and 'inc' parameter without checking if they're valid or not (they may be uninitialized if command is received via QMP)

Signed-off-by Pawit Pornkitprasan <[email protected]>

---
 migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by Eric Blake <[email protected]>

Quoted 3 lines

However, wouldn't it be nice if we improved the qapi generator to guarantee a sane default value for optional parameters, even when has_value is false?

We could do that for bool and pointers, but this wouldn't help integers and enums. Also, even if we had default values, I guess I'd enforce a common idiom for handling optionals as this is also a good practice for preventing bugs.

Quoted 15 lines · Luiz Capitulino

Quoted 12 lines · On Fri, 19 Jul 2013 04:57:51 -0600 Eric Blake <[email protected]> wrote:

Quoted 8 lines · Pawit Pornkitprasan

The qmp_migrate method uses the 'blk' and 'inc' parameter without checking if they're valid or not (they may be uninitialized if command is received via QMP)

Signed-off-by Pawit Pornkitprasan <[email protected]>

---
 migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by Eric Blake <[email protected]>

Quoted 8 lines

Quoted 3 lines

However, wouldn't it be nice if we improved the qapi generator to guarantee a sane default value for optional parameters, even when has_value is false?

We could do that for bool and pointers, but this wouldn't help integers and enums. Also, even if we had default values, I guess I'd enforce a common idiom for handling optionals as this is also a good practice for preventing bugs.

I disagree on pointers.

"have_ptr && ptr->..." is a stupid idiom. The common idiom for safe dereference is "ptr && ptr->...".

"bool have_ptr, FOO *ptr" in a parameter list is unidiomatic C. Idiomatic is just "FOO *ptr".

Conciseness is a virtue.

For non-pointers, there's no special "don't have one" value, so we'd have to declare a default value in the schema to get rid of the have_FOO.

Quoted 31 lines · Markus Armbruster

Quoted 15 lines · Luiz Capitulino

Quoted 12 lines · On Fri, 19 Jul 2013 04:57:51 -0600 Eric Blake <[email protected]> wrote:

Quoted 8 lines · Pawit Pornkitprasan

The qmp_migrate method uses the 'blk' and 'inc' parameter without checking if they're valid or not (they may be uninitialized if command is received via QMP)

Signed-off-by Pawit Pornkitprasan <[email protected]>

---
 migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by Eric Blake <[email protected]>

Quoted 8 lines

Quoted 3 lines

However, wouldn't it be nice if we improved the qapi generator to guarantee a sane default value for optional parameters, even when has_value is false?

We could do that for bool and pointers, but this wouldn't help integers and enums. Also, even if we had default values, I guess I'd enforce a common idiom for handling optionals as this is also a good practice for preventing bugs.

I disagree on pointers.

"have_ptr && ptr->..." is a stupid idiom. The common idiom for safe dereference is "ptr && ptr->...".

Quoted 8 lines

"bool have_ptr, FOO *ptr" in a parameter list is unidiomatic C. Idiomatic is just "FOO *ptr".

Conciseness is a virtue.

For non-pointers, there's no special "don't have one" value, so we'd have to declare a default value in the schema to get rid of the have_FOO.

The problem is the protocol. There are cases where the absence of a value is different than having a default value for the parameter.

This was part of the discussion way back when this all was first introduced. Since everything was open coded and we had to preserve the semantics, that was the only choice we had.

Quoted 3 lines · Anthony Liguori

This was part of the discussion way back when this all was first introduced. Since everything was open coded and we had to preserve the semantics, that was the only choice we had.

Yes, there is a difference between an optional parameter with a default value and one without.

If you got a value QMP clients cannot send, the difference can be
eliminated: just pass this value to stand for "nothing". This is the case for pointers: just pass NULL. Like everybody else does when passing "nothing".

When "nothing" is to be interpreted just like a context-independent default value, then the difference doesn't matter. The client doesn't care, and the function handling the message becomes simpler.

Only when "nothing" can mean different things do you really need both have_FOO and FOO parameters.

I can't see why we couldn't introduce default values into the schema if we wanted, and use them to simplify generated code.

For pointers, we don't even need schema extensions to simplify away the silly have_FOOs.

Quoted 18 lines · Markus Armbruster

Quoted 3 lines · Anthony Liguori

This was part of the discussion way back when this all was first introduced. Since everything was open coded and we had to preserve the semantics, that was the only choice we had.

Yes, there is a difference between an optional parameter with a default value and one without.

If you got a value QMP clients cannot send, the difference can be
eliminated: just pass this value to stand for "nothing". This is the case for pointers: just pass NULL. Like everybody else does when passing "nothing".

When "nothing" is to be interpreted just like a context-independent default value, then the difference doesn't matter. The client doesn't care, and the function handling the message becomes simpler.

Quoted 2 lines

Only when "nothing" can mean different things do you really need both have_FOO and FOO parameters.

Right, and I'm saying that we have cases like this (especially with bools). Unfortunately, there are places in the protocol that have

tristate values for bool parameters and the behavior is distinctly different for false, true, and not-present.

So yes, we could special case those bits and use sentinel values for everything else. Just realize that we need to handle the above case.

Quoted 5 lines

I can't see why we couldn't introduce default values into the schema if we wanted, and use them to simplify generated code.

For pointers, we don't even need schema extensions to simplify away the silly have_FOOs.

Except you have to carefully audit all users to make sure of that.

The qmp_migrate method uses the 'blk' and 'inc' parameter without checking if they're valid or not (they may be uninitialized if command is received via QMP)

Signed-off-by Pawit Pornkitprasan <[email protected]>

---
 migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

migration.c +0 -0

index 9f5a423..f3d1ff7 100644

The qmp_migrate method uses the 'blk' and 'inc' parameter without checking if they're valid or not (they may be uninitialized if command is received via QMP)

Signed-off-by Pawit Pornkitprasan <[email protected]>

---
 migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

migration.c +0 -0

index 9f5a423..f3d1ff7 100644

migration.c +0 -0

     if (s->state == MIG_STATE_ACTIVE) {
         error_set(errp, QERR_MIGRATION_ACTIVE);

This doesn't apply anymore, can you rebase and resend?

By grepping around I can see that there are several instances of this bug in other commands. I'm surprised we never got a single bug report about this... Would you mind to fix all the instances?