Jes Sorensen <jes.soren...@redhat.com> writes: > On 10/11/10 10:51, Markus Armbruster wrote: >> jes.soren...@redhat.com 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.