On Wed, Jun 13, 2018 at 12:29:20AM -0600, Simon Glass wrote: > Hi David, > > On 11 June 2018 at 23:51, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Wed, Jun 06, 2018 at 03:36:59PM -0600, Simon Glass wrote: > >> > >> This adds the sequential-write interface which provides a way to create a > >> device tree from scratch. It also includes various other improvements and > >> additions to the existing functionality. > >> > >> This requires the following v4 patches: > >> > >> pylibfdt: Return string instead of bytearray from getprop() > >> pylibfdt: Allow reading integer values from properties > >> > >> Note: I'm sending this now rather than waiting on comments for patch > 10/10 > >> since there is enough rework here to be getting on with. > > > > I've now applied most of these. A few I had further comments on. > > > > One overall comment that applies to both the sw stuff and your > > existing rw code. At the moment, you mirror the C interface by > > requiring explicit resizing of the buffers holding the dt. That's a > > somewhat awkward interface in C, but we have a reason for it (allowing > > embedding in limited environments without an allocator). In Python, > > that interface is even odder, and there's not much reason for it - > > Python already requires an allocator. > > > > So, I'm wondering if it makes sense to change that interface - remove > > most of the 'size' parameters (except possibly as hints), trap > > FDT_ERR_NOSPACE errors from all the write functions and resize and > > retry. > > > > I've been thinking of adding an (optional) C interface that does that > > for ages, though I've never had time to implement it. In python it > > should be easier to do, though. > > It's been on my mind. There are three reasons I haven't gone this way so > far so far: > > 1. It changes what people might expect the functions to do, so might > introduce confusion (since we use the same names as libfdt) I guess, but remember you still have the "raw" C functions available to Python. This is the class interface on top of that, which can reasonably be expected to do a little more. > 2. It makes it impossible to detect and prevent an expansion of the DT. > This was just a vague feeling, but I now have a use case for this in the > U-Boot binman tool. It wants to set up the size of everything including the > DT so that it can pack the image together and figure out what goes where. > Then later it wants to update the DT without changing the size, so that it > doesn't need to repack. So I do want to support that mode of operation. That's a pretty solid reason, though. > 3. I figured it could be added later > > I do feel that the above reasons have less relevance with the > sequential-write interface, though. > > Since you brought it up, how best to implement it? I've tried hard to avoid > having state in the Fdt class, so for example you have to specify the > 'quiet' parameter to each method rather than setting it for all methods. > But an obvious answer is something like Fdt.set_auto_resize(bool). It seems > easier to justify handling this with a state variable, since people are > likely to want it one way or the other. I wouldn't suggest having it runtime changeable, so I'd make it an __init__ parameter. In fact I'd probably make the class with the init parameter internal and just expose two trivial subclasses which have it either on or off (class Fdt and class FdtResizing, say). > Another option would be to add another arg to the methods, e.g. auto_resize: > > def setprop_u32(self, nodeoffset, prop_name, val, quiet=(), > auto_resize=False): > > Or perhaps we should support both, with the sw interface defaulting to True > for this and the rw interface defaulting to False? In that case the default > value of auto_resize would be None, meaning to do what the current default > says. > > There are also minor questions like how much to expand by each time we run > out of space TBH, I don't think it'll matter much - just adding 1kiB until it works will probably do fine. > and whether to auto-pack at the end for the sw > interface. Well, fdt_finish() does effectively pack anyway. If you want to use sw, then rw functions, you'll generally need to fdt_open_into() after you fdt_finish(). You would, obviously need a method to return the current size. Oh.. which makes me realize - your pack() method should probably truncate self._fdt after calling fdt_pack(). -- 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
