Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails

[Felix Zielcke]

Am Freitag, den 12.06.2009, 12:21 -0400 schrieb Pavel Roskin:
> On Fri, 2009-06-12 at 12:28 +0200, Felix Zielcke wrote:
> 
> > Ok here's a new one which compiles without warnings.
> 
> I suggest that whenever a patch is published, it comes with a detailed
> description.  Surely, it could be gathered by rereading the thread, but
> I think it's not sufficient for two reasons.
> 
> The first reason is that we want the patches reviewed by more people.
> Not everybody has time to read the whole discussion.
> 
> The second reason is that the description the shows how the author sees
> the changes, what the potential benefits are, what code is affected.
> The reviewers would be able to compare the goals to the implementation.
> 
> Even if the description was already published, it should not be hard to
> publish it again, perhaps with some improvements.

Well it does (well should) exactly do the same as the
make_system_path_relative_to_its_root in util/grub-mkconfig_lib except
that it's written in C instead of a bash function.

> Regarding this patch, I don't think we need to add a function to
> hostdisk.c is it's only used in grub-setup.c.  It would be better to
> have it in grub-setup.c unless it's a generic function or there is a
> good chance that it will be used elsewhere.
> 
> grub_make_system_path_relative_to_its_root is a very long name.  There
> is nothing wrong with a long name per se, but it may be in sign that the
> function is too specialized and should not be in hostdisk.c.
> 
> Following is not good for several reasons:
> 
> #warning "The function `grub_make_system_path_relative_to_its_root'
> might not work on your OS correctly."
> 
> The warning will only be seen by those who compile GRUB, but not by the
> end users who installed a binary.  The warning doesn't explain what
> exactly may not work correctly.  Since we are talking about new
> functionality here, I'd rather omit an unsafe implementation if
> realpath() is not available.
> 
> What is free_ptr?  It looks like it's a pointer that the caller should
> free.  I think functions should be self-contained and should not ask the
> caller to do the cleanup (unless they actually return something useful
> in the allocated memory).

Robert anyway already said that this needs discussing.
The function is currently only useful in grub-setup in the case
blocklists are used for core.img
I don't think it will be useful for anything else.

About free_ptr,
yeah I could just copy buf2 + offset to a buf3 and then just free buf2
inside the function so that the caller can just free the return value of
it.
But I got this idea only after I sent the patch and I already told
Vladimir that he doestn't need to look anymore because Robert wants to
have the whole design discussed first.

-- 
Felix Zielcke