2016-08-25 14:19 GMT+03:00 Pino Toscano <ptoscano@redhat.com>:
On Wednesday, 24 August 2016 23:59:54 CEST Matteo Cafasso wrote:
> The internal_find_inode command searches all entries referring to the
> given inode and returns a tsk_dirent structure for each of them.
>
> The command is able to retrieve information regarding deleted
> or unaccessible files where other commands such as stat or find
> would fail.
>
> The gathered list of tsk_dirent structs is serialised into XDR format
> and written to a file by the appliance.
>
> Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
> ---
>  daemon/tsk.c         | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  generator/actions.ml |  9 +++++++
>  src/MAX_PROC_NR      |  2 +-
>  3 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/daemon/tsk.c b/daemon/tsk.c
> index dd368d7..beb92a3 100644
> --- a/daemon/tsk.c
> +++ b/daemon/tsk.c
> @@ -44,6 +44,7 @@ enum tsk_dirent_flags {
>
>  static int open_filesystem (const char *, TSK_IMG_INFO **, TSK_FS_INFO **);
>  static TSK_WALK_RET_ENUM fswalk_callback (TSK_FS_FILE *, const char *, void *);
> +static TSK_WALK_RET_ENUM ifind_callback (TSK_FS_FILE *, const char *, void *);
>  static char file_type (TSK_FS_FILE *);
>  static int file_flags (TSK_FS_FILE *fsfile);
>  static void file_metadata (TSK_FS_META *, guestfs_int_tsk_dirent *);
> @@ -78,6 +79,35 @@ do_internal_filesystem_walk (const mountable_t *mountable)
>    return ret;
>  }
>
> +int
> +do_internal_find_inode (const mountable_t *mountable, int64_t inode)
> +{
> +  int ret = -1;
> +  TSK_FS_INFO *fs = NULL;
> +  TSK_IMG_INFO *img = NULL;  /* Used internally by tsk_fs_dir_walk */
> +  const int flags =
> +    TSK_FS_DIR_WALK_FLAG_ALLOC | TSK_FS_DIR_WALK_FLAG_UNALLOC |
> +    TSK_FS_DIR_WALK_FLAG_RECURSE | TSK_FS_DIR_WALK_FLAG_NOORPHAN;
> +
> +  ret = open_filesystem (mountable->device, &img, &fs);
> +  if (ret < 0)
> +    return ret;
> +
> +  reply (NULL, NULL);  /* Reply message. */
> +
> +  ret = tsk_fs_dir_walk (fs, fs->root_inum, flags, ifind_callback,
> +                         (void *) &inode);

Here 'inode' is int64_t ...

> +  if (ret == 0)
> +    ret = send_file_end (0);  /* File transfer end. */
> +  else
> +    send_file_end (1);  /* Cancel file transfer. */
> +
> +  fs->close (fs);
> +  img->close (img);
> +
> +  return ret;
> +}
> +
>  /* Inspect the device and initialises the img and fs structures.
>   * Return 0 on success, -1 on error.
>   */
> @@ -141,6 +171,51 @@ fswalk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
>    return ret;
>  }
>
> +/* Find inode callback, it gets called on every FS node.
> + * Parse the node, encode it into an XDR structure and send it to the appliance.
> + * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.
> + */
> +static TSK_WALK_RET_ENUM
> +ifind_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
> +{
> +  int ret = 0;
> +  CLEANUP_FREE char *fname = NULL;
> +  uint64_t *inode = (uint64_t *) data;

... but then here you cast it as uint64_t.  I don't see an immediate
issue with it, but please keep in mind that this kind of things in C
(i.e. cast a typed pointer to void*, and cast it back as some other
type) is dangerous, and should be avoided as much as possible.
I do agree with your point. Reason behind the cast is that the comparison few lines below is done against a uint64_t.

Moreover, other related APIs parameters are int64_t (download_inode and download_blocks). This would make the APIs inconsistent between each other.

If I picture myself as a user I would wonder why download_inode() wants a int64_t and find_inode() wants a uint64_t.
 

I guess the proper solution would be adding a new UInt64 type (and UInt
as well) to the generator...
I am not familiar with the generator architecture. What would adding a type to it imply from the practical point of view?

Thanks,
--
Pino Toscano

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