Re: [PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

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 /.)