On Thu, Aug 28, 2008 at 04:18:08PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote: + + + while (got < (sizeof(help)-1)) { + int len; + if ((len = saferead(newstdout, help+got, sizeof(help)-got-1)) < 0) + goto cleanup2; + if (!len) + break; + got += len; + } + help[got] = '\0';
I was going to ask why you are using saferead, then realized that *I* suggested the s/read+EINTR/saferead/ change. Now, while re-reviewing this, I wondered if we could get rid of the 8KB stack buffer and encapsulate the above loop -- at the expense of allocating the memory instead -- by using e.g., virFileReadAll. But virFileReadAll operates on a file name, not a file descriptor. So I wrote/factored a couple of wrappers, and now, with the following patch, you can use this:
char *help = NULL; enum { MAX_HELP_OUTPUT_SIZE = 8192 }; int len = virFileReadLimFD(newstdout, MAX_HELP_OUTPUT_SIZE, &help); if (len < 0) goto ... ...
Then add this somewhere after done with "help": VIR_FREE(help);
This sounds like a nice idea - the loop is rather unpleasant to read as it is. I'll commit my patch shortly
diff --git a/src/util.c b/src/util.c index a81af07..fd30778 100644 --- a/src/util.c +++ b/src/util.c @@ -510,40 +510,63 @@ fread_file_lim (FILE *stream, size_t max_len, size_t *length) return NULL; }
-int __virFileReadAll(const char *path, int maxlen, char **buf) +/* A wrapper around fread_file_lim that maps a failure due to + exceeding the maximum size limitation to EOVERFLOW. */ +static int virFileReadLimFP(FILE *fp, int maxlen, char **buf) { - FILE *fh; - int ret = -1; size_t len; - char *s; + char *s = fread_file_lim (fp, maxlen+1, &len); + if (s == NULL) + return -1; + if (len > maxlen || (int)len != len) { + VIR_FREE(s); + /* There was at least one byte more than MAXLEN. + Set errno accordingly. */ + errno = EOVERFLOW; + return -1; + } + *buf = s; + return len; +} + +/* Like virFileReadLimFP, but use a file descriptor rather than a FILE*. */ +int __virFileReadLimFD(int fd_arg, int maxlen, char **buf) +{ + int fd = dup (fd_arg); + if (0 <= fd) {
Can we stick to 'fd >= 0' or 'fd < 0' or 'fd == -1'. I find the reversed constant-first conditionals rather painful to read. Always have to stop and think about them for too long. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|