[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/7] Introduce strtosz() library function to con
From: |
Jes Sorensen |
Subject: |
Re: [Qemu-devel] [PATCH 1/7] Introduce strtosz() library function to convert a string to a byte count. |
Date: |
Mon, 11 Oct 2010 14:45:28 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.9) Gecko/20100921 Fedora/3.1.4-1.fc13 Lightning/1.0b3pre Thunderbird/3.1.4 |
On 10/11/10 10:51, Markus Armbruster wrote:
> address@hidden writes:
>> +/*
>> + * Convert string to bytes, allowing either K/k for KB, M/m for MB,
>> + * G/g for GB or T/t for TB. Default without any postfix is MB.
>> + * End pointer will be returned in *end, if end is valid.
>> + * Return -1 on error.
>> + */
>> +ssize_t strtosz(const char *nptr, char **end)
>> +{
>> + int64_t value;
>
> long long, please, because that's what strtoll() returns.
I merged patches 1-3 as you suggested, so comments based on the combined
patch.
This is longer an issue as it is switched to strtod().
>> + char *endptr;
>> +
>> + value = strtoll(nptr, &endptr, 0);
>> + switch (*endptr++) {
>> + case 'K':
>> + case 'k':
>> + value <<= 10;
>> + break;
>> + case 0:
>> + case 'M':
>> + case 'm':
>> + value <<= 20;
>> + break;
>> + case 'G':
>> + case 'g':
>> + value <<= 30;
>> + break;
>> + case 'T':
>> + case 't':
>> + value <<= 40;
>> + break;
>> + default:
>> + value = -1;
>> + }
>> +
>> + if (end)
>> + *end = endptr;
>> +
>> + return value;
>
> Casts value to ssize_t, which might truncate.
The new patch does:
int64_t tmpval;
tmpval = (val * mul);
if (tmpval >= ~(size_t)0)
goto fail;
so anything that is out of bounds is checked and caught before returning
a possibly truncated valued.
> Sloppy use of strtoll(). tmpval = (val * mul);
if (tmpval >= ~(size_t)0)
goto fail;
>
> Both tolerable as long as the patch doesn't make things worse. Let's
> see:
>
>> diff --git a/qemu-common.h b/qemu-common.h
>> index 81aafa0..0a062d4 100644
>> --- a/qemu-common.h
>> +++ b/qemu-common.h
>> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
>> int qemu_fls(int i);
>> int qemu_fdatasync(int fd);
>> int fcntl_setfl(int fd, int flag);
>> +ssize_t strtosz(const char *nptr, char **end);
>>
>> /* path.c */
>> void init_paths(const char *prefix);
>> diff --git a/vl.c b/vl.c
>> index df414ef..6043fa2 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -734,16 +734,13 @@ static void numa_add(const char *optarg)
>> if (get_param_value(option, 128, "mem", optarg) == 0) {
>> node_mem[nodenr] = 0;
>> } else {
>> - value = strtoull(option, &endptr, 0);
>> - switch (*endptr) {
>> - case 0: case 'M': case 'm':
>> - value <<= 20;
>> - break;
>> - case 'G': case 'g':
>> - value <<= 30;
>> - break;
>> + ssize_t sval;
>> + sval = strtosz(option, NULL);
>> + if (sval < 0) {
>> + fprintf(stderr, "qemu: invalid numa mem size: %s\n",
>> optarg);
>> + exit(1);
>
> Before After
> Invalid number silently interpreted as zero no change
> Overflow silently capped to ULLONG_MAX LLONG_MAX, then
> trunc ssize_t
> Invalid size suffix silently ignored rejected
What do you mean by invalid number here?
LLONG_MAX seems a reasonable limit, ie. 31 bit on 32 bit hosts or 63
bits on 64 bit hosts. strtosz() is meant to return a size, so IMHO thats
a fair limitation. We could use size_t instead of ssize_t but it would
require ugly casts in any function calling the function to test for error.
> Before After
> Invalid number silently interpreted as zero no change
> Overflow silently capped to ULLONG_MAX LLONG_MAX, then
> trunc ssize_t
> Invalid size suffix rejected no change
>
> A bit more context:
>
>
> /* On 32-bit hosts, QEMU is limited by virtual address
> space */
> if (value > (2047 << 20) && HOST_LONG_BITS == 32) {
> fprintf(stderr, "qemu: at most 2047 MB RAM can be
> simulated\n");
> exit(1);
> }
> if (value != (uint64_t)(ram_addr_t)value) {
> fprintf(stderr, "qemu: ram size too large\n");
> exit(1);
> }
> ram_size = value;
> break;
>
> I'm afraid you break both conditionals for 32 bit hosts.
>
> On such hosts, ssize_t is 32 bits wide. strtosz() parses 64 bits
> internally, but truncates to 32 bits silently.
I believe the combined patch is taking care of this fine with the
>= ~(size_t)0 comparison. If the value is above that, it returns an
error. For 32 bit hosts that means we should be able to specify 2047MB
RAM fine.
The only place where I see the latter being a potential problem is on
P64 systems such as win64, since ram_addr_t is defined to be unsigned
long, but afaik we don't support win64 anyway. On both 32 bit and LP64
systems sizeof(ram_addr_t) == sizeof(ssize_t), so it should be fine.
> The old code reliably rejects values larger than 2047MiB. Your
> truncation can change a value exceeding the limit to one within the
> limit. First conditional becomes unreliable.
>
> The second conditional becomes useless: it sign-extends a non-negative
> 32 bit integer value to 64 bit, then truncates back, and checks the
> value stays the same. It trivially does.
>
> I strongly recommend to make strtosz() sane from the start, not in a
> later patch: proper error checking, including proper handling of
> overflow.
>
> Perhaps squashing 1-3/7 would get us there, or at least closer.
I have squashed 1-3, but I don't think 7 should be squashed. Adding a
new feature and taking away the old one in the same patch isn't right IMHO.
Cheers,
Jes
- [Qemu-devel] [PATCH v4 0/7] Introduce strtosz and make use of it, Jes . Sorensen, 2010/10/08
- [Qemu-devel] [PATCH 2/7] Support human unit formats in strtosz, eg. 1.0G, Jes . Sorensen, 2010/10/08
- [Qemu-devel] [PATCH 3/7] Add more error handling to strtosz(), Jes . Sorensen, 2010/10/08
- [Qemu-devel] [PATCH 4/7] Add support for 'o' octet (bytes) format as monitor parameter., Jes . Sorensen, 2010/10/08
- [Qemu-devel] [PATCH 5/7] Switch migrate_set_speed() to take an 'o' argument rather than a float., Jes . Sorensen, 2010/10/08