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

[Pino Toscano]

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,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170308/346b85d6/attachment.sig>