Re: [Libguestfs] [PATCH v2 2/4] v2v: copy virtio drivers without guestfs handle leak
On Tue, Oct 13, 2015 at 03:50:52PM +0300, Roman Kagan wrote:
Refactor copying of virtio windows drivers into the guest so that
the
matching of the drivers to the guest os flavor and copying the files
happens one next to the other in a single function, and no guestfs
handle (nor any other irrelevant info) is leaked outside.
Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com>
This one makes sense too, apart from a few style-only problems:
+ if is_directory virtio_win then (
+ let cmd = sprintf "cd %s && find -type f" (quote virtio_win) in
+ let paths = external_command cmd in
+ List.iter (
+ fun path ->
+ if (match_vio_path_with_os path inspect.i_arch
+ inspect.i_major_version inspect.i_minor_version
+ inspect.i_product_variant) then (
Don't need parens around if conditions, and
it's easier to read this statement if the parameters
are indented:
if match_vio_path_with_os path inspect.i_arch
inspect.i_major_version
inspect.i_minor_version
inspect.i_product_variant then (
+ if (g2#is_file source ~followsymlinks:false) &&
+ (match_vio_path_with_os path inspect.i_arch
+ inspect.i_major_version inspect.i_minor_version
+ inspect.i_product_variant) then (
As above: you don't need parens around the parameters of &&,
and more indentation needed of match_vio_path_with_os.
+ let target = driverdir // (String.lowercase_ascii
+ (Filename.basename path)) in
Because function application binds most tightly, you can write this
as:
let target = driverdir //
String.lowercase_ascii (Filename.basename path) in
+(* Given a path of a file relative to the root of the directory tree
with
+ * virtio-win drivers, figure out if it's suitable for the specific Windows
+ * flavor.
+ *)
+let match_vio_path_with_os path arch os_major os_minor os_variant =
Be nice if this function had a better, shorter name, but I don't know
what that name should be :-/ I think the problem with this function is
the name starts with 'match', which makes me think of the match
keyword every time I see it being used.
+ (* Skip files without specific extensions. *)
+ let extensions = ["cat"; "inf"; "pdb";
"sys"] in
+ if (not (List.mem extension extensions)) then raise Not_found;
Parens around if condition.
Rest looks fine to me.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org