Re: [PATCH 1/5] xen/lib: Move simple_strtoul from common/vsprintf.c to lib

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2023 21:03, Shawn Anastasio wrote:
> On 7/31/23 10:52 AM, Jan Beulich wrote:
>> On 28.07.2023 23:35, Shawn Anastasio wrote:
>>> Move the simple_strtoul routine which is used throughout the codebase
>>> from vsprintf.c to its own file in xen/lib.
>>>
>>> This allows libfdt to be built on ppc64 even though xen/common doesn't
>>> build yet.
>>>
>>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>>  xen/common/vsprintf.c    | 37 -------------------------------------
>>>  xen/lib/Makefile         |  1 +
>>>  xen/lib/simple_strtoul.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 41 insertions(+), 37 deletions(-)
>>>  create mode 100644 xen/lib/simple_strtoul.c
>>
>> What about its siblings? It'll be irritating to find one here and the
>> other there.
> 
> I was debating whether to do this or not and ultimately decided to only
> make the minimum changes that were required right now. I can go ahead
> and make the change for its siblings as well.
> 
>> Also please no underscores in (new) filenames unless there's a reason
>> for this. In the case here, though, I question the need for "simple"
>> in the file name in the first place.
> 
> From a look at the other files in xen/lib there seemed to be a
> convention of naming files after the exact function they implement.
> Would you rather I rename it to just strtoul.c? Or simple-strotoul.c?

I'd prefer the former over the latter. While your observation of
convention has been true so far, I think the shorter names are to
be preferred here. They're not going to cause issues, as I don't
see us gaining non-simple_* functions.

>>> --- /dev/null
>>> +++ b/xen/lib/simple_strtoul.c
>>> @@ -0,0 +1,40 @@
>>> +/*
>>> + *  Copyright (C) 1991, 1992  Linus Torvalds
>>> + */
>>> +
>>> +#include <xen/ctype.h>
>>> +
>>> +/**
>>> + * simple_strtoul - convert a string to an unsigned long
>>> + * @cp: The start of the string
>>> + * @endp: A pointer to the end of the parsed string will be placed here
>>> + * @base: The number base to use
>>> + */
>>> +unsigned long simple_strtoul(
>>> +    const char *cp, const char **endp, unsigned int base)
>>> +{
>>> +    unsigned long result = 0,value;
>>> +
>>> +    if (!base) {
>>> +        base = 10;
>>> +        if (*cp == '0') {
>>> +            base = 8;
>>> +            cp++;
>>> +            if ((toupper(*cp) == 'X') && isxdigit(cp[1])) {
>>> +                cp++;
>>> +                base = 16;
>>> +            }
>>> +        }
>>> +    } else if (base == 16) {
>>> +        if (cp[0] == '0' && toupper(cp[1]) == 'X')
>>> +            cp += 2;
>>> +    }
>>> +    while (isxdigit(*cp) &&
>>> +           (value = isdigit(*cp) ? *cp-'0' : toupper(*cp)-'A'+10) < base) {
>>> +        result = result*base + value;
>>> +        cp++;
>>> +    }
>>> +    if (endp)
>>> +        *endp = cp;
>>> +    return result;
>>> +}
>>
>> While moving, I think it would be nice if this stopped using neither
>> Xen nor Linux style. I'm not going to insist, but doing such style
>> adjustments right here would be quite nice.
> 
> Especially if I'm going to be moving its siblings, I'd rather just copy
> the functions verbatim for this patch, if that's acceptable.

As said - I wouldn't insist, but in this case maybe I take the time
and make the slightly more "complete" change.

Jan