[Libguestfs] [PATCH v2 1/3] New API: internal_find_block

[Pino Toscano]

On Monday, 19 September 2016 23:26:57 CEST Matteo Cafasso wrote:
> The internal_find_block command searches all entries referring to the
> given filesystem data block and returns a tsk_dirent structure
> for each of them.
> 
> For filesystems such as NTFS which do not delete the block mapping
> when removing files, it is possible to get multiple non-allocated
> entries for the same block.
> 
> 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 at gmail.com>
> ---

The idea is fine, there are few notes below.

> +int
> +do_internal_find_block (const mountable_t *mountable, int64_t block)
> +{
> +  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,
> +                         findblk_callback, (void *) &block);
> +  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;
> +}
> +
> +

(One extra empty line.)

> +static TSK_WALK_RET_ENUM
> +findblk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
> +{
> +  findblk_data blkdata;
> +  const TSK_FS_ATTR *fsattr = NULL;
> +  int ret = 0, count = 0, index = 0;
> +  uint64_t *block = (uint64_t *) data;

Just dereference the pointer directly, so it will not be needed later
on:

  const uint64_t block = * (uint64_t *) data;

Or, even better, this can be done only when needed, ...

> +  const int flags = TSK_FS_FILE_WALK_FLAG_AONLY | TSK_FS_FILE_WALK_FLAG_SLACK;
> +
> +  if (entry_is_dot (fsfile))
> +    return TSK_WALK_CONT;
> +
> +  blkdata.found = false;
> +  blkdata.block = *block;

... here:

  blkdata.block = * (uint64_t *) data;

> +  /* Retrieve block list */
> +  count = tsk_fs_file_attr_getsize (fsfile);
> +
> +  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*) &blkdata);

(Minor style nitpick: space missing between "void" and "*".)

Should the return value of tsk_fs_attr_walk be checked for failures,
and return TSK_WALK_ERROR in case of error?

> +/* Attribute walk, searches the given block within the FS node attributes. */

Even if within 80 columns, I'd split it at 70 for readability.

> +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 (blkaddr == blkdata->block) {
> +    blkdata->found = true;

If I read the sleuthkit API docs, blkaddr will be meaningful only if
flags contains TSK_FS_BLOCK_FLAG_RAW.  Should attrwalk_callback check
for it?

Thanks,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160920/955d3a86/attachment.sig>