dtc: Return code of fdt_get_path()

dtc: Return code of fdt_get_path()

[Scott Wood]

The documentation for fdt_get_path() says it returns zero on success, 
but it actually returns the length of the string.  Is there any 
preference on which one to fix?

-Scott

Re: dtc: Return code of fdt_get_path()

[David Gibson]

On Tue, Aug 05, 2008 at 04:12:22PM -0500, Scott Wood wrote:
> The documentation for fdt_get_path() says it returns zero on success,  
> but it actually returns the length of the string.  Is there any  
> preference on which one to fix?

The documentation I think.  There may be existing users, and the
length of the path is potentially useful.

Although.. I think the confusion came because at one stage I planned
to have it return the full length of the path, even if the buffer was
smaller.  That would let the caller allocate a buffer of exactly the
right size on a second call.  Which is potentially useful, but not
compatible with returning NOSPACE as we do now.

Plus.. looking over that code again, I think there may be more
problems there - I think there may be cases where it runs out of space
but returns BADOFFSET or some error code instead of NOSPACE.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: dtc: Return code of fdt_get_path()

[David Gibson]

On Wed, Aug 06, 2008 at 03:33:10PM +1000, David Gibson wrote:
> On Tue, Aug 05, 2008 at 04:12:22PM -0500, Scott Wood wrote:
> > The documentation for fdt_get_path() says it returns zero on success,  
> > but it actually returns the length of the string.  Is there any  
> > preference on which one to fix?
> 
> The documentation I think.  There may be existing users, and the
> length of the path is potentially useful.
> 
> Although.. I think the confusion came because at one stage I planned
> to have it return the full length of the path, even if the buffer was
> smaller.  That would let the caller allocate a buffer of exactly the
> right size on a second call.  Which is potentially useful, but not
> compatible with returning NOSPACE as we do now.
> 
> Plus.. looking over that code again, I think there may be more
> problems there - I think there may be cases where it runs out of space
> but returns BADOFFSET or some error code instead of NOSPACE.

Ugh, yes, confirmed that it is buggy.  Not sure of the best way to fix
this, I can see three ways to go here.

	1. Minimally fix the error code bug, make sure NOSPACE is
returned when the buffer is too small, length of path otherwise,
update documentation to match.
	2. Make the code match the documentation: return 0 on success,
NOSPACE if the buffer is too small.
	3. Update both code and documentation to match the semantics I
think I must have intended at some point: always return the path
length, even if it exceeds the buffer length; caller is responsible
for checking if the path in the buffer is truncated.

(3) has the possibly useful property that you can use the call with a
small buffer and if it fails the caller now knows how big a buffer it
must allocate.  With (1) and (2) the caller's only way to deal with
NOSPACE is to keep allocating bigger buffers until the call succeeds.
However it makes it more complex for the caller to check the return
value - not just that it's non-negative put also that it's less than
the buffer size.  Plus its arguably stylistically inconsistent with
the read-write functions which return NOSPACE when the buffer is
exceeded with no indication of how much space is needed.

Any views, Jon?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: dtc: Return code of fdt_get_path()

[Jon Loeliger]

On Thu, 2008-08-07 at 16:08 +1000, David Gibson wrote:

> Ugh, yes, confirmed that it is buggy.  Not sure of the best way to fix
> this, I can see three ways to go here.
> 
> 	1. Minimally fix the error code bug, make sure NOSPACE is
> returned when the buffer is too small, length of path otherwise,
> update documentation to match.
> 	2. Make the code match the documentation: return 0 on success,
> NOSPACE if the buffer is too small.
> 	3. Update both code and documentation to match the semantics I
> think I must have intended at some point: always return the path
> length, even if it exceeds the buffer length; caller is responsible
> for checking if the path in the buffer is truncated.
> 
> (3) has the possibly useful property that you can use the call with a
> small buffer and if it fails the caller now knows how big a buffer it
> must allocate.  With (1) and (2) the caller's only way to deal with
> NOSPACE is to keep allocating bigger buffers until the call succeeds.
> However it makes it more complex for the caller to check the return
> value - not just that it's non-negative put also that it's less than
> the buffer size.  Plus its arguably stylistically inconsistent with
> the read-write functions which return NOSPACE when the buffer is
> exceeded with no indication of how much space is needed.
> 
> Any views, Jon?

Just rolling back into town and catching up some....

Has this issue been resolved yet?

jdl

Re: dtc: Return code of fdt_get_path()

[David Gibson]

On Tue, Aug 12, 2008 at 10:57:17AM -0500, Jon Loeliger wrote:
> On Thu, 2008-08-07 at 16:08 +1000, David Gibson wrote:
> 
> > Ugh, yes, confirmed that it is buggy.  Not sure of the best way to fix
> > this, I can see three ways to go here.
> > 
> > 	1. Minimally fix the error code bug, make sure NOSPACE is
> > returned when the buffer is too small, length of path otherwise,
> > update documentation to match.
> > 	2. Make the code match the documentation: return 0 on success,
> > NOSPACE if the buffer is too small.
> > 	3. Update both code and documentation to match the semantics I
> > think I must have intended at some point: always return the path
> > length, even if it exceeds the buffer length; caller is responsible
> > for checking if the path in the buffer is truncated.
> > 
> > (3) has the possibly useful property that you can use the call with a
> > small buffer and if it fails the caller now knows how big a buffer it
> > must allocate.  With (1) and (2) the caller's only way to deal with
> > NOSPACE is to keep allocating bigger buffers until the call succeeds.
> > However it makes it more complex for the caller to check the return
> > value - not just that it's non-negative put also that it's less than
> > the buffer size.  Plus its arguably stylistically inconsistent with
> > the read-write functions which return NOSPACE when the buffer is
> > exceeded with no indication of how much space is needed.
> > 
> > Any views, Jon?
> 
> Just rolling back into town and catching up some....
> 
> Has this issue been resolved yet?

No there's been no motion since this mail, as far as I know.  I was
waiting for your response, and otherwise occupied kernel bug hunting
anyway.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: dtc: Return code of fdt_get_path()

[David Gibson]

On Thu, Aug 07, 2008 at 04:08:20PM +1000, David Gibson wrote:
> On Wed, Aug 06, 2008 at 03:33:10PM +1000, David Gibson wrote:
> > On Tue, Aug 05, 2008 at 04:12:22PM -0500, Scott Wood wrote:
> > > The documentation for fdt_get_path() says it returns zero on success,  
> > > but it actually returns the length of the string.  Is there any  
> > > preference on which one to fix?
> > 
> > The documentation I think.  There may be existing users, and the
> > length of the path is potentially useful.
> > 
> > Although.. I think the confusion came because at one stage I planned
> > to have it return the full length of the path, even if the buffer was
> > smaller.  That would let the caller allocate a buffer of exactly the
> > right size on a second call.  Which is potentially useful, but not
> > compatible with returning NOSPACE as we do now.
> > 
> > Plus.. looking over that code again, I think there may be more
> > problems there - I think there may be cases where it runs out of space
> > but returns BADOFFSET or some error code instead of NOSPACE.
> 
> Ugh, yes, confirmed that it is buggy.  Not sure of the best way to fix
> this, I can see three ways to go here.
> 
> 	1. Minimally fix the error code bug, make sure NOSPACE is
> returned when the buffer is too small, length of path otherwise,
> update documentation to match.
> 	2. Make the code match the documentation: return 0 on success,
> NOSPACE if the buffer is too small.
> 	3. Update both code and documentation to match the semantics I
> think I must have intended at some point: always return the path
> length, even if it exceeds the buffer length; caller is responsible
> for checking if the path in the buffer is truncated.
> 
> (3) has the possibly useful property that you can use the call with a
> small buffer and if it fails the caller now knows how big a buffer it
> must allocate.  With (1) and (2) the caller's only way to deal with
> NOSPACE is to keep allocating bigger buffers until the call succeeds.
> However it makes it more complex for the caller to check the return
> value - not just that it's non-negative put also that it's less than
> the buffer size.  Plus its arguably stylistically inconsistent with
> the read-write functions which return NOSPACE when the buffer is
> exceeded with no indication of how much space is needed.

Ah.. except that on more detailed consideration I don't think (3) is
possible, without either maintaining a stack (which then has
allocation issues) or multiple passes through the tree.  And if we
can't return the path length in general, I don't see that there's a
lot of point returning it ever, so I'll make a patch implementing
option (2).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson