Re: [Libguestfs] [PATCH v4 1/9] lib/osinfo.c: Extract xml processing into a callback

On Tuesday, 7 March 2017 15:26:57 CET Cédric Bosdonnat wrote: > In order to further reuse the osinfo database parsing in OCAML, this > commit extracts the XML processing for the distro ISOs and places it > into a newly created callback. > > This will later help other code to traverse the osinfo DB files and > let them extract what they need from them. > --- Mostly LGTM, just a couple of style issues. >  lib/osinfo.c | 85 +++++++++++++++++++++++++++++++----------------------------- >  1 file changed, 44 insertions(+), 41 deletions(-) > > diff --git a/lib/osinfo.c b/lib/osinfo.c > index ea2a7659a..121fac1ba 100644 > --- a/lib/osinfo.c > +++ b/lib/osinfo.c > @@ -52,6 +52,7 @@ >  #include <string.h> >  #include <unistd.h> >  #include <dirent.h> > +#include <errno.h> >  #include <assert.h> >  #include <sys/types.h> >  #include <libintl.h> > @@ -71,10 +72,14 @@ gl_lock_define_initialized (static, osinfo_db_lock); >  static ssize_t osinfo_db_size = 0; /* 0 = unread, -1 = error, >= 1 = #records */ >  static struct osinfo *osinfo_db = NULL; >   > -static int read_osinfo_db (guestfs_h *g); > -static void free_osinfo_db_entry (struct osinfo *); > - >  #define XMLSTREQ(a,b) (xmlStrEqual((a),(b)) == 1) > +typedef int (*read_osinfo_db_callback) (guestfs_h *g, const char *path, void *opaque); > + > +static int > +read_osinfo_db (guestfs_h *g, > +                read_osinfo_db_callback callback, void *opaque); Forward declarations in a single line please. (Actual implementations are fine as wrapped.) > +static int read_osinfo_db_xml (guestfs_h *g, const char *pathname, void *data); This forward declaration could stay where it was before (i.e. below). > +static void free_osinfo_db_entry (struct osinfo *); >   >  /* Given one or more fields from the header of a CD/DVD/ISO, look up >   * the media in the libosinfo database and return our best guess for > @@ -87,14 +92,24 @@ static void free_osinfo_db_entry (struct osinfo *); >   */ >  int >  guestfs_int_osinfo_map (guestfs_h *g, const struct guestfs_isoinfo *isoinfo, > - const struct osinfo **osinfo_ret) > +                        const struct osinfo **osinfo_ret) Extra indentation change.

>  { >    size_t i; >   >    /* We only need to lock the database when reading it for the first time. */ >    gl_lock_lock (osinfo_db_lock); >    if (osinfo_db_size == 0) { > -    if (read_osinfo_db (g) == -1) { > +    if (read_osinfo_db (g, read_osinfo_db_xml, NULL) == -1) { > +      /* Fatal error: free any database entries which have been read, and > +       * mark the database as having a permanent error. > +       */ > +      if (osinfo_db_size > 0) { > +        for (i = 0; i < (size_t) osinfo_db_size; ++i) > +          free_osinfo_db_entry (&osinfo_db[i]); > +      } > +      free (osinfo_db); > +      osinfo_db = NULL; > +      osinfo_db_size = -1; >        gl_lock_unlock (osinfo_db_lock); >        return -1; >      } > @@ -156,19 +171,18 @@ guestfs_int_osinfo_map (guestfs_h *g, const struct guestfs_isoinfo *isoinfo, >   * Try to use the shared osinfo database layout (and location) first: >   * https://gitlab.com/libosinfo/libosinfo/blob/master/docs/database-layout.txt >   */ > -static int read_osinfo_db_xml (guestfs_h *g, const char *filename); > - > -static int read_osinfo_db_flat (guestfs_h *g, const char *directory); > -static int read_osinfo_db_three_levels (guestfs_h *g, const char *directory); > -static int read_osinfo_db_directory (guestfs_h *g, const char *directory); > +static int read_osinfo_db_flat (guestfs_h *g, const char *directory, > +                                read_osinfo_db_callback callback, void *opaque); > +static int read_osinfo_db_three_levels (guestfs_h *g, const char *directory, > +                                        read_osinfo_db_callback callback, void *opaque); > +static int read_osinfo_db_directory (guestfs_h *g, const char *directory, > +                                     read_osinfo_db_callback callback, void *opaque); Ditto (wrapping). Thanks, _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs