[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] PATCH: 5/5: Make all code use virExec



On Thu, Aug 28, 2008 at 04:18:08PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange redhat com> wrote:
> > On Wed, Aug 20, 2008 at 06:40:37PM +0200, Jim Meyering wrote:
> >> "Daniel P. Berrange" <berrange redhat com> 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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]