| Message ID | 1433251718-3167-21-git-send-email-clayton.shotwell@rockwellcollins.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Clayton Shotwell, On Tue, 2 Jun 2015 08:28:36 -0500, Clayton Shotwell wrote: > diff --git a/package/squashfs/squashfs.mk b/package/squashfs/squashfs.mk > index 8ca9e2e..f44ef9f 100644 > --- a/package/squashfs/squashfs.mk > +++ b/package/squashfs/squashfs.mk > @@ -10,9 +10,6 @@ SQUASHFS_SITE = http://downloads.sourceforge.net/project/squashfs/squashfs/squas > SQUASHFS_LICENSE = GPLv2+ > SQUASHFS_LICENSE_FILES = COPYING > > -# no libattr in BR > -SQUASHFS_MAKE_ARGS = XATTR_SUPPORT=0 > - > ifeq ($(BR2_PACKAGE_SQUASHFS_LZ4),y) > SQUASHFS_DEPENDENCIES += lz4 > SQUASHFS_MAKE_ARGS += LZ4_SUPPORT=1 COMP_DEFAULT=lz4 > @@ -52,13 +49,20 @@ HOST_SQUASHFS_DEPENDENCIES = host-zlib host-lz4 host-lzo host-xz > > # no libattr/xz in BR > HOST_SQUASHFS_MAKE_ARGS = \ > - XATTR_SUPPORT=0 \ > XZ_SUPPORT=1 \ > GZIP_SUPPORT=1 \ > LZ4_SUPPORT=1 \ > LZO_SUPPORT=1 \ > LZMA_XZ_SUPPORT=1 > > +ifeq ($(BR2_PACKAGE_ATTR),y) > + SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=1 > + HOST_SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=1 > +else > + SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=0 > + HOST_SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=0 > +endif For the host package, this is clearly not correct. BR2_PACKAGE_ATTR=y tells you that the *target* attr package has been built. It does not tell you anything about the availability of the *host* attr package. Do you can certainly do: SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=$(if $(BR2_PACKAGE_ATTR),1,0) However, touching HOST_SQUASHFS_MAKE_ARGS depending on BR2_PACKAGE_ATTR does not make sense. If you need xattr support in mksquashfs for the host, then you will need to add a BR2_PACKAGE_HOST_SQUASHFS_XATTR_SUPPORT Config.in.host option, and enable XATTR_SUPPORT=1 when enabled, and add host-attr to the dependencies. Also, your patch is incorrect because it does not add the dependency on the attr package, so nothing guarantees you that the attr package has been built before squashfs. I'll mark your patch as Changes Requested in patchwork. Thanks! Thomas
Thomas, On Mon, Jul 6, 2015 at 5:16 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Clayton Shotwell, > > On Tue, 2 Jun 2015 08:28:36 -0500, Clayton Shotwell wrote: > >> diff --git a/package/squashfs/squashfs.mk b/package/squashfs/squashfs.mk >> index 8ca9e2e..f44ef9f 100644 >> --- a/package/squashfs/squashfs.mk >> +++ b/package/squashfs/squashfs.mk >> @@ -10,9 +10,6 @@ SQUASHFS_SITE = http://downloads.sourceforge.net/project/squashfs/squashfs/squas >> SQUASHFS_LICENSE = GPLv2+ >> SQUASHFS_LICENSE_FILES = COPYING >> >> -# no libattr in BR >> -SQUASHFS_MAKE_ARGS = XATTR_SUPPORT=0 >> - >> ifeq ($(BR2_PACKAGE_SQUASHFS_LZ4),y) >> SQUASHFS_DEPENDENCIES += lz4 >> SQUASHFS_MAKE_ARGS += LZ4_SUPPORT=1 COMP_DEFAULT=lz4 >> @@ -52,13 +49,20 @@ HOST_SQUASHFS_DEPENDENCIES = host-zlib host-lz4 host-lzo host-xz >> >> # no libattr/xz in BR >> HOST_SQUASHFS_MAKE_ARGS = \ >> - XATTR_SUPPORT=0 \ >> XZ_SUPPORT=1 \ >> GZIP_SUPPORT=1 \ >> LZ4_SUPPORT=1 \ >> LZO_SUPPORT=1 \ >> LZMA_XZ_SUPPORT=1 >> >> +ifeq ($(BR2_PACKAGE_ATTR),y) >> + SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=1 >> + HOST_SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=1 >> +else >> + SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=0 >> + HOST_SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=0 >> +endif > > For the host package, this is clearly not correct. > > BR2_PACKAGE_ATTR=y tells you that the *target* attr package has been > built. It does not tell you anything about the availability of the > *host* attr package. Do you can certainly do: > > SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=$(if $(BR2_PACKAGE_ATTR),1,0) > > However, touching HOST_SQUASHFS_MAKE_ARGS depending on BR2_PACKAGE_ATTR > does not make sense. If you need xattr support in mksquashfs for the > host, then you will need to add a > BR2_PACKAGE_HOST_SQUASHFS_XATTR_SUPPORT Config.in.host option, and > enable XATTR_SUPPORT=1 when enabled, and add host-attr to the > dependencies. I would really like to know what I was thinking when I put this patch together because you are completely correct on how terrible this is. I looked back at it and I don't believe the host xattr support should be enabled at all. There are too many host dependencies, unrelated to the xattr package, that need to be met for this to work properly (think extended attributes on the filesystem itself). I am going to drop the host changes and only enable this for the target (where I am actually looking to use it). > Also, your patch is incorrect because it does not add the dependency on > the attr package, so nothing guarantees you that the attr package has > been built before squashfs. I'll add that in for the target package with my next set of patches. > I'll mark your patch as Changes Requested in patchwork. Thanks, Clayton Clayton Shotwell Senior Software Engineer, Rockwell Collins clayton.shotwell@rockwellcollins.com
Clayton, On Fri, 10 Jul 2015 14:54:28 -0500, Clayton Shotwell wrote: > I would really like to know what I was thinking when I put this patch > together because you are completely correct on how terrible this is. I > looked back at it and I don't believe the host xattr support should be > enabled at all. There are too many host dependencies, unrelated to the > xattr package, that need to be met for this to work properly (think > extended attributes on the filesystem itself). I am going to drop the > host changes and only enable this for the target (where I am actually > looking to use it). No problem. However, I'm curious to know what is your use case: you are preparing a SquashFS image on your target? Best regards, Thomas
Thomas, On Fri, Jul 10, 2015 at 3:23 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Clayton, > > On Fri, 10 Jul 2015 14:54:28 -0500, Clayton Shotwell wrote: > >> I would really like to know what I was thinking when I put this patch >> together because you are completely correct on how terrible this is. I >> looked back at it and I don't believe the host xattr support should be >> enabled at all. There are too many host dependencies, unrelated to the >> xattr package, that need to be met for this to work properly (think >> extended attributes on the filesystem itself). I am going to drop the >> host changes and only enable this for the target (where I am actually >> looking to use it). > > No problem. However, I'm curious to know what is your use case: you are > preparing a SquashFS image on your target? We build a x86 SELinux enabled qemu target that repackages our target filesystems with pre-labelled security attributes. Sort of a sandbox approach.
Dear Matthew Weber, On Sat, 11 Jul 2015 12:12:36 -0500, Matthew Weber wrote: > > No problem. However, I'm curious to know what is your use case: you are > > preparing a SquashFS image on your target? > > We build a x86 SELinux enabled qemu target that repackages our target > filesystems with pre-labelled security attributes. Sort of a sandbox > approach. Why is this needed? Can you generate those security attributes at build time? Thanks, Thomas
Thomas, On Wed, Jul 15, 2015 at 3:21 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Matthew Weber, > > On Sat, 11 Jul 2015 12:12:36 -0500, Matthew Weber wrote: > >> > No problem. However, I'm curious to know what is your use case: you are >> > preparing a SquashFS image on your target? >> >> We build a x86 SELinux enabled qemu target that repackages our target >> filesystems with pre-labelled security attributes. Sort of a sandbox >> approach. > > Why is this needed? Can you generate those security attributes at build > time? If you did you would need to modify the (host) Kernel on your build machine to have all of the filesystem types w/ext attributes and selinux enabled that you need for the rootfs creation. Instead we build a inbetween QEMU that performs this in a sandbox as a post build step. Then we can build on any unmodified distribution (Ubuntu/CentOS/Arch/etc).... > > Thanks, > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
Dear Matthew Weber, On Wed, 15 Jul 2015 16:55:20 -0500, Matthew Weber wrote: > > Why is this needed? Can you generate those security attributes at build > > time? > > If you did you would need to modify the (host) Kernel on your build > machine to have all of the filesystem types w/ext attributes and > selinux enabled that you need for the rootfs creation. Instead we > build a inbetween QEMU that performs this in a sandbox as a post build > step. Then we can build on any unmodified distribution > (Ubuntu/CentOS/Arch/etc).... Ah, ok. I was assuming with enough fakeroot-like magic one could achieve that without having to resort to a sandbox approach like the one you're using. But anyway that's fine, having xattr support for squashfs on the target is perfectly fine. Best regards, Thomas
diff --git a/package/squashfs/squashfs.mk b/package/squashfs/squashfs.mk index 8ca9e2e..f44ef9f 100644 --- a/package/squashfs/squashfs.mk +++ b/package/squashfs/squashfs.mk @@ -10,9 +10,6 @@ SQUASHFS_SITE = http://downloads.sourceforge.net/project/squashfs/squashfs/squas SQUASHFS_LICENSE = GPLv2+ SQUASHFS_LICENSE_FILES = COPYING -# no libattr in BR -SQUASHFS_MAKE_ARGS = XATTR_SUPPORT=0 - ifeq ($(BR2_PACKAGE_SQUASHFS_LZ4),y) SQUASHFS_DEPENDENCIES += lz4 SQUASHFS_MAKE_ARGS += LZ4_SUPPORT=1 COMP_DEFAULT=lz4 @@ -52,13 +49,20 @@ HOST_SQUASHFS_DEPENDENCIES = host-zlib host-lz4 host-lzo host-xz # no libattr/xz in BR HOST_SQUASHFS_MAKE_ARGS = \ - XATTR_SUPPORT=0 \ XZ_SUPPORT=1 \ GZIP_SUPPORT=1 \ LZ4_SUPPORT=1 \ LZO_SUPPORT=1 \ LZMA_XZ_SUPPORT=1 +ifeq ($(BR2_PACKAGE_ATTR),y) + SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=1 + HOST_SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=1 +else + SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=0 + HOST_SQUASHFS_MAKE_ARGS += XATTR_SUPPORT=0 +endif + define SQUASHFS_BUILD_CMDS $(TARGET_MAKE_ENV) $(MAKE) \ CC="$(TARGET_CC)" \
Adding extended attribute support for the squashfs tools when the attr package is selected. This is needed for SELinux support. Signed-off-by: Clayton Shotwell <clayton.shotwell@rockwellcollins.com> --- Changes v6 -> v7: - No changes Changes v5 -> v6: - No changes --- package/squashfs/squashfs.mk | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)