[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 1/7] Introduce strtosz() library function to convert a string to a byte count. |
Date: |
Mon, 11 Oct 2010 16:39:02 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Jes Sorensen <address@hidden> writes:
> 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().
Okay, I'll review it.
>>> + 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?
strtoul(nptr, &eptr, base) & friends skip whitespace, eat sign + digits,
stop at first unrecognized character.
What if they can't find any digits? They store nptr in eptr and return
0.
> 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.
64 bit hosts are fine; 2^63 should remain an acceptable implementation
limit for a while.
The use in main() is fine on 32 bit: the limit is 2047MiB, which fits
into a 32 bit ssize_t. Wonder why the limit is 2047, not 2048MiB. Oh
well.
As far as I can see, the use in numa_add() is also fine, because a
node's memory can't exceed total memory.
>> 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.
Misunderstanding? I suggested to squash 1-3 *of* 7, not (1-3 and 7) of 7.
- [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
- [Qemu-devel] [PATCH 6/7] Clarify default values in migration speed argument in monitor, Jes . Sorensen, 2010/10/08