Pino Toscano
2014-Jan-21 18:04 UTC
Re: [Libguestfs] [PATCH] builder: proper consider subkeys in index files
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
Pino Toscano
2014-Jan-21 18:06 UTC
[Libguestfs] [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. (Thanks Rich for the hints, too.)
---
builder/index-parser-c.c | 15 +++++++++++----
builder/index-scan.l | 9 ++++++++-
builder/index-struct.c | 1 +
builder/index-struct.h | 1 +
builder/index_parser.ml | 44 +++++++++++++++++++++++++-------------------
5 files changed, 46 insertions(+), 24 deletions(-)
diff --git a/builder/index-parser-c.c b/builder/index-parser-c.c
index 17e680b..fbbebff 100644
--- a/builder/index-parser-c.c
+++ b/builder/index-parser-c.c
@@ -49,7 +49,7 @@ value
virt_builder_parse_index (value filenamev)
{
CAMLparam1 (filenamev);
- CAMLlocal4 (rv, v, sv, fv);
+ CAMLlocal5 (rv, v, sv, sv2, fv);
struct section *sections;
size_t i, nr_sections;
@@ -83,11 +83,18 @@ 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, Some subkey, value) */
+ if (fields->subkey) {
+ sv2 = caml_copy_string (fields->subkey);
+ sv = caml_alloc (1, 0);
+ Store_field (sv, 0, sv2);
+ } else
+ sv = Val_int (0);
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..da44b21 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 option * string (* key + subkey + value *)
(* Calls yyparse in the C code. *)
external parse_index : string -> sections =
"virt_builder_parse_index"
@@ -149,12 +149,17 @@ 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 (
- eprintf (f_"virt-builder: index is corrupt: %s: field
'%s' appears two or more times\n") n field;
+ fun (field, subkey, _) ->
+ let hashkey = (field, subkey) in
+ if Hashtbl.mem fseen hashkey then (
+ (match subkey with
+ | Some value ->
+ eprintf (f_"virt-builder: index is corrupt: %s: field
'%s[%s]' appears two or more times\n") n field value
+ | None ->
+ 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 +167,26 @@ let get_index ~prog ~debug ~downloader ~sigchecker source
let entries List.map (
fun (n, fields) ->
+ let fields = List.map (fun (k, sk, v) -> (k, sk), v) fields in
let printable_name - try Some (List.assoc "name"
fields) with Not_found -> None in
+ try Some (List.assoc ("name", None) fields) with
Not_found -> None in
let osinfo - try Some (List.assoc "osinfo"
fields) with Not_found -> None in
+ try Some (List.assoc ("osinfo", None) fields) with
Not_found -> None in
let file_uri - try make_absolute_uri (List.assoc
"file" fields)
+ try make_absolute_uri (List.assoc ("file", None) 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 (List.assoc ("sig", None)
fields))
with Not_found -> None in
let checksum_sha512 - try Some (List.assoc
"checksum[sha512]" fields)
+ try Some (List.assoc ("checksum", Some
"sha512") fields)
with Not_found ->
- try Some (List.assoc "checksum" fields)
+ try Some (List.assoc ("checksum", None) fields)
with Not_found -> None in
let revision - try int_of_string (List.assoc
"revision" fields)
+ try int_of_string (List.assoc ("revision", None) fields)
with
| Not_found -> 1
| Failure "int_of_string" ->
@@ -188,9 +194,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 (List.assoc ("format", None) fields) with
Not_found -> None in
let size - try Int64.of_string (List.assoc
"size" fields)
+ try Int64.of_string (List.assoc ("size", None) fields)
with
| Not_found ->
eprintf (f_"virt-builder: no 'size' field for
'%s'\n") n;
@@ -200,7 +206,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 (List.assoc ("compressed_size",
None) fields))
with
| Not_found ->
None
@@ -209,13 +215,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 (List.assoc ("expand", None) fields) with
Not_found -> None in
let lvexpand - try Some (List.assoc "lvexpand"
fields) with Not_found -> None in
+ try Some (List.assoc ("lvexpand", None) fields) with
Not_found -> None in
let notes - try Some (List.assoc "notes" fields)
with Not_found -> None in
+ try Some (List.assoc ("notes", None) fields) with
Not_found -> None in
let hidden - try bool_of_string (List.assoc
"hidden" fields)
+ try bool_of_string (List.assoc ("hidden", None) fields)
with
| Not_found -> false
| Failure "bool_of_string" ->
--
1.8.3.1
Richard W.M. Jones
2014-Jan-21 18:15 UTC
Re: [Libguestfs] [PATCH] builder: proper consider subkeys in index files
On Tue, Jan 21, 2014 at 07:06:57PM +0100, Pino Toscano wrote:> 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. (Thanks Rich for the hints, too.)ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Apparently Analagous Threads
- [PATCH] builder: proper consider subkeys in index files
- Re: [PATCH] builder: proper consider subkeys in index files
- [PATCH 1/3] builder: make the C index parser reentrant
- [PATCH] builder: fix small regression in subkey parsing
- [PATCH v11 5/8] builder: add a template parameter to get_index