[Libguestfs] [PATCH v2 1/6] filesystem_walk: fixed root inode listing

[Pino Toscano]

On Friday, 26 August 2016 15:15:17 CEST noxdafox wrote:
> On 26/08/16 14:15, Pino Toscano wrote:
> > On Thursday, 25 August 2016 23:53:51 CEST Matteo Cafasso wrote:
> >> With the current implementation, the root inode of the given partition
> >> is ignored.
> >>
> >> The root inode is now reported. Its name will be a single dot '.'
> >> reproducing the TSK API.
> >>
> >> Signed-off-by: Matteo Cafasso <noxdafox at gmail.com>
> >> ---
> >>   daemon/tsk.c | 17 ++++++++++++++---
> >>   1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/daemon/tsk.c b/daemon/tsk.c
> >> index dd368d7..6e6df6d 100644
> >> --- a/daemon/tsk.c
> >> +++ b/daemon/tsk.c
> >> @@ -48,6 +48,7 @@ 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 *);
> >>   static int send_dirent_info (guestfs_int_tsk_dirent *);
> >> +static int entry_is_dot(TSK_FS_FILE *);
> >>   static void reply_with_tsk_error (const char *);
> > Since in patch #2 this forward declaration is moved, put it at the
> > right place already in this patch.
> >
> >>   int
> >> @@ -113,9 +114,7 @@ fswalk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
> >>     CLEANUP_FREE char *fname = NULL;
> >>     struct guestfs_int_tsk_dirent dirent;
> >>   
> >> -  /* Ignore ./ and ../ */
> >> -  ret = TSK_FS_ISDOT (fsfile->name->name);
> >> -  if (ret != 0)
> >> +  if (entry_is_dot(fsfile))
> >>       return TSK_WALK_CONT;
> > Nitpick: add a space between the function name and the opening round
> > bracket.
> >
> >>     /* Build the full relative path of the entry */
> >> @@ -271,6 +270,18 @@ reply_with_tsk_error (const char *funcname)
> >>       reply_with_error ("%s: unknown error", funcname);
> >>   }
> >>   
> >> +/* Check whether the entry is dot and is not Root. */
> >> +static int
> >> +entry_is_dot(TSK_FS_FILE *fsfile)
> > Ditto.
> >
> >> +{
> >> +  if (TSK_FS_ISDOT (fsfile->name->name))
> >> +    if (fsfile->fs_info->root_inum != fsfile->name->meta_addr ||
> >> +        strcmp (fsfile->name->name, "."))
> > Simply merge the two if's into a single if (A && B) condition?
> > Also, the strcmp is already done by TSK_FS_ISDOT, isn't it?
> > So this should be like:
> >
> >    if (TSK_FS_ISDOT (fsfile->name->name)
> >        && (fsfile->fs_info->root_inum != fsfile->name->meta_addr))
> It's a bit more complicated, that's why I preferred to keep the two if 
> statements separated.

Most probably I expressed myself in an unclear way:

* TSK_FS_ISDOT (foo) returns truen when foo is "." or ".."

* strcmp (foo, ".") -- btw, please use STREQ/STRNEQ -- returns true
  when foo is not "."

> TSK_FS_ISDOT returns whether the string is either '.' or '..'.
> Yet we want to return Root so we check if that's the case.

OK.

> Problem is that we'd return multiple entries for Root because:
> .
> ..
> etc/..
> bin/..

fsfile->name->name is a filename only, isn't it?

> Are all matching the statement:
> 
> fsfile->fs_info->root_inum != fsfile->name->meta_addr

I don't understand this: if the statement will match all the dot and
dot-dot entries, why is it checked at all?
Or it will be valid for all the dot and dot-dot inodes different than
the ones in the root, right?

-- 
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/20160826/5ae4e3ae/attachment.sig>