[PATCH v1] tests: getcwd.test

[Dmitry V. Levin]

On Sat, Mar 19, 2016 at 06:34:34PM +0530, Jay Joshi wrote:
> On Sat, Mar 19, 2016 at 5:07 AM, Dmitry V. Levin <ldv at altlinux.org> wrote:
> > On Fri, Mar 18, 2016 at 10:14:26AM +0530, Jay Joshi wrote:
> >> >From 6785e2dc6f15104537d2e27387279a04c9bcfb25 Mon Sep 17 00:00:00 2001
> >> From: JayRJoshi <jay.r.joshi100 at gmail.com>
> >> Date: Tue, 15 Mar 2016 16:41:39 +0530
> >> Subject: [PATCH] tests: add getcwd.test
> >
> >> +  res = syscall(__NR_getcwd, cur_dir, sizeof(cur_dir));
> >> +
> >> +  if (res != 0) {
> >> +    printf("getcwd(\"%s\", %zu) = %ld\n", cur_dir, sizeof(cur_dir), res);
> >
> > If cur_dir contains special characters, then this output won't match the
> > output produced by strace.
> 
> Yes, thanks.
> It seems that "uname" will have similar problems.

Good catch, it does.

> >From 21f07a4453e72fd1e54b5147e3af0dd813f2657a Mon Sep 17 00:00:00 2001
> From: JayRJoshi <jay.r.joshi100 at gmail.com>
> Date: Sat, 19 Mar 2016 18:15:53 +0530
> Subject: [PATCH] tests: getcwd.test added and print_escaped_string function
>  added to libtests

Please describe your changes in imperative mood, e.g. "add xyz"
instead of "xyz added", as if you are giving orders to the codebase
to change its behavior.

Also please add a ChangeLog-style entry like other commits do.

> --- /dev/null
> +++ b/tests/getcwd.c
> @@ -0,0 +1,43 @@
> +#include "tests.h"
> +
> +#include <sys/syscall.h>
> +
> +#ifdef __NR_getcwd
> +
> +# include <stdio.h>
> +# include <unistd.h>
> +# include <errno.h>
> +# include <linux/limits.h>

Use <sys/param.h> instead of <linux/limits.h>.

> +#define sample_dir "getcwd.sample_dir"

What is this?

> +int
> +main(void)
> +{
> +  long res;
> +  char cur_dir[PATH_MAX + 1];
> +
> +  res = syscall(__NR_getcwd, cur_dir, sizeof(cur_dir));
> +
> +  if (res != 0) {

Why do you check for != 0?

> +    printf("getcwd(\"");
> +    print_escaped_string(cur_dir);
> +    printf("\", %zu) = %ld\n", sizeof(cur_dir), res);
> +  } else {
> +    perror_msg_and_fail("getcwd");
> +  }

As perror_msg_and_fail never returns, this could be written in a more
compact way:

	if (res < 0)
		perror_msg_and_fail("getcwd");
	printf("getcwd(\"");
	print_escaped_string(cur_dir);
	printf("\", %zu) = %ld\n", sizeof(cur_dir), res);


> +  syscall(__NR_getcwd, cur_dir, 0);
> +
> +  printf("getcwd(%p, 0) = -1 ERANGE (%m)\n", cur_dir);
> +
> +  puts("+++ exited with 0 +++");
> +
> +  return 0;
> +}

We use TAB for indentation, please reindent.

> +
> +#else
> +
> +SKIP_MAIN_UNDEFINED("__NR_getcwd");
> +
> +#endif
> diff --git a/tests/getcwd.test b/tests/getcwd.test
> new file mode 100755
> index 0000000..c744c0e
> --- /dev/null
> +++ b/tests/getcwd.test
> @@ -0,0 +1,11 @@
> +#!/bin/sh

Please add a short comment what's being tested here.

> --- /dev/null
> +++ b/tests/print_escaped_string.c

Please rename print_escaped_string -> print_quoted_string

> @@ -0,0 +1,80 @@
> +#include "tests.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +/* Modified from string_quote() from util.c
> + * Assumes str is NUL-terminated.
> + */
> +
> +void
> +print_escaped_string(const char *str)
> +{
> + const unsigned char *ustr = (const unsigned char *) str;
> + size_t size = sizeof(str);
> +  size_t esc_size = 4 * size;
> +  char *esc_str, *s;
> + s = esc_str = (char *) malloc((char) esc_size);
> +
> +  if(s == NULL)
> +    perror_msg_and_fail("malloc(%zu)", esc_size);
> +
> + unsigned int i=0;
> + int c;
> +
> + while ((c = ustr[i++]) != '\0') {
> + switch (c) {
> + case '\"': case '\\':
> + *s++ = '\\';
> + *s++ = c;
> + break;
> + case '\f':
> + *s++ = '\\';
> + *s++ = 'f';
> + break;
> + case '\n':
> + *s++ = '\\';
> + *s++ = 'n';
> + break;
> + case '\r':
> + *s++ = '\\';
> + *s++ = 'r';
> + break;
> + case '\t':
> + *s++ = '\\';
> + *s++ = 't';
> + break;
> + case '\v':
> + *s++ = '\\';
> + *s++ = 'v';
> + break;
> + default:
> + if (c >= ' ' && c <= 0x7e)
> + *s++ = c;
> + else {
> + /* if str contains characters from ' ' to '~', than
> +           * else won't be required,
> +           * also esc_size would be 2 * size - 1.
> +           */
> + *s++ = '\\';
> + if (i + 1 < size
> +    && ustr[i + 1] >= '0'
> +    && ustr[i + 1] <= '9'
> + ) {
> + *s++ = '0' + (c >> 6);
> + *s++ = '0' + ((c >> 3) & 0x7);
> + } else {
> + if ((c >> 3) != 0) {
> + if ((c >> 6) != 0)
> + *s++ = '0' + (c >> 6);
> + *s++ = '0' + ((c >> 3) & 0x7);
> + }
> + }
> + *s++ = '0' + (c & 0x7);
> + }
> + break;
> + }
> +  }
> +  *s = '\0';
> +  printf("%s",esc_str);
> +}

Unlike string_quote(), this is not indented properly and therefore
hard to read.

esc_str seems to be leaked, but why do you need memory allocation
in the first place?


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20160319/45a749e9/attachment.bin>