tst_run_cmd: added support for stdout and stderr redirection

PATCH Superseded Stanislav Kholmanskikh 2 participants 15 messages Aug 6, 2013 – Aug 12, 2013 4 messages not in the archive Library code

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stanislav Kholmanskikh

author

2013-08-06 09:12 UTC #1 lore raw

PATCH v3 1/1 tst_run_cmd: added support for stdout and stderr redirection

Now there are two functions:
tst_run_cmd() - redirects cmd's stdout/stderr to file descriptors tst_run_cmd_rdr_file() - a wrapper around tst_run_cmd(), redirects its output to files.

------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent caught up. So what steps can you take to put your SQL databases under version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-06 09:12 UTC #2 lore raw

PATCH v3 1/2 tst_run_cmd: added support for stdout and stderr redirection

In reply to a message not in the archive

From: Stanislav kholmanskikh <[email protected]>

For tst_run_cmd added support for stdout/stderr redirection to a file descriptor.

Added function tst_run_cmd_rdr_file which allows to redirect tst_run_cmd output to a file.

Signed-off-by Stanislav kholmanskikh <[email protected]>

---
 include/test.h    |   20 +++++++++++++++-
 lib/tst_run_cmd.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 83 insertions(+), 3 deletions(-)

include/test.h +19 -1

index a76fafc..ab0cdcc 100644
@@ -219,8 +219,26 @@ long tst_ncpus_max(void);
  * @argv: a list of two (at least program name + NULL) or more pointers that
  * represent the argument list to the new program. The array of pointers
  * must be terminated by a NULL pointer.
+ * @stdout_fd: file descriptor where to redirect stdout. Set -1 if
+ * redirection is not needed.
+ * @stderr_fd: file descriptor where to redirect stderr. Set -1 if
+ * redirection is not needed.
  */
-void tst_run_cmd(void (cleanup_fn)(void), char *const argv[]);
+void tst_run_cmd(void (cleanup_fn)(void),
+		char *const argv[],
+		int stdout_fd,
+		int stderr_fd);
+
+/* Executes tst_run_cmd() and redirects its output to a file
+ * @stdout_path: path where to redirect stdout. Set NULL if redirection is
+ * not needed.
+ * @stderr_path: path where to redirect stderr. Set NULL if redirection is
+ * not needed.
+ */
+void tst_run_cmd_rdr_file(void (cleanup_fn)(void),
+			char *const argv[],
+			const char *stdout_path,
+			const char *stderr_path);
 
 #ifdef TST_USE_COMPAT16_SYSCALL
 #define TCID_BIT_SUFFIX "_16"

lib/tst_run_cmd.c +64 -2

87 lines not on this page · raw message

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent 
caught up. So what steps can you take to put your SQL databases under 
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

[email protected]

2013-08-06 11:14 UTC #3 lore raw

Hi!

Quoted 31 lines

For tst_run_cmd added support for stdout/stderr redirection to a file descriptor.

Added function tst_run_cmd_rdr_file which allows to redirect tst_run_cmd output to a file.

Signed-off-by Stanislav kholmanskikh <[email protected]>

---
 include/test.h    |   20 +++++++++++++++-
 lib/tst_run_cmd.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 83 insertions(+), 3 deletions(-)

include/test.h +9 -1

index a76fafc..ab0cdcc 100644
@@ -219,8 +219,26 @@ long tst_ncpus_max(void);
  * @argv: a list of two (at least program name + NULL) or more pointers that
  * represent the argument list to the new program. The array of pointers
  * must be terminated by a NULL pointer.
+ * @stdout_fd: file descriptor where to redirect stdout. Set -1 if
+ * redirection is not needed.
+ * @stderr_fd: file descriptor where to redirect stderr. Set -1 if
+ * redirection is not needed.
  */
-void tst_run_cmd(void (cleanup_fn)(void), char *const argv[]);
+void tst_run_cmd(void (cleanup_fn)(void),
+		char *const argv[],
+		int stdout_fd,
+		int stderr_fd);
+

include/test.h long tst_ncpus_max(void); · line 241

Quoted 10 lines

include/test.h +10 -0

+/* Executes tst_run_cmd() and redirects its output to a file
+ * @stdout_path: path where to redirect stdout. Set NULL if redirection is
+ * not needed.
+ * @stderr_path: path where to redirect stderr. Set NULL if redirection is
+ * not needed.
+ */
+void tst_run_cmd_rdr_file(void (cleanup_fn)(void),
+			char *const argv[],
+			const char *stdout_path,
+			const char *stderr_path);

This is relatively minor but the function names are not best choosen as both of them does the redirections but only one of them has it in the name.

What about

tst_run_cmd_fds() and tst_run_cmd_files()

tst_run_cmd_fds() and just tst_run_cmd()

Naming things right is hard.

Quoted 82 lines

lib/tst_run_cmd.c +54 -2

@@ -19,12 +19,21 @@
  *
  */
 
+#include <errno.h>
 #include <sys/types.h>
+#include <sys/stat.h>
 #include <sys/wait.h>
+#include <fcntl.h>
 #include <unistd.h>
 #include "test.h"
 
-void tst_run_cmd(void (cleanup_fn)(void), char *const argv[])
+#define OPEN_MODE	(S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH)
+#define OPEN_FLAGS	(O_WRONLY | O_APPEND | O_CREAT)
+
+void tst_run_cmd(void (cleanup_fn)(void),
+		char *const argv[],
+		int stdout_fd,
+		int stderr_fd)
 {
 	if (argv == NULL || argv[0] == NULL) {
 		tst_brkm(TBROK, cleanup_fn,

@@ -36,8 +45,20 @@ void tst_run_cmd(void (cleanup_fn)(void), char *const argv[])
 		tst_brkm(TBROK | TERRNO, cleanup_fn, "vfork failed at %s:%d",
 			__FILE__, __LINE__);
 	}
-	if (!pid)
+	if (!pid) {
+		/* redirecting stdout and stderr if needed */
+		if (stdout_fd != -1) {
+			close(STDOUT_FILENO);
+			dup2(stdout_fd, STDOUT_FILENO);
+		}
+
+		if (stderr_fd != -1) {
+			close(STDERR_FILENO);
+			dup2(stderr_fd, STDERR_FILENO);
+		}
+
 		_exit(execvp(argv[0], argv));
+	}
 
 	int ret = -1;
 	if (waitpid(pid, &ret, 0) != pid) {

@@ -50,3 +71,44 @@ void tst_run_cmd(void (cleanup_fn)(void), char *const argv[])
 			argv[0], __FILE__, __LINE__);
 	}
 }
+
+void tst_run_cmd_rdr_file(void (cleanup_fn)(void),
+			char *const argv[],
+			const char *stdout_path,
+			const char *stderr_path)
+{
+	int stdout_fd = -1;
+	int stderr_fd = -1;
+
+	if (stdout_path != NULL) {
+		stdout_fd = open(stdout_path,
+				OPEN_FLAGS, OPEN_MODE);
+
+		if (stdout_fd == -1)
+			tst_resm(TWARN | TERRNO,
+				"open() on %s failed at %s:%d",
+				stdout_path, __FILE__, __LINE__);
+	}
+
+	if (stderr_path != NULL) {
+		stderr_fd = open(stderr_path,
+				OPEN_FLAGS, OPEN_MODE);
+
+		if (stderr_fd == -1)
+			tst_resm(TWARN | TERRNO,
+				"open() on %s failed at %s:%d",
+				stderr_path, __FILE__, __LINE__);
+	}
+
+	tst_run_cmd(cleanup_fn, argv, stdout_fd, stderr_fd);
+

lib/tst_run_cmd.c void tst_run_cmd(void (cleanup_fn)(void), char *const argv[]) · line 114

Quoted 10 lines

lib/tst_run_cmd.c +10 -0

+	if ((stdout_fd != -1) && (close(stdout_fd) == -1))
+		tst_resm(TWARN | TERRNO,
+			"close() on %s failed at %s:%d",
+			stdout_path, __FILE__, __LINE__);
+
+	if ((stderr_fd != -1) && (close(stderr_fd) == -1))
+		tst_resm(TWARN | TERRNO,
+			"close() on %s failed at %s:%d",
+			stderr_path, __FILE__, __LINE__);
+}

The code looks ok.

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent 
caught up. So what steps can you take to put your SQL databases under 
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-06 12:55 UTC #4 lore raw

PATCH v4 1/2 tst_run_cmd: added support for stdout and stderr redirection

From: Stanislav kholmanskikh <[email protected]>

Added function tst_run_cmd_fds which allows redirection of stdout/stderr to file descriptors.

Now function tst_run_cmd is a wrapper to tst_run_cmd_fds.

Signed-off-by Stanislav kholmanskikh <[email protected]>

---
 include/test.h    |   20 +++++++++++++++-
 lib/tst_run_cmd.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 83 insertions(+), 3 deletions(-)

include/test.h +19 -1

index a76fafc..1957148 100644
@@ -219,8 +219,26 @@ long tst_ncpus_max(void);
  * @argv: a list of two (at least program name + NULL) or more pointers that
  * represent the argument list to the new program. The array of pointers
  * must be terminated by a NULL pointer.
+ * @stdout_fd: file descriptor where to redirect stdout. Set -1 if
+ * redirection is not needed.
+ * @stderr_fd: file descriptor where to redirect stderr. Set -1 if
+ * redirection is not needed.
  */
-void tst_run_cmd(void (cleanup_fn)(void), char *const argv[]);
+void tst_run_cmd_fds(void (cleanup_fn)(void),
+			char *const argv[],
+			int stdout_fd,
+			int stderr_fd);
+
+/* Executes tst_run_cmd_fds() and redirects its output to a file
+ * @stdout_path: path where to redirect stdout. Set NULL if redirection is
+ * not needed.
+ * @stderr_path: path where to redirect stderr. Set NULL if redirection is
+ * not needed.
+ */
+void tst_run_cmd(void (cleanup_fn)(void),
+		char *const argv[],
+		const char *stdout_path,
+		const char *stderr_path);
 
 #ifdef TST_USE_COMPAT16_SYSCALL
 #define TCID_BIT_SUFFIX "_16"

lib/tst_run_cmd.c +64 -2

87 lines not on this page · raw message

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent 
caught up. So what steps can you take to put your SQL databases under 
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

[email protected]

2013-08-06 13:04 UTC #5 lore raw

Hi!

Quoted 4 lines

Added function tst_run_cmd_fds which allows redirection of stdout/stderr to file descriptors.
Now function tst_run_cmd is a wrapper to tst_run_cmd_fds.

Both patches pushed.

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent 
caught up. So what steps can you take to put your SQL databases under 
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-06 12:55 UTC #6 lore raw

PATCH v4 2/2 lib/tst_module.c: modification for updated tst_run_cmd specification

In reply to [email protected]

From: Stanislav kholmanskikh <[email protected]>

Signed-off-by Stanislav kholmanskikh <[email protected]>

---
 lib/tst_module.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

lib/tst_module.c +2 -2

index 832da27..c0f2c8f 100644
@@ -94,12 +94,12 @@ void tst_module_load(void (cleanup_fn)(void),
 	for (i = offset; i < size; ++i)
 		mod_argv[i] = argv[i - offset];
 
-	tst_run_cmd(cleanup_fn, mod_argv);
+	tst_run_cmd(cleanup_fn, mod_argv, NULL, NULL);
 	free(mod_path);
 }
 
 void tst_module_unload(void (cleanup_fn)(void), const char *mod_name)
 {
 	char *const argv[] = { "rmmod", mod_name, NULL };
-	tst_run_cmd(cleanup_fn, argv);
+	tst_run_cmd(cleanup_fn, argv, NULL, NULL);
 }

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent 
caught up. So what steps can you take to put your SQL databases under 
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-06 09:12 UTC #7 lore raw

PATCH v3 2/2 lib/tst_module.c: modification for updated tst_run_cmd specification

In reply to a message not in the archive

From: Stanislav kholmanskikh <[email protected]>

Modified lib/tst_module.c:
* for updated tst_run_cmd specification
* to hide gcc warning "initialization discards qualifiers from pointer target type"

Signed-off-by Stanislav kholmanskikh <[email protected]>

---
 lib/tst_module.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

lib/tst_module.c +3 -3

index 832da27..0b94a26 100644
@@ -94,12 +94,12 @@ void tst_module_load(void (cleanup_fn)(void),
 	for (i = offset; i < size; ++i)
 		mod_argv[i] = argv[i - offset];
 
-	tst_run_cmd(cleanup_fn, mod_argv);
+	tst_run_cmd(cleanup_fn, mod_argv, -1, -1);
 	free(mod_path);
 }
 
 void tst_module_unload(void (cleanup_fn)(void), const char *mod_name)
 {
-	char *const argv[] = { "rmmod", mod_name, NULL };
-	tst_run_cmd(cleanup_fn, argv);
+	char *const argv[] = { "rmmod", (char *) mod_name, NULL };
+	tst_run_cmd(cleanup_fn, argv, -1, -1);
 }

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent 
caught up. So what steps can you take to put your SQL databases under 
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

[email protected]

2013-08-06 11:15 UTC #8 lore raw

Re: [LTP] [PATCH 2/2 V3] lib/tst_module.c: modification for updated tst_run_cmd specification

Hi!

Quoted 1 line

* to hide gcc warning "initialization discards qualifiers from pointer target type"

I think that correct solution is to declare the argv as:

const char *const argv[]

So that both the array and the stored strings are constant.

(Feel free to do this in separate patch)

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent 
caught up. So what steps can you take to put your SQL databases under 
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

A message that is not in the archive (sent off-list or not yet received): [email protected]

[email protected]

2013-08-07 10:24 UTC #10 lore raw

Re: [LTP] [PATCH 2/2 V3] lib/tst_module.c: modification for updated tst_run_cmd specification

Hi!

Quoted 2 lines

2) or to change argv in tst_run_cmd function declaration to const char *const argv[]

This is what I had in mind. You have to fix the function API first and then the type of the array that is passed to it.

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-07 11:23 UTC #11 lore raw

PATCH 1/1 tst_run_cmd: const correctness

From: Stanislav kholmanskikh <[email protected]>

Signed-off-by Stanislav kholmanskikh <[email protected]>

---
 include/test.h    |    4 ++--
 lib/tst_module.c  |    4 ++--
 lib/tst_run_cmd.c |    4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

include/test.h +2 -2

index 1957148..75a34ab 100644
@@ -225,7 +225,7 @@ long tst_ncpus_max(void);
  * redirection is not needed.
  */
 void tst_run_cmd_fds(void (cleanup_fn)(void),
-			char *const argv[],
+			const char *const argv[],
 			int stdout_fd,
 			int stderr_fd);

@@ -236,7 +236,7 @@ void tst_run_cmd_fds(void (cleanup_fn)(void),
  * not needed.
  */
 void tst_run_cmd(void (cleanup_fn)(void),
-		char *const argv[],
+		const char *const argv[],
 		const char *stdout_path,
 		const char *stderr_path);

lib/tst_module.c +2 -2

index c0f2c8f..028af0e 100644
@@ -85,7 +85,7 @@ void tst_module_load(void (cleanup_fn)(void),
 	while (argv && argv[size])
 		++size;
 	size += offset;
-	char *mod_argv[size + 1]; /* + NULL in the end */
+	const char *mod_argv[size + 1]; /* + NULL in the end */
 	mod_argv[size] = NULL;
 	mod_argv[0] = "insmod";
 	mod_argv[1] = mod_path;

@@ -100,6 +100,6 @@ void tst_module_load(void (cleanup_fn)(void),
 
 void tst_module_unload(void (cleanup_fn)(void), const char *mod_name)
 {
-	char *const argv[] = { "rmmod", mod_name, NULL };
+	const char *const argv[] = { "rmmod", mod_name, NULL };
 	tst_run_cmd(cleanup_fn, argv, NULL, NULL);
 }

lib/tst_run_cmd.c +2 -2

index 714b508..2c84cfd 100644
@@ -31,7 +31,7 @@
 #define OPEN_FLAGS	(O_WRONLY | O_APPEND | O_CREAT)
 
 void tst_run_cmd_fds(void (cleanup_fn)(void),
-		char *const argv[],
+		const char *const argv[],
 		int stdout_fd,
 		int stderr_fd)
 {

@@ -73,7 +73,7 @@ void tst_run_cmd_fds(void (cleanup_fn)(void),
 }
 
 void tst_run_cmd(void (cleanup_fn)(void),
-		char *const argv[],
+		const char *const argv[],
 		const char *stdout_path,
 		const char *stderr_path)
 {

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

[email protected]

2013-08-07 12:10 UTC #12 lore raw

Re: [LTP] [PATCH] tst_run_cmd: const correctness

Hi!

Quoted 7 lines

void tst_run_cmd_fds(void (cleanup_fn)(void),
include/test.h +1 -1
-			char *const argv[],
+			const char *const argv[],
 			int stdout_fd,
 			int stderr_fd);

include/test.h void tst_run_cmd_fds(void (cleanup_fn)(void), · line 241

Quoted 7 lines

include/test.h +1 -1

@@ -236,7 +236,7 @@ void tst_run_cmd_fds(void (cleanup_fn)(void),
  * not needed.
  */
 void tst_run_cmd(void (cleanup_fn)(void),
-		char *const argv[],
+		const char *const argv[],
 		const char *stdout_path,
 		const char *stderr_path);

Gosh, that fixed the warning in the test but I've overlooked that execvp() takes char *const argv[] argument so it introduced warning in the lib. Now I wonder why execvp() has such strange parameter type...

I'm trying to figure out how to fix it 'right' but it gets unnecessary complicated. I guess that we can fix this patch by casting the array in the lib before it gets to execvp().

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

A message that is not in the archive (sent off-list or not yet received): [email protected]

[email protected]

2013-08-07 12:55 UTC #14 lore raw

Re: [LTP] [PATCH] tst_run_cmd: const correctness

Hi!

Quoted 9 lines

Quoted 3 lines
I'm trying to figure out how to fix it 'right' but it gets unnecessary complicated. I guess that we can fix this patch by casting the array in the lib before it gets to execvp().
I think that casting mod_name to (char *) is easier than casting array before it gets to execvp(). And it doesn't require
any changes of tst_run_cmd interfaces (ie they accepts argv the same way as execvp does)...
Maybe leave everything as is and just cast mod_name to (char *)?

That is easier for one testcase, but will require the cast in each testcase that will use the interface. From this point of view cast in the library is better. Not elegant solution though. :(

But in the end any of the solutions will work...

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

A message that is not in the archive (sent off-list or not yet received): [email protected]

[email protected]

2013-08-07 14:24 UTC #16 lore raw

Re: [LTP] [PATCH] tst_run_cmd: const correctness

Hi!

Quoted 9 lines

Quoted 9 lines
Quoted 6 lines
I think that casting mod_name to (char *) is easier than casting array before it gets to execvp(). And it doesn't require
any changes of tst_run_cmd interfaces (ie they accepts argv the same way as execvp does)...
Maybe leave everything as is and just cast mod_name to (char *)?
That is easier for one testcase, but will require the cast in each testcase that will use the interface. From this point of view cast in the library is better. Not elegant solution though. :(

Quoted 7 lines

I google-ed the following:
http://pubs.opengroup.org/onlinepubs/009604499/functions/exec.html
section RATIONALE
So as I understood from this document execvp expects 'char *const argv[]' to prevent 'non-expert' users from
casting argv, envp from "char *[]" to "const char *[]".

Not exactly, it looks like this was choosen to be backward compatible with older code that declared the array as char *argv[] which couldn't be automatically casted to const char *const argv[]. So they simple choose variant that has at least one const and allows automatic cast from char *argv[].

...
Specifying two levels of const- qualification for the argv[] and envp[] parameters for the exec functions may seem to be the natural choice, given that these functions do not modify either the array of pointers or the characters to which the function points, but this would disallow existing correct code. Instead, only the array of pointers is noted as constant. The table of assignment compatibility for dst= src derived from the ISO C standard summarizes the compatibility:

<snip>

Since all existing code has a source type matching the first row, the column that gives the most valid combinations is the third column. The only other possibility is the fourth column, but using it would require a cast on the argv or envp arguments. It is unfortunate that the fourth column cannot be used, because the declaration a non-expert would naturally use would be that in the second row.
...

They are actually complaining that as the type exec functions use is char *const[] it couldn't be automatically casted from const char *[] as the array would loose the const attribute. Too bad C cannot gain two const attributes and cast from char *[] to const char *const[] is not possible that way exec fuction could have take any string array...

Looks like I've learned something new today ;).

Quoted 4 lines

Given this information and:
Quoted 1 line
But in the end any of the solutions will work...
I vote for a "standard" way i.e. casting array elements to (char *) before calling tst_run_cmd or execvp.

Given that we do not have any existing code passing char *[] array to the tst_run_cmd() I think that we can declare the array as const char *const[] and cast it in the library.

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Stanislav Kholmanskikh

author

2013-08-08 07:18 UTC #17 lore raw

PATCH v2 1/1 tst_run_cmd: const correctness

Signed-off-by Stanislav kholmanskikh <[email protected]>

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 include/test.h    |    4 ++--
 lib/tst_module.c  |    4 ++--
 lib/tst_run_cmd.c |    6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

include/test.h +2 -2

index 1957148..75a34ab 100644
@@ -225,7 +225,7 @@ long tst_ncpus_max(void);
  * redirection is not needed.
  */
 void tst_run_cmd_fds(void (cleanup_fn)(void),
-			char *const argv[],
+			const char *const argv[],
 			int stdout_fd,
 			int stderr_fd);

@@ -236,7 +236,7 @@ void tst_run_cmd_fds(void (cleanup_fn)(void),
  * not needed.
  */
 void tst_run_cmd(void (cleanup_fn)(void),
-		char *const argv[],
+		const char *const argv[],
 		const char *stdout_path,
 		const char *stderr_path);

lib/tst_module.c +2 -2

index c0f2c8f..028af0e 100644
@@ -85,7 +85,7 @@ void tst_module_load(void (cleanup_fn)(void),
 	while (argv && argv[size])
 		++size;
 	size += offset;
-	char *mod_argv[size + 1]; /* + NULL in the end */
+	const char *mod_argv[size + 1]; /* + NULL in the end */
 	mod_argv[size] = NULL;
 	mod_argv[0] = "insmod";
 	mod_argv[1] = mod_path;

@@ -100,6 +100,6 @@ void tst_module_load(void (cleanup_fn)(void),
 
 void tst_module_unload(void (cleanup_fn)(void), const char *mod_name)
 {
-	char *const argv[] = { "rmmod", mod_name, NULL };
+	const char *const argv[] = { "rmmod", mod_name, NULL };
 	tst_run_cmd(cleanup_fn, argv, NULL, NULL);
 }

lib/tst_run_cmd.c +3 -3

index 714b508..804103c 100644
@@ -31,7 +31,7 @@
 #define OPEN_FLAGS	(O_WRONLY | O_APPEND | O_CREAT)
 
 void tst_run_cmd_fds(void (cleanup_fn)(void),
-		char *const argv[],
+		const char *const argv[],
 		int stdout_fd,
 		int stderr_fd)
 {

@@ -57,7 +57,7 @@ void tst_run_cmd_fds(void (cleanup_fn)(void),
 			dup2(stderr_fd, STDERR_FILENO);
 		}
 
-		_exit(execvp(argv[0], argv));
+		_exit(execvp(argv[0], (char *const *)argv));
 	}
 
 	int ret = -1;

@@ -73,7 +73,7 @@ void tst_run_cmd_fds(void (cleanup_fn)(void),
 }
 
 void tst_run_cmd(void (cleanup_fn)(void),
-		char *const argv[],
+		const char *const argv[],
 		const char *stdout_path,
 		const char *stderr_path)
 {

Signature · 13 lines

1.7.1


------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

[email protected]

Applied

2013-08-12 11:51 UTC #18 lore raw

Re: [LTP] [PATCH V2] tst_run_cmd: const correctness

Hi!

Quoted 6 lines

Signed-off-by Stanislav Kholmanskikh <[email protected]>

---
 include/test.h    |    4 ++--
 lib/tst_module.c  |    4 ++--
 lib/tst_run_cmd.c |    6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

Applied, thanks.

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list