Re: [Libguestfs] [PATCH 2/2] dib: use remove_duplicates instead of own code

[Richard W.M. Jones]

On Fri, Sep 23, 2016 at 06:05:25PM +0200, Pino Toscano wrote:
> Use a common function to remove duplicates in an unsorted list.
> 
> Just refactoring, with no behaviour change.

Except it's no longer O(n^2) :-)

ACK series.

Rich.

> ---
>  dib/cmdline.ml | 2 +-
>  dib/utils.ml   | 4 
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/dib/cmdline.ml b/dib/cmdline.ml
> index 1fd6c71..144e5a7 100644
> --- a/dib/cmdline.ml
> +++ b/dib/cmdline.ml
> @@ -107,7 +107,7 @@ read the man page virt-dib(1).
>  
>let formats = ref ["qcow2"] in
>let set_format arg =
> -let fmts = remove_dups (String.nsplit "," arg) in
> +let fmts = remove_duplicates (String.nsplit "," arg) in
>  List.iter (
>function
>| "qcow2" | "tar" | "raw" | "vhd" | "docker" -> ()
> diff --git a/dib/utils.ml b/dib/utils.ml
> index a2046cb..3df5171 100644
> --- a/dib/utils.ml
> +++ b/dib/utils.ml
> @@ -91,10 +91,6 @@ let digit_prefix_compare a b =
>  let do_mkdir dir =
>mkdir_p dir 0o755
>  
> -let rec remove_dups = function
> -  | [] -> []
> -  | x :: xs -> x :: (remove_dups (List.filter ((<>) x) xs))
> -
>  let require_tool tool =
>try ignore (which tool)
>with Executable_not_found tool ->
> -- 
> 2.7.4
> 
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

-- 
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

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH 1/2] mllib: move remove_duplicates from v2v

[Pino Toscano]

Simple code motion.
---
 mllib/common_utils.ml  | 9 +
 mllib/common_utils.mli | 6 ++
 v2v/utils.ml   | 9 -
 v2v/utils.mli  | 3 ---
 4 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
index 81d8202..78618f5 100644
--- a/mllib/common_utils.ml
+++ b/mllib/common_utils.ml
@@ -297,6 +297,15 @@ let sort_uniq ?(cmp = Pervasives.compare) xs =
   let xs = uniq ~cmp xs in
   xs
 
+let remove_duplicates xs =
+  let h = Hashtbl.create (List.length xs) in
+  let rec loop = function
+| [] -> []
+| x :: xs when Hashtbl.mem h x -> xs
+| x :: xs -> Hashtbl.add h x true; x :: loop xs
+  in
+  loop xs
+
 let push_back xsp x = xsp := !xsp @ [x]
 let push_front x xsp = xsp := x :: !xsp
 let pop_back xsp =
diff --git a/mllib/common_utils.mli b/mllib/common_utils.mli
index 68c0d54..ad43345 100644
--- a/mllib/common_utils.mli
+++ b/mllib/common_utils.mli
@@ -147,6 +147,12 @@ val uniq : ?cmp:('a -> 'a -> int) -> 'a list -> 'a list
 val sort_uniq : ?cmp:('a -> 'a -> int) -> 'a list -> 'a list
 (** Sort and uniquify a list. *)
 
+val remove_duplicates : 'a list -> 'a list
+(** Remove duplicates from an unsorted list; useful when the order
+of the elements matter.
+
+Please use [sort_uniq] when the order does not matter. *)
+
 val push_back : 'a list ref -> 'a -> unit
 val push_front : 'a -> 'a list ref -> unit
 val pop_back : 'a list ref -> 'a
diff --git a/v2v/utils.ml b/v2v/utils.ml
index d1ddee7..fb0b802 100644
--- a/v2v/utils.ml
+++ b/v2v/utils.ml
@@ -81,15 +81,6 @@ let compare_app2_versions app1 app2 =
   compare_version app1.Guestfs.app2_release app2.Guestfs.app2_release
   )
 
-let remove_duplicates xs =
-  let h = Hashtbl.create (List.length xs) in
-  let rec loop = function
-| [] -> []
-| x :: xs when Hashtbl.mem h x -> xs
-| x :: xs -> Hashtbl.add h x true; x :: loop xs
-  in
-  loop xs
-
 let du filename =
   (* There's no OCaml binding for st_blocks, so run coreutils 'du'. *)
   let cmd =
diff --git a/v2v/utils.mli b/v2v/utils.mli
index 97d98ff..2bd1329 100644
--- a/v2v/utils.mli
+++ b/v2v/utils.mli
@@ -46,9 +46,6 @@ val find_uefi_firmware : string -> Uefi.uefi_firmware
 val compare_app2_versions : Guestfs.application2 -> Guestfs.application2 -> int
 (** Compare two app versions. *)
 
-val remove_duplicates : 'a list -> 'a list
-(** Remove duplicates from a list. *)
-
 val du : string -> int64
 (** Return the true size of a file in bytes, including any wasted
 space caused by internal fragmentation (the overhead of using
-- 
2.7.4

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH 2/2] dib: use remove_duplicates instead of own code

[Pino Toscano]

Use a common function to remove duplicates in an unsorted list.

Just refactoring, with no behaviour change.
---
 dib/cmdline.ml | 2 +-
 dib/utils.ml   | 4 
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/dib/cmdline.ml b/dib/cmdline.ml
index 1fd6c71..144e5a7 100644
--- a/dib/cmdline.ml
+++ b/dib/cmdline.ml
@@ -107,7 +107,7 @@ read the man page virt-dib(1).
 
   let formats = ref ["qcow2"] in
   let set_format arg =
-let fmts = remove_dups (String.nsplit "," arg) in
+let fmts = remove_duplicates (String.nsplit "," arg) in
 List.iter (
   function
   | "qcow2" | "tar" | "raw" | "vhd" | "docker" -> ()
diff --git a/dib/utils.ml b/dib/utils.ml
index a2046cb..3df5171 100644
--- a/dib/utils.ml
+++ b/dib/utils.ml
@@ -91,10 +91,6 @@ let digit_prefix_compare a b =
 let do_mkdir dir =
   mkdir_p dir 0o755
 
-let rec remove_dups = function
-  | [] -> []
-  | x :: xs -> x :: (remove_dups (List.filter ((<>) x) xs))
-
 let require_tool tool =
   try ignore (which tool)
   with Executable_not_found tool ->
-- 
2.7.4

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v3 1/3] New API: internal_find_block

[NoxDaFox]

2016-09-23 11:52 GMT+03:00 Pino Toscano :

> On Tuesday, 20 September 2016 16:19:30 CEST Matteo Cafasso wrote:
> > +  for (index = 0; index < count; index++) {
> > +fsattr = tsk_fs_file_attr_get_idx (fsfile, index);
> > +
> > +if (fsattr != NULL && fsattr->flags & TSK_FS_ATTR_NONRES)
> > +  tsk_fs_attr_walk (fsattr, flags, attrwalk_callback, (void *)
> );
>
> The return code of tsk_fs_attr_walk must be checked.
>

I'll fix.


>
> > +/* Attribute walk, searches the given block within the FS node
> attributes.
> > + *
> > + * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.
> > + */
> > +static TSK_WALK_RET_ENUM
> > +attrwalk_callback (TSK_FS_FILE *fsfile, TSK_OFF_T offset,
> > +   TSK_DADDR_T blkaddr, char *buf, size_t size,
> > +   TSK_FS_BLOCK_FLAG_ENUM flags, void *data)
> > +{
> > +  findblk_data *blkdata = (findblk_data *) data;
> > +
> > +  if (!(flags & TSK_FS_BLOCK_FLAG_SPARSE) && blkaddr == blkdata->block)
> {
> > +blkdata->found = true;
>
> If we want to ignore sparse blocks, wouldn't it make sense to pass
> TSK_FS_FILE_WALK_FLAG_NOSPARSE as additional flag to tsk_fs_attr_walk
> above?
>
> Also, my concerns about this that I replied in v2 still stand: is the
> documentation obsolete, or does it document what is the expected
> behaviour? In the former case, then it could be ok to partially
> disregard what it said; in the latter, the code should follow what
> it describes.
>
> In any case, you should get in touch with the sleuthkit developers,
> and get their feedback about that.
>

Right, I'll write something on their devs list.


>
> --
> Pino Toscano
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
>
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v3 0/3] New API - find_block

[Pino Toscano]

On Tuesday, 20 September 2016 16:19:29 CEST Matteo Cafasso wrote:
> v3:
> 
>  - fixed attribute walk callback: checking against TSK_FS_BLOCK_FLAG_RAW flag 
> would
>exclude compressed data blocks which are still important.
>Yet we want to exclude sparse blocks (TSK_FS_BLOCK_FLAG_SPARSE) as they 
> are not stored
>on the disk.
> 
> Matteo Cafasso (3):
>   New API: internal_find_block
>   New API: find_block
>   find_block: added API tests

Patches #2, and #3 LGTM.

Thanks,
-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v3 1/3] New API: internal_find_block

[Pino Toscano]

On Tuesday, 20 September 2016 16:19:30 CEST Matteo Cafasso wrote:
> +  for (index = 0; index < count; index++) {
> +fsattr = tsk_fs_file_attr_get_idx (fsfile, index);
> +
> +if (fsattr != NULL && fsattr->flags & TSK_FS_ATTR_NONRES)
> +  tsk_fs_attr_walk (fsattr, flags, attrwalk_callback, (void *) );

The return code of tsk_fs_attr_walk must be checked.

> +/* Attribute walk, searches the given block within the FS node attributes.
> + *
> + * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.
> + */
> +static TSK_WALK_RET_ENUM
> +attrwalk_callback (TSK_FS_FILE *fsfile, TSK_OFF_T offset,
> +   TSK_DADDR_T blkaddr, char *buf, size_t size,
> +   TSK_FS_BLOCK_FLAG_ENUM flags, void *data)
> +{
> +  findblk_data *blkdata = (findblk_data *) data;
> +
> +  if (!(flags & TSK_FS_BLOCK_FLAG_SPARSE) && blkaddr == blkdata->block) {
> +blkdata->found = true;

If we want to ignore sparse blocks, wouldn't it make sense to pass
TSK_FS_FILE_WALK_FLAG_NOSPARSE as additional flag to tsk_fs_attr_walk
above?

Also, my concerns about this that I replied in v2 still stand: is the
documentation obsolete, or does it document what is the expected
behaviour? In the former case, then it could be ok to partially
disregard what it said; in the latter, the code should follow what
it describes.

In any case, you should get in touch with the sleuthkit developers,
and get their feedback about that.

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs