New: xattr and check-package issue

Buildroot bugzilla at busybox.net 3 participants 7 messages Jul 25, 2018 – Aug 16, 2018

Share & tools

bugzilla at busybox.net

author

2018-07-25 15:24 UTC #0 lore raw

https://bugs.busybox.net/show_bug.cgi?id=11191

Bug ID: 11191
Summary: xattr and check-package issue
Product: buildroot
Version: 2018.02.2
Hardware: All
OS: Linux
Status: NEW
Severity: normal
Priority: P5
Component: Other
Assignee: unassigned at buildroot.uclibc.org
Reporter: jpcartal at free.fr
CC: buildroot at uclibc.org
Target Milestone: ---

check-package warns about missing indent with xattr line in permissions of packages, however adding indentation to those lines makes the check-package script happy but then corresponding xattr entries are then silently ignored when creating the image.

e.g., this entry will make check-package issue a warning :

...
define TEST_PERMISSIONS

        /usr/bin/test f 0755 user0 users - - - - -
|xattr cap_kill+eip
|xattr cap_sys_nice+eip
|xattr cap_sys_time+eip

...

package/test/test.mk:618: expected indent with tabs

However changing it to

define TEST_PERMISSIONS

        /usr/bin/test f 0755 user0 users - - - - -
\t|xattr cap_kill+eip
\t|xattr cap_sys_nice+eip
\t|xattr cap_sys_time+eip

...

Will make check-package happy but xattr attributes will not be taken into account without any error message.

Signature · 2 lines

You are receiving this mail because:
You are on the CC list for the bug.

bugzilla at busybox.net

author

2018-07-25 16:12 UTC #1 lore raw

[Buildroot] [Bug 11191] xattr and check-package issue

https://bugs.busybox.net/show_bug.cgi?id=11191

Thomas Petazzoni <[email protected]> changed:

Signature · 2 lines

You are receiving this mail because:
You are on the CC list for the bug.

bugzilla at busybox.net

author

2018-07-26 00:42 UTC #2 lore raw

[Buildroot] [Bug 11191] xattr and check-package issue

In reply to bugzilla at busybox.net

https://bugs.busybox.net/show_bug.cgi?id=11191

Ricardo Martincoski <[email protected]> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ASSIGNED

--- Comment #1 from Ricardo Martincoski <[email protected]> --- I can reproduce both behaviors on current master:
- check-package complaining when tab is not used for |xattr
- xattr being silently ignored by host-makedevs when tab is used for |xattr

The second one occurs because the tab propagates from the package recipe to package/pkg-generic.mk to fs/common.mk and finally to output/build/buildroot-fs/device_table.txt and the code that processes this file does a strict check:
line 513 @ package/makedevs/makedevs.c
if (1 == sscanf(line, "|xattr %254s", xattr)) {

We could fix this inconsistency by a number of ways:
1) change check-package;
2) try to come up with a solution in package/pkg-generic.mk to remove leading tabs from _PERMISSION. It cannot be a simple $$(strip) because it would remove the newlines;
3) change makedevs to ignore leading tabs/spaces for xattr (as it already does for non-xattr lines). Hackish version to replace the code mentioned above:

Quoted 1 line

if (2 == sscanf(line, "%4095s %254s", name, xattr) && !strcmp(name, "|xattr")) {

4) change fs/common.mk to remove the leading tabs/spaces. Something like this:

Review

Quoted 5 lines

+++ b/fs/common.mk

(quoted diff) +1 -0

@@@ -89,2 -89,2 +89,3 @@@ endi
       $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
++      $(SED) "s/^[ \t]*//g" $(FULL_DEVICE_TABLE)
       echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)

I think option 4 makes sense because we also will end up with a better formatted device_table.txt.

But let's wait for more opinions on how to proceed.

Signature · 2 lines

You are receiving this mail because:
You are on the CC list for the bug.

Arnout Vandecappelle

2018-07-26 08:17 UTC #3 lore raw

[Buildroot] Handling leading tabs in makedevs [was: [Bug 11191] xattr and check-package issue]

[Taking this to the mailing list, more appropriate for this discussion IMO.]

Quoted 12 lines

--- Comment #1 from Ricardo Martincoski <[email protected]> --- I can reproduce both behaviors on current master:
- check-package complaining when tab is not used for |xattr
- xattr being silently ignored by host-makedevs when tab is used for |xattr
The second one occurs because the tab propagates from the package recipe to package/pkg-generic.mk to fs/common.mk and finally to output/build/buildroot-fs/device_table.txt and the code that processes this file does a strict check:
line 513 @ package/makedevs/makedevs.c
if (1 == sscanf(line, "|xattr %254s", xattr)) {

Quoted 8 lines

We could fix this inconsistency by a number of ways:
1) change check-package;
2) try to come up with a solution in package/pkg-generic.mk to remove leading tabs from _PERMISSION. It cannot be a simple $$(strip) because it would remove the newlines;
3) change makedevs to ignore leading tabs/spaces for xattr (as it already does for non-xattr lines). Hackish version to replace the code mentioned above:
Quoted 1 line
if (2 == sscanf(line, "%4095s %254s", name, xattr) && !strcmp(name, "|xattr")) {

1 == sscanf(line, " |xattr %254s", xattr)) should be sufficient - " " matches
any amount of whitespace.

Note that since the xattr handling is something we added ourselves to busybox
makedevs, we can easily do this.

Quoted 9 lines

4) change fs/common.mk to remove the leading tabs/spaces. Something like this:
Review
Quoted 5 lines
+++ b/fs/common.mk
(quoted diff) +1 -0
@@@ -89,2 -89,2 +89,3 @@@ endi
       $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
++      $(SED) "s/^[ \t]*//g" $(FULL_DEVICE_TABLE)
       echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
I think option 4 makes sense because we also will end up with a better formatted device_table.txt.

I prefer option 3 or 4 as well. However, I don't agree that it's better
formatted without the tabs - with the tab in front, it's clearer that the xattr belongs to the line above. So I'm more inclined to option 3.

Regards,
Arnout

Quoted 3 lines

But let's wait for more opinions on how to proceed.

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Ricardo Martincoski

2018-08-06 03:12 UTC #4 lore raw

[Buildroot] Handling leading tabs in makedevs [was: [Bug 11191] xattr and check-package issue]

Hello,

On Thu, Jul 26, 2018 at 05:17 AM, Arnout Vandecappelle wrote:

[snip]

Quoted 1 line

Quoted 1 line
--- Comment #1 from Ricardo Martincoski <[email protected]> ---

[snip]

Quoted 11 lines

Quoted 8 lines
We could fix this inconsistency by a number of ways:
1) change check-package;
2) try to come up with a solution in package/pkg-generic.mk to remove leading tabs from _PERMISSION. It cannot be a simple $$(strip) because it would remove the newlines;
3) change makedevs to ignore leading tabs/spaces for xattr (as it already does for non-xattr lines). Hackish version to replace the code mentioned above:
Quoted 1 line
if (2 == sscanf(line, "%4095s %254s", name, xattr) && !strcmp(name, "|xattr")) {
1 == sscanf(line, " |xattr %254s", xattr)) should be sufficient - " " matches
any amount of whitespace.

You are right. I will do this.

Quoted 14 lines

Note that since the xattr handling is something we added ourselves to busybox
makedevs, we can easily do this.
Quoted 9 lines
4) change fs/common.mk to remove the leading tabs/spaces. Something like this:
Review
Quoted 5 lines
+++ b/fs/common.mk
(quoted diff) +1 -0
@@@ -89,2 -89,2 +89,3 @@@ endi
       $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
++      $(SED) "s/^[ \t]*//g" $(FULL_DEVICE_TABLE)
       echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
I think option 4 makes sense because we also will end up with a better formatted device_table.txt.

Quoted 3 lines

I prefer option 3 or 4 as well. However, I don't agree that it's better
formatted without the tabs - with the tab in front, it's clearer that the xattr belongs to the line above. So I'm more inclined to option 3.

But the tabs are also added in front of files when added by _PERMISSIONS, so you won't get the visual effect you want.

When someone uses the default device_table.txt this is what we get when _PERMISSIONS are added for /bin/test:
/dev
/tmp
...
<tab>/bin/test
<tab>|xattr

But that is a file generated and used during the build. We don't care too much if it is not well aligned.

And option 3 also avoids problems if someone is generating a custom device_table.txt and intentionally or by mistake adds leading tabs to xattr.

Regards,
Ricardo

bugzilla at busybox.net

author

2018-08-14 21:20 UTC #5 lore raw

[Buildroot] [Bug 11191] xattr and check-package issue

In reply to bugzilla at busybox.net

https://bugs.busybox.net/show_bug.cgi?id=11191

--- Comment #2 from Yann E. MORIN <[email protected]> --- (In reply to Ricardo Martincoski from comment #1)

In addition to Ricardo's suggestions, here's I believe the technically simplest solution:

5) Require the _PERMISSIONS to not be indented at all.

But I believe hat Ricardo's #3 is the best solution.

Signature · 2 lines

You are receiving this mail because:
You are on the CC list for the bug.

bugzilla at busybox.net

author

2018-08-16 03:01 UTC #6 lore raw

[Buildroot] [Bug 11191] xattr and check-package issue

In reply to bugzilla at busybox.net

https://bugs.busybox.net/show_bug.cgi?id=11191

Ricardo Martincoski <[email protected]> changed:

What |Removed |Added
----------------------------------------------------------------------------

         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #3 from Ricardo Martincoski <[email protected]> --- Fixed in the 2018.02.x branch:
https://git.buildroot.org/buildroot/commit/?h=2018.02.x&id=1369d30a99234b256c9b5bed08d7764393ccd09f
This commit will be part of the next 2018.02.5 release.

The same fix was applied to 2018.05.x and master branches, but on those branches there is another xattr issue that I found while fixing this one:
https://bugs.busybox.net/show_bug.cgi?id=11216

Signature · 2 lines

You are receiving this mail because:
You are on the CC list for the bug.