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

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(a)gmail.com&gt; > --- 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 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs