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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
