[PATCH][RFC] Extend "memparse" to allow a NULL return pointer value.

[PATCH][RFC] Extend "memparse" to allow a NULL return pointer value.

[Robert P. J. Day]

Extend the memparse() routine to allow a caller to use NULL as the
second parameter value if he has no interest in that returned value.

---

  there appear to be quite a number of calls to "memparse" which have
no use for the value returned in the second parameter (the current
pointer after the successful parse), but which are still forced to
supply a valid char** address since they have no choice but to accept
that value coming back.  in many cases, that value is accepted just
before the end of the calling function, making it clear that the value
is ignored entirely, anyway.

  this patch simply allows NULL as a second parameter to show that the
caller has no interest in that returned value, and it makes that
visually more obvious as well.  theoretically, this extension is
entirely backwards compatible.

  compile-tested on i386.  not boot-time tested yet, but i'm just
asking for feedback.


diff --git a/lib/cmdline.c b/lib/cmdline.c
index f596c08..53a668f 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -128,9 +128,11 @@ char *get_options(const char *str, int nints, int *ints)

 unsigned long long memparse (char *ptr, char **retptr)
 {
-	unsigned long long ret = simple_strtoull (ptr, retptr, 0);
+	char *localptr;

-	switch (**retptr) {
+	unsigned long long ret = simple_strtoull (ptr, &localptr, 0);
+
+	switch (*localptr) {
 	case 'G':
 	case 'g':
 		ret <<= 10;
@@ -140,10 +142,15 @@ unsigned long long memparse (char *ptr, char **retptr)
 	case 'K':
 	case 'k':
 		ret <<= 10;
-		(*retptr)++;
+		localptr++;
 	default:
 		break;
 	}
+
+	if (retptr) {
+		*retptr = localptr;
+	}
+
 	return ret;
 }


-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
========================================================================

Re: [PATCH][RFC] Extend "memparse" to allow a NULL return pointer value.

[Oleg Verych]

* Sat, 15 Sep 2007 12:27:07 -0400 (EDT)

> Extend the memparse() routine to allow a caller to use NULL as the
> second parameter value if he has no interest in that returned value.

(not `he', but `it', even if `he', then better `callers' + `they')

> ---
>
>   there appear to be quite a number of calls to "memparse" which have
> no use for the value returned in the second parameter (the current
> pointer after the successful parse), but which are still forced to
> supply a valid char** address since they have no choice but to accept
> that value coming back.  in many cases, that value is accepted just
> before the end of the calling function, making it clear that the value
> is ignored entirely, anyway.

A posteriori value, stored in this pointer serves very important role: it
validates returned result. Caller must do this. But if programmer doesn't
know problems (see below), `must' melts down to `may'.

If you take a look at simple_strtoull(), it already doesn't care if this
pointer is NULL or not. (So patch is NULL :)

But take closer look. If it returns `0' (zero), it is not clear if this
zero was parsed or not, unless you can compare `ptr' and `retptr'.
Another case if entire string have no valid number to parse (see
strtol(3)).

This is problem of this particular function, that is copied form
ordinary C. For instance see <http://bugs.debian.org/431320>.
--
-o--=O`C
 #oo'L O
<___=E M

Re: [PATCH][RFC] Extend "memparse" to allow a NULL return pointer value.

[Robert P. J. Day]

On Sun, 23 Sep 2007, Oleg Verych wrote:

> * Sat, 15 Sep 2007 12:27:07 -0400 (EDT)
>
> > Extend the memparse() routine to allow a caller to use NULL as the
> > second parameter value if he has no interest in that returned value.
>
> (not `he', but `it', even if `he', then better `callers' + `they')
>
> > ---
> >
> >   there appear to be quite a number of calls to "memparse" which
> > have no use for the value returned in the second parameter (the
> > current pointer after the successful parse), but which are still
> > forced to supply a valid char** address since they have no choice
> > but to accept that value coming back.  in many cases, that value
> > is accepted just before the end of the calling function, making it
> > clear that the value is ignored entirely, anyway.
>
> A posteriori value, stored in this pointer serves very important
> role: it validates returned result. Caller must do this. But if
> programmer doesn't know problems (see below), `must' melts down to
> `may'.
>
> If you take a look at simple_strtoull(), it already doesn't care if
> this pointer is NULL or not. (So patch is NULL :)
>
> But take closer look. If it returns `0' (zero), it is not clear if
> this zero was parsed or not, unless you can compare `ptr' and
> `retptr'. Another case if entire string have no valid number to
> parse (see strtol(3)).
>
> This is problem of this particular function, that is copied form
> ordinary C. For instance see <http://bugs.debian.org/431320>.

i'm sorry, i'm not sure what you're getting at here.  all this patch
does is allow memparse to accept a NULL second argument if the caller
has no interest in what is normally passed back in that argument.
and there are certainly *numerous* places in the source where callers
are passing a bogus second argument just because they have to pass
*something*.

allowing NULL as a second arg just makes it more visually obvious that
the caller isn't interested in that return value.  or is there
something more subtle going on here that i'm not understanding?

rday
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
========================================================================