Re: [ccan] [PATCH 2/9] configurator: Reimplement run using popen
On Tue, Sep 20, 2016 at 12:22:41AM -0600, Kevin Locke wrote:
> On 09/19/2016 11:00 PM, David Gibson wrote:
> > On Sun, Sep 18, 2016 at 06:51:59PM -0600, Kevin Locke wrote:
> >> Rather than using fork+pipe+system+waitpid, most of which are only
> >> available on POSIX-like systems, use popen which is also available on
> >> Windows (under the name _popen).
> >
> > Concept looks good, one little wart.
> >
> >> [...]
> >>
> >> -static char *grab_fd(int fd)
> >> +static char *grab_stream(FILE *file)
> >> {
> >> - int ret;
> >> - size_t max, size = 0;
> >> + size_t max, ret, size = 0;
> >>char *buffer;
> >>
> >> - max = 16384;
> >> - buffer = malloc(max+1);
> >> - while ((ret = read(fd, buffer + size, max - size)) > 0) {
> >> + max = BUFSIZ;
> >> + buffer = malloc(max);
> >> + while ((ret = fread(buffer+size, 1, max - size, file)) == max - size) {
> >
> > This assumes that fread() will never return a short read except on EOF
> > or error. I expect that will be true in practice for regular files,
> > but I'm not sure if it's a good idea to rely on it.
>
> Interesting. I was under the impression that the fread couldn't short read.
> POSIX describes the return value as "the number of elements successfully
> read which is less than nitems only if a read error or end-of-file is
> encountered."[1] Now that I think about it, I suppose EINTR could be an
> issue for non-glibc systems. Is that what you had in mind, or are there
> other issues I'm overlooking? (Also for fwrite in 6/9?)
Ah..so it does. I'm afraid I just looked at the Linux man page, not
the POSIX requirements, and it didn't mention this constraint.
I guess it's ok then. Although.. I do wonder a bit whether we can
trust all implementations to actually comply with this requirement.
>
> Kevin
>
> 1. http://pubs.opengroup.org/onlinepubs/9699919799/functions/fread.html
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan
Re: [ccan] [PATCH 2/9] configurator: Reimplement run using popen
On 09/19/2016 11:00 PM, David Gibson wrote:
> On Sun, Sep 18, 2016 at 06:51:59PM -0600, Kevin Locke wrote:
>> Rather than using fork+pipe+system+waitpid, most of which are only
>> available on POSIX-like systems, use popen which is also available on
>> Windows (under the name _popen).
>
> Concept looks good, one little wart.
>
>> [...]
>>
>> -static char *grab_fd(int fd)
>> +static char *grab_stream(FILE *file)
>> {
>> - int ret;
>> - size_t max, size = 0;
>> + size_t max, ret, size = 0;
>>char *buffer;
>>
>> - max = 16384;
>> - buffer = malloc(max+1);
>> - while ((ret = read(fd, buffer + size, max - size)) > 0) {
>> + max = BUFSIZ;
>> + buffer = malloc(max);
>> + while ((ret = fread(buffer+size, 1, max - size, file)) == max -
size) {
>
> This assumes that fread() will never return a short read except on EOF
> or error. I expect that will be true in practice for regular files,
> but I'm not sure if it's a good idea to rely on it.
Interesting. I was under the impression that the fread couldn't short
read. POSIX describes the return value as "the number of elements
successfully read which is less than nitems only if a read error or
end-of-file is encountered."[1] Now that I think about it, I suppose
EINTR could be an issue for non-glibc systems. Is that what you had in
mind, or are there other issues I'm overlooking? (Also for fwrite in 6/9?)
Kevin
1. http://pubs.opengroup.org/onlinepubs/9699919799/functions/fread.html
___
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan
Re: [ccan] [PATCH 2/9] configurator: Reimplement run using popen
On Sun, Sep 18, 2016 at 06:51:59PM -0600, Kevin Locke wrote:
> Rather than using fork+pipe+system+waitpid, most of which are only
> available on POSIX-like systems, use popen which is also available on
> Windows (under the name _popen).
Concept looks good, one little wart.
>
> Signed-off-by: Kevin Locke
> ---
> tools/configurator/configurator.c | 77
> +++
> 1 file changed, 29 insertions(+), 48 deletions(-)
>
> diff --git a/tools/configurator/configurator.c
> b/tools/configurator/configurator.c
> index e322c76..9817fcd 100644
> --- a/tools/configurator/configurator.c
> +++ b/tools/configurator/configurator.c
> @@ -24,14 +24,14 @@
> #include
> #include
> #include
> -#include
> #include
> -#include
> -#include
> -#include
> -#include
> #include
>
> +#ifdef _MSC_VER
> +#define popen _popen
> +#define pclose _pclose
> +#endif
> +
> #define DEFAULT_COMPILER "cc"
> #define DEFAULT_FLAGS "-g3 -ggdb -Wall -Wundef -Wmissing-prototypes
> -Wmissing-declarations -Wstrict-prototypes -Wold-style-definition"
>
> @@ -367,20 +367,19 @@ static struct test tests[] = {
> },
> };
>
> -static char *grab_fd(int fd)
> +static char *grab_stream(FILE *file)
> {
> - int ret;
> - size_t max, size = 0;
> + size_t max, ret, size = 0;
> char *buffer;
>
> - max = 16384;
> - buffer = malloc(max+1);
> - while ((ret = read(fd, buffer + size, max - size)) > 0) {
> + max = BUFSIZ;
> + buffer = malloc(max);
> + while ((ret = fread(buffer+size, 1, max - size, file)) == max - size) {
This assumes that fread() will never return a short read except on EOF
or error. I expect that will be true in practice for regular files,
but I'm not sure if it's a good idea to rely on it.
> size += ret;
> - if (size == max)
> - buffer = realloc(buffer, max *= 2);
> + buffer = realloc(buffer, max *= 2);
> }
> - if (ret < 0)
> + size += ret;
> + if (ferror(file))
> err(1, "reading from command");
> buffer[size] = '\0';
> return buffer;
> @@ -388,43 +387,25 @@ static char *grab_fd(int fd)
>
> static char *run(const char *cmd, int *exitstatus)
> {
> - pid_t pid;
> - int p[2];
> + static const char redir[] = " 2>&1";
> + size_t cmdlen;
> + char *cmdredir;
> + FILE *cmdout;
> char *ret;
> - int status;
>
> - if (pipe(p) != 0)
> - err(1, "creating pipe");
> -
> - pid = fork();
> - if (pid == -1)
> - err(1, "forking");
> -
> - if (pid == 0) {
> - if (dup2(p[1], STDOUT_FILENO) != STDOUT_FILENO
> - || dup2(p[1], STDERR_FILENO) != STDERR_FILENO
> - || close(p[0]) != 0
> - || close(STDIN_FILENO) != 0
> - || open("/dev/null", O_RDONLY) != STDIN_FILENO)
> - exit(128);
> -
> - status = system(cmd);
> - if (WIFEXITED(status))
> - exit(WEXITSTATUS(status));
> - /* Here's a hint... */
> - exit(128 + WTERMSIG(status));
> - }
> + cmdlen = strlen(cmd);
> + cmdredir = malloc(cmdlen + sizeof(redir));
> + memcpy(cmdredir, cmd, cmdlen);
> + memcpy(cmdredir + cmdlen, redir, sizeof(redir));
> +
> + cmdout = popen(cmdredir, "r");
> + if (!cmdout)
> + err(1, "popen \"%s\"", cmdredir);
> +
> + free(cmdredir);
>
> - close(p[1]);
> - ret = grab_fd(p[0]);
> - /* This shouldn't fail... */
> - if (waitpid(pid, &status, 0) != pid)
> - err(1, "Failed to wait for child");
> - close(p[0]);
> - if (WIFEXITED(status))
> - *exitstatus = WEXITSTATUS(status);
> - else
> - *exitstatus = -WTERMSIG(status);
> + ret = grab_stream(cmdout);
> + *exitstatus = pclose(cmdout);
> return ret;
> }
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
___
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan
[ccan] [PATCH 2/9] configurator: Reimplement run using popen
Rather than using fork+pipe+system+waitpid, most of which are only
available on POSIX-like systems, use popen which is also available on
Windows (under the name _popen).
Signed-off-by: Kevin Locke
---
tools/configurator/configurator.c | 77 +++
1 file changed, 29 insertions(+), 48 deletions(-)
diff --git a/tools/configurator/configurator.c
b/tools/configurator/configurator.c
index e322c76..9817fcd 100644
--- a/tools/configurator/configurator.c
+++ b/tools/configurator/configurator.c
@@ -24,14 +24,14 @@
#include
#include
#include
-#include
#include
-#include
-#include
-#include
-#include
#include
+#ifdef _MSC_VER
+#define popen _popen
+#define pclose _pclose
+#endif
+
#define DEFAULT_COMPILER "cc"
#define DEFAULT_FLAGS "-g3 -ggdb -Wall -Wundef -Wmissing-prototypes
-Wmissing-declarations -Wstrict-prototypes -Wold-style-definition"
@@ -367,20 +367,19 @@ static struct test tests[] = {
},
};
-static char *grab_fd(int fd)
+static char *grab_stream(FILE *file)
{
- int ret;
- size_t max, size = 0;
+ size_t max, ret, size = 0;
char *buffer;
- max = 16384;
- buffer = malloc(max+1);
- while ((ret = read(fd, buffer + size, max - size)) > 0) {
+ max = BUFSIZ;
+ buffer = malloc(max);
+ while ((ret = fread(buffer+size, 1, max - size, file)) == max - size) {
size += ret;
- if (size == max)
- buffer = realloc(buffer, max *= 2);
+ buffer = realloc(buffer, max *= 2);
}
- if (ret < 0)
+ size += ret;
+ if (ferror(file))
err(1, "reading from command");
buffer[size] = '\0';
return buffer;
@@ -388,43 +387,25 @@ static char *grab_fd(int fd)
static char *run(const char *cmd, int *exitstatus)
{
- pid_t pid;
- int p[2];
+ static const char redir[] = " 2>&1";
+ size_t cmdlen;
+ char *cmdredir;
+ FILE *cmdout;
char *ret;
- int status;
- if (pipe(p) != 0)
- err(1, "creating pipe");
-
- pid = fork();
- if (pid == -1)
- err(1, "forking");
-
- if (pid == 0) {
- if (dup2(p[1], STDOUT_FILENO) != STDOUT_FILENO
- || dup2(p[1], STDERR_FILENO) != STDERR_FILENO
- || close(p[0]) != 0
- || close(STDIN_FILENO) != 0
- || open("/dev/null", O_RDONLY) != STDIN_FILENO)
- exit(128);
-
- status = system(cmd);
- if (WIFEXITED(status))
- exit(WEXITSTATUS(status));
- /* Here's a hint... */
- exit(128 + WTERMSIG(status));
- }
+ cmdlen = strlen(cmd);
+ cmdredir = malloc(cmdlen + sizeof(redir));
+ memcpy(cmdredir, cmd, cmdlen);
+ memcpy(cmdredir + cmdlen, redir, sizeof(redir));
+
+ cmdout = popen(cmdredir, "r");
+ if (!cmdout)
+ err(1, "popen \"%s\"", cmdredir);
+
+ free(cmdredir);
- close(p[1]);
- ret = grab_fd(p[0]);
- /* This shouldn't fail... */
- if (waitpid(pid, &status, 0) != pid)
- err(1, "Failed to wait for child");
- close(p[0]);
- if (WIFEXITED(status))
- *exitstatus = WEXITSTATUS(status);
- else
- *exitstatus = -WTERMSIG(status);
+ ret = grab_stream(cmdout);
+ *exitstatus = pclose(cmdout);
return ret;
}
--
2.9.3
___
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan
