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

[noxdafox]

On 26/08/16 15:58, Pino Toscano wrote:
> 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 "."
Didn't know about STREQ, it indeed makes the code more readable.

The logic was:

if IS_DOT
. if (IS_NOT_ROOT or IS_NOT_SINGLE_DOT)
.. SKIP_THE_ENTRY
else
. PARSE_THE_ENTRY

I changed it in something more readable:

if IS_DOT
. if (IS_ROOT and IS_SINGLE_DOT)
.. PARSE_THE_ENTRY
. else
.. SKIP_THE_ENTRY
else
. PARSE_THE_ENTRY
>
>> 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?
Yes, the callback returns the fsfile struct and the path in two separate 
parameters.
>
>> 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?
As I explained in the last patch series, the callback will be called for 
every entry.

. <-- the Root entry
etc/.
etc/.. <-- again the Root entry
etc/systemd/.
etc/systemd/..
bin/.
bin/.. <-- again the Root entry

Therefore we need to make sure we return the Root entry only once.
>
>
> _______________________________________________
> 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/20160830/fd81894f/attachment.htm>