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

[G 3]

On Sep 20, 2016, at 9:37 AM, Eric Blake wrote:

> 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?

I originally thought about adding a prefix to all my functions. Ben  
what do you think?
I'm ok with either way.

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

Changing int to size_t isn't a problem. Will do it in my next patch.
> > +{
> > +    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()).

Ok.

> > +
> > +/* 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.

That's fine with me.


> 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).

The malloc() function used in this driver is used to allocate a very  
small amount of space at most. We are realistically talking under 2k.  
Running out of space is highly unlikely. I'm sure the user could do  
something evil to try to crash the driver, but it they succeed, it  
would be their own fault. Under normal use 2k of memory is all that  
is needed

Thank you for reviewing my patch.