Re: [PATCH v5 1/5] Add an initial Python library for libfdt

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5821 bytes --]

On Tue, Feb 21, 2017 at 11:07:58AM -0700, Simon Glass wrote:
> Hi David,
> 
> On 19 February 2017 at 20:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Thu, Feb 16, 2017 at 09:48:22PM -0700, Simon Glass wrote:
> >> Hi David,
> >>
> >> On 14 February 2017 at 22:29, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> >> > On Tue, Feb 14, 2017 at 08:51:56PM -0700, Simon Glass wrote:
> >> >> Add Python bindings for a bare-bones set of libfdt functions. These allow
> >> >> navigating the tree and reading node names and properties.
> >> >>
> >> >> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> >> ---
> >> >>
> >> >> Changes in v5:
> >> >> - Use a 'quiet' parameter instead of quiet versions of functions
> >> >> - Add a Property object to hold a property's name and value
> >> >> - Drop the data() and string() functions which are not needed now
> >> >> - Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
> >> >> - Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
> >> >> - Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
> >> >> - Use $(SWIG) to call swig from the Makefile
> >> >> - Review function comments
> >> >>
> >> >> Changes in v4:
> >> >> - Make the library less pythonic to avoid a shaky illusion
> >> >> - Drop classes for Node and Prop, along with associated methods
> >> >> - Include libfdt.h instead of repeating it
> >> >> - Add support for fdt_getprop()
> >> >> - Bring in all libfdt functions (but Python support is missing for many)
> >> >> - Add full comments for Python methods
> >> >>
> >> >> Changes in v3:
> >> >> - Make the library more pythonic
> >> >> - Add classes for Node and Prop along with methods
> >> >> - Add an exception class
> >> >> - Use Python to generate exeptions instead of SWIG
> >> >>
> >> >> Changes in v2:
> >> >> - Add exceptions when functions return an error
> >> >> - Correct Python naming to following PEP8
> >> >> - Use a class to encapsulate the various methods
> >> >> - Include fdt.h instead of redefining struct fdt_property
> >> >> - Use bytearray to avoid the SWIG warning 454
> >> >> - Add comments
> >> >>
> >> >>  Makefile                   |   1 +
> >> >>  pylibfdt/.gitignore        |   3 +
> >> >>  pylibfdt/Makefile.pylibfdt |  18 ++
> >> >>  pylibfdt/libfdt.swig       | 465 +++++++++++++++++++++++++++++++++++++++++++++
> >> >>  pylibfdt/setup.py          |  34 ++++
> >> >>  5 files changed, 521 insertions(+)
> >> >>  create mode 100644 pylibfdt/.gitignore
> >> >>  create mode 100644 pylibfdt/Makefile.pylibfdt
> >> >>  create mode 100644 pylibfdt/libfdt.swig
> >> >>  create mode 100644 pylibfdt/setup.py
> >> >>
> >>
> >> [..]
> >>
> >> >> +def check_err_null(val, quiet=[]):
> >> >> +    """Raise an error if the return value is NULL
> >> >> +
> >> >> +    This is used to check for a NULL return value from certain libfdt C
> >> >> +    functions
> >> >> +
> >> >> +    Args:
> >> >> +        val: Return value from a libfdt function
> >> >> +        quiet: Errors to ignore (empty to raise on all errors)
> >> >> +
> >> >> +    Returns:
> >> >> +        val if val is a list, None if not
> >> >> +
> >> >> +    Raises
> >> >> +        FdtException if val indicates an error was reported and the error
> >> >> +        is not in @quiet.
> >> >> +    """
> >> >> +    # Normally a tuple is returned which contains the data and its length.
> >> >
> >> > Is it a tuple returned..?
> >> >
> >> >> +    # If we get just an integer error code, it means the function failed.
> >> >> +    if not isinstance(val, list):
> >> >
> >> > ..or a list?  Seems like either the comment or the code must be
> >> > incorrect here.
> >>
> >> OK.
> >>
> >> >
> >> > Come to that, what is it tells swig to map fdt_propery_by_offset() and
> >> > the like to the pair of values?  How does it know how to detect the
> >> > error cases?
> >> >
> >> > From the usage, I take it that in the success case val[0] is a string
> >> > or bytearray containing the relevant chunk of data.  Remind me what
> >> > the second value is?
> >>
> >> It's the length, or error number. The final arg to
> >> fdt_get_property_by_offset() is int *len.
> >
> > Ok.  Why is it sometimes a tuple and sometimes a bare error value
> > then?  I would have expect it to always return a 2-tuple, the first
> > being the return value (maybe NULL/None) and the second being the
> > len/err value returned by reference.
> >
> > I guess that's a swig question rather than  question for you, though.
> 
> Well it's just the way that it seems to work. I think it makes some
> sense in that we don't want to return a NULL pointer into Python. But
> I agree it is a bit odd and it took me a while to get used to it.

I suppose so.  Although None seems a pretty obvious way to translate
the NULL as well.

> Here is the generated code. I'm not sure what SWIG_IsTmpObj() does exactly...
> 
>   result = (struct fdt_property *)fdt_get_property_by_offset((void
> const *)arg1,arg2,arg3);
>   resultobj = SWIG_NewPointerObj(SWIG_as_voidptr(result),
> SWIGTYPE_p_fdt_property, 0 |  0 );
>   if (SWIG_IsTmpObj(res3)) {
>     resultobj = SWIG_Python_AppendOutput(resultobj, SWIG_From_int((*arg3)));
>   } else {
>     int new_flags = SWIG_IsNewObj(res3) ? (SWIG_POINTER_OWN |  0 ) :  0 ;
>     resultobj = SWIG_Python_AppendOutput(resultobj,
> SWIG_NewPointerObj((void*)(arg3), SWIGTYPE_p_int, new_flags));
>   }

Thanks for the info.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]