diff mbox series

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

Message ID 294e48747a0f9aee0be4fd178fcab029fa317528.1690579561.git.sanastasio@raptorengineering.com (mailing list archive)
State Superseded
Headers show
Series xen/ppc: Add PowerNV bare metal support | expand

Commit Message

Shawn Anastasio July 28, 2023, 9:35 p.m. UTC 
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@raptorengineering.com>
---
 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

Comments

Jan Beulich July 31, 2023, 3:52 p.m. UTC | #1 
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@raptorengineering.com>
> ---
>  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.

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.

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

Jan
Shawn Anastasio July 31, 2023, 7:03 p.m. UTC | #2 
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@raptorengineering.com>
>> ---
>>  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?

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

> Jan

Thanks,
Shawn
Jan Beulich Aug. 1, 2023, 6:02 a.m. UTC | #3 
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@raptorengineering.com>
>>> ---
>>>  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
diff mbox series

Patch

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index b278961cc3..86330d8082 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -24,43 +24,6 @@ 
 #include <asm/div64.h>
 #include <asm/page.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;
-}
-
-EXPORT_SYMBOL(simple_strtoul);
-
 /**
  * simple_strtol - convert a string to a signed long
  * @cp: The start of the string
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index b311ea739c..bce76f9742 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -13,6 +13,7 @@  lib-y += memset.o
 lib-y += muldiv64.o
 lib-y += parse-size.o
 lib-y += rbtree.o
+lib-y += simple_strtoul.o
 lib-y += sort.o
 lib-y += strcasecmp.o
 lib-y += strchr.o
diff --git a/xen/lib/simple_strtoul.c b/xen/lib/simple_strtoul.c
new file mode 100644
index 0000000000..e43760ad1d
--- /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;
+}