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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >> ---
> >>
> >> 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.
> [..]
>
>
> >> +
> >> +/* This is used to copy property data into a bytearray */
> >> +%mypybuffer_mutable_binary(char *str, size_t size);
> >> +void pylibfdt_copy_value(char *str, size_t size,
> >> + const struct fdt_property *prop);
> >
> > This still seems convoluted to me. Maybe I'm misunderstanding
> > something about SWIG. But instead of using pylibfdt_copy_value() as
> > an intermediary, couldn't you create a custom typemap that directly
> > maps struct fdt_property *prop (as an outbound value) to a Python
> > tuple, doing the copy right there.
>
> Possibly, but here I am trying to use a standard typemap to do this
> and it would involve changing the error handling.
Hm, ok.
> The other problem is that the output typemap would need access to the
> device tree (fdt input parameter) in order to look up the name of the
> property.
Ah.. yes. But you could have the "raw" function return a tuple with
the name offset and value, then just look up the value in Python
code using fdt_string().
> I suspect a full-custom function could do this, but then it would need
> to written separately for each of the functions that returns struct
> fdt_property.
>
> Also my enthusiasm for more SWIG wrangling is waning faster than a
> schooner at the races...perhaps someone other than myself can plot a
> course to a better solution here?
Heh, fair enough.
>
> >
> > Oh.. also if you do need this function, use 'uint8_t *val' instead
> > of 'char *str' to reinforce the fact that this is a bytestring not a
> > C-style string we're dealing with.
>
> OK.
>
> [...]
>
> >> +
> >> +%apply int *OUTPUT { int *lenp };
> >> +
> >> +/* typemap used for fdt_getprop() */
> >> +%typemap(out) (const void *) {
> >> + if (!$1)
> >> + $result = Py_None;
> >> + else
> >> + /* TODO(sjg@xxxxxxxxxxxx): Can we avoid the 'arg4'? */
> >> + $result = Py_BuildValue("s#", $1, *arg4);
> >> +}
> >
> > Since you already have a custom typemap here for getprop(), couldn't
> > you drop the now unnecessary length (because Python strings know their
> > length) here, instead of doing it from the Python side?
>
> The length is necessary if it is an error number (the last parameter
> to fdt_getprop()).
Ah, of course.
--
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:
signature.asc
Description: PGP signature
