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

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).