* [PATCH 1/2] wic: Fix permissions when using exclude or include path @ 2020-03-04 8:34 Ricardo Ribalda Delgado 2020-03-04 8:34 ` [PATCH 2/2] wic: Add --embed-rootfs argument Ricardo Ribalda Delgado 2020-03-04 9:53 ` [PATCH 1/2] wic: Fix permissions when using exclude or include path Paul Barker 0 siblings, 2 replies; 18+ messages in thread From: Ricardo Ribalda Delgado @ 2020-03-04 8:34 UTC (permalink / raw) To: openembedded-core; +Cc: Ricardo Ribalda Delgado When parameters include_path or exclude_path are passed to the rootfs plugin, it will copy the partition content into a folder and make all the modifications there. This is done using copyhardlinktree(), which does not take into consideration the content of the pseudo folder, which contains the information about the right permissions and ownership of the folders. This results in a rootfs owned by the user that is running the wic command (usually UID 1000), which makes some rootfs unbootable. To fix this we copy the content of the pseudo folders to the new folder and modify the pseudo database using the "pseudo -B" command. Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> --- scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py index 705aeb5563..40419a64b3 100644 --- a/scripts/lib/wic/plugins/source/rootfs.py +++ b/scripts/lib/wic/plugins/source/rootfs.py @@ -16,11 +16,11 @@ import os import shutil import sys -from oe.path import copyhardlinktree +from oe.path import copyhardlinktree, copytree from wic import WicError from wic.pluginbase import SourcePlugin -from wic.misc import get_bitbake_var +from wic.misc import get_bitbake_var, exec_native_cmd logger = logging.getLogger('wic') @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): return os.path.realpath(image_rootfs_dir) + @staticmethod + def __get_pseudo(native_sysroot, rootfs): + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") + return pseudo + @classmethod def do_prepare_partition(cls, part, source_params, cr, cr_workdir, oe_builddir, bootimg_dir, kernel_dir, @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): if os.path.lexists(new_rootfs): shutil.rmtree(os.path.join(new_rootfs)) - copyhardlinktree(part.rootfs_dir, new_rootfs) + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) + copytree(os.path.join(part.rootfs_dir, "../pseudo"), + os.path.join(new_rootfs, "../pseudo")) + pseudo_cmd = "%s -B -m %s -M %s" % (cls.__get_pseudo(native_sysroot,new_rootfs), + part.rootfs_dir, new_rootfs) + exec_native_cmd(pseudo_cmd, native_sysroot) + for path in part.include_path or []: copyhardlinktree(path, new_rootfs) -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] wic: Add --embed-rootfs argument 2020-03-04 8:34 [PATCH 1/2] wic: Fix permissions when using exclude or include path Ricardo Ribalda Delgado @ 2020-03-04 8:34 ` Ricardo Ribalda Delgado 2020-03-04 9:42 ` Paul Barker 2020-03-04 9:53 ` [PATCH 1/2] wic: Fix permissions when using exclude or include path Paul Barker 1 sibling, 1 reply; 18+ messages in thread From: Ricardo Ribalda Delgado @ 2020-03-04 8:34 UTC (permalink / raw) To: openembedded-core; +Cc: Ricardo Ribalda Delgado This option adds the content of a rootfs on a specific location on the rootfs. It is very useful for making a partition that contains the rootfs for a host and a target Eg: / -> Roofs for the host /export/ -> Rootfs for the target (which will netboot) Although today we support making a partition for "/export" this might not be compatible with some upgrade systems, or we might be limited by the number of partitions. With this patch we can use something like: part / --source rootfs --embed-rootfs=target-image:/export on the .wks file. Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> --- scripts/lib/wic/help.py | 7 +++++++ scripts/lib/wic/ksparser.py | 1 + scripts/lib/wic/partition.py | 1 + scripts/lib/wic/plugins/source/rootfs.py | 20 +++++++++++++++++++- 4 files changed, 28 insertions(+), 1 deletion(-) diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py index 4d342fcf05..67a33e6a65 100644 --- a/scripts/lib/wic/help.py +++ b/scripts/lib/wic/help.py @@ -979,6 +979,13 @@ DESCRIPTION copies. This option only has an effect with the rootfs source plugin. + --embed-rootfs: This option is specific to wic. It embeds a rootfs into + the given path to the resulting image. The option + contains two fields, the roofs and the path, separated + by a colon. The rootfs follows the same logic as the + rootfs-dir argument. This option only has an effect + with the rootfs source plugin. + --extra-space: This option is specific to wic. It adds extra space after the space filled by the content of the partition. The final size can go diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py index 650b976223..d422e2a6bb 100644 --- a/scripts/lib/wic/ksparser.py +++ b/scripts/lib/wic/ksparser.py @@ -138,6 +138,7 @@ class KickStart(): part.add_argument('--align', type=int) part.add_argument('--exclude-path', nargs='+') part.add_argument('--include-path', nargs='+') + part.add_argument('--embed-rootfs', nargs='+') part.add_argument("--extra-space", type=sizetype) part.add_argument('--fsoptions', dest='fsopts') part.add_argument('--fstype', default='vfat', diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py index 2d95f78439..13857df82f 100644 --- a/scripts/lib/wic/partition.py +++ b/scripts/lib/wic/partition.py @@ -31,6 +31,7 @@ class Partition(): self.extra_space = args.extra_space self.exclude_path = args.exclude_path self.include_path = args.include_path + self.embed_rootfs = args.embed_rootfs self.fsopts = args.fsopts self.fstype = args.fstype self.label = args.label diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py index 40419a64b3..16359601dc 100644 --- a/scripts/lib/wic/plugins/source/rootfs.py +++ b/scripts/lib/wic/plugins/source/rootfs.py @@ -80,7 +80,7 @@ class RootfsPlugin(SourcePlugin): new_rootfs = None # Handle excluded paths. - if part.exclude_path or part.include_path: + if part.exclude_path or part.include_path or part.embed_rootfs: # We need a new rootfs directory we can delete files from. Copy to # workdir. new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) @@ -100,6 +100,24 @@ class RootfsPlugin(SourcePlugin): for path in part.include_path or []: copyhardlinktree(path, new_rootfs) + for embed in part.embed_rootfs or []: + [embed_rootfs, path] = embed.split(":") + #we need to remove the initial / for os.path.join to work + if os.path.isabs(path): + path = path[1:] + if embed_rootfs in krootfs_dir: + embed_rootfs = krootfs_dir[embed_rootfs] + embed_rootfs = cls.__get_rootfs_dir(embed_rootfs) + tar_file = os.path.realpath(os.path.join(cr_workdir, "aux.tar")) + tar_cmd = "%s tar cpf %s -C %s ." % (cls.__get_pseudo(native_sysroot, + embed_rootfs), tar_file, embed_rootfs) + exec_native_cmd(tar_cmd, native_sysroot) + untar_cmd = "%s tar xf %s -C %s ." % (cls.__get_pseudo(native_sysroot, new_rootfs), + tar_file, os.path.join(new_rootfs, path)) + exec_native_cmd(untar_cmd, native_sysroot, + cls.__get_pseudo(native_sysroot, new_rootfs)) + os.remove(tar_file) + for orig_path in part.exclude_path or []: path = orig_path if os.path.isabs(path): -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] wic: Add --embed-rootfs argument 2020-03-04 8:34 ` [PATCH 2/2] wic: Add --embed-rootfs argument Ricardo Ribalda Delgado @ 2020-03-04 9:42 ` Paul Barker 2020-03-04 9:56 ` Ricardo Ribalda Delgado 0 siblings, 1 reply; 18+ messages in thread From: Paul Barker @ 2020-03-04 9:42 UTC (permalink / raw) To: openembedded-core, Ricardo Ribalda Delgado On Wed, 4 Mar 2020 09:34:38 +0100 Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > This option adds the content of a rootfs on a specific location on the > rootfs. > > It is very useful for making a partition that contains the rootfs for a > host and a target Eg: > > / -> Roofs for the host > /export/ -> Rootfs for the target (which will netboot) > > Although today we support making a partition for "/export" this might > not be compatible with some upgrade systems, or we might be limited by > the number of partitions. > > With this patch we can use something like: > > part / --source rootfs --embed-rootfs=target-image:/export I considered using a syntax like this for --include-path but I chose not to as it's not easy to choose a separator. ':' is a valid character in a directory name. My approach is to handle this in two steps: 1) Add a new task before do_image_wic which creates an 'embedded-rootfs' directory and copies 'target-image' to 'embedded-rootfs/export'. 2) Use '--include-path=embedded-rootfs' in the wks file. > > on the .wks file. > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > --- > scripts/lib/wic/help.py | 7 +++++++ > scripts/lib/wic/ksparser.py | 1 + > scripts/lib/wic/partition.py | 1 + > scripts/lib/wic/plugins/source/rootfs.py | 20 +++++++++++++++++++- > 4 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py > index 4d342fcf05..67a33e6a65 100644 > --- a/scripts/lib/wic/help.py > +++ b/scripts/lib/wic/help.py > @@ -979,6 +979,13 @@ DESCRIPTION > copies. This option only has an effect with the rootfs > source plugin. > > + --embed-rootfs: This option is specific to wic. It embeds a rootfs into > + the given path to the resulting image. The option > + contains two fields, the roofs and the path, separated > + by a colon. The rootfs follows the same logic as the > + rootfs-dir argument. This option only has an effect > + with the rootfs source plugin. > + > --extra-space: This option is specific to wic. It adds extra > space after the space filled by the content > of the partition. The final size can go > diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py > index 650b976223..d422e2a6bb 100644 > --- a/scripts/lib/wic/ksparser.py > +++ b/scripts/lib/wic/ksparser.py > @@ -138,6 +138,7 @@ class KickStart(): > part.add_argument('--align', type=int) > part.add_argument('--exclude-path', nargs='+') > part.add_argument('--include-path', nargs='+') > + part.add_argument('--embed-rootfs', nargs='+') > part.add_argument("--extra-space", type=sizetype) > part.add_argument('--fsoptions', dest='fsopts') > part.add_argument('--fstype', default='vfat', > diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py > index 2d95f78439..13857df82f 100644 > --- a/scripts/lib/wic/partition.py > +++ b/scripts/lib/wic/partition.py > @@ -31,6 +31,7 @@ class Partition(): > self.extra_space = args.extra_space > self.exclude_path = args.exclude_path > self.include_path = args.include_path > + self.embed_rootfs = args.embed_rootfs > self.fsopts = args.fsopts > self.fstype = args.fstype > self.label = args.label > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > index 40419a64b3..16359601dc 100644 > --- a/scripts/lib/wic/plugins/source/rootfs.py > +++ b/scripts/lib/wic/plugins/source/rootfs.py > @@ -80,7 +80,7 @@ class RootfsPlugin(SourcePlugin): > > new_rootfs = None > # Handle excluded paths. > - if part.exclude_path or part.include_path: > + if part.exclude_path or part.include_path or part.embed_rootfs: > # We need a new rootfs directory we can delete files from. Copy to > # workdir. > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) > @@ -100,6 +100,24 @@ class RootfsPlugin(SourcePlugin): > for path in part.include_path or []: > copyhardlinktree(path, new_rootfs) > > + for embed in part.embed_rootfs or []: > + [embed_rootfs, path] = embed.split(":") > + #we need to remove the initial / for os.path.join to work > + if os.path.isabs(path): > + path = path[1:] > + if embed_rootfs in krootfs_dir: > + embed_rootfs = krootfs_dir[embed_rootfs] > + embed_rootfs = cls.__get_rootfs_dir(embed_rootfs) > + tar_file = os.path.realpath(os.path.join(cr_workdir, "aux.tar")) > + tar_cmd = "%s tar cpf %s -C %s ." % (cls.__get_pseudo(native_sysroot, > + embed_rootfs), tar_file, embed_rootfs) > + exec_native_cmd(tar_cmd, native_sysroot) > + untar_cmd = "%s tar xf %s -C %s ." % (cls.__get_pseudo(native_sysroot, new_rootfs), > + tar_file, os.path.join(new_rootfs, path)) > + exec_native_cmd(untar_cmd, native_sysroot, > + cls.__get_pseudo(native_sysroot, new_rootfs)) > + os.remove(tar_file) > + Why are you using an intermediate tar file here? > for orig_path in part.exclude_path or []: > path = orig_path > if os.path.isabs(path): -- Paul Barker Konsulko Group ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] wic: Add --embed-rootfs argument 2020-03-04 9:42 ` Paul Barker @ 2020-03-04 9:56 ` Ricardo Ribalda Delgado 2020-03-04 10:08 ` Paul Barker 0 siblings, 1 reply; 18+ messages in thread From: Ricardo Ribalda Delgado @ 2020-03-04 9:56 UTC (permalink / raw) To: Paul Barker; +Cc: openembedded-core Hi Paul Thanks for your reply On Wed, Mar 4, 2020 at 10:43 AM Paul Barker <pbarker@konsulko.com> wrote: > > On Wed, 4 Mar 2020 09:34:38 +0100 > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > This option adds the content of a rootfs on a specific location on the > > rootfs. > > > > It is very useful for making a partition that contains the rootfs for a > > host and a target Eg: > > > > / -> Roofs for the host > > /export/ -> Rootfs for the target (which will netboot) > > > > Although today we support making a partition for "/export" this might > > not be compatible with some upgrade systems, or we might be limited by > > the number of partitions. > > > > With this patch we can use something like: > > > > part / --source rootfs --embed-rootfs=target-image:/export > > I considered using a syntax like this for --include-path but I chose not to > as it's not easy to choose a separator. ':' is a valid character in a > directory name. I think we have to live with not being able to copy files to a directory with a colon :( > > My approach is to handle this in two steps: > > 1) Add a new task before do_image_wic which creates an 'embedded-rootfs' > directory and copies 'target-image' to 'embedded-rootfs/export'. > > 2) Use '--include-path=embedded-rootfs' in the wks file. > The biggest problem with that approach is permissions. We need files with UID 0, siticky bits, etc. We lose the pseudo database if we simply copy to a folder. Also, if we have a complex system with multiple partitions I prefer to handle all in a single .wks file instead of a script + wks file > > > > on the .wks file. > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > --- > > scripts/lib/wic/help.py | 7 +++++++ > > scripts/lib/wic/ksparser.py | 1 + > > scripts/lib/wic/partition.py | 1 + > > scripts/lib/wic/plugins/source/rootfs.py | 20 +++++++++++++++++++- > > 4 files changed, 28 insertions(+), 1 deletion(-) > > > > diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py > > index 4d342fcf05..67a33e6a65 100644 > > --- a/scripts/lib/wic/help.py > > +++ b/scripts/lib/wic/help.py > > @@ -979,6 +979,13 @@ DESCRIPTION > > copies. This option only has an effect with the rootfs > > source plugin. > > > > + --embed-rootfs: This option is specific to wic. It embeds a rootfs into > > + the given path to the resulting image. The option > > + contains two fields, the roofs and the path, separated > > + by a colon. The rootfs follows the same logic as the > > + rootfs-dir argument. This option only has an effect > > + with the rootfs source plugin. > > + > > --extra-space: This option is specific to wic. It adds extra > > space after the space filled by the content > > of the partition. The final size can go > > diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py > > index 650b976223..d422e2a6bb 100644 > > --- a/scripts/lib/wic/ksparser.py > > +++ b/scripts/lib/wic/ksparser.py > > @@ -138,6 +138,7 @@ class KickStart(): > > part.add_argument('--align', type=int) > > part.add_argument('--exclude-path', nargs='+') > > part.add_argument('--include-path', nargs='+') > > + part.add_argument('--embed-rootfs', nargs='+') > > part.add_argument("--extra-space", type=sizetype) > > part.add_argument('--fsoptions', dest='fsopts') > > part.add_argument('--fstype', default='vfat', > > diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py > > index 2d95f78439..13857df82f 100644 > > --- a/scripts/lib/wic/partition.py > > +++ b/scripts/lib/wic/partition.py > > @@ -31,6 +31,7 @@ class Partition(): > > self.extra_space = args.extra_space > > self.exclude_path = args.exclude_path > > self.include_path = args.include_path > > + self.embed_rootfs = args.embed_rootfs > > self.fsopts = args.fsopts > > self.fstype = args.fstype > > self.label = args.label > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > index 40419a64b3..16359601dc 100644 > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > @@ -80,7 +80,7 @@ class RootfsPlugin(SourcePlugin): > > > > new_rootfs = None > > # Handle excluded paths. > > - if part.exclude_path or part.include_path: > > + if part.exclude_path or part.include_path or part.embed_rootfs: > > # We need a new rootfs directory we can delete files from. Copy to > > # workdir. > > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) > > @@ -100,6 +100,24 @@ class RootfsPlugin(SourcePlugin): > > for path in part.include_path or []: > > copyhardlinktree(path, new_rootfs) > > > > + for embed in part.embed_rootfs or []: > > + [embed_rootfs, path] = embed.split(":") > > + #we need to remove the initial / for os.path.join to work > > + if os.path.isabs(path): > > + path = path[1:] > > + if embed_rootfs in krootfs_dir: > > + embed_rootfs = krootfs_dir[embed_rootfs] > > + embed_rootfs = cls.__get_rootfs_dir(embed_rootfs) > > + tar_file = os.path.realpath(os.path.join(cr_workdir, "aux.tar")) > > + tar_cmd = "%s tar cpf %s -C %s ." % (cls.__get_pseudo(native_sysroot, > > + embed_rootfs), tar_file, embed_rootfs) > > + exec_native_cmd(tar_cmd, native_sysroot) > > + untar_cmd = "%s tar xf %s -C %s ." % (cls.__get_pseudo(native_sysroot, new_rootfs), > > + tar_file, os.path.join(new_rootfs, path)) > > + exec_native_cmd(untar_cmd, native_sysroot, > > + cls.__get_pseudo(native_sysroot, new_rootfs)) > > + os.remove(tar_file) > > + > > Why are you using an intermediate tar file here? For the permissions :( > > > for orig_path in part.exclude_path or []: > > path = orig_path > > if os.path.isabs(path): > > > > -- > Paul Barker > Konsulko Group -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] wic: Add --embed-rootfs argument 2020-03-04 9:56 ` Ricardo Ribalda Delgado @ 2020-03-04 10:08 ` Paul Barker 2020-03-04 10:14 ` Ricardo Ribalda Delgado 0 siblings, 1 reply; 18+ messages in thread From: Paul Barker @ 2020-03-04 10:08 UTC (permalink / raw) To: Ricardo Ribalda Delgado; +Cc: openembedded-core On Wed, 4 Mar 2020 10:56:20 +0100 Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > Hi Paul > > Thanks for your reply > > On Wed, Mar 4, 2020 at 10:43 AM Paul Barker <pbarker@konsulko.com> wrote: > > > > On Wed, 4 Mar 2020 09:34:38 +0100 > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > This option adds the content of a rootfs on a specific location on the > > > rootfs. > > > > > > It is very useful for making a partition that contains the rootfs for a > > > host and a target Eg: > > > > > > / -> Roofs for the host > > > /export/ -> Rootfs for the target (which will netboot) > > > > > > Although today we support making a partition for "/export" this might > > > not be compatible with some upgrade systems, or we might be limited by > > > the number of partitions. > > > > > > With this patch we can use something like: > > > > > > part / --source rootfs --embed-rootfs=target-image:/export > > > > I considered using a syntax like this for --include-path but I chose not to > > as it's not easy to choose a separator. ':' is a valid character in a > > directory name. > > I think we have to live with not being able to copy files to a > directory with a colon :( I wouldn't like to place a limitation like that on users and I'd like to avoid the proliferation of similar arguments here. How about modifying '--include-path' to take two arguments, a source path and a destination prefix? The python argument parser should be able to handle this and you can build a list of tuples (source_path, dest_prefix) when parsing the arguments. The '--include-path' argument hasn't been part of a Yocto release yet it's just on the master branch so I think if we're going to fix it with a breaking change, now is definitely the time. > > > > > My approach is to handle this in two steps: > > > > 1) Add a new task before do_image_wic which creates an 'embedded-rootfs' > > directory and copies 'target-image' to 'embedded-rootfs/export'. > > > > 2) Use '--include-path=embedded-rootfs' in the wks file. > > > > The biggest problem with that approach is permissions. We need files > with UID 0, siticky bits, etc. We lose the pseudo database if we > simply copy to a folder. > > Also, if we have a complex system with multiple partitions I prefer to > handle all in a single .wks file instead of a script + wks file > > > > > > > on the .wks file. > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > --- > > > scripts/lib/wic/help.py | 7 +++++++ > > > scripts/lib/wic/ksparser.py | 1 + > > > scripts/lib/wic/partition.py | 1 + > > > scripts/lib/wic/plugins/source/rootfs.py | 20 +++++++++++++++++++- > > > 4 files changed, 28 insertions(+), 1 deletion(-) > > > > > > diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py > > > index 4d342fcf05..67a33e6a65 100644 > > > --- a/scripts/lib/wic/help.py > > > +++ b/scripts/lib/wic/help.py > > > @@ -979,6 +979,13 @@ DESCRIPTION > > > copies. This option only has an effect with the rootfs > > > source plugin. > > > > > > + --embed-rootfs: This option is specific to wic. It embeds a rootfs into > > > + the given path to the resulting image. The option > > > + contains two fields, the roofs and the path, separated > > > + by a colon. The rootfs follows the same logic as the > > > + rootfs-dir argument. This option only has an effect > > > + with the rootfs source plugin. > > > + > > > --extra-space: This option is specific to wic. It adds extra > > > space after the space filled by the content > > > of the partition. The final size can go > > > diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py > > > index 650b976223..d422e2a6bb 100644 > > > --- a/scripts/lib/wic/ksparser.py > > > +++ b/scripts/lib/wic/ksparser.py > > > @@ -138,6 +138,7 @@ class KickStart(): > > > part.add_argument('--align', type=int) > > > part.add_argument('--exclude-path', nargs='+') > > > part.add_argument('--include-path', nargs='+') > > > + part.add_argument('--embed-rootfs', nargs='+') > > > part.add_argument("--extra-space", type=sizetype) > > > part.add_argument('--fsoptions', dest='fsopts') > > > part.add_argument('--fstype', default='vfat', > > > diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py > > > index 2d95f78439..13857df82f 100644 > > > --- a/scripts/lib/wic/partition.py > > > +++ b/scripts/lib/wic/partition.py > > > @@ -31,6 +31,7 @@ class Partition(): > > > self.extra_space = args.extra_space > > > self.exclude_path = args.exclude_path > > > self.include_path = args.include_path > > > + self.embed_rootfs = args.embed_rootfs > > > self.fsopts = args.fsopts > > > self.fstype = args.fstype > > > self.label = args.label > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > index 40419a64b3..16359601dc 100644 > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > @@ -80,7 +80,7 @@ class RootfsPlugin(SourcePlugin): > > > > > > new_rootfs = None > > > # Handle excluded paths. > > > - if part.exclude_path or part.include_path: > > > + if part.exclude_path or part.include_path or part.embed_rootfs: > > > # We need a new rootfs directory we can delete files from. Copy to > > > # workdir. > > > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) > > > @@ -100,6 +100,24 @@ class RootfsPlugin(SourcePlugin): > > > for path in part.include_path or []: > > > copyhardlinktree(path, new_rootfs) > > > > > > + for embed in part.embed_rootfs or []: > > > + [embed_rootfs, path] = embed.split(":") > > > + #we need to remove the initial / for os.path.join to work > > > + if os.path.isabs(path): > > > + path = path[1:] > > > + if embed_rootfs in krootfs_dir: > > > + embed_rootfs = krootfs_dir[embed_rootfs] > > > + embed_rootfs = cls.__get_rootfs_dir(embed_rootfs) > > > + tar_file = os.path.realpath(os.path.join(cr_workdir, "aux.tar")) > > > + tar_cmd = "%s tar cpf %s -C %s ." % (cls.__get_pseudo(native_sysroot, > > > + embed_rootfs), tar_file, embed_rootfs) > > > + exec_native_cmd(tar_cmd, native_sysroot) > > > + untar_cmd = "%s tar xf %s -C %s ." % (cls.__get_pseudo(native_sysroot, new_rootfs), > > > + tar_file, os.path.join(new_rootfs, path)) > > > + exec_native_cmd(untar_cmd, native_sysroot, > > > + cls.__get_pseudo(native_sysroot, new_rootfs)) > > > + os.remove(tar_file) > > > + > > > > Why are you using an intermediate tar file here? > > For the permissions :( Your previous patch handled this using 'pseudo -B'. It would be good to be consistent if possible - either use an intermediate tar file in all cases if it's necessary or use 'pseudo -B' in all cases. -- Paul Barker Konsulko Group ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] wic: Add --embed-rootfs argument 2020-03-04 10:08 ` Paul Barker @ 2020-03-04 10:14 ` Ricardo Ribalda Delgado 2020-03-04 13:49 ` Ricardo Ribalda Delgado 0 siblings, 1 reply; 18+ messages in thread From: Ricardo Ribalda Delgado @ 2020-03-04 10:14 UTC (permalink / raw) To: Paul Barker; +Cc: openembedded-core Hi Paul On Wed, Mar 4, 2020 at 11:09 AM Paul Barker <pbarker@konsulko.com> wrote: > > On Wed, 4 Mar 2020 10:56:20 +0100 > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > Hi Paul > > > > Thanks for your reply > > > > On Wed, Mar 4, 2020 at 10:43 AM Paul Barker <pbarker@konsulko.com> wrote: > > > > > > On Wed, 4 Mar 2020 09:34:38 +0100 > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > This option adds the content of a rootfs on a specific location on the > > > > rootfs. > > > > > > > > It is very useful for making a partition that contains the rootfs for a > > > > host and a target Eg: > > > > > > > > / -> Roofs for the host > > > > /export/ -> Rootfs for the target (which will netboot) > > > > > > > > Although today we support making a partition for "/export" this might > > > > not be compatible with some upgrade systems, or we might be limited by > > > > the number of partitions. > > > > > > > > With this patch we can use something like: > > > > > > > > part / --source rootfs --embed-rootfs=target-image:/export > > > > > > I considered using a syntax like this for --include-path but I chose not to > > > as it's not easy to choose a separator. ':' is a valid character in a > > > directory name. > > > > I think we have to live with not being able to copy files to a > > directory with a colon :( > > I wouldn't like to place a limitation like that on users and I'd like to > avoid the proliferation of similar arguments here. > > How about modifying '--include-path' to take two arguments, a source path and > a destination prefix? The python argument parser should be able to handle > this and you can build a list of tuples (source_path, dest_prefix) when > parsing the arguments. > > The '--include-path' argument hasn't been part of a Yocto release yet it's > just on the master branch so I think if we're going to fix it with a breaking > change, now is definitely the time. That sounds good to me. I do not mind having two arguments. for embed-rootfs. and probably will look better on include-path > > > > > > > > > My approach is to handle this in two steps: > > > > > > 1) Add a new task before do_image_wic which creates an 'embedded-rootfs' > > > directory and copies 'target-image' to 'embedded-rootfs/export'. > > > > > > 2) Use '--include-path=embedded-rootfs' in the wks file. > > > > > > > The biggest problem with that approach is permissions. We need files > > with UID 0, siticky bits, etc. We lose the pseudo database if we > > simply copy to a folder. > > > > Also, if we have a complex system with multiple partitions I prefer to > > handle all in a single .wks file instead of a script + wks file > > > > > > > > > > on the .wks file. > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > > --- > > > > scripts/lib/wic/help.py | 7 +++++++ > > > > scripts/lib/wic/ksparser.py | 1 + > > > > scripts/lib/wic/partition.py | 1 + > > > > scripts/lib/wic/plugins/source/rootfs.py | 20 +++++++++++++++++++- > > > > 4 files changed, 28 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py > > > > index 4d342fcf05..67a33e6a65 100644 > > > > --- a/scripts/lib/wic/help.py > > > > +++ b/scripts/lib/wic/help.py > > > > @@ -979,6 +979,13 @@ DESCRIPTION > > > > copies. This option only has an effect with the rootfs > > > > source plugin. > > > > > > > > + --embed-rootfs: This option is specific to wic. It embeds a rootfs into > > > > + the given path to the resulting image. The option > > > > + contains two fields, the roofs and the path, separated > > > > + by a colon. The rootfs follows the same logic as the > > > > + rootfs-dir argument. This option only has an effect > > > > + with the rootfs source plugin. > > > > + > > > > --extra-space: This option is specific to wic. It adds extra > > > > space after the space filled by the content > > > > of the partition. The final size can go > > > > diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py > > > > index 650b976223..d422e2a6bb 100644 > > > > --- a/scripts/lib/wic/ksparser.py > > > > +++ b/scripts/lib/wic/ksparser.py > > > > @@ -138,6 +138,7 @@ class KickStart(): > > > > part.add_argument('--align', type=int) > > > > part.add_argument('--exclude-path', nargs='+') > > > > part.add_argument('--include-path', nargs='+') > > > > + part.add_argument('--embed-rootfs', nargs='+') > > > > part.add_argument("--extra-space", type=sizetype) > > > > part.add_argument('--fsoptions', dest='fsopts') > > > > part.add_argument('--fstype', default='vfat', > > > > diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py > > > > index 2d95f78439..13857df82f 100644 > > > > --- a/scripts/lib/wic/partition.py > > > > +++ b/scripts/lib/wic/partition.py > > > > @@ -31,6 +31,7 @@ class Partition(): > > > > self.extra_space = args.extra_space > > > > self.exclude_path = args.exclude_path > > > > self.include_path = args.include_path > > > > + self.embed_rootfs = args.embed_rootfs > > > > self.fsopts = args.fsopts > > > > self.fstype = args.fstype > > > > self.label = args.label > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > > index 40419a64b3..16359601dc 100644 > > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > > @@ -80,7 +80,7 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > new_rootfs = None > > > > # Handle excluded paths. > > > > - if part.exclude_path or part.include_path: > > > > + if part.exclude_path or part.include_path or part.embed_rootfs: > > > > # We need a new rootfs directory we can delete files from. Copy to > > > > # workdir. > > > > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) > > > > @@ -100,6 +100,24 @@ class RootfsPlugin(SourcePlugin): > > > > for path in part.include_path or []: > > > > copyhardlinktree(path, new_rootfs) > > > > > > > > + for embed in part.embed_rootfs or []: > > > > + [embed_rootfs, path] = embed.split(":") > > > > + #we need to remove the initial / for os.path.join to work > > > > + if os.path.isabs(path): > > > > + path = path[1:] > > > > + if embed_rootfs in krootfs_dir: > > > > + embed_rootfs = krootfs_dir[embed_rootfs] > > > > + embed_rootfs = cls.__get_rootfs_dir(embed_rootfs) > > > > + tar_file = os.path.realpath(os.path.join(cr_workdir, "aux.tar")) > > > > + tar_cmd = "%s tar cpf %s -C %s ." % (cls.__get_pseudo(native_sysroot, > > > > + embed_rootfs), tar_file, embed_rootfs) > > > > + exec_native_cmd(tar_cmd, native_sysroot) > > > > + untar_cmd = "%s tar xf %s -C %s ." % (cls.__get_pseudo(native_sysroot, new_rootfs), > > > > + tar_file, os.path.join(new_rootfs, path)) > > > > + exec_native_cmd(untar_cmd, native_sysroot, > > > > + cls.__get_pseudo(native_sysroot, new_rootfs)) > > > > + os.remove(tar_file) > > > > + > > > > > > Why are you using an intermediate tar file here? > > > > For the permissions :( > > Your previous patch handled this using 'pseudo -B'. It would be good to be > consistent if possible - either use an intermediate tar file in all cases if > it's necessary or use 'pseudo -B' in all cases. Unfortunately, you cannot use pseudo -B in this case. You use pseudo -B when you move one pseudo-aware partition to somewhere else. But now we are merging two pseudo-aware partitions. Also I would not use the intermediate tar on the first place because it is a waste of space + cpu resources. Copyhardlink is the right tool > > -- > Paul Barker > Konsulko Group -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] wic: Add --embed-rootfs argument 2020-03-04 10:14 ` Ricardo Ribalda Delgado @ 2020-03-04 13:49 ` Ricardo Ribalda Delgado 2020-03-04 14:31 ` Joshua Watt 0 siblings, 1 reply; 18+ messages in thread From: Ricardo Ribalda Delgado @ 2020-03-04 13:49 UTC (permalink / raw) To: Paul Barker; +Cc: openembedded-core Hi Paul On Wed, Mar 4, 2020 at 11:14 AM Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > Hi Paul > > On Wed, Mar 4, 2020 at 11:09 AM Paul Barker <pbarker@konsulko.com> wrote: > > > > On Wed, 4 Mar 2020 10:56:20 +0100 > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > Hi Paul > > > > > > Thanks for your reply > > > > > > On Wed, Mar 4, 2020 at 10:43 AM Paul Barker <pbarker@konsulko.com> wrote: > > > > > > > > On Wed, 4 Mar 2020 09:34:38 +0100 > > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > > > This option adds the content of a rootfs on a specific location on the > > > > > rootfs. > > > > > > > > > > It is very useful for making a partition that contains the rootfs for a > > > > > host and a target Eg: > > > > > > > > > > / -> Roofs for the host > > > > > /export/ -> Rootfs for the target (which will netboot) > > > > > > > > > > Although today we support making a partition for "/export" this might > > > > > not be compatible with some upgrade systems, or we might be limited by > > > > > the number of partitions. > > > > > > > > > > With this patch we can use something like: > > > > > > > > > > part / --source rootfs --embed-rootfs=target-image:/export > > > > > > > > I considered using a syntax like this for --include-path but I chose not to > > > > as it's not easy to choose a separator. ':' is a valid character in a > > > > directory name. > > > > > > I think we have to live with not being able to copy files to a > > > directory with a colon :( > > > > I wouldn't like to place a limitation like that on users and I'd like to > > avoid the proliferation of similar arguments here. > > > > How about modifying '--include-path' to take two arguments, a source path and > > a destination prefix? The python argument parser should be able to handle > > this and you can build a list of tuples (source_path, dest_prefix) when > > parsing the arguments. > > > > The '--include-path' argument hasn't been part of a Yocto release yet it's > > just on the master branch so I think if we're going to fix it with a breaking > > change, now is definitely the time. > > That sounds good to me. I do not mind having two arguments. for > embed-rootfs. and probably will look better on include-path How do you think that the parameters should look like: --embed-rootfs target-image /export target-image2 /export2 --embed-rootfs (target-image, /export) (target-image2, /export2) ?? Shall I change it for include-path and embed-rootfs or you want to take care of include-path? > > > > > > > > > > > > > > My approach is to handle this in two steps: > > > > > > > > 1) Add a new task before do_image_wic which creates an 'embedded-rootfs' > > > > directory and copies 'target-image' to 'embedded-rootfs/export'. > > > > > > > > 2) Use '--include-path=embedded-rootfs' in the wks file. > > > > > > > > > > The biggest problem with that approach is permissions. We need files > > > with UID 0, siticky bits, etc. We lose the pseudo database if we > > > simply copy to a folder. > > > > > > Also, if we have a complex system with multiple partitions I prefer to > > > handle all in a single .wks file instead of a script + wks file > > > > > > > > > > > > > on the .wks file. > > > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > > > --- > > > > > scripts/lib/wic/help.py | 7 +++++++ > > > > > scripts/lib/wic/ksparser.py | 1 + > > > > > scripts/lib/wic/partition.py | 1 + > > > > > scripts/lib/wic/plugins/source/rootfs.py | 20 +++++++++++++++++++- > > > > > 4 files changed, 28 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py > > > > > index 4d342fcf05..67a33e6a65 100644 > > > > > --- a/scripts/lib/wic/help.py > > > > > +++ b/scripts/lib/wic/help.py > > > > > @@ -979,6 +979,13 @@ DESCRIPTION > > > > > copies. This option only has an effect with the rootfs > > > > > source plugin. > > > > > > > > > > + --embed-rootfs: This option is specific to wic. It embeds a rootfs into > > > > > + the given path to the resulting image. The option > > > > > + contains two fields, the roofs and the path, separated > > > > > + by a colon. The rootfs follows the same logic as the > > > > > + rootfs-dir argument. This option only has an effect > > > > > + with the rootfs source plugin. > > > > > + > > > > > --extra-space: This option is specific to wic. It adds extra > > > > > space after the space filled by the content > > > > > of the partition. The final size can go > > > > > diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py > > > > > index 650b976223..d422e2a6bb 100644 > > > > > --- a/scripts/lib/wic/ksparser.py > > > > > +++ b/scripts/lib/wic/ksparser.py > > > > > @@ -138,6 +138,7 @@ class KickStart(): > > > > > part.add_argument('--align', type=int) > > > > > part.add_argument('--exclude-path', nargs='+') > > > > > part.add_argument('--include-path', nargs='+') > > > > > + part.add_argument('--embed-rootfs', nargs='+') > > > > > part.add_argument("--extra-space", type=sizetype) > > > > > part.add_argument('--fsoptions', dest='fsopts') > > > > > part.add_argument('--fstype', default='vfat', > > > > > diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py > > > > > index 2d95f78439..13857df82f 100644 > > > > > --- a/scripts/lib/wic/partition.py > > > > > +++ b/scripts/lib/wic/partition.py > > > > > @@ -31,6 +31,7 @@ class Partition(): > > > > > self.extra_space = args.extra_space > > > > > self.exclude_path = args.exclude_path > > > > > self.include_path = args.include_path > > > > > + self.embed_rootfs = args.embed_rootfs > > > > > self.fsopts = args.fsopts > > > > > self.fstype = args.fstype > > > > > self.label = args.label > > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > > > index 40419a64b3..16359601dc 100644 > > > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > > > @@ -80,7 +80,7 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > > > new_rootfs = None > > > > > # Handle excluded paths. > > > > > - if part.exclude_path or part.include_path: > > > > > + if part.exclude_path or part.include_path or part.embed_rootfs: > > > > > # We need a new rootfs directory we can delete files from. Copy to > > > > > # workdir. > > > > > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) > > > > > @@ -100,6 +100,24 @@ class RootfsPlugin(SourcePlugin): > > > > > for path in part.include_path or []: > > > > > copyhardlinktree(path, new_rootfs) > > > > > > > > > > + for embed in part.embed_rootfs or []: > > > > > + [embed_rootfs, path] = embed.split(":") > > > > > + #we need to remove the initial / for os.path.join to work > > > > > + if os.path.isabs(path): > > > > > + path = path[1:] > > > > > + if embed_rootfs in krootfs_dir: > > > > > + embed_rootfs = krootfs_dir[embed_rootfs] > > > > > + embed_rootfs = cls.__get_rootfs_dir(embed_rootfs) > > > > > + tar_file = os.path.realpath(os.path.join(cr_workdir, "aux.tar")) > > > > > + tar_cmd = "%s tar cpf %s -C %s ." % (cls.__get_pseudo(native_sysroot, > > > > > + embed_rootfs), tar_file, embed_rootfs) > > > > > + exec_native_cmd(tar_cmd, native_sysroot) > > > > > + untar_cmd = "%s tar xf %s -C %s ." % (cls.__get_pseudo(native_sysroot, new_rootfs), > > > > > + tar_file, os.path.join(new_rootfs, path)) > > > > > + exec_native_cmd(untar_cmd, native_sysroot, > > > > > + cls.__get_pseudo(native_sysroot, new_rootfs)) > > > > > + os.remove(tar_file) > > > > > + > > > > > > > > Why are you using an intermediate tar file here? > > > > > > For the permissions :( > > > > Your previous patch handled this using 'pseudo -B'. It would be good to be > > consistent if possible - either use an intermediate tar file in all cases if > > it's necessary or use 'pseudo -B' in all cases. > > Unfortunately, you cannot use pseudo -B in this case. > > You use pseudo -B when you move one pseudo-aware partition to > somewhere else. But now we are merging two pseudo-aware partitions. > > Also I would not use the intermediate tar on the first place because > it is a waste of space + cpu resources. Copyhardlink is the right > tool > > > > > > -- > > Paul Barker > > Konsulko Group > > > > -- > Ricardo Ribalda -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] wic: Add --embed-rootfs argument 2020-03-04 13:49 ` Ricardo Ribalda Delgado @ 2020-03-04 14:31 ` Joshua Watt 0 siblings, 0 replies; 18+ messages in thread From: Joshua Watt @ 2020-03-04 14:31 UTC (permalink / raw) To: Ricardo Ribalda Delgado, Paul Barker; +Cc: openembedded-core On 3/4/20 7:49 AM, Ricardo Ribalda Delgado wrote: > Hi Paul > > On Wed, Mar 4, 2020 at 11:14 AM Ricardo Ribalda Delgado > <ricardo@ribalda.com> wrote: >> Hi Paul >> >> On Wed, Mar 4, 2020 at 11:09 AM Paul Barker <pbarker@konsulko.com> wrote: >>> On Wed, 4 Mar 2020 10:56:20 +0100 >>> Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: >>> >>>> Hi Paul >>>> >>>> Thanks for your reply >>>> >>>> On Wed, Mar 4, 2020 at 10:43 AM Paul Barker <pbarker@konsulko.com> wrote: >>>>> On Wed, 4 Mar 2020 09:34:38 +0100 >>>>> Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: >>>>> >>>>>> This option adds the content of a rootfs on a specific location on the >>>>>> rootfs. >>>>>> >>>>>> It is very useful for making a partition that contains the rootfs for a >>>>>> host and a target Eg: >>>>>> >>>>>> / -> Roofs for the host >>>>>> /export/ -> Rootfs for the target (which will netboot) >>>>>> >>>>>> Although today we support making a partition for "/export" this might >>>>>> not be compatible with some upgrade systems, or we might be limited by >>>>>> the number of partitions. >>>>>> >>>>>> With this patch we can use something like: >>>>>> >>>>>> part / --source rootfs --embed-rootfs=target-image:/export >>>>> I considered using a syntax like this for --include-path but I chose not to >>>>> as it's not easy to choose a separator. ':' is a valid character in a >>>>> directory name. >>>> I think we have to live with not being able to copy files to a >>>> directory with a colon :( >>> I wouldn't like to place a limitation like that on users and I'd like to >>> avoid the proliferation of similar arguments here. >>> >>> How about modifying '--include-path' to take two arguments, a source path and >>> a destination prefix? The python argument parser should be able to handle >>> this and you can build a list of tuples (source_path, dest_prefix) when >>> parsing the arguments. >>> >>> The '--include-path' argument hasn't been part of a Yocto release yet it's >>> just on the master branch so I think if we're going to fix it with a breaking >>> change, now is definitely the time. >> That sounds good to me. I do not mind having two arguments. for >> embed-rootfs. and probably will look better on include-path > How do you think that the parameters should look like: > > --embed-rootfs target-image /export target-image2 /export2 > --embed-rootfs (target-image, /export) (target-image2, /export2) > ?? FWIW, it should probably end up looking like this: --embed-rootfs target-image /export --embed-rootfs target-image2 /export2 The python argparse snippet you would use for this would be something like: parser.add_argument('--embed-rootfs', nargs=2, action='append', default=(), help='...') > > Shall I change it for include-path and embed-rootfs or you want to > take care of include-path? > >>>>> My approach is to handle this in two steps: >>>>> >>>>> 1) Add a new task before do_image_wic which creates an 'embedded-rootfs' >>>>> directory and copies 'target-image' to 'embedded-rootfs/export'. >>>>> >>>>> 2) Use '--include-path=embedded-rootfs' in the wks file. >>>>> >>>> The biggest problem with that approach is permissions. We need files >>>> with UID 0, siticky bits, etc. We lose the pseudo database if we >>>> simply copy to a folder. >>>> >>>> Also, if we have a complex system with multiple partitions I prefer to >>>> handle all in a single .wks file instead of a script + wks file >>>> >>>>>> on the .wks file. >>>>>> >>>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> >>>>>> --- >>>>>> scripts/lib/wic/help.py | 7 +++++++ >>>>>> scripts/lib/wic/ksparser.py | 1 + >>>>>> scripts/lib/wic/partition.py | 1 + >>>>>> scripts/lib/wic/plugins/source/rootfs.py | 20 +++++++++++++++++++- >>>>>> 4 files changed, 28 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py >>>>>> index 4d342fcf05..67a33e6a65 100644 >>>>>> --- a/scripts/lib/wic/help.py >>>>>> +++ b/scripts/lib/wic/help.py >>>>>> @@ -979,6 +979,13 @@ DESCRIPTION >>>>>> copies. This option only has an effect with the rootfs >>>>>> source plugin. >>>>>> >>>>>> + --embed-rootfs: This option is specific to wic. It embeds a rootfs into >>>>>> + the given path to the resulting image. The option >>>>>> + contains two fields, the roofs and the path, separated >>>>>> + by a colon. The rootfs follows the same logic as the >>>>>> + rootfs-dir argument. This option only has an effect >>>>>> + with the rootfs source plugin. >>>>>> + >>>>>> --extra-space: This option is specific to wic. It adds extra >>>>>> space after the space filled by the content >>>>>> of the partition. The final size can go >>>>>> diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py >>>>>> index 650b976223..d422e2a6bb 100644 >>>>>> --- a/scripts/lib/wic/ksparser.py >>>>>> +++ b/scripts/lib/wic/ksparser.py >>>>>> @@ -138,6 +138,7 @@ class KickStart(): >>>>>> part.add_argument('--align', type=int) >>>>>> part.add_argument('--exclude-path', nargs='+') >>>>>> part.add_argument('--include-path', nargs='+') >>>>>> + part.add_argument('--embed-rootfs', nargs='+') >>>>>> part.add_argument("--extra-space", type=sizetype) >>>>>> part.add_argument('--fsoptions', dest='fsopts') >>>>>> part.add_argument('--fstype', default='vfat', >>>>>> diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py >>>>>> index 2d95f78439..13857df82f 100644 >>>>>> --- a/scripts/lib/wic/partition.py >>>>>> +++ b/scripts/lib/wic/partition.py >>>>>> @@ -31,6 +31,7 @@ class Partition(): >>>>>> self.extra_space = args.extra_space >>>>>> self.exclude_path = args.exclude_path >>>>>> self.include_path = args.include_path >>>>>> + self.embed_rootfs = args.embed_rootfs >>>>>> self.fsopts = args.fsopts >>>>>> self.fstype = args.fstype >>>>>> self.label = args.label >>>>>> diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py >>>>>> index 40419a64b3..16359601dc 100644 >>>>>> --- a/scripts/lib/wic/plugins/source/rootfs.py >>>>>> +++ b/scripts/lib/wic/plugins/source/rootfs.py >>>>>> @@ -80,7 +80,7 @@ class RootfsPlugin(SourcePlugin): >>>>>> >>>>>> new_rootfs = None >>>>>> # Handle excluded paths. >>>>>> - if part.exclude_path or part.include_path: >>>>>> + if part.exclude_path or part.include_path or part.embed_rootfs: >>>>>> # We need a new rootfs directory we can delete files from. Copy to >>>>>> # workdir. >>>>>> new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) >>>>>> @@ -100,6 +100,24 @@ class RootfsPlugin(SourcePlugin): >>>>>> for path in part.include_path or []: >>>>>> copyhardlinktree(path, new_rootfs) >>>>>> >>>>>> + for embed in part.embed_rootfs or []: >>>>>> + [embed_rootfs, path] = embed.split(":") >>>>>> + #we need to remove the initial / for os.path.join to work >>>>>> + if os.path.isabs(path): >>>>>> + path = path[1:] >>>>>> + if embed_rootfs in krootfs_dir: >>>>>> + embed_rootfs = krootfs_dir[embed_rootfs] >>>>>> + embed_rootfs = cls.__get_rootfs_dir(embed_rootfs) >>>>>> + tar_file = os.path.realpath(os.path.join(cr_workdir, "aux.tar")) >>>>>> + tar_cmd = "%s tar cpf %s -C %s ." % (cls.__get_pseudo(native_sysroot, >>>>>> + embed_rootfs), tar_file, embed_rootfs) >>>>>> + exec_native_cmd(tar_cmd, native_sysroot) >>>>>> + untar_cmd = "%s tar xf %s -C %s ." % (cls.__get_pseudo(native_sysroot, new_rootfs), >>>>>> + tar_file, os.path.join(new_rootfs, path)) >>>>>> + exec_native_cmd(untar_cmd, native_sysroot, >>>>>> + cls.__get_pseudo(native_sysroot, new_rootfs)) >>>>>> + os.remove(tar_file) >>>>>> + >>>>> Why are you using an intermediate tar file here? >>>> For the permissions :( >>> Your previous patch handled this using 'pseudo -B'. It would be good to be >>> consistent if possible - either use an intermediate tar file in all cases if >>> it's necessary or use 'pseudo -B' in all cases. >> Unfortunately, you cannot use pseudo -B in this case. >> >> You use pseudo -B when you move one pseudo-aware partition to >> somewhere else. But now we are merging two pseudo-aware partitions. >> >> Also I would not use the intermediate tar on the first place because >> it is a waste of space + cpu resources. Copyhardlink is the right >> tool >> >> >>> -- >>> Paul Barker >>> Konsulko Group >> >> >> -- >> Ricardo Ribalda > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-03-04 8:34 [PATCH 1/2] wic: Fix permissions when using exclude or include path Ricardo Ribalda Delgado 2020-03-04 8:34 ` [PATCH 2/2] wic: Add --embed-rootfs argument Ricardo Ribalda Delgado @ 2020-03-04 9:53 ` Paul Barker 2020-03-04 10:02 ` Ricardo Ribalda Delgado 1 sibling, 1 reply; 18+ messages in thread From: Paul Barker @ 2020-03-04 9:53 UTC (permalink / raw) To: Ricardo Ribalda Delgado; +Cc: openembedded-core On Wed, 4 Mar 2020 09:34:37 +0100 Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > When parameters include_path or exclude_path are passed to the rootfs > plugin, it will copy the partition content into a folder and make all > the modifications there. > > This is done using copyhardlinktree(), which does not take into > consideration the content of the pseudo folder, which contains the > information about the right permissions and ownership of the folders. How are you running wic here? In the do_image_wic task it's executed under pseudo so all this is handled already. Executing wic outside of bitbake may need some more testing here. > > This results in a rootfs owned by the user that is running the wic > command (usually UID 1000), which makes some rootfs unbootable. > > To fix this we copy the content of the pseudo folders to the new folder > and modify the pseudo database using the "pseudo -B" command. > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > --- > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > index 705aeb5563..40419a64b3 100644 > --- a/scripts/lib/wic/plugins/source/rootfs.py > +++ b/scripts/lib/wic/plugins/source/rootfs.py > @@ -16,11 +16,11 @@ import os > import shutil > import sys > > -from oe.path import copyhardlinktree > +from oe.path import copyhardlinktree, copytree > > from wic import WicError > from wic.pluginbase import SourcePlugin > -from wic.misc import get_bitbake_var > +from wic.misc import get_bitbake_var, exec_native_cmd > > logger = logging.getLogger('wic') > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > return os.path.realpath(image_rootfs_dir) > > + @staticmethod > + def __get_pseudo(native_sysroot, rootfs): > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > + return pseudo > + > @classmethod > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > oe_builddir, bootimg_dir, kernel_dir, > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > if os.path.lexists(new_rootfs): > shutil.rmtree(os.path.join(new_rootfs)) > - > copyhardlinktree(part.rootfs_dir, new_rootfs) > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > + os.path.join(new_rootfs, "../pseudo")) I don't like stepping up the directory tree like this. We should be more explicit with the paths. > + pseudo_cmd = "%s -B -m %s -M %s" % (cls.__get_pseudo(native_sysroot,new_rootfs), > + part.rootfs_dir, new_rootfs) > + exec_native_cmd(pseudo_cmd, native_sysroot) > + > for path in part.include_path or []: > copyhardlinktree(path, new_rootfs) ^^^^^^^^^^^^^^^^ If this is the right approach I imagine you would also need to fix things up with pseudo after the copyhardlinktree call above. -- Paul Barker Konsulko Group ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-03-04 9:53 ` [PATCH 1/2] wic: Fix permissions when using exclude or include path Paul Barker @ 2020-03-04 10:02 ` Ricardo Ribalda Delgado 2020-03-05 9:28 ` Paul Barker 0 siblings, 1 reply; 18+ messages in thread From: Ricardo Ribalda Delgado @ 2020-03-04 10:02 UTC (permalink / raw) To: Paul Barker; +Cc: openembedded-core Hi Paul On Wed, Mar 4, 2020 at 10:53 AM Paul Barker <pbarker@konsulko.com> wrote: > > On Wed, 4 Mar 2020 09:34:37 +0100 > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > When parameters include_path or exclude_path are passed to the rootfs > > plugin, it will copy the partition content into a folder and make all > > the modifications there. > > > > This is done using copyhardlinktree(), which does not take into > > consideration the content of the pseudo folder, which contains the > > information about the right permissions and ownership of the folders. > > How are you running wic here? In the do_image_wic task it's executed under > pseudo so all this is handled already. Executing wic outside of bitbake may > need some more testing here. I am running wic outside bitbake. But even if it is run under bitbake, it should also fail. The pseudo directory needs to be present on the target image > > > > > This results in a rootfs owned by the user that is running the wic > > command (usually UID 1000), which makes some rootfs unbootable. > > > > To fix this we copy the content of the pseudo folders to the new folder > > and modify the pseudo database using the "pseudo -B" command. > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > --- > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > index 705aeb5563..40419a64b3 100644 > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > @@ -16,11 +16,11 @@ import os > > import shutil > > import sys > > > > -from oe.path import copyhardlinktree > > +from oe.path import copyhardlinktree, copytree > > > > from wic import WicError > > from wic.pluginbase import SourcePlugin > > -from wic.misc import get_bitbake_var > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > logger = logging.getLogger('wic') > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > return os.path.realpath(image_rootfs_dir) > > > > + @staticmethod > > + def __get_pseudo(native_sysroot, rootfs): > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > + return pseudo > > + > > @classmethod > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > oe_builddir, bootimg_dir, kernel_dir, > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > if os.path.lexists(new_rootfs): > > shutil.rmtree(os.path.join(new_rootfs)) > > - > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > > + os.path.join(new_rootfs, "../pseudo")) > > I don't like stepping up the directory tree like this. We should be more > explicit with the paths. You are thinking on: os.path.dirname(directory) > > > + pseudo_cmd = "%s -B -m %s -M %s" % (cls.__get_pseudo(native_sysroot,new_rootfs), > > + part.rootfs_dir, new_rootfs) > > + exec_native_cmd(pseudo_cmd, native_sysroot) > > + > > for path in part.include_path or []: > > copyhardlinktree(path, new_rootfs) > > ^^^^^^^^^^^^^^^^ > > If this is the right approach I imagine you would also need to fix things up > with pseudo after the copyhardlinktree call above. I do not think it is needed. include_path does not contain its own pseudo directory > > -- > Paul Barker > Konsulko Group -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-03-04 10:02 ` Ricardo Ribalda Delgado @ 2020-03-05 9:28 ` Paul Barker 2020-03-05 9:46 ` Ricardo Ribalda Delgado 0 siblings, 1 reply; 18+ messages in thread From: Paul Barker @ 2020-03-05 9:28 UTC (permalink / raw) To: Ricardo Ribalda Delgado; +Cc: openembedded-core On Wed, 4 Mar 2020 11:02:47 +0100 Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > Hi Paul > > On Wed, Mar 4, 2020 at 10:53 AM Paul Barker <pbarker@konsulko.com> wrote: > > > > On Wed, 4 Mar 2020 09:34:37 +0100 > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > When parameters include_path or exclude_path are passed to the rootfs > > > plugin, it will copy the partition content into a folder and make all > > > the modifications there. > > > > > > This is done using copyhardlinktree(), which does not take into > > > consideration the content of the pseudo folder, which contains the > > > information about the right permissions and ownership of the folders. > > > > How are you running wic here? In the do_image_wic task it's executed under > > pseudo so all this is handled already. Executing wic outside of bitbake may > > need some more testing here. > > I am running wic outside bitbake. But even if it is run under bitbake, > it should also fail. The pseudo directory needs to be present on the > target image If you're running wic outside of bitbake, is there any guarantee that pseudo is available? > > > > > > > > > This results in a rootfs owned by the user that is running the wic > > > command (usually UID 1000), which makes some rootfs unbootable. > > > > > > To fix this we copy the content of the pseudo folders to the new folder > > > and modify the pseudo database using the "pseudo -B" command. > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > --- > > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > index 705aeb5563..40419a64b3 100644 > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > @@ -16,11 +16,11 @@ import os > > > import shutil > > > import sys > > > > > > -from oe.path import copyhardlinktree > > > +from oe.path import copyhardlinktree, copytree > > > > > > from wic import WicError > > > from wic.pluginbase import SourcePlugin > > > -from wic.misc import get_bitbake_var > > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > > > logger = logging.getLogger('wic') > > > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > > > return os.path.realpath(image_rootfs_dir) > > > > > > + @staticmethod > > > + def __get_pseudo(native_sysroot, rootfs): > > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > > + return pseudo > > > + > > > @classmethod > > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > > oe_builddir, bootimg_dir, kernel_dir, > > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > > > if os.path.lexists(new_rootfs): > > > shutil.rmtree(os.path.join(new_rootfs)) > > > - > > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > > > + os.path.join(new_rootfs, "../pseudo")) > > > > I don't like stepping up the directory tree like this. We should be more > > explicit with the paths. > > You are thinking on: > os.path.dirname(directory) No, I'm wondering why we're taking a step up the directory tree from `part.rootfs_dir`. I can point that at any path using the `--rootfs-dir` argument and there's no guarantee that ../pseudo exists or is relevant to the path I gave to `--rootfs-dir`. > > > > > > + pseudo_cmd = "%s -B -m %s -M %s" % (cls.__get_pseudo(native_sysroot,new_rootfs), > > > + part.rootfs_dir, new_rootfs) > > > + exec_native_cmd(pseudo_cmd, native_sysroot) > > > + > > > for path in part.include_path or []: > > > copyhardlinktree(path, new_rootfs) > > > > ^^^^^^^^^^^^^^^^ > > > > If this is the right approach I imagine you would also need to fix things up > > with pseudo after the copyhardlinktree call above. > > I do not think it is needed. include_path does not contain its own > pseudo directory That's not necessarily true. The include_path could have been built by Yocto using pseudo. I can see that there is an issue using these arguments to wic outside of bitbake but I'm not sure this is the correct solution. -- Paul Barker Konsulko Group ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-03-05 9:28 ` Paul Barker @ 2020-03-05 9:46 ` Ricardo Ribalda Delgado 2020-04-03 19:53 ` [OE-core] " Ricardo Ribalda 0 siblings, 1 reply; 18+ messages in thread From: Ricardo Ribalda Delgado @ 2020-03-05 9:46 UTC (permalink / raw) To: Paul Barker; +Cc: openembedded-core Hi Paul On Thu, Mar 5, 2020 at 10:32 AM Paul Barker <pbarker@konsulko.com> wrote: > > On Wed, 4 Mar 2020 11:02:47 +0100 > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > Hi Paul > > > > On Wed, Mar 4, 2020 at 10:53 AM Paul Barker <pbarker@konsulko.com> wrote: > > > > > > On Wed, 4 Mar 2020 09:34:37 +0100 > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > When parameters include_path or exclude_path are passed to the rootfs > > > > plugin, it will copy the partition content into a folder and make all > > > > the modifications there. > > > > > > > > This is done using copyhardlinktree(), which does not take into > > > > consideration the content of the pseudo folder, which contains the > > > > information about the right permissions and ownership of the folders. > > > > > > How are you running wic here? In the do_image_wic task it's executed under > > > pseudo so all this is handled already. Executing wic outside of bitbake may > > > need some more testing here. > > > > I am running wic outside bitbake. But even if it is run under bitbake, > > it should also fail. The pseudo directory needs to be present on the > > target image > > If you're running wic outside of bitbake, is there any guarantee that pseudo > is available? Yes, the same guarantee that the ext3_tools are available. So I believe we are safe here. Actually in my docker pseudo is not installed and when I invoke with wic, everything is fine. > > > > > > > > > > > > > > This results in a rootfs owned by the user that is running the wic > > > > command (usually UID 1000), which makes some rootfs unbootable. > > > > > > > > To fix this we copy the content of the pseudo folders to the new folder > > > > and modify the pseudo database using the "pseudo -B" command. > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > > --- > > > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > > index 705aeb5563..40419a64b3 100644 > > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > > @@ -16,11 +16,11 @@ import os > > > > import shutil > > > > import sys > > > > > > > > -from oe.path import copyhardlinktree > > > > +from oe.path import copyhardlinktree, copytree > > > > > > > > from wic import WicError > > > > from wic.pluginbase import SourcePlugin > > > > -from wic.misc import get_bitbake_var > > > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > > > > > logger = logging.getLogger('wic') > > > > > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > return os.path.realpath(image_rootfs_dir) > > > > > > > > + @staticmethod > > > > + def __get_pseudo(native_sysroot, rootfs): > > > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > > > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > > > + return pseudo > > > > + > > > > @classmethod > > > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > > > oe_builddir, bootimg_dir, kernel_dir, > > > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > if os.path.lexists(new_rootfs): > > > > shutil.rmtree(os.path.join(new_rootfs)) > > > > - > > > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > > > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > > > > + os.path.join(new_rootfs, "../pseudo")) > > > > > > I don't like stepping up the directory tree like this. We should be more > > > explicit with the paths. > > > > You are thinking on: > > os.path.dirname(directory) > > No, I'm wondering why we're taking a step up the directory tree from > `part.rootfs_dir`. I can point that at any path using the `--rootfs-dir` > argument and there's no guarantee that ../pseudo exists or is relevant to the > path I gave to `--rootfs-dir`. Because we are asuming that the rootfs is being generated with OE/yocto, and there is where the pseudo folder lives. It is the same asumption part.prepare_rootfs() is taking. > > > > > > > > > > + pseudo_cmd = "%s -B -m %s -M %s" % (cls.__get_pseudo(native_sysroot,new_rootfs), > > > > + part.rootfs_dir, new_rootfs) > > > > + exec_native_cmd(pseudo_cmd, native_sysroot) > > > > + > > > > for path in part.include_path or []: > > > > copyhardlinktree(path, new_rootfs) > > > > > > ^^^^^^^^^^^^^^^^ > > > > > > If this is the right approach I imagine you would also need to fix things up > > > with pseudo after the copyhardlinktree call above. > > > > I do not think it is needed. include_path does not contain its own > > pseudo directory > > That's not necessarily true. The include_path could have been built by Yocto > using pseudo. Then you need to provide where the pseudo folder is. Eg --include_path folder/A/B/C/D/file Where is the pseudo database? in folder/A/B/C/D/pseudo, folder/A/B/C/pseudo , folder/A/B/C/pseudo /... > > I can see that there is an issue using these arguments to wic outside of > bitbake but I'm not sure this is the correct solution. > > -- > Paul Barker > Konsulko Group -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [OE-core] [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-03-05 9:46 ` Ricardo Ribalda Delgado @ 2020-04-03 19:53 ` Ricardo Ribalda 2020-04-07 18:12 ` Paul Barker 0 siblings, 1 reply; 18+ messages in thread From: Ricardo Ribalda @ 2020-04-03 19:53 UTC (permalink / raw) To: Paul Barker, Richard Purdie; +Cc: openembedded-core ping? On Thu, Mar 5, 2020 at 10:46 AM Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > Hi Paul > > On Thu, Mar 5, 2020 at 10:32 AM Paul Barker <pbarker@konsulko.com> wrote: > > > > On Wed, 4 Mar 2020 11:02:47 +0100 > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > Hi Paul > > > > > > On Wed, Mar 4, 2020 at 10:53 AM Paul Barker <pbarker@konsulko.com> wrote: > > > > > > > > On Wed, 4 Mar 2020 09:34:37 +0100 > > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > > > When parameters include_path or exclude_path are passed to the rootfs > > > > > plugin, it will copy the partition content into a folder and make all > > > > > the modifications there. > > > > > > > > > > This is done using copyhardlinktree(), which does not take into > > > > > consideration the content of the pseudo folder, which contains the > > > > > information about the right permissions and ownership of the folders. > > > > > > > > How are you running wic here? In the do_image_wic task it's executed under > > > > pseudo so all this is handled already. Executing wic outside of bitbake may > > > > need some more testing here. > > > > > > I am running wic outside bitbake. But even if it is run under bitbake, > > > it should also fail. The pseudo directory needs to be present on the > > > target image > > > > If you're running wic outside of bitbake, is there any guarantee that pseudo > > is available? > > Yes, the same guarantee that the ext3_tools are available. So I > believe we are safe here. Actually in my docker pseudo is not > installed and when I invoke with wic, everything is fine. > > > > > > > > > > > > > > > > > > > > This results in a rootfs owned by the user that is running the wic > > > > > command (usually UID 1000), which makes some rootfs unbootable. > > > > > > > > > > To fix this we copy the content of the pseudo folders to the new folder > > > > > and modify the pseudo database using the "pseudo -B" command. > > > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > > > --- > > > > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > > > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > > > index 705aeb5563..40419a64b3 100644 > > > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > > > @@ -16,11 +16,11 @@ import os > > > > > import shutil > > > > > import sys > > > > > > > > > > -from oe.path import copyhardlinktree > > > > > +from oe.path import copyhardlinktree, copytree > > > > > > > > > > from wic import WicError > > > > > from wic.pluginbase import SourcePlugin > > > > > -from wic.misc import get_bitbake_var > > > > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > > > > > > > logger = logging.getLogger('wic') > > > > > > > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > > > return os.path.realpath(image_rootfs_dir) > > > > > > > > > > + @staticmethod > > > > > + def __get_pseudo(native_sysroot, rootfs): > > > > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > > > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > > > > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > > > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > > > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > > > > + return pseudo > > > > > + > > > > > @classmethod > > > > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > > > > oe_builddir, bootimg_dir, kernel_dir, > > > > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > > > if os.path.lexists(new_rootfs): > > > > > shutil.rmtree(os.path.join(new_rootfs)) > > > > > - > > > > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > > > > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > > > > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > > > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > > > > > + os.path.join(new_rootfs, "../pseudo")) > > > > > > > > I don't like stepping up the directory tree like this. We should be more > > > > explicit with the paths. > > > > > > You are thinking on: > > > os.path.dirname(directory) > > > > No, I'm wondering why we're taking a step up the directory tree from > > `part.rootfs_dir`. I can point that at any path using the `--rootfs-dir` > > argument and there's no guarantee that ../pseudo exists or is relevant to the > > path I gave to `--rootfs-dir`. > > Because we are asuming that the rootfs is being generated with > OE/yocto, and there is where the pseudo folder lives. > It is the same asumption part.prepare_rootfs() is taking. > > > > > > > > > > > > > > > + pseudo_cmd = "%s -B -m %s -M %s" % (cls.__get_pseudo(native_sysroot,new_rootfs), > > > > > + part.rootfs_dir, new_rootfs) > > > > > + exec_native_cmd(pseudo_cmd, native_sysroot) > > > > > + > > > > > for path in part.include_path or []: > > > > > copyhardlinktree(path, new_rootfs) > > > > > > > > ^^^^^^^^^^^^^^^^ > > > > > > > > If this is the right approach I imagine you would also need to fix things up > > > > with pseudo after the copyhardlinktree call above. > > > > > > I do not think it is needed. include_path does not contain its own > > > pseudo directory > > > > That's not necessarily true. The include_path could have been built by Yocto > > using pseudo. > > Then you need to provide where the pseudo folder is. Eg > > --include_path folder/A/B/C/D/file > > Where is the pseudo database? in folder/A/B/C/D/pseudo, > folder/A/B/C/pseudo , folder/A/B/C/pseudo /... > > > > > > > I can see that there is an issue using these arguments to wic outside of > > bitbake but I'm not sure this is the correct solution. > > > > -- > > Paul Barker > > Konsulko Group > > > > -- > Ricardo Ribalda -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [OE-core] [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-04-03 19:53 ` [OE-core] " Ricardo Ribalda @ 2020-04-07 18:12 ` Paul Barker 2020-04-07 18:40 ` Ricardo Ribalda 0 siblings, 1 reply; 18+ messages in thread From: Paul Barker @ 2020-04-07 18:12 UTC (permalink / raw) To: Ricardo Ribalda Delgado; +Cc: Richard Purdie, openembedded-core On Fri, 3 Apr 2020 21:53:39 +0200 Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > ping? I think that '../pseudo' should not be used here. I'll explain inline... > > > > This results in a rootfs owned by the user that is running the wic > > command (usually UID 1000), which makes some rootfs unbootable. > > > > To fix this we copy the content of the pseudo folders to the new folder > > and modify the pseudo database using the "pseudo -B" command. > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > --- > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > index 705aeb5563..40419a64b3 100644 > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > @@ -16,11 +16,11 @@ import os > > import shutil > > import sys > > > > -from oe.path import copyhardlinktree > > +from oe.path import copyhardlinktree, copytree > > > > from wic import WicError > > from wic.pluginbase import SourcePlugin > > -from wic.misc import get_bitbake_var > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > logger = logging.getLogger('wic') > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > return os.path.realpath(image_rootfs_dir) > > > > + @staticmethod > > + def __get_pseudo(native_sysroot, rootfs): > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > + return pseudo > > + > > @classmethod > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > oe_builddir, bootimg_dir, kernel_dir, > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > if os.path.lexists(new_rootfs): > > shutil.rmtree(os.path.join(new_rootfs)) > > - > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): new_rootfs is set by the following statement a few lines above: new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) Consider that `cr_workdir` may contain multiple rootfs staging directories corresponding to multiple lines in the wks file, for example if a rootfs image is duplicated into multiple partitions for redundancy. In that case `os.path.join(new_rootfs, "../pseudo")` will clash between these different rootfs copies. Let's use an explicit path instead, such as: new_pseudo = os.path.realpath(os.path.join(cr_workdir, "pseudo%d" % part.lineno)) > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), part.rootfs_dir is whatever is given as the option to `--rootfs-dir`. There is no guarantee that `../psuedo` is valid or if it corresponds to the rootfs directory given. It's unsafe to step up the directory tree and make assumptions like this. > > + os.path.join(new_rootfs, "../pseudo")) > > + pseudo_cmd = "%s -B -m %s -M %s" % (cls.__get_pseudo(native_sysroot,new_rootfs), > > + part.rootfs_dir, new_rootfs) > > + exec_native_cmd(pseudo_cmd, native_sysroot) > > + > > for path in part.include_path or []: > > copyhardlinktree(path, new_rootfs) -- Paul Barker Konsulko Group ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [OE-core] [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-04-07 18:12 ` Paul Barker @ 2020-04-07 18:40 ` Ricardo Ribalda 2020-04-07 19:02 ` Paul Barker 0 siblings, 1 reply; 18+ messages in thread From: Ricardo Ribalda @ 2020-04-07 18:40 UTC (permalink / raw) To: Paul Barker; +Cc: Richard Purdie, openembedded-core Hi Paul Thanks for your review, It has been already merged, but if there is something wrong we can send a patch fixing it. https://git.openembedded.org/openembedded-core/commit/?id=36993eea89d1c011397b7692b9b8d61b499d0171 On Tue, Apr 7, 2020 at 8:13 PM Paul Barker <pbarker@konsulko.com> wrote: > > On Fri, 3 Apr 2020 21:53:39 +0200 > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > ping? > > I think that '../pseudo' should not be used here. I'll explain inline... > > > > > > > This results in a rootfs owned by the user that is running the wic > > > command (usually UID 1000), which makes some rootfs unbootable. > > > > > > To fix this we copy the content of the pseudo folders to the new folder > > > and modify the pseudo database using the "pseudo -B" command. > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > --- > > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > index 705aeb5563..40419a64b3 100644 > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > @@ -16,11 +16,11 @@ import os > > > import shutil > > > import sys > > > > > > -from oe.path import copyhardlinktree > > > +from oe.path import copyhardlinktree, copytree > > > > > > from wic import WicError > > > from wic.pluginbase import SourcePlugin > > > -from wic.misc import get_bitbake_var > > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > > > logger = logging.getLogger('wic') > > > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > > > return os.path.realpath(image_rootfs_dir) > > > > > > + @staticmethod > > > + def __get_pseudo(native_sysroot, rootfs): > > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > > + return pseudo > > > + > > > @classmethod > > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > > oe_builddir, bootimg_dir, kernel_dir, > > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > > > if os.path.lexists(new_rootfs): > > > shutil.rmtree(os.path.join(new_rootfs)) > > > - > > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > > new_rootfs is set by the following statement a few lines above: > > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) > > Consider that `cr_workdir` may contain multiple rootfs staging directories > corresponding to multiple lines in the wks file, for example if a rootfs > image is duplicated into multiple partitions for redundancy. In that case > `os.path.join(new_rootfs, "../pseudo")` will clash between these different > rootfs copies. > > Let's use an explicit path instead, such as: > > new_pseudo = os.path.realpath(os.path.join(cr_workdir, "pseudo%d" % part.lineno)) The reason to have that path was to follow the same structure as the real image.bb. If there are multiple partitions on the .wic file the different partitions are done one by one, not in parallel. So ../pseudo will be created for partition1 then it will be used to generate the partition1 ../pseudo will be deleted ../pseudo will be created for partition2 Even if they use the same partition, the code works (and ../pseudo is useless once the partition is generated) > > > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > > part.rootfs_dir is whatever is given as the option to `--rootfs-dir`. There > is no guarantee that `../psuedo` is valid or if it corresponds to the rootfs > directory given. It's unsafe to step up the directory tree and make > assumptions like this. I think that if we do not pass a real rootfs to the rootfs plugin it is an error from the user. We can add a more beautiful error message instead of a backtrace. Or if you believe that it is a valid usecase to not pass a rootfs then we can continue with a warning/debug message and explicitly telling the user that the permissions are going to be invalid, because what he is using as a roofs is an unknow directory for bitbake. I have no personal preference for any of the two, tell me what do you prefer (or a different option) and I will implement it. Thans again for the review. > > > > + os.path.join(new_rootfs, "../pseudo")) > > > + pseudo_cmd = "%s -B -m %s -M %s" % (cls.__get_pseudo(native_sysroot,new_rootfs), > > > + part.rootfs_dir, new_rootfs) > > > + exec_native_cmd(pseudo_cmd, native_sysroot) > > > + > > > for path in part.include_path or []: > > > copyhardlinktree(path, new_rootfs) > > -- > Paul Barker > Konsulko Group > -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [OE-core] [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-04-07 18:40 ` Ricardo Ribalda @ 2020-04-07 19:02 ` Paul Barker 2020-04-07 19:19 ` Ricardo Ribalda 0 siblings, 1 reply; 18+ messages in thread From: Paul Barker @ 2020-04-07 19:02 UTC (permalink / raw) To: Ricardo Ribalda Delgado; +Cc: Richard Purdie, openembedded-core On Tue, 7 Apr 2020 20:40:18 +0200 Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > Hi Paul > > Thanks for your review, It has been already merged, but if there is > something wrong we can send a patch fixing it. > > https://git.openembedded.org/openembedded-core/commit/?id=36993eea89d1c011397b7692b9b8d61b499d0171 > > On Tue, Apr 7, 2020 at 8:13 PM Paul Barker <pbarker@konsulko.com> wrote: > > > > On Fri, 3 Apr 2020 21:53:39 +0200 > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > ping? > > > > I think that '../pseudo' should not be used here. I'll explain inline... > > > > > > > > > > This results in a rootfs owned by the user that is running the wic > > > > command (usually UID 1000), which makes some rootfs unbootable. > > > > > > > > To fix this we copy the content of the pseudo folders to the new folder > > > > and modify the pseudo database using the "pseudo -B" command. > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > > --- > > > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > > index 705aeb5563..40419a64b3 100644 > > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > > @@ -16,11 +16,11 @@ import os > > > > import shutil > > > > import sys > > > > > > > > -from oe.path import copyhardlinktree > > > > +from oe.path import copyhardlinktree, copytree > > > > > > > > from wic import WicError > > > > from wic.pluginbase import SourcePlugin > > > > -from wic.misc import get_bitbake_var > > > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > > > > > logger = logging.getLogger('wic') > > > > > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > return os.path.realpath(image_rootfs_dir) > > > > > > > > + @staticmethod > > > > + def __get_pseudo(native_sysroot, rootfs): > > > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > > > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > > > + return pseudo > > > > + > > > > @classmethod > > > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > > > oe_builddir, bootimg_dir, kernel_dir, > > > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > if os.path.lexists(new_rootfs): > > > > shutil.rmtree(os.path.join(new_rootfs)) > > > > - > > > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > > > > new_rootfs is set by the following statement a few lines above: > > > > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) > > > > Consider that `cr_workdir` may contain multiple rootfs staging directories > > corresponding to multiple lines in the wks file, for example if a rootfs > > image is duplicated into multiple partitions for redundancy. In that case > > `os.path.join(new_rootfs, "../pseudo")` will clash between these different > > rootfs copies. > > > > Let's use an explicit path instead, such as: > > > > new_pseudo = os.path.realpath(os.path.join(cr_workdir, "pseudo%d" % part.lineno)) > > The reason to have that path was to follow the same structure as the > real image.bb. > > If there are multiple partitions on the .wic file the different > partitions are done one by one, not > in parallel. > > So > ../pseudo will be created for partition1 > then it will be used to generate the partition1 > > ../pseudo will be deleted > ../pseudo will be created for partition2 > > Even if they use the same partition, the code works (and ../pseudo is > useless once the partition is generated) > Having these separate is important for debugging though, it lets you look through the different copies after wic exits if something is wrong. > > > > > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > > > > part.rootfs_dir is whatever is given as the option to `--rootfs-dir`. There > > is no guarantee that `../psuedo` is valid or if it corresponds to the rootfs > > directory given. It's unsafe to step up the directory tree and make > > assumptions like this. > > I think that if we do not pass a real rootfs to the rootfs plugin it > is an error from the user. > > We can add a more beautiful error message instead of a backtrace. > > Or if you believe that it is a valid usecase to not pass a rootfs then > we can continue with a warning/debug message and explicitly telling > the user that the permissions are going to be invalid, because what he > is using as a roofs is an unknow directory for bitbake. This is a valid and existing usecase. This is how data partitions are populated and how you separate /home or another directory into its own partiton (e.g. https://stackoverflow.com/questions/56187209/yocto-create-and-populate-a-separate-home-partition). > > I have no personal preference for any of the two, tell me what do you > prefer (or a different option) and I will implement it. > > Thans again for the review. > This patch needs reverting from master/dunfell. I hope it hasn't gone into the M4 build... -- Paul Barker Konsulko Group ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [OE-core] [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-04-07 19:02 ` Paul Barker @ 2020-04-07 19:19 ` Ricardo Ribalda 2020-04-07 19:43 ` Paul Barker 0 siblings, 1 reply; 18+ messages in thread From: Ricardo Ribalda @ 2020-04-07 19:19 UTC (permalink / raw) To: Paul Barker; +Cc: Richard Purdie, openembedded-core Hi On Tue, Apr 7, 2020 at 9:02 PM Paul Barker <pbarker@konsulko.com> wrote: > > On Tue, 7 Apr 2020 20:40:18 +0200 > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > Hi Paul > > > > Thanks for your review, It has been already merged, but if there is > > something wrong we can send a patch fixing it. > > > > https://git.openembedded.org/openembedded-core/commit/?id=36993eea89d1c011397b7692b9b8d61b499d0171 > > > > On Tue, Apr 7, 2020 at 8:13 PM Paul Barker <pbarker@konsulko.com> wrote: > > > > > > On Fri, 3 Apr 2020 21:53:39 +0200 > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > ping? > > > > > > I think that '../pseudo' should not be used here. I'll explain inline... > > > > > > > > > > > > > This results in a rootfs owned by the user that is running the wic > > > > > command (usually UID 1000), which makes some rootfs unbootable. > > > > > > > > > > To fix this we copy the content of the pseudo folders to the new folder > > > > > and modify the pseudo database using the "pseudo -B" command. > > > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > > > --- > > > > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > > > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > > > index 705aeb5563..40419a64b3 100644 > > > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > > > @@ -16,11 +16,11 @@ import os > > > > > import shutil > > > > > import sys > > > > > > > > > > -from oe.path import copyhardlinktree > > > > > +from oe.path import copyhardlinktree, copytree > > > > > > > > > > from wic import WicError > > > > > from wic.pluginbase import SourcePlugin > > > > > -from wic.misc import get_bitbake_var > > > > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > > > > > > > logger = logging.getLogger('wic') > > > > > > > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > > > return os.path.realpath(image_rootfs_dir) > > > > > > > > > > + @staticmethod > > > > > + def __get_pseudo(native_sysroot, rootfs): > > > > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > > > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > > > > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > > > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > > > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > > > > + return pseudo > > > > > + > > > > > @classmethod > > > > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > > > > oe_builddir, bootimg_dir, kernel_dir, > > > > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > > > if os.path.lexists(new_rootfs): > > > > > shutil.rmtree(os.path.join(new_rootfs)) > > > > > - > > > > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > > > > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > > > > > > new_rootfs is set by the following statement a few lines above: > > > > > > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) > > > > > > Consider that `cr_workdir` may contain multiple rootfs staging directories > > > corresponding to multiple lines in the wks file, for example if a rootfs > > > image is duplicated into multiple partitions for redundancy. In that case > > > `os.path.join(new_rootfs, "../pseudo")` will clash between these different > > > rootfs copies. > > > > > > Let's use an explicit path instead, such as: > > > > > > new_pseudo = os.path.realpath(os.path.join(cr_workdir, "pseudo%d" % part.lineno)) > > > > The reason to have that path was to follow the same structure as the > > real image.bb. > > > > If there are multiple partitions on the .wic file the different > > partitions are done one by one, not > > in parallel. > > > > So > > ../pseudo will be created for partition1 > > then it will be used to generate the partition1 > > > > ../pseudo will be deleted > > ../pseudo will be created for partition2 > > > > Even if they use the same partition, the code works (and ../pseudo is > > useless once the partition is generated) > > > > Having these separate is important for debugging though, it lets you look > through the different copies after wic exits if something is wrong. I see your point, I can make a patch for that > > > > > > > > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > > > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > > > > > > part.rootfs_dir is whatever is given as the option to `--rootfs-dir`. There > > > is no guarantee that `../psuedo` is valid or if it corresponds to the rootfs > > > directory given. It's unsafe to step up the directory tree and make > > > assumptions like this. > > > > I think that if we do not pass a real rootfs to the rootfs plugin it > > is an error from the user. > > > > We can add a more beautiful error message instead of a backtrace. > > > > Or if you believe that it is a valid usecase to not pass a rootfs then > > we can continue with a warning/debug message and explicitly telling > > the user that the permissions are going to be invalid, because what he > > is using as a roofs is an unknow directory for bitbake. > > This is a valid and existing usecase. This is how data partitions are > populated and how you separate /home or another directory into its own > partiton (e.g. > https://stackoverflow.com/questions/56187209/yocto-create-and-populate-a-separate-home-partition). Luckily on the usecase on that stackoverflow the patch will still work. we need to combine partition split + embed-roots/include_path/exlude_path to make it fail By the way, by looking at your example I think that all the partition split executed from outside bitbake will have invalid partitions on their content. We need to make something like: part /home --source rootfs --root-dir=/home --ondisk sda --fstype=ext4 --label home > > > > > I have no personal preference for any of the two, tell me what do you > > prefer (or a different option) and I will implement it. > > > > Thans again for the review. > > > > This patch needs reverting from master/dunfell. I hope it hasn't gone into > the M4 build... Instead of reverting I believe it is simpler to prepare a patch that solves both of your concerns - pseduoX instead of ../pseudo (although this does not break the code) - keep the old behaviour if the pseudo folder is not found (wrong permissions, but no backtrace) And we can start talking about a way to split partitions and keep the right permissions/username Best regards. > > -- > Paul Barker > Konsulko Group > -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [OE-core] [PATCH 1/2] wic: Fix permissions when using exclude or include path 2020-04-07 19:19 ` Ricardo Ribalda @ 2020-04-07 19:43 ` Paul Barker 0 siblings, 0 replies; 18+ messages in thread From: Paul Barker @ 2020-04-07 19:43 UTC (permalink / raw) To: Ricardo Ribalda; +Cc: Richard Purdie, openembedded-core On Tue, 7 Apr 2020 21:19:32 +0200 "Ricardo Ribalda" <ricardo.ribalda@gmail.com> wrote: > Hi > > On Tue, Apr 7, 2020 at 9:02 PM Paul Barker <pbarker@konsulko.com> wrote: > > > > On Tue, 7 Apr 2020 20:40:18 +0200 > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > Hi Paul > > > > > > Thanks for your review, It has been already merged, but if there is > > > something wrong we can send a patch fixing it. > > > > > > https://git.openembedded.org/openembedded-core/commit/?id=36993eea89d1c011397b7692b9b8d61b499d0171 > > > > > > On Tue, Apr 7, 2020 at 8:13 PM Paul Barker <pbarker@konsulko.com> wrote: > > > > > > > > On Fri, 3 Apr 2020 21:53:39 +0200 > > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > > > ping? > > > > > > > > I think that '../pseudo' should not be used here. I'll explain inline... > > > > > > > > > > > > > > > > This results in a rootfs owned by the user that is running the wic > > > > > > command (usually UID 1000), which makes some rootfs unbootable. > > > > > > > > > > > > To fix this we copy the content of the pseudo folders to the new folder > > > > > > and modify the pseudo database using the "pseudo -B" command. > > > > > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > > > > --- > > > > > > scripts/lib/wic/plugins/source/rootfs.py | 22 +++++++++++++++++++--- > > > > > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py > > > > > > index 705aeb5563..40419a64b3 100644 > > > > > > --- a/scripts/lib/wic/plugins/source/rootfs.py > > > > > > +++ b/scripts/lib/wic/plugins/source/rootfs.py > > > > > > @@ -16,11 +16,11 @@ import os > > > > > > import shutil > > > > > > import sys > > > > > > > > > > > > -from oe.path import copyhardlinktree > > > > > > +from oe.path import copyhardlinktree, copytree > > > > > > > > > > > > from wic import WicError > > > > > > from wic.pluginbase import SourcePlugin > > > > > > -from wic.misc import get_bitbake_var > > > > > > +from wic.misc import get_bitbake_var, exec_native_cmd > > > > > > > > > > > > logger = logging.getLogger('wic') > > > > > > > > > > > > @@ -44,6 +44,15 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > > > > > return os.path.realpath(image_rootfs_dir) > > > > > > > > > > > > + @staticmethod > > > > > > + def __get_pseudo(native_sysroot, rootfs): > > > > > > + pseudo = "export PSEUDO_PREFIX=%s/usr;" % native_sysroot > > > > > > + pseudo += "export PSEUDO_LOCALSTATEDIR=%s;" % os.path.join(rootfs, "../pseudo") > > > > > > + pseudo += "export PSEUDO_PASSWD=%s;" % rootfs > > > > > > + pseudo += "export PSEUDO_NOSYMLINKEXP=1;" > > > > > > + pseudo += "%s " % get_bitbake_var("FAKEROOTCMD") > > > > > > + return pseudo > > > > > > + > > > > > > @classmethod > > > > > > def do_prepare_partition(cls, part, source_params, cr, cr_workdir, > > > > > > oe_builddir, bootimg_dir, kernel_dir, > > > > > > @@ -78,9 +87,16 @@ class RootfsPlugin(SourcePlugin): > > > > > > > > > > > > if os.path.lexists(new_rootfs): > > > > > > shutil.rmtree(os.path.join(new_rootfs)) > > > > > > - > > > > > > copyhardlinktree(part.rootfs_dir, new_rootfs) > > > > > > > > > > > > + if os.path.lexists(os.path.join(new_rootfs, "../pseudo")): > > > > > > > > new_rootfs is set by the following statement a few lines above: > > > > > > > > new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno)) > > > > > > > > Consider that `cr_workdir` may contain multiple rootfs staging directories > > > > corresponding to multiple lines in the wks file, for example if a rootfs > > > > image is duplicated into multiple partitions for redundancy. In that case > > > > `os.path.join(new_rootfs, "../pseudo")` will clash between these different > > > > rootfs copies. > > > > > > > > Let's use an explicit path instead, such as: > > > > > > > > new_pseudo = os.path.realpath(os.path.join(cr_workdir, "pseudo%d" % part.lineno)) > > > > > > The reason to have that path was to follow the same structure as the > > > real image.bb. > > > > > > If there are multiple partitions on the .wic file the different > > > partitions are done one by one, not > > > in parallel. > > > > > > So > > > ../pseudo will be created for partition1 > > > then it will be used to generate the partition1 > > > > > > ../pseudo will be deleted > > > ../pseudo will be created for partition2 > > > > > > Even if they use the same partition, the code works (and ../pseudo is > > > useless once the partition is generated) > > > > > > > Having these separate is important for debugging though, it lets you look > > through the different copies after wic exits if something is wrong. > > I see your point, I can make a patch for that > > > > > > > > > > > > > + shutil.rmtree(os.path.join(new_rootfs, "../pseudo")) > > > > > > + copytree(os.path.join(part.rootfs_dir, "../pseudo"), > > > > > > > > part.rootfs_dir is whatever is given as the option to `--rootfs-dir`. There > > > > is no guarantee that `../psuedo` is valid or if it corresponds to the rootfs > > > > directory given. It's unsafe to step up the directory tree and make > > > > assumptions like this. > > > > > > I think that if we do not pass a real rootfs to the rootfs plugin it > > > is an error from the user. > > > > > > We can add a more beautiful error message instead of a backtrace. > > > > > > Or if you believe that it is a valid usecase to not pass a rootfs then > > > we can continue with a warning/debug message and explicitly telling > > > the user that the permissions are going to be invalid, because what he > > > is using as a roofs is an unknow directory for bitbake. > > > > This is a valid and existing usecase. This is how data partitions are > > populated and how you separate /home or another directory into its own > > partiton (e.g. > > https://stackoverflow.com/questions/56187209/yocto-create-and-populate-a-separate-home-partition). > > Luckily on the usecase on that stackoverflow the patch will still work. > > we need to combine partition split + > embed-roots/include_path/exlude_path to make it fail > > By the way, by looking at your example I think that all the partition > split executed from outside bitbake will have invalid partitions on > their content. > > We need to make something like: > > part /home --source rootfs --root-dir=/home --ondisk sda --fstype=ext4 > --label home > Agreed, there is an issue here when running wic outside bitbake that we need to solve. I think this patch is most of the way towards fixing it but does introduce other problems as I said. > > > > > > > > > I have no personal preference for any of the two, tell me what do you > > > prefer (or a different option) and I will implement it. > > > > > > Thans again for the review. > > > > > > > This patch needs reverting from master/dunfell. I hope it hasn't gone into > > the M4 build... > > Instead of reverting I believe it is simpler to prepare a patch that > solves both of your concerns > > - pseduoX instead of ../pseudo (although this does not break the code) > - keep the old behaviour if the pseudo folder is not found (wrong > permissions, but no backtrace) > > And we can start talking about a way to split partitions and keep the > right permissions/username > We can do that after the 3.1 release. We're supposed to be in feature freeze right now, this shouldn't have been merged so the best solution is to revert it. Thanks, -- Paul Barker Konsulko Group ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-04-07 19:47 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-04 8:34 [PATCH 1/2] wic: Fix permissions when using exclude or include path Ricardo Ribalda Delgado 2020-03-04 8:34 ` [PATCH 2/2] wic: Add --embed-rootfs argument Ricardo Ribalda Delgado 2020-03-04 9:42 ` Paul Barker 2020-03-04 9:56 ` Ricardo Ribalda Delgado 2020-03-04 10:08 ` Paul Barker 2020-03-04 10:14 ` Ricardo Ribalda Delgado 2020-03-04 13:49 ` Ricardo Ribalda Delgado 2020-03-04 14:31 ` Joshua Watt 2020-03-04 9:53 ` [PATCH 1/2] wic: Fix permissions when using exclude or include path Paul Barker 2020-03-04 10:02 ` Ricardo Ribalda Delgado 2020-03-05 9:28 ` Paul Barker 2020-03-05 9:46 ` Ricardo Ribalda Delgado 2020-04-03 19:53 ` [OE-core] " Ricardo Ribalda 2020-04-07 18:12 ` Paul Barker 2020-04-07 18:40 ` Ricardo Ribalda 2020-04-07 19:02 ` Paul Barker 2020-04-07 19:19 ` Ricardo Ribalda 2020-04-07 19:43 ` Paul Barker
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.