On Mon, Feb 11, 2008 at 04:48:26PM +0100, Fabian Greffrath wrote:
> >Is there really a need to strdup() it?
>
> Yep. If you return the pointer (i.e. to `path') you'll get memory
> corruption as soon as you try to free (device_name).
You put that function in a separate file, which indicates it is meant to be
a general-purpose function, but its spec is constrained by the code in
grub-probe (i.e. you strdup() not because it is needed for the purpose of
this function, but because grub-probe already calls free()).
In case of a general-purpose function, it doesn't make much sense to strdup
it. Consider what would a caller have to do in case it wasn't using free()
already:
ret = check (device);
if (! ret)
fatal ("%s is bad", device);
free (ret);
instead of just:
if (! check (device))
fatal ("%s is bad", device);
> >I find it confusing that you change the meaning of the `path' variable
> >without
> >renaming it. Also, the variable that describes what `path' is
> >(argument_is_device) is passed separately.
>
> Allright. Maybe the `path' variable should be renamed to `argument'.
That works too ... but please put them together (i.e. both as arguments to
probe()).
> >I suspect advertising it here might lead users to think they can pass a
> >device
> >to it, without caring about [...] this option.
>
> Allright, this should be changed to something like this:
> "Probe device information for a given path (or device, if the -d option is
> given)."
Ok.
--
Robert Millan
<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
