Patchwork [1/1] V2 Fix libpam's chmod

login
register
mail settings
Submitter Robert Yang
Date March 2, 2012, 10:34 a.m.
Message ID <81a428ea0e7515ab09e707fbdaba9b1a1b5050b6.1330684260.git.liezhi.yang@windriver.com>
Download mbox | patch
Permalink /patch/22579/
State Accepted
Commit 0725c7f01b173b1cc2090f4a03a274c7017b8a1a
Headers show

Comments

Robert Yang - March 2, 2012, 10:34 a.m.
The libpam's has an error when generating the rootfs:

chmod: cannot access `/usr/sbin/unix_chkpwd': No such file or directory

This is because the following code in libpam_1.1.5.bb:

pkg_postinst_pam-plugin-unix () {
    # below is necessary to allow unix_chkpwd get user info from shadow file
    # on lsb images
    chmod 4755 ${sbindir}/unix_chkpwd
}

This is to set the setuid permission for unix_chkpwd (the lsb test
requires this), but it lacks a "${D}", and we can do this in the install
stage.

[YOCTO #2049]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 meta/recipes-extended/pam/libpam_1.1.5.bb |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
Richard Purdie - March 2, 2012, 12:25 p.m.
On Fri, 2012-03-02 at 18:34 +0800, Robert Yang wrote:
> The libpam's has an error when generating the rootfs:
> 
> chmod: cannot access `/usr/sbin/unix_chkpwd': No such file or directory
> 
> This is because the following code in libpam_1.1.5.bb:
> 
> pkg_postinst_pam-plugin-unix () {
>     # below is necessary to allow unix_chkpwd get user info from shadow file
>     # on lsb images
>     chmod 4755 ${sbindir}/unix_chkpwd
> }
> 
> This is to set the setuid permission for unix_chkpwd (the lsb test
> requires this), but it lacks a "${D}", and we can do this in the install
> stage.
> 
> [YOCTO #2049]
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> ---
>  meta/recipes-extended/pam/libpam_1.1.5.bb |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)

Merged to master, thanks.

Richard
Richard Purdie - March 2, 2012, 1:26 p.m.
On Fri, 2012-03-02 at 18:34 +0800, Robert Yang wrote:
> The libpam's has an error when generating the rootfs:
> 
> chmod: cannot access `/usr/sbin/unix_chkpwd': No such file or directory
> 
> This is because the following code in libpam_1.1.5.bb:
> 
> pkg_postinst_pam-plugin-unix () {
>     # below is necessary to allow unix_chkpwd get user info from shadow file
>     # on lsb images
>     chmod 4755 ${sbindir}/unix_chkpwd
> }
> 
> This is to set the setuid permission for unix_chkpwd (the lsb test
> requires this), but it lacks a "${D}", and we can do this in the install
> stage.
> 
> [YOCTO #2049]
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>

Robert, in future patches could you use a subject line like:

[PATCH 1/1 v2] Fix libpam's chmod

instead of:

[PATCH 1/1] V2 Fix libpam's chmod

so that when the patches get applied, the V2 is stripped off?

The reason is that the value isn't particularly within the repository
once merged and doesn't look good when looking through commit history.

Thanks!

Richard
Koen Kooi - March 2, 2012, 1:31 p.m.
Op 2 mrt. 2012, om 14:26 heeft Richard Purdie het volgende geschreven:

> On Fri, 2012-03-02 at 18:34 +0800, Robert Yang wrote:
>> The libpam's has an error when generating the rootfs:
>> 
>> chmod: cannot access `/usr/sbin/unix_chkpwd': No such file or directory
>> 
>> This is because the following code in libpam_1.1.5.bb:
>> 
>> pkg_postinst_pam-plugin-unix () {
>>    # below is necessary to allow unix_chkpwd get user info from shadow file
>>    # on lsb images
>>    chmod 4755 ${sbindir}/unix_chkpwd
>> }
>> 
>> This is to set the setuid permission for unix_chkpwd (the lsb test
>> requires this), but it lacks a "${D}", and we can do this in the install
>> stage.
>> 
>> [YOCTO #2049]
>> 
>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> 
> Robert, in future patches could you use a subject line like:
> 
> [PATCH 1/1 v2] Fix libpam's chmod
> 
> instead of:
> 
> [PATCH 1/1] V2 Fix libpam's chmod
> 
> so that when the patches get applied, the V2 is stripped off?
> 
> The reason is that the value isn't particularly within the repository
> once merged and doesn't look good when looking through commit history.

Actually both versions above are wrong, the right version would be similar to:

libpam 1.1.5: fix chmod in postinst

We can debate the usefulness of the version specifier, but we do need to follow the commit guide for the rest.

regards,

Koen
Richard Purdie - March 2, 2012, 1:39 p.m.
On Fri, 2012-03-02 at 14:31 +0100, Koen Kooi wrote:
> Op 2 mrt. 2012, om 14:26 heeft Richard Purdie het volgende geschreven:
> 
> > On Fri, 2012-03-02 at 18:34 +0800, Robert Yang wrote:
> >> The libpam's has an error when generating the rootfs:
> >> 
> >> chmod: cannot access `/usr/sbin/unix_chkpwd': No such file or directory
> >> 
> >> This is because the following code in libpam_1.1.5.bb:
> >> 
> >> pkg_postinst_pam-plugin-unix () {
> >>    # below is necessary to allow unix_chkpwd get user info from shadow file
> >>    # on lsb images
> >>    chmod 4755 ${sbindir}/unix_chkpwd
> >> }
> >> 
> >> This is to set the setuid permission for unix_chkpwd (the lsb test
> >> requires this), but it lacks a "${D}", and we can do this in the install
> >> stage.
> >> 
> >> [YOCTO #2049]
> >> 
> >> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> > 
> > Robert, in future patches could you use a subject line like:
> > 
> > [PATCH 1/1 v2] Fix libpam's chmod
> > 
> > instead of:
> > 
> > [PATCH 1/1] V2 Fix libpam's chmod
> > 
> > so that when the patches get applied, the V2 is stripped off?
> > 
> > The reason is that the value isn't particularly within the repository
> > once merged and doesn't look good when looking through commit history.
> 
> Actually both versions above are wrong, the right version would be similar to:
> 
> libpam 1.1.5: fix chmod in postinst
> 
> We can debate the usefulness of the version specifier, but we do need to follow the commit guide for the rest.

Good point, I was just commenting that the patch version specifier
didn't look good. As you say, the package name should be first too, as
per http://wiki.openembedded.org/wiki/Commit_Patch_Message_Guidelines
under Common Errors in Patch and Commit Messages: '- Short log does not
start with the file or component being modified. Such as "foo: Update to
new upstream version 5.8.9"'.

If I was going to be picky, I'd suggest capitalisation of "fix" in your
version ;-)

Cheers,

Richard
Koen Kooi - March 2, 2012, 2:05 p.m.
Op 2 mrt. 2012, om 14:39 heeft Richard Purdie het volgende geschreven:

> On Fri, 2012-03-02 at 14:31 +0100, Koen Kooi wrote:
>> Op 2 mrt. 2012, om 14:26 heeft Richard Purdie het volgende geschreven:
>> 
>>> On Fri, 2012-03-02 at 18:34 +0800, Robert Yang wrote:
>>>> The libpam's has an error when generating the rootfs:
>>>> 
>>>> chmod: cannot access `/usr/sbin/unix_chkpwd': No such file or directory
>>>> 
>>>> This is because the following code in libpam_1.1.5.bb:
>>>> 
>>>> pkg_postinst_pam-plugin-unix () {
>>>>   # below is necessary to allow unix_chkpwd get user info from shadow file
>>>>   # on lsb images
>>>>   chmod 4755 ${sbindir}/unix_chkpwd
>>>> }
>>>> 
>>>> This is to set the setuid permission for unix_chkpwd (the lsb test
>>>> requires this), but it lacks a "${D}", and we can do this in the install
>>>> stage.
>>>> 
>>>> [YOCTO #2049]
>>>> 
>>>> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
>>> 
>>> Robert, in future patches could you use a subject line like:
>>> 
>>> [PATCH 1/1 v2] Fix libpam's chmod
>>> 
>>> instead of:
>>> 
>>> [PATCH 1/1] V2 Fix libpam's chmod
>>> 
>>> so that when the patches get applied, the V2 is stripped off?
>>> 
>>> The reason is that the value isn't particularly within the repository
>>> once merged and doesn't look good when looking through commit history.
>> 
>> Actually both versions above are wrong, the right version would be similar to:
>> 
>> libpam 1.1.5: fix chmod in postinst
>> 
>> We can debate the usefulness of the version specifier, but we do need to follow the commit guide for the rest.
> 
> Good point, I was just commenting that the patch version specifier
> didn't look good. As you say, the package name should be first too, as
> per http://wiki.openembedded.org/wiki/Commit_Patch_Message_Guidelines
> under Common Errors in Patch and Commit Messages: '- Short log does not
> start with the file or component being modified. Such as "foo: Update to
> new upstream version 5.8.9"'.
> 
> If I was going to be picky, I'd suggest capitalisation of "fix" in your
> version ;-)

I'm a bit conflicted on that one. If I'm going to treat it as a sentence by using caps, I'd also like to use proper punctuation, which the guidelines don't allow. And that's why I said 'similar' :)

regards,

Koen

Patch

diff --git a/meta/recipes-extended/pam/libpam_1.1.5.bb b/meta/recipes-extended/pam/libpam_1.1.5.bb
index 283f477..8dd5ac5 100644
--- a/meta/recipes-extended/pam/libpam_1.1.5.bb
+++ b/meta/recipes-extended/pam/libpam_1.1.5.bb
@@ -9,7 +9,7 @@  SECTION = "base"
 LICENSE = "GPLv2+ | BSD"
 LIC_FILES_CHKSUM = "file://COPYING;md5=ca0395de9a86191a078b8b79302e3083"
 
-PR = "r2"
+PR = "r3"
 
 SRC_URI = "https://fedorahosted.org/releases/l/i/linux-pam/Linux-PAM-${PV}.tar.bz2 \
            file://99_pam \
@@ -85,10 +85,7 @@  do_install() {
 
 	install -d ${D}${sysconfdir}/pam.d/     
 	install -m 0644 ${WORKDIR}/pam.d/* ${D}${sysconfdir}/pam.d/
-}
 
-pkg_postinst_pam-plugin-unix () {
-    # below is necessary to allow unix_chkpwd get user info from shadow file
-    # on lsb images
-    chmod 4755 ${sbindir}/unix_chkpwd
+    # The lsb requires unix_chkpwd has setuid permission
+    chmod 4755 ${D}${sbindir}/unix_chkpwd
 }