Re: [PATCH] Allow build without dirent.d_type, fix "ls (host)" crash

[Marco Gerards]

Christian Franke <address@hidden> writes:

> Marco Gerards wrote:
>
>>> ...
>>> +static int
>>> +is_dir(const char *path, const char *name)
>>>
>>
>> is_dir (
>>
>>
>
> OK. Too many projects, too many policies, sorry :-)

np, don't worry :-)

Hopefully you are not annoyed by my comments.  A consistent style
throughout GRUB is important to me.

>>> +{
>>> +  int len1 = strlen(path), len2 = strlen(name);
>>>
>>
>> Please split this up.  Or even better use separate declarations and
>> code.
>>
>>
>
> Yes. No. See comment below.
>
>>> +  char pathname[len1+1+len2+1+13];
>>>
>>
>> Please add spaces around binary operators.
>>
>>
>>> +  struct stat st;
>>> +  strcpy (pathname, path);
>>>
>>
>> Please add more blank lines between your code to make it easier to
>> read. :-)
>>
>>
>
> OK.
>
>
>>> ...
>>> +    strcat (pathname, "/");
>>> +  strcat (pathname, name);
>>> +  if (stat (pathname, &st))
>>> +    return 0;
>>> +  return S_ISDIR(st.st_mode);
>>> +}
>>> +#endif
>>>
>>
>> Why can't you just use S_ISDIR?
>>
>>
>
> ??

Nevermind, I wasn't really awake ;)

>>> ...
>>> @@ -81,6 +108,7 @@ grub_hostfs_read (grub_file_t file, char
>>>    FILE *f;
>>>     f = (FILE *) file->data;
>>> +  fseek (f, file->offset, SEEK_SET);
>>>    int s= fread (buf, 1, len, f);
>>>
>>
>> "int s = "
>>
>>
>
> Code janitor work outside the scope of this patch ... done ;-)

LOL!  Sorry, I was a bit enthousiastic ;-)

Thanks! :)

> BTW: This line uses "declaration statement" syntax, therefore this is
> apparently accepted in GRUB2 codebase :-)
> I definitely prefer this (first use = declaration = initialization)
> style, which is state of the art in most (all?) modern languages.
> This C++ (1986) feature and early GCC (1.*) extension was finally
> included into C99, so it is portable now.
> is_dir() rewritten accordingly.

It is.  The problem is that you had two function calls on one line.  I
want to avoid that.

> Christian
>
> 2007-11-09  Christian Franke  <address@hidden>
>
>       * util/hostfs.c (is_dir): New function.
>       (grub_hostfs_dir):  Handle missing dirent.d_type case.
>       (grub_hostfs_read): Add missing fseek().
>       (grub_hostfs_label): Clear label pointer.  This fixes a crash
>       of grub-emu on "ls (host)".

Looks fine to me.

btw, when do you use (host)?  It's intended as a debugging tool.

--
Marco