Libguestfs - Sep 2017 - Re: [PATCH v2 2/5] lib: qemu: Factor out common code for reading and writing cache files.

On Tuesday, 12 September 2017 14:29:13 CEST Richard W.M. Jones
wrote:
> +/**
> + * Generic functions for reading and writing the cache files, used
> + * where we are just reading and writing plain text strings.
> + */
> +static int
> +generic_read_cache (guestfs_h *g, const char *filename, char **strp)
> +{
> +  if (access (filename, R_OK) == -1 && errno == ENOENT)
> +    return 0;                   /* no cache, run the test instead */
This will go ahead if access() failed for any other error though;
IMHO a better check could be:

  if (access (filename, R_OK) == -1) {
    if (errno == ENOENT)
      return 0;                   /* no cache, run the test instead */
    perrorf (g, "access: %s", filename);
    return -1;
  }
> +static int
> +generic_write_cache (guestfs_h *g, const char *filename, const char *str)
> +{
> +  CLEANUP_FCLOSE FILE *fp = fopen (filename, "w");
> +  if (fp == NULL) {
> +    perrorf (g, "%s", filename);
> +    return -1;
> +  }
> +
> +  if (fprintf (fp, "%s", str) == -1) {
> +    perrorf (g, "%s: write", filename);
> +    return -1;
> +  }
> +
> +  return 0;
> +}
While this is the same code already used, IMHO it would make more sense
to use open/write directly (since we have a buffer already, it will be
faster than using sprintf); there are snippets that call write() in a
loop until the whole buffer is written in different parts of the
library, so factorizing them could help.

Thanks,
-- 
Pino Toscano

On Tue, Sep 12, 2017 at 03:05:43PM +0200, Pino Toscano
wrote:
> On Tuesday, 12 September 2017 14:29:13 CEST Richard W.M. Jones wrote:
> > +/**
> > + * Generic functions for reading and writing the cache files, used
> > + * where we are just reading and writing plain text strings.
> > + */
> > +static int
> > +generic_read_cache (guestfs_h *g, const char *filename, char **strp)
> > +{
> > +  if (access (filename, R_OK) == -1 && errno == ENOENT)
> > +    return 0;                   /* no cache, run the test instead */
> 
> This will go ahead if access() failed for any other error though;
> IMHO a better check could be:
> 
>   if (access (filename, R_OK) == -1) {
>     if (errno == ENOENT)
>       return 0;                   /* no cache, run the test instead */
>     perrorf (g, "access: %s", filename);
>     return -1;
>   }
But isn't it OK since the following line will return the true error:

  if (guestfs_int_read_whole_file (g, filename, strp, NULL) == -1)
  ...

?
> > +static int
> > +generic_write_cache (guestfs_h *g, const char *filename, const char
*str)
> > +{
> > +  CLEANUP_FCLOSE FILE *fp = fopen (filename, "w");
> > +  if (fp == NULL) {
> > +    perrorf (g, "%s", filename);
> > +    return -1;
> > +  }
> > +
> > +  if (fprintf (fp, "%s", str) == -1) {
> > +    perrorf (g, "%s: write", filename);
> > +    return -1;
> > +  }
> > +
> > +  return 0;
> > +}
> 
> While this is the same code already used, IMHO it would make more sense
> to use open/write directly (since we have a buffer already, it will be
> faster than using sprintf); there are snippets that call write() in a
> loop until the whole buffer is written in different parts of the
> library, so factorizing them could help.
OK (using full_write).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Tuesday, 12 September 2017 15:08:28 CEST Richard W.M. Jones
wrote:
> On Tue, Sep 12, 2017 at 03:05:43PM +0200, Pino Toscano wrote:
> > On Tuesday, 12 September 2017 14:29:13 CEST Richard W.M. Jones wrote:
> > > +/**
> > > + * Generic functions for reading and writing the cache files,
used
> > > + * where we are just reading and writing plain text strings.
> > > + */
> > > +static int
> > > +generic_read_cache (guestfs_h *g, const char *filename, char
**strp)
> > > +{
> > > +  if (access (filename, R_OK) == -1 && errno == ENOENT)
> > > +    return 0;                   /* no cache, run the test
instead */
> > 
> > This will go ahead if access() failed for any other error though;
> > IMHO a better check could be:
> > 
> >   if (access (filename, R_OK) == -1) {
> >     if (errno == ENOENT)
> >       return 0;                   /* no cache, run the test instead */
> >     perrorf (g, "access: %s", filename);
> >     return -1;
> >   }
> 
> But isn't it OK since the following line will return the true error:
> 
>   if (guestfs_int_read_whole_file (g, filename, strp, NULL) == -1)
>   ...
> 
> ?
It will, although it feels a bit awkward for the function to progress
if it is in an error situation already.
> > > +static int
> > > +generic_write_cache (guestfs_h *g, const char *filename, const
char *str)
> > > +{
> > > +  CLEANUP_FCLOSE FILE *fp = fopen (filename, "w");
> > > +  if (fp == NULL) {
> > > +    perrorf (g, "%s", filename);
> > > +    return -1;
> > > +  }
> > > +
> > > +  if (fprintf (fp, "%s", str) == -1) {
> > > +    perrorf (g, "%s: write", filename);
> > > +    return -1;
> > > +  }
> > > +
> > > +  return 0;
> > > +}
> > 
> > While this is the same code already used, IMHO it would make more
sense
> > to use open/write directly (since we have a buffer already, it will be
> > faster than using sprintf); there are snippets that call write() in a
> > loop until the whole buffer is written in different parts of the
> > library, so factorizing them could help.
> 
> OK (using full_write).
D'oh, missed it, thanks for spotting it (and for writing it).

-- 
Pino Toscano