[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails
From: |
Felix Zielcke |
Subject: |
Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails |
Date: |
Fri, 12 Jun 2009 18:33:55 +0200 |
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
- Re: grub-install --root-directory=/mnt /dev/sda1 fails, Felix Zielcke, 2009/06/01
- [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails, Felix Zielcke, 2009/06/08
- Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails, Vladimir 'phcoder' Serbinenko, 2009/06/09
- Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails, Felix Zielcke, 2009/06/10
- Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails, Felix Zielcke, 2009/06/11
- Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails, Felix Zielcke, 2009/06/11
- Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails, Felix Zielcke, 2009/06/12
- Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails, Pavel Roskin, 2009/06/12
- Re: [PATCH] Re: grub-install --root-directory=/mnt /dev/sda1 fails,
Felix Zielcke <=