Libguestfs - Jan 2014 - [PATCH] builder: proper consider subkeys in index files

The index files already allowed the 'key[subkey]=...' syntax for keys,
but considering such string as whole key. Proper split the parsing and
the handling of the subkeys, so they can be searched a bit easier.

This causes no actual behaviour changes, it is just internal
micro-refactoring.
---
 builder/index-parser-c.c |  8 +++++---
 builder/index-scan.l     |  9 ++++++++-
 builder/index-struct.c   |  1 +
 builder/index-struct.h   |  1 +
 builder/index_parser.ml  | 44 ++++++++++++++++++++++++++------------------
 5 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/builder/index-parser-c.c b/builder/index-parser-c.c
index 17e680b..baf2b62 100644
--- a/builder/index-parser-c.c
+++ b/builder/index-parser-c.c
@@ -83,11 +83,13 @@ virt_builder_parse_index (value filenamev)
 
     for (j = 0, fields = sections->fields; fields != NULL;
          j++, fields = fields->next) {
-      v = caml_alloc_tuple (2);
+      v = caml_alloc_tuple (3);
       sv = caml_copy_string (fields->key);
-      Store_field (v, 0, sv);   /* (key, value) */
-      sv = caml_copy_string (fields->value);
+      Store_field (v, 0, sv);   /* (key, subkey, value) */
+      sv = caml_copy_string (fields->subkey ? fields->subkey :
"");
       Store_field (v, 1, sv);
+      sv = caml_copy_string (fields->value);
+      Store_field (v, 2, sv);
       Store_field (fv, j, v);   /* assign to return array of fields */
     }
 
diff --git a/builder/index-scan.l b/builder/index-scan.l
index 9a6a0e3..7a9618f 100644
--- a/builder/index-scan.l
+++ b/builder/index-scan.l
@@ -58,10 +58,17 @@ extern void yyerror (const char *);
 
   /* field=value or field[subfield]=value */
 ^[A-Za-z0-9_.]+("["[A-Za-z0-9_,.]+"]")?"=".*\n {
-                      size_t i = strcspn (yytext, "=");
+                      size_t i = strcspn (yytext, "=[");
                       yylval.field = malloc (sizeof (struct field));
                       yylval.field->next = NULL;
                       yylval.field->key = strndup (yytext, i);
+                      if (yytext[i] == '[') {
+                        size_t j = strcspn (yytext+i+1, "]");
+                        yylval.field->subkey = strndup (yytext+i+1, j);
+                        i += 1+j+2;
+                      } else {
+                        yylval.field->subkey = NULL;
+                      }
                       /* Note we chop the final \n off here. */
                       yylval.field->value = strndup (yytext+i+1,
yyleng-(i+2));
                       return FIELD;
diff --git a/builder/index-struct.c b/builder/index-struct.c
index 26bed24..fe5b0e3 100644
--- a/builder/index-struct.c
+++ b/builder/index-struct.c
@@ -52,6 +52,7 @@ free_field (struct field *field)
   if (field) {
     free_field (field->next);
     free (field->key);
+    free (field->subkey);
     free (field->value);
     free (field);
   }
diff --git a/builder/index-struct.h b/builder/index-struct.h
index ac8a3dd..f92e01d 100644
--- a/builder/index-struct.h
+++ b/builder/index-struct.h
@@ -32,6 +32,7 @@ struct section {
 struct field {
   struct field *next;
   char *key;
+  char *subkey;
   char *value;
 };
 
diff --git a/builder/index_parser.ml b/builder/index_parser.ml
index 453a3a1..4b070cb 100644
--- a/builder/index_parser.ml
+++ b/builder/index_parser.ml
@@ -101,7 +101,7 @@ let print_entry chan (name, { printable_name =
printable_name;
 type sections = section array
 and section = string * fields           (* [name] + fields *)
 and fields = field array
-and field = string * string             (* key + value *)
+and field = string * string * string    (* key + subkey + value *)
 
 (* Calls yyparse in the C code. *)
 external parse_index : string -> sections =
"virt_builder_parse_index"
@@ -149,12 +149,13 @@ let get_index ~prog ~debug ~downloader ~sigchecker source 
fun (n, fields) ->
         let fseen = Hashtbl.create 13 in
         List.iter (
-          fun (field, _) ->
-            if Hashtbl.mem fseen field then (
+          fun (field, subkey, _) ->
+            let hashkey = (field, subkey) in
+            if Hashtbl.mem fseen hashkey then (
               eprintf (f_"virt-builder: index is corrupt: %s: field
'%s' appears two or more times\n") n field;
               corrupt_file ()
             );
-            Hashtbl.add fseen field true
+            Hashtbl.add fseen hashkey true
         ) fields
     ) sections;
 
@@ -162,25 +163,32 @@ let get_index ~prog ~debug ~downloader ~sigchecker source 
let entries        List.map (
         fun (n, fields) ->
+          let find_elem key subkey fields +            match List.filter (
+                fun (iterkey, itersubkey, itervalue) ->
+                  iterkey = key && itersubkey = subkey
+              ) fields with
+            | [] -> raise Not_found
+            | (_, _, value) :: _ -> value in
           let printable_name -            try Some (List.assoc "name"
fields) with Not_found -> None in
+            try Some (find_elem "name" "" fields) with
Not_found -> None in
           let osinfo -            try Some (List.assoc "osinfo"
fields) with Not_found -> None in
+            try Some (find_elem "osinfo" "" fields) with
Not_found -> None in
           let file_uri -            try make_absolute_uri (List.assoc
"file" fields)
+            try make_absolute_uri (find_elem "file" ""
fields)
             with Not_found ->
               eprintf (f_"virt-builder: no 'file' (URI) entry for
'%s'\n") n;
             corrupt_file () in
           let signature_uri -            try Some (make_absolute_uri
(List.assoc "sig" fields))
+            try Some (make_absolute_uri (find_elem "sig" ""
fields))
             with Not_found -> None in
           let checksum_sha512 -            try Some (List.assoc
"checksum[sha512]" fields)
+            try Some (find_elem "checksum" "sha512" fields)
             with Not_found ->
-              try Some (List.assoc "checksum" fields)
+              try Some (find_elem "checksum" "" fields)
               with Not_found -> None in
           let revision -            try int_of_string (List.assoc
"revision" fields)
+            try int_of_string (find_elem "revision" ""
fields)
             with
             | Not_found -> 1
             | Failure "int_of_string" ->
@@ -188,9 +196,9 @@ let get_index ~prog ~debug ~downloader ~sigchecker source   
n;
               corrupt_file () in
           let format -            try Some (List.assoc "format"
fields) with Not_found -> None in
+            try Some (find_elem "format" "" fields) with
Not_found -> None in
           let size -            try Int64.of_string (List.assoc
"size" fields)
+            try Int64.of_string (find_elem "size" ""
fields)
             with
             | Not_found ->
               eprintf (f_"virt-builder: no 'size' field for
'%s'\n") n;
@@ -200,7 +208,7 @@ let get_index ~prog ~debug ~downloader ~sigchecker source   
n;
               corrupt_file () in
           let compressed_size -            try Some (Int64.of_string
(List.assoc "compressed_size" fields))
+            try Some (Int64.of_string (find_elem "compressed_size"
"" fields))
             with
             | Not_found ->
               None
@@ -209,13 +217,13 @@ let get_index ~prog ~debug ~downloader ~sigchecker source 
n;
               corrupt_file () in
           let expand -            try Some (List.assoc "expand"
fields) with Not_found -> None in
+            try Some (find_elem "expand" "" fields) with
Not_found -> None in
           let lvexpand -            try Some (List.assoc "lvexpand"
fields) with Not_found -> None in
+            try Some (find_elem "lvexpand" "" fields) with
Not_found -> None in
           let notes -            try Some (List.assoc "notes" fields)
with Not_found -> None in
+            try Some (find_elem "notes" "" fields) with
Not_found -> None in
           let hidden -            try bool_of_string (List.assoc
"hidden" fields)
+            try bool_of_string (find_elem "hidden" ""
fields)
             with
             | Not_found -> false
             | Failure "bool_of_string" ->
-- 
1.8.3.1

On Tue, Jan 21, 2014 at 05:18:27PM +0100, Pino Toscano
wrote:
> +      sv = caml_copy_string (fields->subkey ? fields->subkey :
"");
>        Store_field (v, 1, sv);
Heh, sure would be nice if this was an option type :-)

I believe the following should work:

(1) Change CAMLlocal4 (..) at the top of the function to:

  CAMLlocal5 (rv, v, sv, sv2, fv);

(2) Then the new code is:

  if (fields->subkey) {                      /* Some subkey */
    sv2 = caml_copy_string (fields->subkey);
    sv = caml_alloc (1, 0);
    Store_field (sv, 0, sv2);
  } else                                     /* None */
    sv = Val_int (0);
  Store_field (v, 1, sv);

Also you will need to make the corresponding adjustment to the field
type, ie. (string, string option, string) instead of this:
> +and field = string * string * string    (* key + subkey + value *)
I'll have a look at the rest in a minute.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Tue, Jan 21, 2014 at 05:18:27PM +0100, Pino Toscano
wrote:
>      let entries >        List.map (
>          fun (n, fields) ->
> +          let find_elem key subkey fields > +            match
List.filter (
> +                fun (iterkey, itersubkey, itervalue) ->
> +                  iterkey = key && itersubkey = subkey
> +              ) fields with
> +            | [] -> raise Not_found
> +            | (_, _, value) :: _ -> value in
>
>            let printable_name > -            try Some (List.assoc
"name" fields) with Not_found -> None in
> +            try Some (find_elem "name" "" fields) with
Not_found -> None in

What you could do here, which is a bit nicer, is (earlier on):

  let fields = List.map (fun (k,sk,v) -> (k,sk),v) fields in

Delete the find_elem function and use instead:

-        try Some (List.assoc "name" fields) with Not_found -> None
in
+        try Some (List.assoc ("name",None) fields) with Not_found
-> None in

etc. since List.assoc works for any key type, not just strings.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
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 21 January 2014 16:37:20 Richard W.M. Jones
wrote:
> On Tue, Jan 21, 2014 at 05:18:27PM +0100, Pino Toscano wrote:
> > +      sv = caml_copy_string (fields->subkey ? fields->subkey :
"");
> > 
> >        Store_field (v, 1, sv);
> 
> Heh, sure would be nice if this was an option type :-)
> 
> I believe the following should work:
> 
> (1) Change CAMLlocal4 (..) at the top of the function to:
> 
>   CAMLlocal5 (rv, v, sv, sv2, fv);
> 
> (2) Then the new code is:
> 
>   if (fields->subkey) {                      /* Some subkey */
>     sv2 = caml_copy_string (fields->subkey);
>     sv = caml_alloc (1, 0);
>     Store_field (sv, 0, sv2);
>   } else                                     /* None */
>     sv = Val_int (0);
>   Store_field (v, 1, sv);
> 
> Also you will need to make the corresponding adjustment to the field
> 
> type, ie. (string, string option, string) instead of this:
> > +and field = string * string * string    (* key + subkey + value *)
Indeed, they work that way. It seemed not totally clear for the 
documentations I've read.
> >      let entries > >      
> >        List.map (
> >        
> >          fun (n, fields) ->
> > 
> > +          let find_elem key subkey fields > > +           
match List.filter (
> > +                fun (iterkey, itersubkey, itervalue) ->
> > +                  iterkey = key && itersubkey = subkey
> > +              ) fields with
> > +            | [] -> raise Not_found
> > +            | (_, _, value) :: _ -> value in
> > 
> >            let printable_name > > 
> > -            try Some (List.assoc "name" fields) with
Not_found ->
> > None in
> > +            try Some (find_elem "name" "" fields)
with
> > Not_found -> None in
> What you could do here, which is a bit nicer, is (earlier on):
> 
>   let fields = List.map (fun (k,sk,v) -> (k,sk),v) fields in
> 
> Delete the find_elem function and use instead:
> 
> -        try Some (List.assoc "name" fields) with Not_found ->
None in
> +        try Some (List.assoc ("name",None) fields) with
Not_found ->
> None in
> 
> etc. since List.assoc works for any key type, not just strings.
Ah OK.

-- 
Pino Toscano