[PATCH v1] Added string conversion utility functions

[Pekka Paalanen]

On Tue, 25 Nov 2014 22:17:24 +0200
Imran Zaman <imran.zaman at gmail.com> wrote:

> Thanks Bill for your comments. Plz see my comments inline.
> 
> On Tue, Nov 25, 2014 at 9:26 PM, Bill Spitzak <spitzak at gmail.com> wrote:
> >
> >
> > On 11/24/2014 11:12 PM, Imran Zaman wrote:
> >>
> >> On Tue, Nov 25, 2014 at 1:15 AM, Bill Spitzak <spitzak at gmail.com> wrote:
> >>>
> >>>
> >>>
> >>> On 11/24/2014 11:32 AM, Imran Zaman wrote:
> >>>
> >>>> [IZ2] This is the case where endptr is used in patch. I can remove
> >>>> endptr but it seems its used so i have to keep the existing
> >>>> functionality in place.
> >>>> + if (!weston_strtoi(pid, &end, 0, &other) || end != pid + 10)
> >>>
> >>>
> >>>
> >>> Use strlen(pid) != 10 instead.
> >>
> >>
> >> This will not exactly replace the implemented logic e.g.
> >> pid="123abcdefg".
> >> strlen(pid) = 10
> >> end = 3 (end != pid + 10)
> >> So cant use the solution which u propose...
> >
> >
> > But weston_strtoi will return false in that case.
> pid="123456789".
> strlen(pid) = 10
> end = pid + 10
> .. endptr is needed by the caller of the function..

Let's look at what that piece of code originally intends to do:

xwayland/launcher.c

static int
create_lockfile(int display, char *lockfile, size_t lsize)
{
	char pid[16], *end;
	int fd, size;
	pid_t other;

	snprintf(lockfile, lsize, "/tmp/.X%d-lock", display);
	fd = open(lockfile, O_WRONLY | O_CLOEXEC | O_CREAT | O_EXCL, 0444);
	if (fd < 0 && errno == EEXIST) {
		fd = open(lockfile, O_CLOEXEC | O_RDONLY);
		if (fd < 0 || read(fd, pid, 11) != 11) {
			weston_log("can't read lock file %s: %s\n",
				lockfile, strerror(errno));
			if (fd >= 0)
				close (fd);

			errno = EEXIST;
			return -1;
		}

		other = strtol(pid, &end, 0);
		if (end != pid + 10) {
			weston_log("can't parse lock file %s\n",
				lockfile);
			close(fd);
			errno = EEXIST;
			return -1;
		}

		if (kill(other, 0) < 0 && errno == ESRCH) {
			/* stale lock file; unlink and try again */
			weston_log("unlinking stale lock file %s\n", lockfile);
			close(fd);
			if (unlink(lockfile))
				/* If we fail to unlink, return EEXIST
				   so we try the next display number.*/
				errno = EEXIST;
			else
				errno = EAGAIN;
			return -1;
		}

		close(fd);
		errno = EEXIST;
		return -1;
	} else if (fd < 0) {
		weston_log("failed to create lock file %s: %s\n",
			lockfile, strerror(errno));
		return -1;
	}

	/* Subtle detail: we use the pid of the wayland
	 * compositor, not the xserver in the lock file. */
	size = snprintf(pid, sizeof pid, "%10d\n", getpid());
	if (write(fd, pid, size) != size) {
		unlink(lockfile);
		close(fd);
		return -1;
	}

	close(fd);

	return 0;
}


'pid' is a fixed size string read in from the lock file, which is
converted into a number of type pid_t. Because the number is assumed to
be printed by "%10d\n", the file should have at least 11 bytes
available, and we assume all the 10 characters form a valid number
(with leading spaces). It's all just a way to avoid dealing with
unexpected EOF when reading the file, and to avoid not knowing in
advance how many characters long the number is.

This code still wants to parse the whole string as a single number, but
it also knows the number will end in a newline instead of nul. It
wouldn't be difficult to replace that newline with nul before parsing,
if you really want to convert this code to use a helper. But while
doing so, you have to ask yourself: does this actually make the code
any easier to understand or more correct?

> >>> If I write the following code:
> >>>
> >>>    if (strtol(string, 0, 0, 0)) ...
> >>>
> >>> I want it to crash so I notice that I passed 0 for the val output
> >>> pointer.
> >>> There is no reason to every pass null here so it must be a programmer
> >>> mistake. As you have written it this will act like there is a syntax
> >>> error
> >>> in string, which could lead a person trying to debug their code down the
> >>> wrong path.
> >>>
> >> Sorry, why is it a programmer mistake and how would it crash? Let me
> >> add the exact test code and will come back to you. It should not crash
> >> for sure IMO.
> >
> >
> > struct Foo {
> >   int first_member;
> >   int integer;
> > } Foo;
> >
> > struct Foo* f(char* text) {
> >   struct Foo* pointer;
> >   pointer = code_that_returns_NULL_unexpectedly();
> >   weston_strtoi(text, 0, 0, &(p->integer)); // crash!
> >   return pointer;
> > }
> Crash happens before entering the function :-) The whole weston code

It might not happen before entering, because evaluating &(p->integer)
does not require reading the memory pointed to by p.

> base uses pointers, so cant be avoided; its user of the functions
> which should pass in the valid arguments. So there is no need to worry
> about it :-)

Bill is right here.

If someone passes a NULL as the output pointer to weston_strtoi(), the
expected behaviour is to crash or abort.

That is, do not:
	if (!out_ptr)
		return false;

Instead, do:
	assert(out_ptr);

If assertions are enabled which is the default, you get a clear error
message and the program aborts. If assertions are not enabled or the
pointer is invalid but not NULL, you get a crash very likely. These are
the expected behaviours here for weston_strto*() specifically.

Assertions are meant for programmer errors, and passing NULL as an
output pointer is a programmer error in this particular case.

> >> At least one case above is using endptr.. if we can replace it with
> >> exactly similarly logic, I can remove it otherwies I dont see any harm
> >> in its usage

It's a trade-off between simplicity/readability and use case coverage.

Whenever you convert a call to use your new helpers, ask yourself: what
value does this change add? Changing code just for change's sake is bad.

For instance, handle_osc() in clients/terminal.c: The conversion you
did in your patch is not useful. You do not handle the
failure any way better than the original code does. If you don't handle
the failure, then what's the point of using your new helpers?

> > I have looked into that a bit more. The only use of endptr (other than to
> > replicate the tests that your function does) is in the xpm parser. In that
> > case I am not sure if it may be depending on getting a zero for zero-length.
> > It may be best to leave that code calling the original function rather than
> > risk breaking the parser.

read_xpm_color() is used by xpm_to_ximage(), which is used by
glmatrix.c. Leave that use of strtol() alone. The string parsed with
strtol is an XPM color definition, which by definition (AFAIK) is in
hex, without 0x prefix. That's actually the '#' prefix that says it's
hexadecimal.

You have to pick the cases you convert to helpers. Blindly converting
everything only causes a net increase in complexity, while the use of
helpers is supposed to reduce it.

Also remember, that when one calls a helper, it adds a tiny bit of
semantic indirection. The helper must be worth of the effort required
from the (human) reader to learn and recall what that function does.


Thanks,
pq