[PATCH v2 0/5] #11662 - wic should mount /boot

[PATCH v2 0/5] #11662 - wic should mount /boot

[Ed Bartosh]

Hi,

This patchset adds /boot to the /etc/fstab of root partition, making
it mounted on boot. It also fixes reporting and testing issues
caused by this change.

The patchset also fixes long standing bug: wic updated fstab
inplace in rootfs directory. This causes other tasks working with
rootfs directory to produce incorrect results or crash. This is
fixed by copying rootfs to the temporary directory before updating
fstab.

Changes in v2: squashed patches by reviewer's request

The following changes since commit be2f804a2ed7bb9b000fae14e75bdef43fd3eae6:

  wic: avoid unnecessary dependencies (2017-06-27 16:24:25 +0300)

are available in the git repository at:

  git://git.yoctoproject.org/poky-contrib ed/wip
  http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=ed/wip

Ed Bartosh (5):
  wic: copy rootfs directory before changing fstab
  wic: use absolute paths in rootfs plugin
  wic: rootfs: fix rootfs path reporting
  wic: rootfs: make copied rootfs unique
  wic: add /boot mount point to fstab by default

 meta/lib/oeqa/selftest/cases/wic.py      |  2 +-
 scripts/lib/wic/plugins/imager/direct.py | 31 ++++++++++++++++++++-----------
 scripts/lib/wic/plugins/source/rootfs.py | 16 +++++++---------
 3 files changed, 28 insertions(+), 21 deletions(-)

--
Regards,
Ed

[PATCH v2 1/5] wic: copy rootfs directory before changing fstab

[Ed Bartosh]

wic updates /etc/fstab on root partition if there are
valid mount points in .wks

When wic runs from bitbake this can cause incorrect results
or even breakage of other tasks working with rootfs directory
in parallel with do_image_wic.

Implemented copying rootfs directory to a temporary location
using copyhardlinktree before updating fstab to avoid conflicts with
other tasks working with rootfs.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 scripts/lib/wic/plugins/imager/direct.py | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
index aa9cc9f..f707365 100644
--- a/scripts/lib/wic/plugins/imager/direct.py
+++ b/scripts/lib/wic/plugins/imager/direct.py
@@ -32,6 +32,8 @@ import uuid
 
 from time import strftime
 
+from oe.path import copyhardlinktree
+
 from wic import WicError
 from wic.filemap import sparse_copy
 from wic.ksparser import KickStart, KickStartError
@@ -115,12 +117,16 @@ class DirectPlugin(ImagerPlugin):
             fstab_lines = fstab.readlines()
 
         if self._update_fstab(fstab_lines, self.parts):
-            shutil.copyfile(fstab_path, fstab_path + ".orig")
+            # copy rootfs dir to workdir to update fstab
+            # as rootfs can be used by other tasks and can't be modified
+            new_rootfs = os.path.realpath(os.path.join(self.workdir, "rootfs_copy"))
+            copyhardlinktree(image_rootfs, new_rootfs)
+            fstab_path = os.path.join(new_rootfs, 'etc/fstab')
 
             with open(fstab_path, "w") as fstab:
                 fstab.writelines(fstab_lines)
 
-            return fstab_path
+            return new_rootfs
 
     def _update_fstab(self, fstab_lines, parts):
         """Assume partition order same as in wks"""
@@ -156,7 +162,10 @@ class DirectPlugin(ImagerPlugin):
         filesystems from the artifacts directly and combine them into
         a partitioned image.
         """
-        fstab_path = self._write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
+        new_rootfs = self._write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
+        if new_rootfs:
+            # rootfs was copied to update fstab
+            self.rootfs_dir['ROOTFS_DIR'] = new_rootfs
 
         for part in self.parts:
             # get rootfs size from bitbake variable if it's not set in .ks file
@@ -172,12 +181,7 @@ class DirectPlugin(ImagerPlugin):
                     if rsize_bb:
                         part.size = int(round(float(rsize_bb)))
 
-        try:
-            self._image.prepare(self)
-        finally:
-            if fstab_path:
-                shutil.move(fstab_path + ".orig", fstab_path)
-
+        self._image.prepare(self)
         self._image.layout_partitions()
         self._image.create()
 
-- 
2.1.4

[PATCH v2 2/5] wic: use absolute paths in rootfs plugin

[Ed Bartosh]

Using relative paths can cause copyhardlinktree API to fail as
it changes current directory when working. Converted all paths
to absolute paths using os.path.realpath.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 scripts/lib/wic/plugins/source/rootfs.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py
index 4dc4cb8..c08f760 100644
--- a/scripts/lib/wic/plugins/source/rootfs.py
+++ b/scripts/lib/wic/plugins/source/rootfs.py
@@ -48,7 +48,7 @@ class RootfsPlugin(SourcePlugin):
     @staticmethod
     def __get_rootfs_dir(rootfs_dir):
         if os.path.isdir(rootfs_dir):
-            return rootfs_dir
+            return os.path.realpath(rootfs_dir)
 
         image_rootfs_dir = get_bitbake_var("IMAGE_ROOTFS", rootfs_dir)
         if not os.path.isdir(image_rootfs_dir):
@@ -56,7 +56,7 @@ class RootfsPlugin(SourcePlugin):
                            "named %s has been found at %s, exiting." %
                            (rootfs_dir, image_rootfs_dir))
 
-        return image_rootfs_dir
+        return os.path.realpath(image_rootfs_dir)
 
     @classmethod
     def do_prepare_partition(cls, part, source_params, cr, cr_workdir,
-- 
2.1.4

[PATCH v2 3/5] wic: rootfs: fix rootfs path reporting

[Ed Bartosh]

wic gets rootfs paths from partition object property
'rootfs_dir' and shows them in final report.

rootfs plugin sets this property to the temporary path,
which causes temporary paths appearing in the report.

wic includes rootfs path in the final report. When fstab needs to be
updated wic copies rootfs to a temporary location. This causes path
to the copy of rootfs to appear in the final report.

Changed the code to prevent storing temporary rootfs path
in part.rootfs_dir. This should fix the report.

Saved original rootfs path when rootfs is copied to include original
path to the report.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 scripts/lib/wic/plugins/imager/direct.py |  7 ++++++-
 scripts/lib/wic/plugins/source/rootfs.py | 10 ++++------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
index f707365..e90f9f9 100644
--- a/scripts/lib/wic/plugins/imager/direct.py
+++ b/scripts/lib/wic/plugins/imager/direct.py
@@ -62,6 +62,7 @@ class DirectPlugin(ImagerPlugin):
 
         # parse possible 'rootfs=name' items
         self.rootfs_dir = dict(rdir.split('=') for rdir in rootfs_dir.split(' '))
+        self.rootfs_dir_orig = None
         self.bootimg_dir = bootimg_dir
         self.kernel_dir = kernel_dir
         self.native_sysroot = native_sysroot
@@ -165,6 +166,7 @@ class DirectPlugin(ImagerPlugin):
         new_rootfs = self._write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
         if new_rootfs:
             # rootfs was copied to update fstab
+            self.rootfs_dir_orig = self.rootfs_dir['ROOTFS_DIR']
             self.rootfs_dir['ROOTFS_DIR'] = new_rootfs
 
         for part in self.parts:
@@ -234,11 +236,14 @@ class DirectPlugin(ImagerPlugin):
         for part in self.parts:
             if part.rootfs_dir is None:
                 continue
+            rootfs_dir = part.rootfs_dir
             if part.mountpoint == '/':
                 suffix = ':'
+                if self.rootfs_dir_orig:
+                    rootfs_dir = self.rootfs_dir_orig
             else:
                 suffix = '["%s"]:' % (part.mountpoint or part.label)
-            msg += '  ROOTFS_DIR%s%s\n' % (suffix.ljust(20), part.rootfs_dir)
+            msg += '  ROOTFS_DIR%s%s\n' % (suffix.ljust(20), rootfs_dir)
 
         msg += '  BOOTIMG_DIR:                  %s\n' % self.bootimg_dir
         msg += '  KERNEL_DIR:                   %s\n' % self.kernel_dir
diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py
index c08f760..e438158 100644
--- a/scripts/lib/wic/plugins/source/rootfs.py
+++ b/scripts/lib/wic/plugins/source/rootfs.py
@@ -81,8 +81,9 @@ class RootfsPlugin(SourcePlugin):
                 raise WicError("Couldn't find --rootfs-dir=%s connection or "
                                "it is not a valid path, exiting" % part.rootfs_dir)
 
-        real_rootfs_dir = cls.__get_rootfs_dir(rootfs_dir)
+        part.rootfs_dir = cls.__get_rootfs_dir(rootfs_dir)
 
+        new_rootfs = None
         # Handle excluded paths.
         if part.exclude_path is not None:
             # We need a new rootfs directory we can delete files from. Copy to
@@ -92,9 +93,7 @@ class RootfsPlugin(SourcePlugin):
             if os.path.lexists(new_rootfs):
                 shutil.rmtree(os.path.join(new_rootfs))
 
-            copyhardlinktree(real_rootfs_dir, new_rootfs)
-
-            real_rootfs_dir = new_rootfs
+            copyhardlinktree(part.rootfs_dir, new_rootfs)
 
             for orig_path in part.exclude_path:
                 path = orig_path
@@ -123,6 +122,5 @@ class RootfsPlugin(SourcePlugin):
                     # Delete whole directory.
                     shutil.rmtree(full_path)
 
-        part.rootfs_dir = real_rootfs_dir
         part.prepare_rootfs(cr_workdir, oe_builddir,
-                            real_rootfs_dir, native_sysroot)
+                            new_rootfs or part.rootfs_dir, native_sysroot)
-- 
2.1.4

[PATCH v2 4/5] wic: rootfs: make copied rootfs unique

[Ed Bartosh]

Used unique suffix (line number from .wks file) for the
copied rootfs directory to avoid possible conflicts.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 scripts/lib/wic/plugins/source/rootfs.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py
index e438158..aec720f 100644
--- a/scripts/lib/wic/plugins/source/rootfs.py
+++ b/scripts/lib/wic/plugins/source/rootfs.py
@@ -88,7 +88,7 @@ class RootfsPlugin(SourcePlugin):
         if part.exclude_path is not None:
             # 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"))
+            new_rootfs = os.path.realpath(os.path.join(cr_workdir, "rootfs%d" % part.lineno))
 
             if os.path.lexists(new_rootfs):
                 shutil.rmtree(os.path.join(new_rootfs))
-- 
2.1.4

[PATCH v2 5/5] wic: add /boot mount point to fstab by default

[Ed Bartosh]

wic avoided adding /boot to fstab for no reason.
This exception was hardcoded in the wic code.

There is no need for this as mountpoint in .wks file is an optional
field. It can be used only if user wants to have partitions
automatically mounted on system boot.

Fixed test_quemu test case failure caused by changed
amount of mounted partitions.

[YOCTO #11662]

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 meta/lib/oeqa/selftest/cases/wic.py      | 2 +-
 scripts/lib/wic/plugins/imager/direct.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/meta/lib/oeqa/selftest/cases/wic.py b/meta/lib/oeqa/selftest/cases/wic.py
index dd72db5..e27dbe9 100644
--- a/meta/lib/oeqa/selftest/cases/wic.py
+++ b/meta/lib/oeqa/selftest/cases/wic.py
@@ -622,7 +622,7 @@ part /etc --source rootfs --ondisk mmcblk0 --fstype=ext4 --exclude-path bin/ --r
             cmd = "mount |grep '^/dev/' | cut -f1,3 -d ' '"
             status, output = qemu.run_serial(cmd)
             self.assertEqual(1, status, 'Failed to run command "%s": %s' % (cmd, output))
-            self.assertEqual(output, '/dev/root /\r\n/dev/sda3 /mnt')
+            self.assertEqual(output, '/dev/root /\r\n/dev/sda1 /boot\r\n/dev/sda3 /mnt')
 
     @only_for_arch(['i586', 'i686', 'x86_64'])
     @OETestID(1852)
diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
index e90f9f9..f6f29aa 100644
--- a/scripts/lib/wic/plugins/imager/direct.py
+++ b/scripts/lib/wic/plugins/imager/direct.py
@@ -134,7 +134,7 @@ class DirectPlugin(ImagerPlugin):
         updated = False
         for part in parts:
             if not part.realnum or not part.mountpoint \
-               or part.mountpoint in ("/", "/boot"):
+               or part.mountpoint == "/":
                 continue
 
             # mmc device partitions are named mmcblk0p1, mmcblk0p2..
-- 
2.1.4

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Otavio Salvador]

On Tue, Jun 27, 2017 at 10:28 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> The patchset also fixes long standing bug: wic updated fstab
> inplace in rootfs directory. This causes other tasks working with
> rootfs directory to produce incorrect results or crash. This is
> fixed by copying rootfs to the temporary directory before updating
> fstab.

As you is working on this, please also include Fabio's patch on the
patchset. It includes a command like option to disable fstab change at
all. For delta-based updates this is imperative.

Fabio, could you point him the last patch revision?

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Fabio Berton]

The last patch I sent is here: 
https://patchwork.openembedded.org/patch/139252/

We're using this patch internally with Pyro branch. I can rework to 
apply on master.

On 06/27/2017 05:35 PM, Otavio Salvador wrote:
> On Tue, Jun 27, 2017 at 10:28 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
>> The patchset also fixes long standing bug: wic updated fstab
>> inplace in rootfs directory. This causes other tasks working with
>> rootfs directory to produce incorrect results or crash. This is
>> fixed by copying rootfs to the temporary directory before updating
>> fstab.
> 
> As you is working on this, please also include Fabio's patch on the
> patchset. It includes a command like option to disable fstab change at
> all. For delta-based updates this is imperative.
> 
> Fabio, could you point him the last patch revision?
> 

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Ed Bartosh]

On Tue, Jun 27, 2017 at 05:41:45PM -0300, Fabio Berton wrote:
> The last patch I sent is here:
> https://patchwork.openembedded.org/patch/139252/
> 
> We're using this patch internally with Pyro branch. I can rework to
> apply on master.
> 
> On 06/27/2017 05:35 PM, Otavio Salvador wrote:
> >On Tue, Jun 27, 2017 at 10:28 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> >>The patchset also fixes long standing bug: wic updated fstab
> >>inplace in rootfs directory. This causes other tasks working with
> >>rootfs directory to produce incorrect results or crash. This is
> >>fixed by copying rootfs to the temporary directory before updating
> >>fstab.
> >
> >As you is working on this, please also include Fabio's patch on the
> >patchset. It includes a command like option to disable fstab change at
> >all. For delta-based updates this is imperative.
> >
> >Fabio, could you point him the last patch revision?
> >

Do we really need that?

JFYI: Mount point in .wks is an optional field. It makes sense to use it only
if partition needs to be mounted on boot. fstab will not be updated
unless it's explicitly requested by specifying mount points in .wks

--
Regards,
Ed

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Otavio Salvador]

On Wed, Jun 28, 2017 at 4:31 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> On Tue, Jun 27, 2017 at 05:41:45PM -0300, Fabio Berton wrote:
>> The last patch I sent is here:
>> https://patchwork.openembedded.org/patch/139252/
>>
>> We're using this patch internally with Pyro branch. I can rework to
>> apply on master.
>>
>> On 06/27/2017 05:35 PM, Otavio Salvador wrote:
>> >On Tue, Jun 27, 2017 at 10:28 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
>> >>The patchset also fixes long standing bug: wic updated fstab
>> >>inplace in rootfs directory. This causes other tasks working with
>> >>rootfs directory to produce incorrect results or crash. This is
>> >>fixed by copying rootfs to the temporary directory before updating
>> >>fstab.
>> >
>> >As you is working on this, please also include Fabio's patch on the
>> >patchset. It includes a command like option to disable fstab change at
>> >all. For delta-based updates this is imperative.
>> >
>> >Fabio, could you point him the last patch revision?
>> >
>
> Do we really need that?
>
> JFYI: Mount point in .wks is an optional field. It makes sense to use it only
> if partition needs to be mounted on boot. fstab will not be updated
> unless it's explicitly requested by specifying mount points in .wks

It should have support to not touch it. For images which we intend to
do delta updates, the content cannot be changed besides the original
rootfs generation. So yes, we need that.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Ed Bartosh]

On Wed, Jun 28, 2017 at 10:32:27AM -0300, Otavio Salvador wrote:
> On Wed, Jun 28, 2017 at 4:31 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> > On Tue, Jun 27, 2017 at 05:41:45PM -0300, Fabio Berton wrote:
> >> The last patch I sent is here:
> >> https://patchwork.openembedded.org/patch/139252/
> >>
> >> We're using this patch internally with Pyro branch. I can rework to
> >> apply on master.
> >>
> >> On 06/27/2017 05:35 PM, Otavio Salvador wrote:
> >> >On Tue, Jun 27, 2017 at 10:28 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> >> >>The patchset also fixes long standing bug: wic updated fstab
> >> >>inplace in rootfs directory. This causes other tasks working with
> >> >>rootfs directory to produce incorrect results or crash. This is
> >> >>fixed by copying rootfs to the temporary directory before updating
> >> >>fstab.
> >> >
> >> >As you is working on this, please also include Fabio's patch on the
> >> >patchset. It includes a command like option to disable fstab change at
> >> >all. For delta-based updates this is imperative.
> >> >
> >> >Fabio, could you point him the last patch revision?
> >> >
> >
> > Do we really need that?
> >
> > JFYI: Mount point in .wks is an optional field. It makes sense to use it only
> > if partition needs to be mounted on boot. fstab will not be updated
> > unless it's explicitly requested by specifying mount points in .wks
> 
> It should have support to not touch it. For images which we intend to
> do delta updates, the content cannot be changed besides the original
> rootfs generation. So yes, we need that.

I'm not sure I understand this. If you don't want fstab to be changed
you should not specify mount points in .wks
There is only one reason to have mount points in .wks: to make wic to
change /etc/fstab, which you apparently don't want. So, don't specify
mount points and you'll have what you want.

Having additional option for this looks redundand to me.

--
Regards,
Ed

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Ed Bartosh]

On Thu, Jun 29, 2017 at 11:39:42AM +0300, Ed Bartosh wrote:
> On Wed, Jun 28, 2017 at 10:32:27AM -0300, Otavio Salvador wrote:
> > On Wed, Jun 28, 2017 at 4:31 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> > > On Tue, Jun 27, 2017 at 05:41:45PM -0300, Fabio Berton wrote:
> > >> The last patch I sent is here:
> > >> https://patchwork.openembedded.org/patch/139252/
> > >>
> > >> We're using this patch internally with Pyro branch. I can rework to
> > >> apply on master.
> > >>
> > >> On 06/27/2017 05:35 PM, Otavio Salvador wrote:
> > >> >On Tue, Jun 27, 2017 at 10:28 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> > >> >>The patchset also fixes long standing bug: wic updated fstab
> > >> >>inplace in rootfs directory. This causes other tasks working with
> > >> >>rootfs directory to produce incorrect results or crash. This is
> > >> >>fixed by copying rootfs to the temporary directory before updating
> > >> >>fstab.
> > >> >
> > >> >As you is working on this, please also include Fabio's patch on the
> > >> >patchset. It includes a command like option to disable fstab change at
> > >> >all. For delta-based updates this is imperative.
> > >> >
> > >> >Fabio, could you point him the last patch revision?
> > >> >
> > >
> > > Do we really need that?
> > >
> > > JFYI: Mount point in .wks is an optional field. It makes sense to use it only
> > > if partition needs to be mounted on boot. fstab will not be updated
> > > unless it's explicitly requested by specifying mount points in .wks
> > 
> > It should have support to not touch it. For images which we intend to
> > do delta updates, the content cannot be changed besides the original
> > rootfs generation. So yes, we need that.
> 
> I'm not sure I understand this. If you don't want fstab to be changed
> you should not specify mount points in .wks
> There is only one reason to have mount points in .wks: to make wic to
> change /etc/fstab, which you apparently don't want. So, don't specify
> mount points and you'll have what you want.
> 
> Having additional option for this looks redundand to me.

After thinking a bit more about it I'd propose to have global wic option
to avoid rootfs content changes. Not just fstab updates, but any
changes. For now this option (--no-rootfs-update ?) should prevent creating
images if either mount points are specified or --exclude-path is used in .wks
In future if any other rootfs changing functionality is added to wic it must
conflict with this option.

Does this make sense?

--
Regards,
Ed

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Patrick Ohly]

On Fri, 2017-06-30 at 11:37 +0300, Ed Bartosh wrote:
> > I'm not sure I understand this. If you don't want fstab to be
> changed
> > you should not specify mount points in .wks
> > There is only one reason to have mount points in .wks: to make wic
> to
> > change /etc/fstab, which you apparently don't want. So, don't
> specify
> > mount points and you'll have what you want.
> > 
> > Having additional option for this looks redundand to me.
> 
> After thinking a bit more about it I'd propose to have global wic
> option
> to avoid rootfs content changes. Not just fstab updates, but any
> changes. For now this option (--no-rootfs-update ?) should prevent
> creating
> images if either mount points are specified or --exclude-path is used
> in .wks

Why does --exclude-path conflict with --no-rootfs-update? Is that a
conceptual problem or an implementation problem?

If I'm not mistaken, --exclude-path merely means "take this rootfs, but
exclude certain parts". That's in line with --no-rootfs-update == "do
not modify the content of the rootfs", as it just helps with choosing
where content goes (the "single rootfs" -> "different partitions"
approach).

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Ed Bartosh]

On Fri, Jun 30, 2017 at 11:02:13AM +0200, Patrick Ohly wrote:
> On Fri, 2017-06-30 at 11:37 +0300, Ed Bartosh wrote:
> > > I'm not sure I understand this. If you don't want fstab to be
> > changed
> > > you should not specify mount points in .wks
> > > There is only one reason to have mount points in .wks: to make wic
> > to
> > > change /etc/fstab, which you apparently don't want. So, don't
> > specify
> > > mount points and you'll have what you want.
> > > 
> > > Having additional option for this looks redundand to me.
> > 
> > After thinking a bit more about it I'd propose to have global wic
> > option
> > to avoid rootfs content changes. Not just fstab updates, but any
> > changes. For now this option (--no-rootfs-update ?) should prevent
> > creating
> > images if either mount points are specified or --exclude-path is used
> > in .wks
> 
> Why does --exclude-path conflict with --no-rootfs-update? Is that a
> conceptual problem or an implementation problem?
>

I thought that removing directories from original rootfs is a
modification.

> If I'm not mistaken, --exclude-path merely means "take this rootfs, but
> exclude certain parts". That's in line with --no-rootfs-update == "do
> not modify the content of the rootfs", as it just helps with choosing
> where content goes (the "single rootfs" -> "different partitions"
> approach).
That's questionable statement, but I agree it makes sense in some cases.
If nobody objects I'm ok with this. Let's assume that removing part of
the content is not a modifiation :)

--
Regards,
Ed

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Patrick Ohly]

On Fri, 2017-06-30 at 15:23 +0300, Ed Bartosh wrote:
> On Fri, Jun 30, 2017 at 11:02:13AM +0200, Patrick Ohly wrote:
> > On Fri, 2017-06-30 at 11:37 +0300, Ed Bartosh wrote:
> > > > I'm not sure I understand this. If you don't want fstab to be
> > > changed
> > > > you should not specify mount points in .wks
> > > > There is only one reason to have mount points in .wks: to make wic
> > > to
> > > > change /etc/fstab, which you apparently don't want. So, don't
> > > specify
> > > > mount points and you'll have what you want.
> > > > 
> > > > Having additional option for this looks redundand to me.
> > > 
> > > After thinking a bit more about it I'd propose to have global wic
> > > option
> > > to avoid rootfs content changes. Not just fstab updates, but any
> > > changes. For now this option (--no-rootfs-update ?) should prevent
> > > creating
> > > images if either mount points are specified or --exclude-path is used
> > > in .wks
> > 
> > Why does --exclude-path conflict with --no-rootfs-update? Is that a
> > conceptual problem or an implementation problem?
> >
> 
> I thought that removing directories from original rootfs is a
> modification.

But it's not actually removed from the original roofs directory, right?
For me, "not modified" refers to that and the files in it.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Otavio Salvador]

On Fri, Jun 30, 2017 at 10:16 AM, Patrick Ohly <patrick.ohly@intel.com> wrote:
> On Fri, 2017-06-30 at 15:23 +0300, Ed Bartosh wrote:
>> On Fri, Jun 30, 2017 at 11:02:13AM +0200, Patrick Ohly wrote:
>> > On Fri, 2017-06-30 at 11:37 +0300, Ed Bartosh wrote:
>> > > > I'm not sure I understand this. If you don't want fstab to be
>> > > changed
>> > > > you should not specify mount points in .wks
>> > > > There is only one reason to have mount points in .wks: to make wic
>> > > to
>> > > > change /etc/fstab, which you apparently don't want. So, don't
>> > > specify
>> > > > mount points and you'll have what you want.
>> > > >
>> > > > Having additional option for this looks redundand to me.
>> > >
>> > > After thinking a bit more about it I'd propose to have global wic
>> > > option
>> > > to avoid rootfs content changes. Not just fstab updates, but any
>> > > changes. For now this option (--no-rootfs-update ?) should prevent
>> > > creating
>> > > images if either mount points are specified or --exclude-path is used
>> > > in .wks
>> >
>> > Why does --exclude-path conflict with --no-rootfs-update? Is that a
>> > conceptual problem or an implementation problem?
>> >
>>
>> I thought that removing directories from original rootfs is a
>> modification.
>
> But it's not actually removed from the original roofs directory, right?
> For me, "not modified" refers to that and the files in it.

My problem is with the fstab change. If I explicitly ask wic to drop
something I know it is doing it so it is under my control.

Adding --no-fstab-change solves in an elegant way my problem.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Ed Bartosh]

On Fri, Jun 30, 2017 at 10:58:27AM -0300, Otavio Salvador wrote:
> On Fri, Jun 30, 2017 at 10:16 AM, Patrick Ohly <patrick.ohly@intel.com> wrote:
> > On Fri, 2017-06-30 at 15:23 +0300, Ed Bartosh wrote:
> >> On Fri, Jun 30, 2017 at 11:02:13AM +0200, Patrick Ohly wrote:
> >> > On Fri, 2017-06-30 at 11:37 +0300, Ed Bartosh wrote:
> >> > > > I'm not sure I understand this. If you don't want fstab to be
> >> > > changed
> >> > > > you should not specify mount points in .wks
> >> > > > There is only one reason to have mount points in .wks: to make wic
> >> > > to
> >> > > > change /etc/fstab, which you apparently don't want. So, don't
> >> > > specify
> >> > > > mount points and you'll have what you want.
> >> > > >
> >> > > > Having additional option for this looks redundand to me.
> >> > >
> >> > > After thinking a bit more about it I'd propose to have global wic
> >> > > option
> >> > > to avoid rootfs content changes. Not just fstab updates, but any
> >> > > changes. For now this option (--no-rootfs-update ?) should prevent
> >> > > creating
> >> > > images if either mount points are specified or --exclude-path is used
> >> > > in .wks
> >> >
> >> > Why does --exclude-path conflict with --no-rootfs-update? Is that a
> >> > conceptual problem or an implementation problem?
> >> >
> >>
> >> I thought that removing directories from original rootfs is a
> >> modification.
> >
> > But it's not actually removed from the original roofs directory, right?
> > For me, "not modified" refers to that and the files in it.
> 
> My problem is with the fstab change. If I explicitly ask wic to drop
> something I know it is doing it so it is under my control.
> 
> Adding --no-fstab-change solves in an elegant way my problem.

What if wic at some point will modify rootfs for one or another
reason? We'd need to introduce --no-hosts-change --no-exports-change
--no-whatever-is-changed-change etc. It doesn't look too elegant to me to be
honest. Adding mount points to .wks and then disabling fstab update
(which is the only purpose and outcome of having mount points in .wks)
doesn't look good neither.

Thoughts?

--
Regards,
Ed

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Ed Bartosh]

On Fri, Jun 30, 2017 at 03:16:33PM +0200, Patrick Ohly wrote:
> On Fri, 2017-06-30 at 15:23 +0300, Ed Bartosh wrote:
> > On Fri, Jun 30, 2017 at 11:02:13AM +0200, Patrick Ohly wrote:
> > > On Fri, 2017-06-30 at 11:37 +0300, Ed Bartosh wrote:
> > > > > I'm not sure I understand this. If you don't want fstab to be
> > > > changed
> > > > > you should not specify mount points in .wks
> > > > > There is only one reason to have mount points in .wks: to make wic
> > > > to
> > > > > change /etc/fstab, which you apparently don't want. So, don't
> > > > specify
> > > > > mount points and you'll have what you want.
> > > > > 
> > > > > Having additional option for this looks redundand to me.
> > > > 
> > > > After thinking a bit more about it I'd propose to have global wic
> > > > option
> > > > to avoid rootfs content changes. Not just fstab updates, but any
> > > > changes. For now this option (--no-rootfs-update ?) should prevent
> > > > creating
> > > > images if either mount points are specified or --exclude-path is used
> > > > in .wks
> > > 
> > > Why does --exclude-path conflict with --no-rootfs-update? Is that a
> > > conceptual problem or an implementation problem?
> > >
> > 
> > I thought that removing directories from original rootfs is a
> > modification.
> 
> But it's not actually removed from the original roofs directory, right?
> For me, "not modified" refers to that and the files in it.

We're talking about different things here, I guess. For me if target
rootfs content in the image is different from rootfs produced by bitbake
it means it's modified.

BTW, if this patchset is accepted wic will not be changing fstab in original rootfs anymore:
http://lists.openembedded.org/pipermail/openembedded-core/2017-June/138803.html
However, it would not mean that rootfs is not changed.

--
Regards,
Ed

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Otavio Salvador]

On Fri, Jun 30, 2017 at 12:44 PM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> We're talking about different things here, I guess. For me if target
> rootfs content in the image is different from rootfs produced by bitbake
> it means it's modified.
>
> BTW, if this patchset is accepted wic will not be changing fstab in original rootfs anymore:
> http://lists.openembedded.org/pipermail/openembedded-core/2017-June/138803.html
> However, it would not mean that rootfs is not changed.

Honestly it is quite confusing to me that wic changes fstab content.
It is confusing as you add something on base-files for fstab and it
may, or not, be what is used.

As I mentioned before, our needs is to have a way to forbid this
behavior and we use standard wks files specially when doing
development so an option makes it easy to adopt one way or another in
a distro for example.

Other possible rootfs changes also should be possible to be disabled
but IMO it should be per-feature (one for fstab, one for exclude,
...).

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Patrick Ohly]

On Fri, 2017-06-30 at 14:33 -0300, Otavio Salvador wrote:
> Other possible rootfs changes also should be possible to be disabled
> but IMO it should be per-feature (one for fstab, one for exclude,
> ...).

I also think it should be per-feature, it necessary at all.

I still do not fully understand under which circumstances wic modifies
the rootfs. If that happens only when explicitly requested in the wks
file as Ed said, then I don't see a need for any additional flags. Just
don't use the features which result in a rootfs modification.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Ed Bartosh]

On Fri, Jun 30, 2017 at 08:34:30PM +0200, Patrick Ohly wrote:
> On Fri, 2017-06-30 at 14:33 -0300, Otavio Salvador wrote:
> > Other possible rootfs changes also should be possible to be disabled
> > but IMO it should be per-feature (one for fstab, one for exclude,
> > ...).
> 
> I also think it should be per-feature, it necessary at all.
> 
> I still do not fully understand under which circumstances wic modifies
> the rootfs. If that happens only when explicitly requested in the wks
> file as Ed said, then I don't see a need for any additional flags.

Yes, that happens only if it explicitly requested, i.e. if there are valid
mount points in .wks file

> then I don't see a need for any additional flags. Just
> don't use the features which result in a rootfs modification.

I also didn't see it till last message from Otavio. Now I do - they
don't want to change .wks files. They're using standard wks from
scripts/lib/wic/canned-wks or from standard layers and they don't want
to duplicate them when they don't want rootfs modifications.

It could be a valid reason to have --no-fstab-update option I think.
However, I'm still not 100% convinced I'm ok with this if nobody else
objects.

--
Regards,
Ed

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Patrick Ohly]

On Mon, 2017-07-03 at 10:31 +0300, Ed Bartosh wrote:
> On Fri, Jun 30, 2017 at 08:34:30PM +0200, Patrick Ohly wrote:
> > then I don't see a need for any additional flags. Just
> > don't use the features which result in a rootfs modification.
> 
> I also didn't see it till last message from Otavio. Now I do - they
> don't want to change .wks files. They're using standard wks from
> scripts/lib/wic/canned-wks or from standard layers and they don't want
> to duplicate them when they don't want rootfs modifications.
> 
> It could be a valid reason to have --no-fstab-update option I think.
> However, I'm still not 100% convinced I'm ok with this if nobody else
> objects.

Okay, now I see what the purpose is.

I prefer a --no-fstab-update over a general --no-rootfs-update because
for each case where wic would normally modify the rootfs, some other
mechanism must be in place which makes that modification redundant (like
using PARTUUID). Having separate parameters forces the developers to
think about it. Just my 2 cents...

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH v2 0/5] #11662 - wic should mount /boot

[Ed Bartosh]

On Mon, Jul 03, 2017 at 09:53:32AM +0200, Patrick Ohly wrote:
> On Mon, 2017-07-03 at 10:31 +0300, Ed Bartosh wrote:
> > On Fri, Jun 30, 2017 at 08:34:30PM +0200, Patrick Ohly wrote:
> > > then I don't see a need for any additional flags. Just
> > > don't use the features which result in a rootfs modification.
> > 
> > I also didn't see it till last message from Otavio. Now I do - they
> > don't want to change .wks files. They're using standard wks from
> > scripts/lib/wic/canned-wks or from standard layers and they don't want
> > to duplicate them when they don't want rootfs modifications.
> > 
> > It could be a valid reason to have --no-fstab-update option I think.
> > However, I'm still not 100% convinced I'm ok with this if nobody else
> > objects.
> 
> Okay, now I see what the purpose is.
> 
> I prefer a --no-fstab-update over a general --no-rootfs-update because
> for each case where wic would normally modify the rootfs, some other
> mechanism must be in place which makes that modification redundant (like
> using PARTUUID). Having separate parameters forces the developers to
> think about it. Just my 2 cents...
>

Tha makes sense to me.

From other point of view if the goal is to have rootfs unmodified
--no-rootfs-update would make it easier to achive. Moreover it will
guarantee that rootfs is unmodified even if wic introduces new
functionality that modifies rootfs.

--
Regards,
Ed

Re: [PATCH v2 1/5] wic: copy rootfs directory before changing fstab

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 3853 bytes --]

Bad news: the failure this was meant to fix happened again.

| tar: ./opt/ltp/testcases/bin/dup01: file changed as we read it
| ERROR: Function failed: do_image_tar (log file is located at
/home/pokybuild/yocto-autobuilder/yocto-worker/nightly-arm/build/build/tmp/work/beaglebone-poky-linux-gnueabi/core-image-sato-sdk-ptest/1.0-r0/temp/log.do_image_tar.5535)

As before, image_wic was also running.

Is there any chance this could still be wic, or should we look at the jffs2
image generator?

Ross

On 27 June 2017 at 14:28, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
>
> wic updates /etc/fstab on root partition if there are
> valid mount points in .wks
>
> When wic runs from bitbake this can cause incorrect results
> or even breakage of other tasks working with rootfs directory
> in parallel with do_image_wic.
>
> Implemented copying rootfs directory to a temporary location
> using copyhardlinktree before updating fstab to avoid conflicts with
> other tasks working with rootfs.
>
> Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
> ---
>  scripts/lib/wic/plugins/imager/direct.py | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/lib/wic/plugins/imager/direct.py
b/scripts/lib/wic/plugins/imager/direct.py
> index aa9cc9f..f707365 100644
> --- a/scripts/lib/wic/plugins/imager/direct.py
> +++ b/scripts/lib/wic/plugins/imager/direct.py
> @@ -32,6 +32,8 @@ import uuid
>
>  from time import strftime
>
> +from oe.path import copyhardlinktree
> +
>  from wic import WicError
>  from wic.filemap import sparse_copy
>  from wic.ksparser import KickStart, KickStartError
> @@ -115,12 +117,16 @@ class DirectPlugin(ImagerPlugin):
>              fstab_lines = fstab.readlines()
>
>          if self._update_fstab(fstab_lines, self.parts):
> -            shutil.copyfile(fstab_path, fstab_path + ".orig")
> +            # copy rootfs dir to workdir to update fstab
> +            # as rootfs can be used by other tasks and can't be modified
> +            new_rootfs = os.path.realpath(os.path.join(self.workdir,
"rootfs_copy"))
> +            copyhardlinktree(image_rootfs, new_rootfs)
> +            fstab_path = os.path.join(new_rootfs, 'etc/fstab')
>
>              with open(fstab_path, "w") as fstab:
>                  fstab.writelines(fstab_lines)
>
> -            return fstab_path
> +            return new_rootfs
>
>      def _update_fstab(self, fstab_lines, parts):
>          """Assume partition order same as in wks"""
> @@ -156,7 +162,10 @@ class DirectPlugin(ImagerPlugin):
>          filesystems from the artifacts directly and combine them into
>          a partitioned image.
>          """
> -        fstab_path = self._write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
> +        new_rootfs = self._write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
> +        if new_rootfs:
> +            # rootfs was copied to update fstab
> +            self.rootfs_dir['ROOTFS_DIR'] = new_rootfs
>
>          for part in self.parts:
>              # get rootfs size from bitbake variable if it's not set in
.ks file
> @@ -172,12 +181,7 @@ class DirectPlugin(ImagerPlugin):
>                      if rsize_bb:
>                          part.size = int(round(float(rsize_bb)))
>
> -        try:
> -            self._image.prepare(self)
> -        finally:
> -            if fstab_path:
> -                shutil.move(fstab_path + ".orig", fstab_path)
> -
> +        self._image.prepare(self)
>          self._image.layout_partitions()
>          self._image.create()
>
> --
> 2.1.4
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

[-- Attachment #2: Type: text/html, Size: 4934 bytes --]

Re: [PATCH v2 1/5] wic: copy rootfs directory before changing fstab

[Ed Bartosh]

On Tue, Jul 04, 2017 at 11:16:23PM +0100, Burton, Ross wrote:
> Bad news: the failure this was meant to fix happened again.
> 
> | tar: ./opt/ltp/testcases/bin/dup01: file changed as we read it
> | ERROR: Function failed: do_image_tar (log file is located at
> /home/pokybuild/yocto-autobuilder/yocto-worker/nightly-arm/build/build/tmp/work/beaglebone-poky-linux-gnueabi/core-image-sato-sdk-ptest/1.0-r0/temp/log.do_image_tar.5535)
> 
> As before, image_wic was also running.
> 
> Is there any chance this could still be wic, or should we look at the jffs2
> image generator?
> 

There is always a chance. Which commit is this? I'll try to reproduce this.

Regards,
Ed


> Ross
> 
> On 27 June 2017 at 14:28, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> >
> > wic updates /etc/fstab on root partition if there are
> > valid mount points in .wks
> >
> > When wic runs from bitbake this can cause incorrect results
> > or even breakage of other tasks working with rootfs directory
> > in parallel with do_image_wic.
> >
> > Implemented copying rootfs directory to a temporary location
> > using copyhardlinktree before updating fstab to avoid conflicts with
> > other tasks working with rootfs.
> >
> > Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
> > ---
> >  scripts/lib/wic/plugins/imager/direct.py | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/scripts/lib/wic/plugins/imager/direct.py
> b/scripts/lib/wic/plugins/imager/direct.py
> > index aa9cc9f..f707365 100644
> > --- a/scripts/lib/wic/plugins/imager/direct.py
> > +++ b/scripts/lib/wic/plugins/imager/direct.py
> > @@ -32,6 +32,8 @@ import uuid
> >
> >  from time import strftime
> >
> > +from oe.path import copyhardlinktree
> > +
> >  from wic import WicError
> >  from wic.filemap import sparse_copy
> >  from wic.ksparser import KickStart, KickStartError
> > @@ -115,12 +117,16 @@ class DirectPlugin(ImagerPlugin):
> >              fstab_lines = fstab.readlines()
> >
> >          if self._update_fstab(fstab_lines, self.parts):
> > -            shutil.copyfile(fstab_path, fstab_path + ".orig")
> > +            # copy rootfs dir to workdir to update fstab
> > +            # as rootfs can be used by other tasks and can't be modified
> > +            new_rootfs = os.path.realpath(os.path.join(self.workdir,
> "rootfs_copy"))
> > +            copyhardlinktree(image_rootfs, new_rootfs)
> > +            fstab_path = os.path.join(new_rootfs, 'etc/fstab')
> >
> >              with open(fstab_path, "w") as fstab:
> >                  fstab.writelines(fstab_lines)
> >
> > -            return fstab_path
> > +            return new_rootfs
> >
> >      def _update_fstab(self, fstab_lines, parts):
> >          """Assume partition order same as in wks"""
> > @@ -156,7 +162,10 @@ class DirectPlugin(ImagerPlugin):
> >          filesystems from the artifacts directly and combine them into
> >          a partitioned image.
> >          """
> > -        fstab_path = self._write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
> > +        new_rootfs = self._write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
> > +        if new_rootfs:
> > +            # rootfs was copied to update fstab
> > +            self.rootfs_dir['ROOTFS_DIR'] = new_rootfs
> >
> >          for part in self.parts:
> >              # get rootfs size from bitbake variable if it's not set in
> .ks file
> > @@ -172,12 +181,7 @@ class DirectPlugin(ImagerPlugin):
> >                      if rsize_bb:
> >                          part.size = int(round(float(rsize_bb)))
> >
> > -        try:
> > -            self._image.prepare(self)
> > -        finally:
> > -            if fstab_path:
> > -                shutil.move(fstab_path + ".orig", fstab_path)
> > -
> > +        self._image.prepare(self)
> >          self._image.layout_partitions()
> >          self._image.create()
> >
> > --
> > 2.1.4
> >
> > --
> > _______________________________________________
> > Openembedded-core mailing list
> > Openembedded-core@lists.openembedded.org
> > http://lists.openembedded.org/mailman/listinfo/openembedded-core

-- 
--
Regards,
Ed

Re: [PATCH v2 1/5] wic: copy rootfs directory before changing fstab

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

On 5 July 2017 at 09:01, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:

> There is always a chance. Which commit is this? I'll try to reproduce this.
>

poky-contrib:ross/mut.  Lots of unrelated changes in there though.

Ross

[-- Attachment #2: Type: text/html, Size: 652 bytes --]