Re: [Qemu-devel] [RFC PATCH 09/56] balloon: Make balloon size unsigned in QAPI/QMP

2017-08-09 Thread Dr. David Alan Gilbert 
* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  writes:
> 
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> Sizes should use QAPI type 'size' (uint64_t).  balloon parameter
> >> @value is 'int' (int64_t).  qmp_balloon() implicitly converts to
> >> ram_addr_t, i.e. uint64_t.  BALLOON_CHANGE parameter @actual and
> >> BalloonInfo member @actual are also 'int'.
> >> virtio_balloon_set_config() and virtio_balloon_stat() implicitly
> >> convert from ram_addr_t.
> >> 
> >> Change all three to 'size', and adjust the code using them.
> >> 
> >> balloon now accepts size values between 2^63 and 2^64-1.  It accepts
> >> negative values as before, because that's how the QObject input
> >> visitor works for backward compatibility.
> >> 
> >> Doing the same for HMP's balloon deserves its own commit (the next
> >> one).
> >> 
> >> BALLOON_CHANGE and query-balloon now report sizes above 2^63-1
> >> correctly instead of their (negative) two's complement.
> >> 
> >> So does HMP's "info balloon".
> >> 
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  balloon.c| 2 +-
> >>  hmp.c| 2 +-
> >>  qapi-schema.json | 4 ++--
> >>  qapi/event.json  | 2 +-
> >>  4 files changed, 5 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/balloon.c b/balloon.c
> >> index 1d720ff..2ecca76 100644
> >> --- a/balloon.c
> >> +++ b/balloon.c
> >> @@ -102,7 +102,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
> >>  return info;
> >>  }
> >>  
> >> -void qmp_balloon(int64_t target, Error **errp)
> >> +void qmp_balloon(uint64_t target, Error **errp)
> >>  {
> >>  if (!have_balloon(errp)) {
> >>  return;
> >
> > Can't you remove the:
> >   if (target <= 0) {
> >
> > check?
> 
> Functional change when target == 0.  Impact is not clear to me.

Hmm yes; I don't know whether 0 is legal.

Dave

> > (The type of the trace_balloon_event probably needs fixing
> > to be uint64_t rather than the unsigned long)
> 
> You're right.  I'll fix it.
> 
> >> diff --git a/hmp.c b/hmp.c
> >> index 8257dd0..4556045 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -781,7 +781,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict)
> >>  return;
> >>  }
> >>  
> >> -monitor_printf(mon, "balloon: actual=%" PRId64 "\n", info->actual >> 
> >> 20);
> >> +monitor_printf(mon, "balloon: actual=%" PRIu64 "\n", info->actual >> 
> >> 20);
> >>  
> >>  qapi_free_BalloonInfo(info);
> >>  }
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index 3ad2bc0..23eb60d 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -2003,7 +2003,7 @@
> >>  # Since: 0.14.0
> >>  #
> >>  ##
> >> -{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
> >> +{ 'struct': 'BalloonInfo', 'data': {'actual': 'size' } }
> >>  
> >>  ##
> >>  # @query-balloon:
> >> @@ -2603,7 +2603,7 @@
> >>  # <- { "return": {} }
> >>  #
> >>  ##
> >> -{ 'command': 'balloon', 'data': {'value': 'int'} }
> >> +{ 'command': 'balloon', 'data': {'value': 'size'} }
> >>  
> >>  ##
> >>  # @Abort:
> >> diff --git a/qapi/event.json b/qapi/event.json
> >> index 6d22b02..9dfc70b 100644
> >> --- a/qapi/event.json
> >> +++ b/qapi/event.json
> >> @@ -488,7 +488,7 @@
> >>  #
> >>  ##
> >>  { 'event': 'BALLOON_CHANGE',
> >> -  'data': { 'actual': 'int' } }
> >> +  'data': { 'actual': 'size' } }
> >
> > I was going to ask whether that was a problem for any external users,
> > but there again libvirt looks like it reads it into an unsigned long
> > long.
> 
> Yes.  See also my reply to Juan's review of PATCH 15.
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH 09/56] balloon: Make balloon size unsigned in QAPI/QMP

2017-08-08 Thread Markus Armbruster 
"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> Sizes should use QAPI type 'size' (uint64_t).  balloon parameter
>> @value is 'int' (int64_t).  qmp_balloon() implicitly converts to
>> ram_addr_t, i.e. uint64_t.  BALLOON_CHANGE parameter @actual and
>> BalloonInfo member @actual are also 'int'.
>> virtio_balloon_set_config() and virtio_balloon_stat() implicitly
>> convert from ram_addr_t.
>> 
>> Change all three to 'size', and adjust the code using them.
>> 
>> balloon now accepts size values between 2^63 and 2^64-1.  It accepts
>> negative values as before, because that's how the QObject input
>> visitor works for backward compatibility.
>> 
>> Doing the same for HMP's balloon deserves its own commit (the next
>> one).
>> 
>> BALLOON_CHANGE and query-balloon now report sizes above 2^63-1
>> correctly instead of their (negative) two's complement.
>> 
>> So does HMP's "info balloon".
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  balloon.c| 2 +-
>>  hmp.c| 2 +-
>>  qapi-schema.json | 4 ++--
>>  qapi/event.json  | 2 +-
>>  4 files changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/balloon.c b/balloon.c
>> index 1d720ff..2ecca76 100644
>> --- a/balloon.c
>> +++ b/balloon.c
>> @@ -102,7 +102,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
>>  return info;
>>  }
>>  
>> -void qmp_balloon(int64_t target, Error **errp)
>> +void qmp_balloon(uint64_t target, Error **errp)
>>  {
>>  if (!have_balloon(errp)) {
>>  return;
>
> Can't you remove the:
>   if (target <= 0) {
>
> check?

Functional change when target == 0.  Impact is not clear to me.

> (The type of the trace_balloon_event probably needs fixing
> to be uint64_t rather than the unsigned long)

You're right.  I'll fix it.

>> diff --git a/hmp.c b/hmp.c
>> index 8257dd0..4556045 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -781,7 +781,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict)
>>  return;
>>  }
>>  
>> -monitor_printf(mon, "balloon: actual=%" PRId64 "\n", info->actual >> 
>> 20);
>> +monitor_printf(mon, "balloon: actual=%" PRIu64 "\n", info->actual >> 
>> 20);
>>  
>>  qapi_free_BalloonInfo(info);
>>  }
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 3ad2bc0..23eb60d 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2003,7 +2003,7 @@
>>  # Since: 0.14.0
>>  #
>>  ##
>> -{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
>> +{ 'struct': 'BalloonInfo', 'data': {'actual': 'size' } }
>>  
>>  ##
>>  # @query-balloon:
>> @@ -2603,7 +2603,7 @@
>>  # <- { "return": {} }
>>  #
>>  ##
>> -{ 'command': 'balloon', 'data': {'value': 'int'} }
>> +{ 'command': 'balloon', 'data': {'value': 'size'} }
>>  
>>  ##
>>  # @Abort:
>> diff --git a/qapi/event.json b/qapi/event.json
>> index 6d22b02..9dfc70b 100644
>> --- a/qapi/event.json
>> +++ b/qapi/event.json
>> @@ -488,7 +488,7 @@
>>  #
>>  ##
>>  { 'event': 'BALLOON_CHANGE',
>> -  'data': { 'actual': 'int' } }
>> +  'data': { 'actual': 'size' } }
>
> I was going to ask whether that was a problem for any external users,
> but there again libvirt looks like it reads it into an unsigned long
> long.

Yes.  See also my reply to Juan's review of PATCH 15.

Re: [Qemu-devel] [RFC PATCH 09/56] balloon: Make balloon size unsigned in QAPI/QMP

2017-08-08 Thread Dr. David Alan Gilbert 
* Markus Armbruster (arm...@redhat.com) wrote:
> Sizes should use QAPI type 'size' (uint64_t).  balloon parameter
> @value is 'int' (int64_t).  qmp_balloon() implicitly converts to
> ram_addr_t, i.e. uint64_t.  BALLOON_CHANGE parameter @actual and
> BalloonInfo member @actual are also 'int'.
> virtio_balloon_set_config() and virtio_balloon_stat() implicitly
> convert from ram_addr_t.
> 
> Change all three to 'size', and adjust the code using them.
> 
> balloon now accepts size values between 2^63 and 2^64-1.  It accepts
> negative values as before, because that's how the QObject input
> visitor works for backward compatibility.
> 
> Doing the same for HMP's balloon deserves its own commit (the next
> one).
> 
> BALLOON_CHANGE and query-balloon now report sizes above 2^63-1
> correctly instead of their (negative) two's complement.
> 
> So does HMP's "info balloon".
> 
> Signed-off-by: Markus Armbruster 
> ---
>  balloon.c| 2 +-
>  hmp.c| 2 +-
>  qapi-schema.json | 4 ++--
>  qapi/event.json  | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/balloon.c b/balloon.c
> index 1d720ff..2ecca76 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -102,7 +102,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
>  return info;
>  }
>  
> -void qmp_balloon(int64_t target, Error **errp)
> +void qmp_balloon(uint64_t target, Error **errp)
>  {
>  if (!have_balloon(errp)) {
>  return;

Can't you remove the:
  if (target <= 0) {

check?

(The type of the trace_balloon_event probably needs fixing
to be uint64_t rather than the unsigned long)

> diff --git a/hmp.c b/hmp.c
> index 8257dd0..4556045 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -781,7 +781,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict)
>  return;
>  }
>  
> -monitor_printf(mon, "balloon: actual=%" PRId64 "\n", info->actual >> 20);
> +monitor_printf(mon, "balloon: actual=%" PRIu64 "\n", info->actual >> 20);
>  
>  qapi_free_BalloonInfo(info);
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3ad2bc0..23eb60d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2003,7 +2003,7 @@
>  # Since: 0.14.0
>  #
>  ##
> -{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
> +{ 'struct': 'BalloonInfo', 'data': {'actual': 'size' } }
>  
>  ##
>  # @query-balloon:
> @@ -2603,7 +2603,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'balloon', 'data': {'value': 'int'} }
> +{ 'command': 'balloon', 'data': {'value': 'size'} }
>  
>  ##
>  # @Abort:
> diff --git a/qapi/event.json b/qapi/event.json
> index 6d22b02..9dfc70b 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -488,7 +488,7 @@
>  #
>  ##
>  { 'event': 'BALLOON_CHANGE',
> -  'data': { 'actual': 'int' } }
> +  'data': { 'actual': 'size' } }

I was going to ask whether that was a problem for any external users,
but there again libvirt looks like it reads it into an unsigned long
long.

Dave

>  ##
>  # @GUEST_PANICKED:
> -- 
> 2.7.5
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [RFC PATCH 09/56] balloon: Make balloon size unsigned in QAPI/QMP

2017-08-07 Thread Markus Armbruster 
Sizes should use QAPI type 'size' (uint64_t).  balloon parameter
@value is 'int' (int64_t).  qmp_balloon() implicitly converts to
ram_addr_t, i.e. uint64_t.  BALLOON_CHANGE parameter @actual and
BalloonInfo member @actual are also 'int'.
virtio_balloon_set_config() and virtio_balloon_stat() implicitly
convert from ram_addr_t.

Change all three to 'size', and adjust the code using them.

balloon now accepts size values between 2^63 and 2^64-1.  It accepts
negative values as before, because that's how the QObject input
visitor works for backward compatibility.

Doing the same for HMP's balloon deserves its own commit (the next
one).

BALLOON_CHANGE and query-balloon now report sizes above 2^63-1
correctly instead of their (negative) two's complement.

So does HMP's "info balloon".

Signed-off-by: Markus Armbruster 
---
 balloon.c| 2 +-
 hmp.c| 2 +-
 qapi-schema.json | 4 ++--
 qapi/event.json  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/balloon.c b/balloon.c
index 1d720ff..2ecca76 100644
--- a/balloon.c
+++ b/balloon.c
@@ -102,7 +102,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
 return info;
 }
 
-void qmp_balloon(int64_t target, Error **errp)
+void qmp_balloon(uint64_t target, Error **errp)
 {
 if (!have_balloon(errp)) {
 return;
diff --git a/hmp.c b/hmp.c
index 8257dd0..4556045 100644
--- a/hmp.c
+++ b/hmp.c
@@ -781,7 +781,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict)
 return;
 }
 
-monitor_printf(mon, "balloon: actual=%" PRId64 "\n", info->actual >> 20);
+monitor_printf(mon, "balloon: actual=%" PRIu64 "\n", info->actual >> 20);
 
 qapi_free_BalloonInfo(info);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 3ad2bc0..23eb60d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2003,7 +2003,7 @@
 # Since: 0.14.0
 #
 ##
-{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
+{ 'struct': 'BalloonInfo', 'data': {'actual': 'size' } }
 
 ##
 # @query-balloon:
@@ -2603,7 +2603,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'balloon', 'data': {'value': 'int'} }
+{ 'command': 'balloon', 'data': {'value': 'size'} }
 
 ##
 # @Abort:
diff --git a/qapi/event.json b/qapi/event.json
index 6d22b02..9dfc70b 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -488,7 +488,7 @@
 #
 ##
 { 'event': 'BALLOON_CHANGE',
-  'data': { 'actual': 'int' } }
+  'data': { 'actual': 'size' } }
 
 ##
 # @GUEST_PANICKED:
-- 
2.7.5