[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Add option to grub-probe to accept system devices as argumen
From: |
Robert Millan |
Subject: |
Re: [PATCH] Add option to grub-probe to accept system devices as arguments |
Date: |
Mon, 11 Feb 2008 17:12:30 +0100 |
User-agent: |
Mutt/1.5.13 (2006-08-11) |
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 /.)
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Fabian Greffrath, 2008/02/11
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments,
Robert Millan <=
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Fabian Greffrath, 2008/02/12
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Fabian Greffrath, 2008/02/12
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Fabian Greffrath, 2008/02/13
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Robert Millan, 2008/02/13
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Fabian Greffrath, 2008/02/13
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Robert Millan, 2008/02/13
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Fabian Greffrath, 2008/02/14
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Robert Millan, 2008/02/17
- Re: [PATCH] Add option to grub-probe to accept system devices as arguments, Fabian Greffrath, 2008/02/18