Re: [Qemu-devel] [PATCH v2] add resolutions via command-line

[Eric Blake]

On 09/19/2016 11:28 PM, G 3 wrote:
> Add the ability to add resolutions from the command-line. This patch
> works by
> looking for a property called 'resolutions' in the options node of
> OpenBIOS.
> If it is found all the resolutions are parsed and loaded.
> 

> +/* strlen() implementation */
> +static int strlen(const char *str)

Is it worth renaming this to make it obvious that it is your
(non-optimal) replacement, intentionally because you don't want to use
the libc version?

By the way, strlen() normally returns size_t; redefining a standard
function to return a different type is just asking for problems.

> +{
> +    int count = 0;
> +   
> +    for( ; *str != '\0'; str++) {
> +        count++;   
> +    }
> +   
> +    return count;
> +}
> +
> +

> +/* convert ascii string to number */
> +static int atoi(const char *str)
> +{
> +    int result = 0, multiplier;
> +
> +    multiplier = strlen(str) - 1;
> +    for ( ; *str != '\0'; str++) {
> +        result += (*str - '0') * pow(10, multiplier);
> +        multiplier--;
> +        if (multiplier < 0)  /* Something might be wrong if returning
> here */
> +            return result;
> +    }

EWWWW.  If you're going to (poorly) reimplement a standard function, at
the very minimum you should NOT be doing it with a dirt-slow algorithm.
atoi() already has undefined behavior on integer overflow; use that to
your advantage.  pow() (and to a lesser extent strlen()) should NEVER be
part of atoi().  Try something more like:

static int my_atoi(const char *str)
{
    int result = 0;
    while (*str) {
        result = result * 10 + *str - '0';
        str++;
    }
}

There you go.  Much nicer, shorter, and drags in fewer dependencies (and
I renamed it because I do NOT detect leading whitespace or '-', which is
a requirement of the libc atoi()).

> +
> +/* An interesting way of emulating memory allocation. */
> +static char *malloc(int size)

Again, if you're going to override a standard function, either change
the name (to make it obvious what you are doing), or at a bare minimum
match the standard signature (void *malloc(size_t)).  And overriding
malloc() without also overriding free(), realloc(), calloc(), and who
knows what else, is likely a recipe for disaster if a later programmer
sees your use of malloc and tries to add in other uses of memory
management, without realizing your override is NOT the same as the libc
version.  So my recommendation: PLEASE rename this to something that is
NOT 'malloc()', to make it OBVIOUS that you are NOT allocating heap memory.

Meanwhile, I seriously doubt you really want to be reimplementing
malloc(); you are likely headed to disaster (if you can't even override
atoi() in a sane manner, I dread what will happen when your driver runs
out of malloc() space).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature