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

[NoxDaFox]

2016-09-20 11:38 GMT+03:00 Pino Toscano <ptoscano at redhat.com>:

> 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?
>
I was thinking the same but I then took a look at its usage within the
sleuthkit tool and it seems not being checked.
https://github.com/sleuthkit/sleuthkit/blob/develop/tsk/fs/ifind_lib.c#L479

Not sure whether to trust the API docs or the code. My main concern is
ignoring some relevant blocks.

I will test it with both solutions and see.

I will also fix the rest.

Thanks for the comments.


>
> Thanks,
> --
> Pino Toscano
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160920/ec3e55ca/attachment.htm>