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

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

[bugzilla at busybox.net]

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.

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

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

[bugzilla at busybox.net]

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

Thomas Petazzoni <thomas.petazzoni@free-electrons.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at buildroot.uclibc |ricardo.martincoski at datacom
                   |.org                        |.ind.br

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

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

[bugzilla at busybox.net]

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

Ricardo Martincoski <ricardo.martincoski@datacom.ind.br> changed:

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

--- Comment #1 from Ricardo Martincoski <ricardo.martincoski@datacom.ind.br> ---
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:
> 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:
>+++ b/fs/common.mk
>@@@ -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.

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

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

[Arnout Vandecappelle]

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


> --- Comment #1 from Ricardo Martincoski <ricardo.martincoski@datacom.ind.br> ---
> 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:
>> 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.

> 4) change fs/common.mk to remove the leading tabs/spaces. Something like this:
>> +++ b/fs/common.mk
>> @@@ -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

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

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

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

[Ricardo Martincoski]

Hello,

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

[snip]
>> --- Comment #1 from Ricardo Martincoski <ricardo.martincoski@datacom.ind.br> ---
[snip]
>> 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:
>>> 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.

> 
>  Note that since the xattr handling is something we added ourselves to busybox
> makedevs, we can easily do this.
> 
>> 4) change fs/common.mk to remove the leading tabs/spaces. Something like this:
>>> +++ b/fs/common.mk
>>> @@@ -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.

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

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

[bugzilla at busybox.net]

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

--- Comment #2 from Yann E. MORIN <yann.morin.1998@free.fr> ---
(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.

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

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

[bugzilla at busybox.net]

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

Ricardo Martincoski <ricardo.martincoski@datacom.com.br> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #3 from Ricardo Martincoski <ricardo.martincoski@datacom.com.br> ---
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

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