From: Marco Gerards <mgerards@xs4all.nl>
To: The development of GRUB 2 <grub-devel@gnu.org>
References: <471E5075.9090309@t-online.de> <472B8FA5.5030400@t-online.de>
	<874pfvmpta.fsf@xs4all.nl> <4734CBEB.7080500@t-online.de>
Mail-Copies-To: mgerards@xs4all.nl
Date: Sat, 10 Nov 2007 17:03:48 +0100
In-Reply-To: <4734CBEB.7080500@t-online.de> (Christian Franke's message of
	"Fri, 09 Nov 2007 22:06:51 +0100")
Message-ID: <87ve8agk9n.fsf@xs4all.nl>
User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: Re: [PATCH] Allow build without dirent.d_type, fix "ls (host)" crash
Precedence: list
Reply-To: The development of GRUB 2 <grub-devel@gnu.org>

Christian Franke <Christian.Franke@t-online.de> 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  <franke@computer.org>
>
> 	* 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