On Sat, Sep 07, 2019 at 10:37:24AM -0700, Dmitry Torokhov wrote: > On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote: > > On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote: > > > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote: > > > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote: > > > > > > > + } else if (src->type == DEV_PROP_REF) { > > > > > + /* All reference properties must be arrays */ > > > > > + return -EINVAL; > > > > > > > > Hmm... What about to duplicate pointer under value union and use > > > > is_array to > > > > distinguish which one to use? Because... > > > > > > Then we have to special-case copying this entry, similar to the pains we > > > are going with the strings. > > > > I can't see it as a pain. Simple do the same kmemdup() for the case when > > is_array = false and DEV_TYPE_REF? > > And then you need to make sure it is freed on error paths and when we > remove property entries. This requires more checks and code. In contrast > we already know how to handle out of line objects of arbitrary size.
We can put it one level up to be a sibling to value / pointer unions. In that case is_array can be anything (we just don't care). Actually strings aren't inlined. > > By the way, don't we need to update property_entry_{get,set}_pointer()? > > I do not see these, where are they? swnode.c. I meant property_{get,set}_pointer(). > > > > > + .is_array = true, > > > > > \ > > > > > > > > I really don't like this "cheating". > > > > > > This is not cheating. Any single value can be represented as an array of > > > one element. Actually, the only reason we have this "is_array" business > > > is because for scalar values and short strings it is much cheaper to > > > store single value in-line instead of out of line + pointer, especially > > > on 64 bit arches. > > > > Yes, and this is a lot of benefit! > > Yes, nobody argues against it. Here however we are dealing with a larger > structure. There is absolutely no benefit of trying to separate single > value vs array here. Thus, moving to upper layer makes more sense. Right? > > > If you want we can change is_array into is_inline. > > > > Nope, is_array is exactly what it tells us about the content. Its functional > > load is to distinguish which union (value vs. pointer) we are using. > > No, it signifies whether the value is stored within property entry or > outside. I can fit probably 8 bytes arrays into property entry > structure, in which case is_array will definitely not reflect the data > type. Nope, since strings are not inlined AFAICS. > It is the type-specific accessors that know how to parse and fetch data > from properties. -- With Best Regards, Andy Shevchenko