review request

review request

[Damian, Alexandru]

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

Hello,

I have a set of patches for review on:

http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=toaster/master

The last one, edb12586c27ccddb054737ee6e081f663e2c60da, is pretty invasive
as it
changes the package storage model, from different tables for target and
build packages,
to a single table.

Can you please review and comment on this patchset? and let me know
whenever it's ok to submit upstream ?

Cheers,
Alex


-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 840 bytes --]

Re: review request

[Paul Eggleton]

Hi Alex,

On Tuesday 26 November 2013 18:28:21 Damian, Alexandru wrote:
> I have a set of patches for review on:
> 
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=toaster/master

Sorry for the delay (although we've had some changes in the mean time with the 
pkgdata changes). Reviewing the latest branch, most of the changes look OK, 
however I did notice the following:

> bitbake: toasterui: record recipe licensing information

If do_populate_lic doesn't execute in this build we won't have the license file 
with this method, right? Is that going to be a problem?

> bitbake: toaster: update to Django 1.5.4

I think if we do do this we need to provide people with instructions on how to 
use virtualenv to install 1.5.4 on their system. I'm also a bit concerned 
about us requiring one specific minor release because a number of distros are 
using 1.5 versions other than 1.5.4 - there shouldn't be any incompatible 
changes introduced in minor releases, so we ought to be able to work with any 
1.5 release >= 1.5.4.

> bitbake: toaster: enable debug mode

You don't say *why* you're enabling this for all users - we don't normally 
turn on BBDEBUG by default. Why is this needed?

> bitbake: toaster: save extra data related to executed tasks
>
>+import pprint

This doesn't seem to be needed.

> + 'workdir' : d.getVar("WORKDIR", True),

We can't do this. Apart from a few bits of old code that really shouldn't, 
Bitbake knows nothing about WORKDIR, and that's intentional. If you absolutely 
need this, you'll need to collect it another way. I'm not even sure it makes 
sense when we're talking about a remote context, which we are going to be in 
soon; perhaps we should review how we handle this at the design level.

> + 'filename' : d.getVarFlag(t, "filename"),
> + 'lineno' : d.getVarFlag(t, "lineno"),

I've run this past Richard, and he thinks that we should try partially 
enabling the variable tracking just for functions - sorry for going back and 
forth on this. What we'd need to do is check the performance impact of doing 
so - would you be able to look into that?

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: review request

[Damian, Alexandru]

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

Hi,

Thanks for reviewing this. Comments below, also for Belen.

Alex

On Thu, Dec 5, 2013 at 3:27 PM, Paul Eggleton <paul.eggleton@linux.intel.com
> wrote:

> Hi Alex,
>
> On Tuesday 26 November 2013 18:28:21 Damian, Alexandru wrote:
> > I have a set of patches for review on:
> >
> >
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=toaster/master
>
> Sorry for the delay (although we've had some changes in the mean time with
> the
> pkgdata changes). Reviewing the latest branch, most of the changes look OK,
> however I did notice the following:
>
> > bitbake: toasterui: record recipe licensing information
>
> If do_populate_lic doesn't execute in this build we won't have the license
> file
> with this method, right? Is that going to be a problem?
>

Yep, license info in the required form is dependent on executing
do_populate_lic.
Belen - we need to either return to recipe-based info (relative paths) or
live with missing info for Prebuilt tasks.
Which way to go ?

> bitbake: toaster: update to Django 1.5.4
>
> I think if we do do this we need to provide people with instructions on
> how to
> use virtualenv to install 1.5.4 on their system. I'm also a bit concerned
> about us requiring one specific minor release because a number of distros
> are
> using 1.5 versions other than 1.5.4 - there shouldn't be any incompatible
> changes introduced in minor releases, so we ought to be able to work with
> any
> 1.5 release >= 1.5.4.
>

Installation instructions are included in the patch.  Since we don't have
the capability to test on multiple Django
versions, I think the requirement to use a specific version is acceptable.

Alternatively, we could just print an warning if a supposedly compatible
but not identical Django version is used,
instead of refusing to start. What do you think ?



> > bitbake: toaster: enable debug mode
>
> You don't say *why* you're enabling this for all users - we don't normally
> turn on BBDEBUG by default. Why is this needed?
>

Debug mode is enabled only if one starts toaster with "source toaster start
debug" command line, i.e. not for all
runs, just for debug runs. Since bitbake supports this, seems that toaster
support is needed.


> > bitbake: toaster: save extra data related to executed tasks
> >
> >+import pprint
>
> This doesn't seem to be needed.
>
> > + 'workdir' : d.getVar("WORKDIR", True),
>
> We can't do this. Apart from a few bits of old code that really shouldn't,
> Bitbake knows nothing about WORKDIR, and that's intentional. If you
> absolutely
> need this, you'll need to collect it another way. I'm not even sure it
> makes
> sense when we're talking about a remote context, which we are going to be
> in
> soon; perhaps we should review how we handle this at the design level.
>
> > + 'filename' : d.getVarFlag(t, "filename"),
> > + 'lineno' : d.getVarFlag(t, "lineno"),
>
> I've run this past Richard, and he thinks that we should try partially
> enabling the variable tracking just for functions - sorry for going back
> and
> forth on this. What we'd need to do is check the performance impact of
> doing
> so - would you be able to look into that?
>

Actually, now I think that using variable history is not a good idea - for
AST-parsed
functions, the variable history points to the ast.py file :(.

This patch needs more rework, as I discovered other corner cases, where the
function name is inherited at runtime, e.g. autotools_do_configure is
called instead of do_configure,
but I still think that varflags is the right way to track the function
definition.

What problems do you see with that ?


>
> Cheers,
> Paul
>
> --
>
> Paul Eggleton
> Intel Open Source Technology Centre
>



-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 5420 bytes --]

Re: review request

[Paul Eggleton]

On Thursday 05 December 2013 18:18:16 Damian, Alexandru wrote:
> On Thu, Dec 5, 2013 at 3:27 PM, Paul Eggleton <paul.eggleton@linux.intel.com
> > If do_populate_lic doesn't execute in this build we won't have the license
> > file with this method, right? Is that going to be a problem?
> 
> Yep, license info in the required form is dependent on executing
> do_populate_lic.
> Belen - we need to either return to recipe-based info (relative paths) or
> live with missing info for Prebuilt tasks.
> Which way to go ?

I'm not an expert in this area, but I'd assume the most important aspect of 
this is about what went into the image, so it might be similar to the rest of 
the individual package information.

> > bitbake: toaster: update to Django 1.5.4
> > 
> > I think if we do do this we need to provide people with instructions on
> > how to use virtualenv to install 1.5.4 on their system. I'm also a bit
> > concerned about us requiring one specific minor release because a number of
> > distros are using 1.5 versions other than 1.5.4 - there shouldn't be any
> > incompatible changes introduced in minor releases, so we ought to be able
> > to work with any 1.5 release >= 1.5.4.
> 
> Installation instructions are included in the patch.  Since we don't have
> the capability to test on multiple Django versions, I think the requirement
> to use a specific version is acceptable.

We handle differing installed versions in the rest of the build system; QA 
ensure that things keep on working. I don't see how this is different.

> Alternatively, we could just print an warning if a supposedly compatible
> but not identical Django version is used, instead of refusing to start. What
> do you think ?

I think the first question is where we expect this to be usable. I think the 
answer almost certainly is "almost every distro we currently support, where 
practical". From there, we'll know what we need to support in Toaster.

> > > bitbake: toaster: enable debug mode
> > 
> > You don't say *why* you're enabling this for all users - we don't normally
> > turn on BBDEBUG by default. Why is this needed?
> 
> Debug mode is enabled only if one starts toaster with "source toaster start
> debug" command line, i.e. not for all runs, just for debug runs. Since
> bitbake supports this, seems that toaster support is needed.

OK, understood.
 
> > > bitbake: toaster: save extra data related to executed tasks
> > >
> > >+import pprint
> > 
> > This doesn't seem to be needed.
> > 
> > > + 'workdir' : d.getVar("WORKDIR", True),
> > 
> > We can't do this. Apart from a few bits of old code that really shouldn't,
> > Bitbake knows nothing about WORKDIR, and that's intentional. If you
> > absolutely need this, you'll need to collect it another way. I'm not even
> > sure it makes sense when we're talking about a remote context, which we
> > are going to be in soon; perhaps we should review how we handle this at
> > the design level.
> > 
> > > + 'filename' : d.getVarFlag(t, "filename"),
> > > + 'lineno' : d.getVarFlag(t, "lineno"),
> > 
> > I've run this past Richard, and he thinks that we should try partially
> > enabling the variable tracking just for functions - sorry for going back
> > and forth on this. What we'd need to do is check the performance impact of
> > doing so - would you be able to look into that?
> 
> Actually, now I think that using variable history is not a good idea - for
> AST-parsed functions, the variable history points to the ast.py file :(.
> 
> This patch needs more rework, as I discovered other corner cases, where the
> function name is inherited at runtime, e.g. autotools_do_configure is
> called instead of do_configure, but I still think that varflags is the right
> way to track the function definition.
> 
> What problems do you see with that ?

It occurs to me that the trouble with our initial model of a single file / line 
is it isn't going to be able to effectively reflect how our task functions came 
to be - they're really just a special case of variables and can thus be 
prepended/appended in a number of different places to get the final function. 
This kind of suggests that the history is what we ought to be using.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: review request

[Barros Pena, Belen]

On 06/12/2013 11:44, "Paul Eggleton" <paul.eggleton@linux.intel.com> wrote:

>On Thursday 05 December 2013 18:18:16 Damian, Alexandru wrote:
>> On Thu, Dec 5, 2013 at 3:27 PM, Paul Eggleton
>><paul.eggleton@linux.intel.com
>> > If do_populate_lic doesn't execute in this build we won't have the
>>license
>> > file with this method, right? Is that going to be a problem?
>> 
>> Yep, license info in the required form is dependent on executing
>> do_populate_lic.
>> Belen - we need to either return to recipe-based info (relative paths)
>>or
>> live with missing info for Prebuilt tasks.
>> Which way to go ?
>
>I'm not an expert in this area, but I'd assume the most important aspect
>of 
>this is about what went into the image, so it might be similar to the
>rest of 
>the individual package information.

I spoke to Beth Flanagan about what kind of information is useful when it
comes to licensing, and Paul is right. It is useful to know which licenses
apply to a certain recipe, but when it comes to license files, you really
want to provide information at the package level. Sorry about this: I
should have spoken to Beth sooner.

So what we should be showing is the full path to the license files in
/build/tmp/deploy/licenses/


So, for busybox:

.../build/tmp/deploy/licenses/busybox/generic_bzip2.txt

.../build/tmp/deploy/licenses/busybox/generic_GPLv2.txt

.../build/tmp/deploy/licenses/busybox/LICENSE.txt


I can open enhancements to remove licensing_info from the recipes table
and add a license_files field to the package information. Let me know if
that would be ok. We should probably also provide the path to the license
manifest as part of the image information.


>
>> > bitbake: toaster: update to Django 1.5.4
>> > 
>> > I think if we do do this we need to provide people with instructions
>>on
>> > how to use virtualenv to install 1.5.4 on their system. I'm also a bit
>> > concerned about us requiring one specific minor release because a
>>number of
>> > distros are using 1.5 versions other than 1.5.4 - there shouldn't be
>>any
>> > incompatible changes introduced in minor releases, so we ought to be
>>able
>> > to work with any 1.5 release >= 1.5.4.
>> 
>> Installation instructions are included in the patch.  Since we don't
>>have
>> the capability to test on multiple Django versions, I think the
>>requirement
>> to use a specific version is acceptable.
>
>We handle differing installed versions in the rest of the build system;
>QA 
>ensure that things keep on working. I don't see how this is different.
>
>> Alternatively, we could just print an warning if a supposedly compatible
>> but not identical Django version is used, instead of refusing to start.
>>What
>> do you think ?
>
>I think the first question is where we expect this to be usable. I think
>the 
>answer almost certainly is "almost every distro we currently support,
>where 
>practical". From there, we'll know what we need to support in Toaster.
>
>> > > bitbake: toaster: enable debug mode
>> > 
>> > You don't say *why* you're enabling this for all users - we don't
>>normally
>> > turn on BBDEBUG by default. Why is this needed?
>> 
>> Debug mode is enabled only if one starts toaster with "source toaster
>>start
>> debug" command line, i.e. not for all runs, just for debug runs. Since
>> bitbake supports this, seems that toaster support is needed.
>
>OK, understood.
> 
>> > > bitbake: toaster: save extra data related to executed tasks
>> > >
>> > >+import pprint
>> > 
>> > This doesn't seem to be needed.
>> > 
>> > > + 'workdir' : d.getVar("WORKDIR", True),
>> > 
>> > We can't do this. Apart from a few bits of old code that really
>>shouldn't,
>> > Bitbake knows nothing about WORKDIR, and that's intentional. If you
>> > absolutely need this, you'll need to collect it another way. I'm not
>>even
>> > sure it makes sense when we're talking about a remote context, which
>>we
>> > are going to be in soon; perhaps we should review how we handle this
>>at
>> > the design level.
>> > 
>> > > + 'filename' : d.getVarFlag(t, "filename"),
>> > > + 'lineno' : d.getVarFlag(t, "lineno"),
>> > 
>> > I've run this past Richard, and he thinks that we should try partially
>> > enabling the variable tracking just for functions - sorry for going
>>back
>> > and forth on this. What we'd need to do is check the performance
>>impact of
>> > doing so - would you be able to look into that?
>> 
>> Actually, now I think that using variable history is not a good idea -
>>for
>> AST-parsed functions, the variable history points to the ast.py file :(.
>> 
>> This patch needs more rework, as I discovered other corner cases, where
>>the
>> function name is inherited at runtime, e.g. autotools_do_configure is
>> called instead of do_configure, but I still think that varflags is the
>>right
>> way to track the function definition.
>> 
>> What problems do you see with that ?
>
>It occurs to me that the trouble with our initial model of a single file
>/ line 
>is it isn't going to be able to effectively reflect how our task
>functions came 
>to be - they're really just a special case of variables and can thus be
>prepended/appended in a number of different places to get the final
>function. 
>This kind of suggests that the history is what we ought to be using.
>
>Cheers,
>Paul
>
>-- 
>
>Paul Eggleton
>Intel Open Source Technology Centre

Re: review request

[Damian, Alexandru]

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

Ok, can you please open a bug for the changes to licensing info ?

I'm working on the other items under review.

Alex


On Fri, Dec 6, 2013 at 1:45 PM, Barros Pena, Belen <
belen.barros.pena@intel.com> wrote:

>
>
> On 06/12/2013 11:44, "Paul Eggleton" <paul.eggleton@linux.intel.com>
> wrote:
>
> >On Thursday 05 December 2013 18:18:16 Damian, Alexandru wrote:
> >> On Thu, Dec 5, 2013 at 3:27 PM, Paul Eggleton
> >><paul.eggleton@linux.intel.com
> >> > If do_populate_lic doesn't execute in this build we won't have the
> >>license
> >> > file with this method, right? Is that going to be a problem?
> >>
> >> Yep, license info in the required form is dependent on executing
> >> do_populate_lic.
> >> Belen - we need to either return to recipe-based info (relative paths)
> >>or
> >> live with missing info for Prebuilt tasks.
> >> Which way to go ?
> >
> >I'm not an expert in this area, but I'd assume the most important aspect
> >of
> >this is about what went into the image, so it might be similar to the
> >rest of
> >the individual package information.
>
> I spoke to Beth Flanagan about what kind of information is useful when it
> comes to licensing, and Paul is right. It is useful to know which licenses
> apply to a certain recipe, but when it comes to license files, you really
> want to provide information at the package level. Sorry about this: I
> should have spoken to Beth sooner.
>
> So what we should be showing is the full path to the license files in
> /build/tmp/deploy/licenses/
>
>
> So, for busybox:
>
> .../build/tmp/deploy/licenses/busybox/generic_bzip2.txt
>
> .../build/tmp/deploy/licenses/busybox/generic_GPLv2.txt
>
> .../build/tmp/deploy/licenses/busybox/LICENSE.txt
>
>
> I can open enhancements to remove licensing_info from the recipes table
> and add a license_files field to the package information. Let me know if
> that would be ok. We should probably also provide the path to the license
> manifest as part of the image information.
>
>
> >
> >> > bitbake: toaster: update to Django 1.5.4
> >> >
> >> > I think if we do do this we need to provide people with instructions
> >>on
> >> > how to use virtualenv to install 1.5.4 on their system. I'm also a bit
> >> > concerned about us requiring one specific minor release because a
> >>number of
> >> > distros are using 1.5 versions other than 1.5.4 - there shouldn't be
> >>any
> >> > incompatible changes introduced in minor releases, so we ought to be
> >>able
> >> > to work with any 1.5 release >= 1.5.4.
> >>
> >> Installation instructions are included in the patch.  Since we don't
> >>have
> >> the capability to test on multiple Django versions, I think the
> >>requirement
> >> to use a specific version is acceptable.
> >
> >We handle differing installed versions in the rest of the build system;
> >QA
> >ensure that things keep on working. I don't see how this is different.
> >
> >> Alternatively, we could just print an warning if a supposedly compatible
> >> but not identical Django version is used, instead of refusing to start.
> >>What
> >> do you think ?
> >
> >I think the first question is where we expect this to be usable. I think
> >the
> >answer almost certainly is "almost every distro we currently support,
> >where
> >practical". From there, we'll know what we need to support in Toaster.
> >
> >> > > bitbake: toaster: enable debug mode
> >> >
> >> > You don't say *why* you're enabling this for all users - we don't
> >>normally
> >> > turn on BBDEBUG by default. Why is this needed?
> >>
> >> Debug mode is enabled only if one starts toaster with "source toaster
> >>start
> >> debug" command line, i.e. not for all runs, just for debug runs. Since
> >> bitbake supports this, seems that toaster support is needed.
> >
> >OK, understood.
> >
> >> > > bitbake: toaster: save extra data related to executed tasks
> >> > >
> >> > >+import pprint
> >> >
> >> > This doesn't seem to be needed.
> >> >
> >> > > + 'workdir' : d.getVar("WORKDIR", True),
> >> >
> >> > We can't do this. Apart from a few bits of old code that really
> >>shouldn't,
> >> > Bitbake knows nothing about WORKDIR, and that's intentional. If you
> >> > absolutely need this, you'll need to collect it another way. I'm not
> >>even
> >> > sure it makes sense when we're talking about a remote context, which
> >>we
> >> > are going to be in soon; perhaps we should review how we handle this
> >>at
> >> > the design level.
> >> >
> >> > > + 'filename' : d.getVarFlag(t, "filename"),
> >> > > + 'lineno' : d.getVarFlag(t, "lineno"),
> >> >
> >> > I've run this past Richard, and he thinks that we should try partially
> >> > enabling the variable tracking just for functions - sorry for going
> >>back
> >> > and forth on this. What we'd need to do is check the performance
> >>impact of
> >> > doing so - would you be able to look into that?
> >>
> >> Actually, now I think that using variable history is not a good idea -
> >>for
> >> AST-parsed functions, the variable history points to the ast.py file :(.
> >>
> >> This patch needs more rework, as I discovered other corner cases, where
> >>the
> >> function name is inherited at runtime, e.g. autotools_do_configure is
> >> called instead of do_configure, but I still think that varflags is the
> >>right
> >> way to track the function definition.
> >>
> >> What problems do you see with that ?
> >
> >It occurs to me that the trouble with our initial model of a single file
> >/ line
> >is it isn't going to be able to effectively reflect how our task
> >functions came
> >to be - they're really just a special case of variables and can thus be
> >prepended/appended in a number of different places to get the final
> >function.
> >This kind of suggests that the history is what we ought to be using.
> >
> >Cheers,
> >Paul
> >
> >--
> >
> >Paul Eggleton
> >Intel Open Source Technology Centre
>
>


-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 7911 bytes --]

Re: review request

[Barros Pena, Belen]

Done

On 09/12/2013 10:40, "Damian, Alexandru" <alexandru.damian@intel.com>
wrote:

>I can open enhancements to remove licensing_info from the recipes table

https://bugzilla.yoctoproject.org/show_bug.cgi?id=5647

>and add a license_files field to the package information.

https://bugzilla.yoctoproject.org/show_bug.cgi?id=5648

>Let me know if
>that would be ok. We should probably also provide the path to the license
>manifest as part of the image information.

https://bugzilla.yoctoproject.org/show_bug.cgi?id=5649

>

Re: review request

[Barros Pena, Belen]

On 05/12/2013 15:27, "Paul Eggleton" <paul.eggleton@linux.intel.com> wrote:

>Hi Alex,
>
>On Tuesday 26 November 2013 18:28:21 Damian, Alexandru wrote:
>> I have a set of patches for review on:
>> 
>> 
>>http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=toaster/mas
>>ter
>
>Sorry for the delay (although we've had some changes in the mean time
>with the 
>pkgdata changes). Reviewing the latest branch, most of the changes look
>OK, 
>however I did notice the following:
>
>> bitbake: toasterui: record recipe licensing information
>
>If do_populate_lic doesn't execute in this build we won't have the
>license file 
>with this method, right? Is that going to be a problem?

That would explain this bug:
https://bugzilla.yoctoproject.org/show_bug.cgi?id=5629

I am inclined to think it is a problem.

>
>> bitbake: toaster: update to Django 1.5.4
>
>I think if we do do this we need to provide people with instructions on
>how to 
>use virtualenv to install 1.5.4 on their system. I'm also a bit concerned
>about us requiring one specific minor release because a number of distros
>are 
>using 1.5 versions other than 1.5.4 - there shouldn't be any incompatible
>changes introduced in minor releases, so we ought to be able to work with
>any 
>1.5 release >= 1.5.4.
>
>> bitbake: toaster: enable debug mode
>
>You don't say *why* you're enabling this for all users - we don't
>normally 
>turn on BBDEBUG by default. Why is this needed?
>
>> bitbake: toaster: save extra data related to executed tasks
>>
>>+import pprint
>
>This doesn't seem to be needed.
>
>> + 'workdir' : d.getVar("WORKDIR", True),
>
>We can't do this. Apart from a few bits of old code that really
>shouldn't, 
>Bitbake knows nothing about WORKDIR, and that's intentional. If you
>absolutely 
>need this, you'll need to collect it another way. I'm not even sure it
>makes 
>sense when we're talking about a remote context, which we are going to be
>in 
>soon; perhaps we should review how we handle this at the design level.

Well, I guess we either show it or not. The reason why I thought this
piece of information would be useful is that the work directory contains a
whole bunch of files people might want to look into (package source, etc).
It is handy to be told where they are instead of having to look for them.
It also opens the possibility of remote access to those files, if we ever
want to provide that.

But I could be wrong, of course.

>
>> + 'filename' : d.getVarFlag(t, "filename"),
>> + 'lineno' : d.getVarFlag(t, "lineno"),
>
>I've run this past Richard, and he thinks that we should try partially
>enabling the variable tracking just for functions - sorry for going back
>and 
>forth on this. What we'd need to do is check the performance impact of
>doing 
>so - would you be able to look into that?
>
>Cheers,
>Paul
>
>-- 
>
>Paul Eggleton
>Intel Open Source Technology Centre
>_______________________________________________
>toaster mailing list
>toaster@yoctoproject.org
>https://lists.yoctoproject.org/listinfo/toaster

Re: review request

[Damian, Alexandru]

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

Hi Paul,

I've pushed a new toaster-master with either the review changes applied, or
the changes entirely taken out if they were too troublesome (workdir,
licensing_info changes).

Can you please check it and let me know if ok ?

Cheers,
Alex


On Thu, Dec 5, 2013 at 3:27 PM, Paul Eggleton <paul.eggleton@linux.intel.com
> wrote:

> Hi Alex,
>
> On Tuesday 26 November 2013 18:28:21 Damian, Alexandru wrote:
> > I have a set of patches for review on:
> >
> >
> http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/log/?h=toaster/master
>
> Sorry for the delay (although we've had some changes in the mean time with
> the
> pkgdata changes). Reviewing the latest branch, most of the changes look OK,
> however I did notice the following:
>
> > bitbake: toasterui: record recipe licensing information
>
> If do_populate_lic doesn't execute in this build we won't have the license
> file
> with this method, right? Is that going to be a problem?
>
> > bitbake: toaster: update to Django 1.5.4
>
> I think if we do do this we need to provide people with instructions on
> how to
> use virtualenv to install 1.5.4 on their system. I'm also a bit concerned
> about us requiring one specific minor release because a number of distros
> are
> using 1.5 versions other than 1.5.4 - there shouldn't be any incompatible
> changes introduced in minor releases, so we ought to be able to work with
> any
> 1.5 release >= 1.5.4.
>
> > bitbake: toaster: enable debug mode
>
> You don't say *why* you're enabling this for all users - we don't normally
> turn on BBDEBUG by default. Why is this needed?
>
> > bitbake: toaster: save extra data related to executed tasks
> >
> >+import pprint
>
> This doesn't seem to be needed.
>
> > + 'workdir' : d.getVar("WORKDIR", True),
>
> We can't do this. Apart from a few bits of old code that really shouldn't,
> Bitbake knows nothing about WORKDIR, and that's intentional. If you
> absolutely
> need this, you'll need to collect it another way. I'm not even sure it
> makes
> sense when we're talking about a remote context, which we are going to be
> in
> soon; perhaps we should review how we handle this at the design level.
>
> > + 'filename' : d.getVarFlag(t, "filename"),
> > + 'lineno' : d.getVarFlag(t, "lineno"),
>
> I've run this past Richard, and he thinks that we should try partially
> enabling the variable tracking just for functions - sorry for going back
> and
> forth on this. What we'd need to do is check the performance impact of
> doing
> so - would you be able to look into that?
>
> Cheers,
> Paul
>
> --
>
> Paul Eggleton
> Intel Open Source Technology Centre
>



-- 
Alex Damian
Yocto Project
SSG / OTC

[-- Attachment #2: Type: text/html, Size: 3588 bytes --]

Review Request

[Divya Indi]

[PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Hi,

Please review the patch that follows. 

v3 addresses the previously raised concerns. Changes include -

1. To resolve the race where the timer can kick in before request
has been sent out, we now add the request to the list after sending out
the request.

2. To handle the race where the response can come in before we got a 
chance to add the req to the list, sending and adding the request to
request list is done under spinlock - request_lock.

3. To make sure there is no blocking op/delay while holding the spinlock,
using GFP_NOWAIT for memory allocation.

Thanks Jason for providing your valuable feedback. 

Let me know if you have any suggestions or concerns.

Thanks,
Divya

Re: Review Request

[Leon Romanovsky]

On Mon, Jun 08, 2020 at 07:46:15AM -0700, Divya Indi wrote:
> [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg
>
> Hi,
>
> Please review the patch that follows.

Please read Documentation/process/submitting-patches.rst
14) The canonical patch format

You don't need an extra email "Review request" and Changelog should be
put inside the patch itself under "---" marker.

Thanks

Re: Review Request

[Divya Indi]

Thanks Leon, Noted!

On 6/9/20 12:03 AM, Leon Romanovsky wrote:
> On Mon, Jun 08, 2020 at 07:46:15AM -0700, Divya Indi wrote:
>> [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg
>>
>> Hi,
>>
>> Please review the patch that follows.
> Please read Documentation/process/submitting-patches.rst
> 14) The canonical patch format
>
> You don't need an extra email "Review request" and Changelog should be
> put inside the patch itself under "---" marker.
>
> Thanks

Review Request

[Divya Indi]

[PATCH v2] IB/sa: Resolving use-after-free in ib_nl_send_msg.

Hi,

This is the v2 of the patch that addresses the comments received for v1 -

- Using atomic bit ops for setting and testing IB_SA_NL_QUERY_SENT.
- Rewording and adding comments.


Thanks,
Divya

Review Request

[Adam C. Emerson]

Creators of the Storage Squid,

If you're interested in less use of the allocator, you are interested in
Context* elimination. If so, please review the top two commits on the
wip-decontextualization branch of:

https://github.com/cohortfsllc/cohortfs-ceph

They provide an enhanced version of std::function that allows you to
specify how much space gets preallocated for function objects or disable
allocation entirely.

Also, I have two pull requests, wip-cxx11time and wip-cxx11concurrency
that pass check on which I would appreciate any feedback.

Thank you, Comrades!

Re: Review Request

[Gregory Farnum]

On Wed, Sep 16, 2015 at 11:09 AM, Adam C. Emerson <aemerson@redhat.com> wrote:
> Creators of the Storage Squid,
>
> If you're interested in less use of the allocator, you are interested in
> Context* elimination. If so, please review the top two commits on the
> wip-decontextualization branch of:
>
> https://github.com/cohortfsllc/cohortfs-ceph
>
> They provide an enhanced version of std::function that allows you to
> specify how much space gets preallocated for function objects or disable
> allocation entirely.

Can you provide a little background (links or text) for those of us
who aren't up on C++11x/14x? I looked at them briefly but having only
the vaguest idea about some of them quickly got lost. :)
-Greg

>
> Also, I have two pull requests, wip-cxx11time and wip-cxx11concurrency
> that pass check on which I would appreciate any feedback.
>
> Thank you, Comrades!
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Review Request

[Adam C. Emerson]

On 09/16/2015 02:31 PM, Gregory Farnum wrote:
> Can you provide a little background (links or text) for those of us
> who aren't up on C++11x/14x? I looked at them briefly but having only
> the vaguest idea about some of them quickly got lost. :)

Surely, sir.

So, in summary, Context* is the means we have been using to handle
callbacks. It has two big problems:

(1) Every Context is allocated at the point of use and freed when it's
called. So by their nature they make heavy use of the heap. (If you
overload complete there's a few exceptions, but by and large Context is
very heapy.)

(2) Context accepts an int. That's it. You can get around this by
storing other things in it and passing pointers to it and so forth. But
it would be nice to have more variety of type in our callbacks.

C++11 (and following) have a whole pile of things that can be used as
functions. There are function objects (objects with an operator()
defined on them), lambdas with variable capture, function member
pointers, reference wrappers pointing to function objects and the like.
This gives you a good bit of flexibility, allows things like variable
capture, let's you allocate one object once and just pass references to
it, and so forth.

Now, these objects all have different sizes and different types. So you
can't just shove them in an object naively. Because a class one of whose
members is a function pointer is going to be a different type than a
class holding a function object. (Which itself is different to a type
holding a reference to a function object.)

std::function exists as a solution to this problem. It provides a
uniform type that can hold any object satisfying the requirement of
being callable with given argument types and a given return value. So,
for example, if you have some 'stat'-like call, you could specify it as
taking a function taking an error code, a size, and a date. Anything
that can be called with such would be accepted, anything that can't
(wrong argument types, say) would be rejected at compile time. And it
could be uniformly stored in a list of Operations.

The downside is that if the thing you provide is too big, std::function
will allocate. 'too big' depends on your library vendor and there's no
way to find it out what. Thus the purpose of the ceph::function class.
If we have a pretty good idea how large most of the callback function
objects we expect to hold are, we can tell it to statically allocate
that much space. This gives us a tool to get allocations out of our fast
path. (For example, if we preallocate a bunch of classes with a
ceph::function that preallocates enough space to hold likely callbacks,
we can just pick them off and have no allocations, in the usual case.)

If we know /everything/ we'll ever get, we can disable allocations
entirely. This is mostly so we can catch situations where we're trying
to shove something unexpectedly huge somewhere it ought not go. An
internal sanity check.

Now, ceph::function, like std::function, is still an abstraction with a
virtual call in it and because it copies things around it reduces
opportunities for inlining. Thus, if you aren't storing a callback on an
object that's supposed to be in a queue, you shouldn't use it. You
should do something like:

template<typename Fun>
void do_stuff(Fun&& f) {
  ...;
  f(some, values);
};

This allows inlining, and (based on my experiments trying multiple
implementations and looking at the generated assembly) if f is called in
a loop, the code is just as good as if you'd open-coded the loop and
written the body of 'f' in it. Functions like this can have the
potential to use closures (lambda expression) effectively for free.

So, the summary is:

Context* is very heapy. We have less heapy alternatives. This implements
a foundation for one of them. We also get a bit more flexibility.

Thank you.

Re: Review Request

[John Spray]

This is neat, I had been hankering to lambda-ize various things, but
hadn't worked through what the allocation would looked like in
practice.

Do you know if there's a reason the standard is defined so as to not
let us override the reserved size of a std::function?  Are we taking
any kind of hit by doing this?

John


P.S. I have just gone on Amazon to order a C++11 book because I need
to start using more than just lambda, auto, and pretty for loops ;-)

On Wed, Sep 16, 2015 at 8:06 PM, Adam C. Emerson <aemerson@redhat.com> wrote:
> On 09/16/2015 02:31 PM, Gregory Farnum wrote:
>> Can you provide a little background (links or text) for those of us
>> who aren't up on C++11x/14x? I looked at them briefly but having only
>> the vaguest idea about some of them quickly got lost. :)
>
> Surely, sir.
>
> So, in summary, Context* is the means we have been using to handle
> callbacks. It has two big problems:
>
> (1) Every Context is allocated at the point of use and freed when it's
> called. So by their nature they make heavy use of the heap. (If you
> overload complete there's a few exceptions, but by and large Context is
> very heapy.)
>
> (2) Context accepts an int. That's it. You can get around this by
> storing other things in it and passing pointers to it and so forth. But
> it would be nice to have more variety of type in our callbacks.
>
> C++11 (and following) have a whole pile of things that can be used as
> functions. There are function objects (objects with an operator()
> defined on them), lambdas with variable capture, function member
> pointers, reference wrappers pointing to function objects and the like.
> This gives you a good bit of flexibility, allows things like variable
> capture, let's you allocate one object once and just pass references to
> it, and so forth.
>
> Now, these objects all have different sizes and different types. So you
> can't just shove them in an object naively. Because a class one of whose
> members is a function pointer is going to be a different type than a
> class holding a function object. (Which itself is different to a type
> holding a reference to a function object.)
>
> std::function exists as a solution to this problem. It provides a
> uniform type that can hold any object satisfying the requirement of
> being callable with given argument types and a given return value. So,
> for example, if you have some 'stat'-like call, you could specify it as
> taking a function taking an error code, a size, and a date. Anything
> that can be called with such would be accepted, anything that can't
> (wrong argument types, say) would be rejected at compile time. And it
> could be uniformly stored in a list of Operations.
>
> The downside is that if the thing you provide is too big, std::function
> will allocate. 'too big' depends on your library vendor and there's no
> way to find it out what. Thus the purpose of the ceph::function class.
> If we have a pretty good idea how large most of the callback function
> objects we expect to hold are, we can tell it to statically allocate
> that much space. This gives us a tool to get allocations out of our fast
> path. (For example, if we preallocate a bunch of classes with a
> ceph::function that preallocates enough space to hold likely callbacks,
> we can just pick them off and have no allocations, in the usual case.)
>
> If we know /everything/ we'll ever get, we can disable allocations
> entirely. This is mostly so we can catch situations where we're trying
> to shove something unexpectedly huge somewhere it ought not go. An
> internal sanity check.
>
> Now, ceph::function, like std::function, is still an abstraction with a
> virtual call in it and because it copies things around it reduces
> opportunities for inlining. Thus, if you aren't storing a callback on an
> object that's supposed to be in a queue, you shouldn't use it. You
> should do something like:
>
> template<typename Fun>
> void do_stuff(Fun&& f) {
>   ...;
>   f(some, values);
> };
>
> This allows inlining, and (based on my experiments trying multiple
> implementations and looking at the generated assembly) if f is called in
> a loop, the code is just as good as if you'd open-coded the loop and
> written the body of 'f' in it. Functions like this can have the
> potential to use closures (lambda expression) effectively for free.
>
> So, the summary is:
>
> Context* is very heapy. We have less heapy alternatives. This implements
> a foundation for one of them. We also get a bit more flexibility.
>
> Thank you.
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Review Request

[Adam C. Emerson]

On 09/16/2015 06:15 PM, John Spray wrote:
> This is neat, I had been hankering to lambda-ize various things, but
> hadn't worked through what the allocation would looked like in
> practice.
> 
> Do you know if there's a reason the standard is defined so as to not
> let us override the reserved size of a std::function?  Are we taking
> any kind of hit by doing this?

There are tradeoffs, but I do not believe they burden us.

Small Function Optimization is not actually required by the standard.
It's just that all implementations of std::function I'm familiar with
implement it to at least some degree. In the GNU implementation, the
structure always has space for a pointer to point to heap allocated
stuff. If your 'function' is just a pointer (or something as big as a
pointer) it stores that. Since there aren't really any tradeoffs with
providing at least /some/ form of Small Function Optimization, there's
no reason for an implementation not to write it in. (This is in contrast
to Short String Operation, where it can clash with Copy-on-Write.)

So, the main reason the Standard doesn't allow one to specify the amount
of space reserved for Small Functions is that it doesn't talk about it.

There is a trade-off, however.

  ceph::function<void(int), 4 * sizeof(void*)>

Is a different type than

  ceph::function<void(int), 3 * sizeof(void*)>

This is good, in that it means that in our heap_allocations::forbid
case, we can reject, at compile time anything that the type system can't
guarantee will fit in our reserved space.

The cost is that they differ by type. A reference to one type won't
reference an object of the other type. This means that you'll need to
use a template and universal reference, like:

  template<typename F>
  void do_stuff(F&& f) {
    auto o = new Thing(std::forward<F>(f));
  }

And if you want to overload do_stuff, you'll need to use enable_if to
disambiguate the function from the other stuff. One day we shall have
Concepts. And if they don't give us too broken down and wimpified a
version it should make these problems go away. But for now we need
enable_if. There's a good chance that passing functions by universal
reference and forward right until the point they get stored somewhere is
going to be slightly more efficient at runtime than having the function
take a std::function&/ceph::function& parameter, depending on what the
compiler does (I haven't looked at the assembler for this case but I
suspect it would call the std::function constructor at the point of
function call and the move constructor at the point of storing it into
the data structure), so you might want to do this anyway.

It also means that you can't store pointer/reference to functions with a
different reserved size in the same container. In practice this doesn't
really come up, since if I'm a subsystem with a bunch of OnComplete
functions that I've stored and I want to gather up a list of references
to them I already control what I've stored. Or if you really need to you
can combine a ceph::function with a reserved size of 1 * sizeof(void*)
with std::ref to get a 'universal function reference' at the cost of a
double indirection and two virtual function calls where one would
normally be required.

There is also a potential runtime cost in the move constructor. If our
ceph::function heap allocates its storage and we then use swap, move
assignment, or move construction, we just end up copying a pointer at
each move. If everything fits in reserved space, all these pointer
copies become fairly substantial memcpies. Since Ceph normally stores a
callback function in the structure describing an operation, leaves it
there, and then calls it, this shouldn't hurt us too much. (And is one
of the reasons I recommend using a universal reference and std::forward
above rather than accepting a ceph::function reference and then
std::moving it into its final home.) For cases where we do want to
'move' them around a lot, we can use std::ref and other tricks to turn
the memcpies back into pointer copies.

Thank you!

review request

[Elso Andras]

Hi,

Before i send the pull request, please review this commits:
https://github.com/Elbandi/ceph/compare/master...wip-getlayout

Elbandi

Re: review request

[Sage Weil]

On Fri, 3 May 2013, Elso Andras wrote:
> Hi,
> 
> Before i send the pull request, please review this commits:
> https://github.com/Elbandi/ceph/compare/master...wip-getlayout

Nice!  A pull request is actually the easiest way to review as it will 
capture any comments we make on github for future reference.  Each 
subsequent revision can just be repushed to the same branch and github 
will (mostly) do the right thing.

Thanks!
sage