[Qemu-devel] [PATCH 04/22] vl: convert -tb-size to qemu_strtoul

[Paolo Bonzini]

Reviewed-by: Richard Henderson 
Signed-off-by: Paolo Bonzini 
---
 accel/tcg/tcg-all.c| 2 +-
 include/sysemu/accel.h | 2 +-
 vl.c   | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index dba9931..e327d90 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -28,7 +28,7 @@
 #include "sysemu/sysemu.h"
 #include "qom/object.h"
 
-int tcg_tb_size;
+unsigned long tcg_tb_size;
 static bool tcg_allowed = true;
 
 static int tcg_init(MachineState *ms)
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index ecc5c84..5a632ce 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -63,7 +63,7 @@ typedef struct AccelClass {
 #define ACCEL_GET_CLASS(obj) \
 OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
 
-extern int tcg_tb_size;
+extern unsigned long tcg_tb_size;
 
 void configure_accelerator(MachineState *ms);
 /* Register accelerator specific global properties */
diff --git a/vl.c b/vl.c
index 36ff3f4..ea8ef5f 100644
--- a/vl.c
+++ b/vl.c
@@ -3933,9 +3933,9 @@ int main(int argc, char **argv, char **envp)
 configure_rtc(opts);
 break;
 case QEMU_OPTION_tb_size:
-tcg_tb_size = strtol(optarg, NULL, 0);
-if (tcg_tb_size < 0) {
-tcg_tb_size = 0;
+if (qemu_strtoul(optarg, NULL, 0, _tb_size) < 0) {
+error_report("Invalid argument to -tb-size");
+exit(1);
 }
 break;
 case QEMU_OPTION_icount:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 04/22] vl: convert -tb-size to qemu_strtoul

[Paolo Bonzini]

On 03/07/2017 21:46, Richard Henderson wrote:
> On 07/03/2017 09:34 AM, Paolo Bonzini wrote:
>> -extern int tcg_tb_size;
>> +extern unsigned long tcg_tb_size;
> 
> size_t would be more natural.
> 
> I don't think we support any hosts for which sizeof(size_t) !=
> sizeof(unsigned long), but perhaps

There's Win64...  Another place where to do the range check
could be tcg_exec_init.  That's where the actual bug lies.

The previous code's error handling was even worse, since strtol's output
was completely unchecked.  tcg_exec_init can be fixed later.

Paolo

> unsigned lomg tmp;
> if (qemu_strtoul(optarg, NULL, 0, ) < 0
> || tmp != (size_t)tmp) {
>   error_report("Invalid argument to -tb-size");
>   exit(1);
> }
> tcg_tb_size = tmp;
> 
> where I'd expect the extra comparison to be optimized away.
> 
> But I'm not overly concerned either way, so
> 
> Reviewed-by: Richard Henderson 
> 
> 
> r~

Re: [Qemu-devel] [PATCH 04/22] vl: convert -tb-size to qemu_strtoul

[Daniel P. Berrange]

On Mon, Jul 03, 2017 at 12:46:06PM -0700, Richard Henderson wrote:
> On 07/03/2017 09:34 AM, Paolo Bonzini wrote:
> > -extern int tcg_tb_size;
> > +extern unsigned long tcg_tb_size;
> 
> size_t would be more natural.
> 
> I don't think we support any hosts for which sizeof(size_t) !=
> sizeof(unsigned long), but perhaps
> 
>   unsigned lomg tmp;
>   if (qemu_strtoul(optarg, NULL, 0, ) < 0
> || tmp != (size_t)tmp) {
> error_report("Invalid argument to -tb-size");
> exit(1);
>   }
>   tcg_tb_size = tmp;
> 
> where I'd expect the extra comparison to be optimized away.

If we wanted to use size_t, then I'd suggest we added a qemu_strtoXXX
variant that directly returned a size_t, and did suitabled bounds
checking

> But I'm not overly concerned either way, so

Agreed, doesn't need fixing now

> Reviewed-by: Richard Henderson 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 04/22] vl: convert -tb-size to qemu_strtoul

[Richard Henderson]

On 07/03/2017 09:34 AM, Paolo Bonzini wrote:

-extern int tcg_tb_size;
+extern unsigned long tcg_tb_size;


size_t would be more natural.

I don't think we support any hosts for which sizeof(size_t) != sizeof(unsigned 
long), but perhaps


unsigned lomg tmp;
if (qemu_strtoul(optarg, NULL, 0, ) < 0
|| tmp != (size_t)tmp) {
  error_report("Invalid argument to -tb-size");
  exit(1);
}
tcg_tb_size = tmp;

where I'd expect the extra comparison to be optimized away.

But I'm not overly concerned either way, so

Reviewed-by: Richard Henderson 


r~

[Qemu-devel] [PATCH 04/22] vl: convert -tb-size to qemu_strtoul

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 accel/tcg/tcg-all.c| 2 +-
 include/sysemu/accel.h | 2 +-
 vl.c   | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index dba9931..e327d90 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -28,7 +28,7 @@
 #include "sysemu/sysemu.h"
 #include "qom/object.h"
 
-int tcg_tb_size;
+unsigned long tcg_tb_size;
 static bool tcg_allowed = true;
 
 static int tcg_init(MachineState *ms)
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index ecc5c84..5a632ce 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -63,7 +63,7 @@ typedef struct AccelClass {
 #define ACCEL_GET_CLASS(obj) \
 OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
 
-extern int tcg_tb_size;
+extern unsigned long tcg_tb_size;
 
 void configure_accelerator(MachineState *ms);
 /* Register accelerator specific global properties */
diff --git a/vl.c b/vl.c
index 36ff3f4..611ddfe 100644
--- a/vl.c
+++ b/vl.c
@@ -3933,9 +3933,9 @@ int main(int argc, char **argv, char **envp)
 configure_rtc(opts);
 break;
 case QEMU_OPTION_tb_size:
-tcg_tb_size = strtol(optarg, NULL, 0);
-if (tcg_tb_size < 0) {
-tcg_tb_size = 0;
+if (qemu_strtoul(optarg, NULL, 0, _tb_size) < 0) {
+error_report("Invalid argument to -tb-size");
+exit(1);
 }
 break;
 case QEMU_OPTION_icount:
-- 
1.8.3.1