Libguestfs - Aug 2016 - Re: [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

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

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_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