On Thu, Aug 28, 2008 at 04:18:08PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> > On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote:
> >> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list