[Qemu-devel] using fdt_setprop() to set properties to empty values

[Qemu-devel] using fdt_setprop() to set properties to empty values

[Peter Maydell]

What's the right way to use libfdt's fdt_setprop to set a property
to have an empty value? At the moment in QEMU we tend to use
  fdt_setprop(fdt, nodeoffset, "propertyname", NULL, 0);

and git grep 'fdt_setprop.*NULL' produces examples of this usage in
PPC and ARM fdt creation code.

However the fdt_setprop() documentation doesn't document that a NULL
value pointer is OK if the length is 0, and indeed the implementation
unconditionally calls memcpy(prop->data, val, len), which is
undefined behaviour, and warned about by clang sanitizers if you
build libfdt with them:
 dtc/libfdt/fdt_rw.c:288:21: runtime error: null pointer passed
   as argument 2, which is declared to never be null

So what's the best thing to do here? I can't offhand think of a
non-ugly/non-confusing way to pass a valid pointer here...

thanks
-- PMM

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[Eric Blake]

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

On 02/23/2017 06:33 AM, Peter Maydell wrote:
> What's the right way to use libfdt's fdt_setprop to set a property
> to have an empty value? At the moment in QEMU we tend to use
>   fdt_setprop(fdt, nodeoffset, "propertyname", NULL, 0);
> 
> and git grep 'fdt_setprop.*NULL' produces examples of this usage in
> PPC and ARM fdt creation code.
> 
> However the fdt_setprop() documentation doesn't document that a NULL
> value pointer is OK if the length is 0, and indeed the implementation
> unconditionally calls memcpy(prop->data, val, len), which is
> undefined behaviour, and warned about by clang sanitizers if you
> build libfdt with them:
>  dtc/libfdt/fdt_rw.c:288:21: runtime error: null pointer passed
>    as argument 2, which is declared to never be null
> 
> So what's the best thing to do here? I can't offhand think of a
> non-ugly/non-confusing way to pass a valid pointer here...

Does fdt_setprop(fdt, nodeoffset, "propertyname", "", 0) do the right thing?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[David Gibson]

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

On Thu, Feb 23, 2017 at 08:52:01AM -0600, Eric Blake wrote:
> On 02/23/2017 06:33 AM, Peter Maydell wrote:
> > What's the right way to use libfdt's fdt_setprop to set a property
> > to have an empty value? At the moment in QEMU we tend to use
> >   fdt_setprop(fdt, nodeoffset, "propertyname", NULL, 0);

That's what I've always used..

> > and git grep 'fdt_setprop.*NULL' produces examples of this usage in
> > PPC and ARM fdt creation code.
> > 
> > However the fdt_setprop() documentation doesn't document that a NULL
> > value pointer is OK if the length is 0, and indeed the implementation
> > unconditionally calls memcpy(prop->data, val, len), which is
> > undefined behaviour, and warned about by clang sanitizers if you
> > build libfdt with them:
> >  dtc/libfdt/fdt_rw.c:288:21: runtime error: null pointer passed
> >    as argument 2, which is declared to never be null

..but indeed that isn't really technically right.

> > So what's the best thing to do here? I can't offhand think of a
> > non-ugly/non-confusing way to pass a valid pointer here...
> 
> Does fdt_setprop(fdt, nodeoffset, "propertyname", "", 0) do the right thing?

That should work, but it's a bit clunky.

I guess I should add an fdt_setprop_empty() to libfdt.

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

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[David Gibson]

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

On Fri, Feb 24, 2017 at 09:49:11AM +1100, David Gibson wrote:
> On Thu, Feb 23, 2017 at 08:52:01AM -0600, Eric Blake wrote:
> > On 02/23/2017 06:33 AM, Peter Maydell wrote:
> > > What's the right way to use libfdt's fdt_setprop to set a property
> > > to have an empty value? At the moment in QEMU we tend to use
> > >   fdt_setprop(fdt, nodeoffset, "propertyname", NULL, 0);
> 
> That's what I've always used..
> 
> > > and git grep 'fdt_setprop.*NULL' produces examples of this usage in
> > > PPC and ARM fdt creation code.
> > > 
> > > However the fdt_setprop() documentation doesn't document that a NULL
> > > value pointer is OK if the length is 0, and indeed the implementation
> > > unconditionally calls memcpy(prop->data, val, len), which is
> > > undefined behaviour, and warned about by clang sanitizers if you
> > > build libfdt with them:
> > >  dtc/libfdt/fdt_rw.c:288:21: runtime error: null pointer passed
> > >    as argument 2, which is declared to never be null
> 
> ..but indeed that isn't really technically right.
> 
> > > So what's the best thing to do here? I can't offhand think of a
> > > non-ugly/non-confusing way to pass a valid pointer here...
> > 
> > Does fdt_setprop(fdt, nodeoffset, "propertyname", "", 0) do the right thing?
> 
> That should work, but it's a bit clunky.
> 
> I guess I should add an fdt_setprop_empty() to libfdt.

Ok, I've pushed libfdt upstream patches to (a) make passing NULL to
setprop() with zero length explicitly safe and (b) add an
fdt_setprop_empty() helper macro.  Do you want me to make a pullreq to
update the qemu submodule?

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

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[Peter Maydell]

On 24 February 2017 at 00:16, David Gibson <david@gibson.dropbear.id.au> wrote:
> Ok, I've pushed libfdt upstream patches to (a) make passing NULL to
> setprop() with zero length explicitly safe and (b) add an
> fdt_setprop_empty() helper macro.  Do you want me to make a pullreq to
> update the qemu submodule?

Yes, please. Are we OK with using a random libfdt commit or do
we update only to proper release tags? There's no real rush with
this so if you have a release due shortly it might be better
to wait for that.

thanks
-- PMM

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[David Gibson]

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

On Fri, Feb 24, 2017 at 10:35:35AM +0000, Peter Maydell wrote:
> On 24 February 2017 at 00:16, David Gibson <david@gibson.dropbear.id.au> wrote:
> > Ok, I've pushed libfdt upstream patches to (a) make passing NULL to
> > setprop() with zero length explicitly safe and (b) add an
> > fdt_setprop_empty() helper macro.  Do you want me to make a pullreq to
> > update the qemu submodule?
> 
> Yes, please. Are we OK with using a random libfdt commit or do
> we update only to proper release tags?

I'm find with a random SHA, but that's not really my department - I'm
upstream libfdt maintainer, but update policy in the qemu tree seems
like a qemu side decision.

> There's no real rush with
> this so if you have a release due shortly it might be better
> to wait for that.

dtc/libfdt releases are a rather haphazard affair.  Usually they
happen when somebody complains that there hasn't been a release with
some feature they want.  Our tests are both fast to run and have
reasonaably good coverage, so random commits are usually good.  So a
"release" is usually just slapping a new version number onto whatever
is in master and making a tag and tarball.

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

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[Peter Maydell]

On 27 February 2017 at 01:05, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Fri, Feb 24, 2017 at 10:35:35AM +0000, Peter Maydell wrote:
>> On 24 February 2017 at 00:16, David Gibson <david@gibson.dropbear.id.au> wrote:
>> > Ok, I've pushed libfdt upstream patches to (a) make passing NULL to
>> > setprop() with zero length explicitly safe and (b) add an
>> > fdt_setprop_empty() helper macro.  Do you want me to make a pullreq to
>> > update the qemu submodule?
>>
>> Yes, please. Are we OK with using a random libfdt commit or do
>> we update only to proper release tags?
>
> I'm find with a random SHA, but that's not really my department - I'm
> upstream libfdt maintainer, but update policy in the qemu tree seems
> like a qemu side decision.
>
>> There's no real rush with
>> this so if you have a release due shortly it might be better
>> to wait for that.
>
> dtc/libfdt releases are a rather haphazard affair.  Usually they
> happen when somebody complains that there hasn't been a release with
> some feature they want.  Our tests are both fast to run and have
> reasonaably good coverage, so random commits are usually good.  So a
> "release" is usually just slapping a new version number onto whatever
> is in master and making a tag and tarball.

>From my end I think we'd rather use a proper release version
(if only because it's then easier to refer to and to state
as a dependency for packaged versions if required). It looks
like we've done that for our previous updates (starting
with 1.3.0 and then moving to 1.4.0 and 1.4.2) so I think
we should continue using released versions.

thanks
-- PMM

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[David Gibson]

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

On Mon, Feb 27, 2017 at 10:11:57AM +0000, Peter Maydell wrote:
> On 27 February 2017 at 01:05, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Fri, Feb 24, 2017 at 10:35:35AM +0000, Peter Maydell wrote:
> >> On 24 February 2017 at 00:16, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> > Ok, I've pushed libfdt upstream patches to (a) make passing NULL to
> >> > setprop() with zero length explicitly safe and (b) add an
> >> > fdt_setprop_empty() helper macro.  Do you want me to make a pullreq to
> >> > update the qemu submodule?
> >>
> >> Yes, please. Are we OK with using a random libfdt commit or do
> >> we update only to proper release tags?
> >
> > I'm find with a random SHA, but that's not really my department - I'm
> > upstream libfdt maintainer, but update policy in the qemu tree seems
> > like a qemu side decision.
> >
> >> There's no real rush with
> >> this so if you have a release due shortly it might be better
> >> to wait for that.
> >
> > dtc/libfdt releases are a rather haphazard affair.  Usually they
> > happen when somebody complains that there hasn't been a release with
> > some feature they want.  Our tests are both fast to run and have
> > reasonaably good coverage, so random commits are usually good.  So a
> > "release" is usually just slapping a new version number onto whatever
> > is in master and making a tag and tarball.
> 
> >From my end I think we'd rather use a proper release version
> (if only because it's then easier to refer to and to state
> as a dependency for packaged versions if required). It looks
> like we've done that for our previous updates (starting
> with 1.3.0 and then moving to 1.4.0 and 1.4.2) so I think
> we should continue using released versions.

Ok; I just tagged and released dtc 1.4.3.  I'll send a qemu update
when I get a chance to remind myself how to do submodule updates
again.

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

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[David Gibson]

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

On Wed, Mar 01, 2017 at 11:16:02AM +1100, David Gibson wrote:
> On Mon, Feb 27, 2017 at 10:11:57AM +0000, Peter Maydell wrote:
> > On 27 February 2017 at 01:05, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > On Fri, Feb 24, 2017 at 10:35:35AM +0000, Peter Maydell wrote:
> > >> On 24 February 2017 at 00:16, David Gibson <david@gibson.dropbear.id.au> wrote:
> > >> > Ok, I've pushed libfdt upstream patches to (a) make passing NULL to
> > >> > setprop() with zero length explicitly safe and (b) add an
> > >> > fdt_setprop_empty() helper macro.  Do you want me to make a pullreq to
> > >> > update the qemu submodule?
> > >>
> > >> Yes, please. Are we OK with using a random libfdt commit or do
> > >> we update only to proper release tags?
> > >
> > > I'm find with a random SHA, but that's not really my department - I'm
> > > upstream libfdt maintainer, but update policy in the qemu tree seems
> > > like a qemu side decision.
> > >
> > >> There's no real rush with
> > >> this so if you have a release due shortly it might be better
> > >> to wait for that.
> > >
> > > dtc/libfdt releases are a rather haphazard affair.  Usually they
> > > happen when somebody complains that there hasn't been a release with
> > > some feature they want.  Our tests are both fast to run and have
> > > reasonaably good coverage, so random commits are usually good.  So a
> > > "release" is usually just slapping a new version number onto whatever
> > > is in master and making a tag and tarball.
> > 
> > >From my end I think we'd rather use a proper release version
> > (if only because it's then easier to refer to and to state
> > as a dependency for packaged versions if required). It looks
> > like we've done that for our previous updates (starting
> > with 1.3.0 and then moving to 1.4.0 and 1.4.2) so I think
> > we should continue using released versions.
> 
> Ok; I just tagged and released dtc 1.4.3.  I'll send a qemu update
> when I get a chance to remind myself how to do submodule updates
> again.

Uh.. actually, first I need the qemu mirror of dtc to be updated to
include the new release.  Is that synced automatically, or does
someone need to kick it?

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

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[David Gibson]

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

On Mon, Feb 27, 2017 at 10:11:57AM +0000, Peter Maydell wrote:
> On 27 February 2017 at 01:05, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Fri, Feb 24, 2017 at 10:35:35AM +0000, Peter Maydell wrote:
> >> On 24 February 2017 at 00:16, David Gibson <david@gibson.dropbear.id.au> wrote:
> >> > Ok, I've pushed libfdt upstream patches to (a) make passing NULL to
> >> > setprop() with zero length explicitly safe and (b) add an
> >> > fdt_setprop_empty() helper macro.  Do you want me to make a pullreq to
> >> > update the qemu submodule?
> >>
> >> Yes, please. Are we OK with using a random libfdt commit or do
> >> we update only to proper release tags?
> >
> > I'm find with a random SHA, but that's not really my department - I'm
> > upstream libfdt maintainer, but update policy in the qemu tree seems
> > like a qemu side decision.
> >
> >> There's no real rush with
> >> this so if you have a release due shortly it might be better
> >> to wait for that.
> >
> > dtc/libfdt releases are a rather haphazard affair.  Usually they
> > happen when somebody complains that there hasn't been a release with
> > some feature they want.  Our tests are both fast to run and have
> > reasonaably good coverage, so random commits are usually good.  So a
> > "release" is usually just slapping a new version number onto whatever
> > is in master and making a tag and tarball.
> 
> >From my end I think we'd rather use a proper release version
> (if only because it's then easier to refer to and to state
> as a dependency for packaged versions if required). It looks
> like we've done that for our previous updates (starting
> with 1.3.0 and then moving to 1.4.0 and 1.4.2) so I think
> we should continue using released versions.

Ok, I've made the upstream release, and sent a pull request with an
update for the qemu submodule.  I've bundled it up with a SLOF update,
which I forgot to put in the ppc pull request I also made today.

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