Pino Toscano
2016-Aug-26 11:15 UTC
Re: [Libguestfs] [PATCH v2 1/6] filesystem_walk: fixed root inode listing
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@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)) -- Pino Toscano
noxdafox
2016-Aug-26 12:15 UTC
Re: [Libguestfs] [PATCH v2 1/6] filesystem_walk: fixed root inode listing
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@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. TSK_FS_ISDOT returns whether the string is either '.' or '..'. Yet we want to return Root so we check if that's the case. Problem is that we'd return multiple entries for Root because: . .. etc/.. bin/.. Are all matching the statement: fsfile->fs_info->root_inum != fsfile->name->meta_addr Either we check that we return it only once (which complicates the logic quite a bit) or we strictly ensure the entry 'meta_addr' is root and the name is '.'. I'll add a comment to make the logic more clear. Yet if you have better ideas they're more than welcome.> > > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs
Pino Toscano
2016-Aug-26 12:58 UTC
Re: [Libguestfs] [PATCH v2 1/6] filesystem_walk: fixed root inode listing
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@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_addrI 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
Apparently Analagous Threads
- Re: [PATCH v2 1/6] filesystem_walk: fixed root inode listing
- [PATCH] filesystem_walk: fixed root inode listing
- Re: [PATCH v2 1/6] filesystem_walk: fixed root inode listing
- [PATCH v4 0/6] New API: find_inode
- [PATCH v2 1/6] filesystem_walk: fixed root inode listing