[PATCH 1/2] wic: original fstab restored too early

[PATCH 1/2] wic: original fstab restored too early

[Maciej Borzecki]

Contents of /etc/fstab were restored too early, thus overwriting the
autogenerated ones.

Signed-off-by: Maciej Borzecki <maciej.borzecki@open-rnd.pl>
Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
---
 scripts/lib/mic/imager/direct.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/lib/mic/imager/direct.py b/scripts/lib/mic/imager/direct.py
index 2cf4c8d..beae372 100644
--- a/scripts/lib/mic/imager/direct.py
+++ b/scripts/lib/mic/imager/direct.py
@@ -266,7 +266,6 @@ class DirectImageCreator(BaseImageCreator):
                       self.bootimg_dir, self.kernel_dir, self.native_sysroot)
 
             fstab = self.__write_fstab(p.get_rootfs())
-            self._restore_fstab(fstab)
 
             self.__instimage.add_partition(int(p.size),
                                            p.disk,
@@ -278,6 +277,9 @@ class DirectImageCreator(BaseImageCreator):
                                            boot = p.active,
                                            align = p.align,
                                            part_type = p.part_type)
+
+            self._restore_fstab(fstab)
+
         self.__instimage.layout_partitions(self._ptable_format)
 
         self.__imgdir = self.workdir
-- 
1.9.0

[PATCH 2/2] wic: --fsoptions handling

[Maciej Borzecki]

Add handling of --fsoptions in parition definition. Default value of
mount option is 'defaults', all user defined options are appended.

Signed-off-by: Maciej Borzecki <maciej.borzecki@open-rnd.pl>
Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
---
 scripts/lib/mic/imager/direct.py | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/lib/mic/imager/direct.py b/scripts/lib/mic/imager/direct.py
index beae372..024cd09 100644
--- a/scripts/lib/mic/imager/direct.py
+++ b/scripts/lib/mic/imager/direct.py
@@ -113,7 +113,14 @@ class DirectImageCreator(BaseImageCreator):
                 device_name = "/dev/" + p.disk + str(num + 1)
             else:
                 device_name = "/dev/" + p.disk + str(num)
-            fstab_entry = device_name + "\t" + p.mountpoint + "\t" + p.fstype + "\tdefaults\t0\t0\n"
+            opts = "defaults"
+            if p.fsopts:
+                opts += ","
+                opts += p.fsopts
+            fstab_entry = device_name + "\t" + \
+                          p.mountpoint + "\t" + \
+                          p.fstype + "\t" + \
+                          opts + "\t0\t0\n"
             fstab_lines.append(fstab_entry)
 
     def _write_fstab(self, fstab, fstab_lines):
-- 
1.9.0

Re: [PATCH 2/2] wic: --fsoptions handling

[Tom Zanussi]

On Mon, 2014-07-21 at 23:34 +0200, Maciej Borzecki wrote:
> Add handling of --fsoptions in parition definition. Default value of
> mount option is 'defaults', all user defined options are appended.
> 
> Signed-off-by: Maciej Borzecki <maciej.borzecki@open-rnd.pl>
> Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
> ---
>  scripts/lib/mic/imager/direct.py | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/lib/mic/imager/direct.py b/scripts/lib/mic/imager/direct.py
> index beae372..024cd09 100644
> --- a/scripts/lib/mic/imager/direct.py
> +++ b/scripts/lib/mic/imager/direct.py
> @@ -113,7 +113,14 @@ class DirectImageCreator(BaseImageCreator):
>                  device_name = "/dev/" + p.disk + str(num + 1)
>              else:
>                  device_name = "/dev/" + p.disk + str(num)
> -            fstab_entry = device_name + "\t" + p.mountpoint + "\t" + p.fstype + "\tdefaults\t0\t0\n"
> +            opts = "defaults"
> +            if p.fsopts:
> +                opts += ","
> +                opts += p.fsopts

This is better than the hard-coded 'defaults' there now, but shouldn't
the logic be more like 'if the user specifies --fsoptions, use only
those directly, otherwise use the default 'defaults'?

IOW, if users want to append to 'defaults' they can do it themselves in
the --fsoptions they specify.  Otherwise, there's no way for a user to
specify their own options without 'defaults'.

Tom

> +            fstab_entry = device_name + "\t" + \
> +                          p.mountpoint + "\t" + \
> +                          p.fstype + "\t" + \
> +                          opts + "\t0\t0\n"
>              fstab_lines.append(fstab_entry)
>  
>      def _write_fstab(self, fstab, fstab_lines):

[PATCH v2] wic: --fsoptions handling

[Maciej Borzecki]

Add handling of --fsoptions in parition definition. If no options are
specified, 'defaults' is used.

Signed-off-by: Maciej Borzecki <maciej.borzecki@open-rnd.pl>
Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
---
 scripts/lib/mic/imager/direct.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/scripts/lib/mic/imager/direct.py b/scripts/lib/mic/imager/direct.py
index 77a118a..7e2b63a 100644
--- a/scripts/lib/mic/imager/direct.py
+++ b/scripts/lib/mic/imager/direct.py
@@ -113,7 +113,15 @@ class DirectImageCreator(BaseImageCreator):
                 device_name = "/dev/" + p.disk + str(num + 1)
             else:
                 device_name = "/dev/" + p.disk + str(num)
-            fstab_entry = device_name + "\t" + p.mountpoint + "\t" + p.fstype + "\tdefaults\t0\t0\n"
+
+            opts = "defaults"
+            if p.fsopts:
+                opts = p.fsopts
+
+            fstab_entry = device_name + "\t" + \
+                          p.mountpoint + "\t" + \
+                          p.fstype + "\t" + \
+                          opts + "\t0\t0\n"
             fstab_lines.append(fstab_entry)
 
     def _write_fstab(self, fstab, fstab_lines):
-- 
1.9.0

Re: [PATCH v2] wic: --fsoptions handling

[Tom Zanussi]

On Thu, 2014-07-24 at 14:17 +0200, Maciej Borzecki wrote:
> Add handling of --fsoptions in parition definition. If no options are
> specified, 'defaults' is used.
> 
> Signed-off-by: Maciej Borzecki <maciej.borzecki@open-rnd.pl>
> Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>

Acked-by: Tom Zanussi <tom.zanussi@intel.com>

> ---
>  scripts/lib/mic/imager/direct.py | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/lib/mic/imager/direct.py b/scripts/lib/mic/imager/direct.py
> index 77a118a..7e2b63a 100644
> --- a/scripts/lib/mic/imager/direct.py
> +++ b/scripts/lib/mic/imager/direct.py
> @@ -113,7 +113,15 @@ class DirectImageCreator(BaseImageCreator):
>                  device_name = "/dev/" + p.disk + str(num + 1)
>              else:
>                  device_name = "/dev/" + p.disk + str(num)
> -            fstab_entry = device_name + "\t" + p.mountpoint + "\t" + p.fstype + "\tdefaults\t0\t0\n"
> +
> +            opts = "defaults"
> +            if p.fsopts:
> +                opts = p.fsopts
> +
> +            fstab_entry = device_name + "\t" + \
> +                          p.mountpoint + "\t" + \
> +                          p.fstype + "\t" + \
> +                          opts + "\t0\t0\n"
>              fstab_lines.append(fstab_entry)
>  
>      def _write_fstab(self, fstab, fstab_lines):

Re: [PATCH 1/2] wic: original fstab restored too early

[Tom Zanussi]

Sorry, I'm not seeing how this fixes the problem (best would be to
explain that in the patch description too), or for that matter, how to
reproduce the problem.  Could you elaborate?

Tom

On Mon, 2014-07-21 at 23:34 +0200, Maciej Borzecki wrote:
> Contents of /etc/fstab were restored too early, thus overwriting the
> autogenerated ones.
> 
> Signed-off-by: Maciej Borzecki <maciej.borzecki@open-rnd.pl>
> Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
> ---
>  scripts/lib/mic/imager/direct.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/lib/mic/imager/direct.py b/scripts/lib/mic/imager/direct.py
> index 2cf4c8d..beae372 100644
> --- a/scripts/lib/mic/imager/direct.py
> +++ b/scripts/lib/mic/imager/direct.py
> @@ -266,7 +266,6 @@ class DirectImageCreator(BaseImageCreator):
>                        self.bootimg_dir, self.kernel_dir, self.native_sysroot)
>  
>              fstab = self.__write_fstab(p.get_rootfs())
> -            self._restore_fstab(fstab)
>  
>              self.__instimage.add_partition(int(p.size),
>                                             p.disk,
> @@ -278,6 +277,9 @@ class DirectImageCreator(BaseImageCreator):
>                                             boot = p.active,
>                                             align = p.align,
>                                             part_type = p.part_type)
> +
> +            self._restore_fstab(fstab)
> +
>          self.__instimage.layout_partitions(self._ptable_format)
>  
>          self.__imgdir = self.workdir

Re: [PATCH 1/2] wic: original fstab restored too early

[Maciek Borzecki]

On śro, 2014-07-23 at 14:21 -0500, Tom Zanussi wrote:
> Sorry, I'm not seeing how this fixes the problem (best would be to
> explain that in the patch description too), or for that matter, how to
> reproduce the problem.  Could you elaborate?

Right, the wording might be unfortunate and not reflect what actually
happens behind the scenes.

DirectImageCreator.__write_fstab() generates new /etc/fstab in sysroot.
The contents are the original entries in /etc/fstab + the partitions
listed in *.wks (skipping / and /boot). A backup of the
original /etc/fstab is made in temp.

A counterpart call to DirectImageCreator.__restore_fstab() brings back
the original version into sysroot, so that the whole process does not
leave any files in the original tree.

The problem is that calling __restore_fstab() before
__instimage.add_partition() brings back the original fstab before the
partition file actually is built. As such, any extra partitions listed
in *.wks never make it to /etc/fstab. 

For instance I add an extra user data partition like this:

part /media/data --ondisk mmcblk0p --fstype=vfat --label data --...

will not be reflected in /etc/fstab and not mounted when system is
brought up. The patch resolves this problem.


-- 
Maciej Borzęcki
Senior Software Developer at Open-RnD Sp. z o.o., Poland
www.open-rnd.pl
mobile: +48 889 117 365, fax: +48 42 657 9079

Niniejsza wiadomość wraz z załącznikami może zawierać chronione prawem
lub poufne informacje i została wysłana wyłącznie do wiadomości i
użytku osób, do których została zaadresowana. Jeśli wiadomość została
otrzymana przypadkowo zabrania się jej kopiowania lub rozsyłania do
osób trzecich. W takim przypadku uprasza się o natychmiastowe
zniszczenie wiadomości oraz poinformowanie nadawcy o zaistniałej
sytuacji za pomocą wiadomości zwrotnej. Dziękujemy.

This message, including any attachments hereto, may contain privileged
or confidential information and is sent solely for the attention and
use of the intended addressee(s). If you are not an intended addressee,
you may neither use this message nor copy or deliver it to anyone. In
such case, you should immediately destroy this message and kindly notify
the sender by reply email. Thank you.

[PATCH v2] wic: do not overwrite autogenerated /etc/fstab with original too early

[Maciej Borzecki]

DirectImageCreator.__write_fstab() generates new /etc/fstab in sysroot
with rootfs contents. The fstab entries are generated base on the
initialn contents of /etc/fstab, plus any extra (other than / or
/boot) partitions listed in *.wks. A backup of original /etc/fstab is
done in a temp location. Subsequent call to __restore_fstab() restores
the backup copy, replacing the autogenerated one.

Calling __restore_fstab() before Wic_PartData.prepare() brings back the
original fstab before the partition image file actually is created. As
such, the autogenerated /etc/fstab will not make it to the partition.

Signed-off-by: Maciej Borzecki <maciej.borzecki@open-rnd.pl>
Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
---

Notes:
    Previous version got messed up during merges and faulty code was sent
    out. This one works as expected.

 scripts/lib/mic/imager/direct.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/lib/mic/imager/direct.py b/scripts/lib/mic/imager/direct.py
index 2cf4c8d..77a118a 100644
--- a/scripts/lib/mic/imager/direct.py
+++ b/scripts/lib/mic/imager/direct.py
@@ -262,10 +262,12 @@ class DirectImageCreator(BaseImageCreator):
             # when/if we need to actually do package selection we
             # should modify things to use those objects, but for now
             # we can avoid that.
+
+            fstab = self.__write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
+
             p.prepare(self, self.workdir, self.oe_builddir, self.rootfs_dir,
                       self.bootimg_dir, self.kernel_dir, self.native_sysroot)
 
-            fstab = self.__write_fstab(p.get_rootfs())
             self._restore_fstab(fstab)
 
             self.__instimage.add_partition(int(p.size),
@@ -278,6 +280,7 @@ class DirectImageCreator(BaseImageCreator):
                                            boot = p.active,
                                            align = p.align,
                                            part_type = p.part_type)
+
         self.__instimage.layout_partitions(self._ptable_format)
 
         self.__imgdir = self.workdir
-- 
1.9.0

Re: [PATCH v2] wic: do not overwrite autogenerated /etc/fstab with original too early

[Tom Zanussi]

On Thu, 2014-07-24 at 14:27 +0200, Maciej Borzecki wrote:
> DirectImageCreator.__write_fstab() generates new /etc/fstab in sysroot
> with rootfs contents. The fstab entries are generated base on the
> initialn contents of /etc/fstab, plus any extra (other than / or
> /boot) partitions listed in *.wks. A backup of original /etc/fstab is
> done in a temp location. Subsequent call to __restore_fstab() restores
> the backup copy, replacing the autogenerated one.
> 
> Calling __restore_fstab() before Wic_PartData.prepare() brings back the
> original fstab before the partition image file actually is created. As
> such, the autogenerated /etc/fstab will not make it to the partition.
> 

OK, I knew there was something funny about this, and it wasn't really
fixing the problem.  I also knew that it had previously worked, and
digging around realized that the problem was that the recent patch 'wic:
Extend --rootfs-dir to connect rootfs-dirs' is what actually broke
things.

So this patch shouldn't be applied - I need to look at it a bit more and
come up with a proper fix..

Tom

> Signed-off-by: Maciej Borzecki <maciej.borzecki@open-rnd.pl>
> Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
> ---
> 
> Notes:
>     Previous version got messed up during merges and faulty code was sent
>     out. This one works as expected.
> 
>  scripts/lib/mic/imager/direct.py | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/lib/mic/imager/direct.py b/scripts/lib/mic/imager/direct.py
> index 2cf4c8d..77a118a 100644
> --- a/scripts/lib/mic/imager/direct.py
> +++ b/scripts/lib/mic/imager/direct.py
> @@ -262,10 +262,12 @@ class DirectImageCreator(BaseImageCreator):
>              # when/if we need to actually do package selection we
>              # should modify things to use those objects, but for now
>              # we can avoid that.
> +
> +            fstab = self.__write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
> +
>              p.prepare(self, self.workdir, self.oe_builddir, self.rootfs_dir,
>                        self.bootimg_dir, self.kernel_dir, self.native_sysroot)
>  
> -            fstab = self.__write_fstab(p.get_rootfs())
>              self._restore_fstab(fstab)
>  
>              self.__instimage.add_partition(int(p.size),
> @@ -278,6 +280,7 @@ class DirectImageCreator(BaseImageCreator):
>                                             boot = p.active,
>                                             align = p.align,
>                                             part_type = p.part_type)
> +
>          self.__instimage.layout_partitions(self._ptable_format)
>  
>          self.__imgdir = self.workdir

Re: [PATCH v2] wic: do not overwrite autogenerated /etc/fstab with original too early

[Tom Zanussi]

On Thu, 2014-07-24 at 19:49 -0500, Tom Zanussi wrote:
> On Thu, 2014-07-24 at 14:27 +0200, Maciej Borzecki wrote:
> > DirectImageCreator.__write_fstab() generates new /etc/fstab in sysroot
> > with rootfs contents. The fstab entries are generated base on the
> > initialn contents of /etc/fstab, plus any extra (other than / or
> > /boot) partitions listed in *.wks. A backup of original /etc/fstab is
> > done in a temp location. Subsequent call to __restore_fstab() restores
> > the backup copy, replacing the autogenerated one.
> > 
> > Calling __restore_fstab() before Wic_PartData.prepare() brings back the
> > original fstab before the partition image file actually is created. As
> > such, the autogenerated /etc/fstab will not make it to the partition.
> > 
> 
> OK, I knew there was something funny about this, and it wasn't really
> fixing the problem.  I also knew that it had previously worked, and
> digging around realized that the problem was that the recent patch 'wic:
> Extend --rootfs-dir to connect rootfs-dirs' is what actually broke
> things.
> 
> So this patch shouldn't be applied - I need to look at it a bit more and
> come up with a proper fix..
> 

OK, I take that back - this patch is fine to apply.

Acked-by: Tom Zanussi <tom.zanussi@intel.com>

What confused me was that your first patch was clearly wrong, but you
moved the __write_fstab() to before .prepare in your second patch, which
is correct.

In the future, for v2 and beyond patches, please note what changed from
the previous version.

Thanks,

Tom


> Tom
> 
> > Signed-off-by: Maciej Borzecki <maciej.borzecki@open-rnd.pl>
> > Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
> > ---
> > 
> > Notes:
> >     Previous version got messed up during merges and faulty code was sent
> >     out. This one works as expected.
> > 
> >  scripts/lib/mic/imager/direct.py | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/lib/mic/imager/direct.py b/scripts/lib/mic/imager/direct.py
> > index 2cf4c8d..77a118a 100644
> > --- a/scripts/lib/mic/imager/direct.py
> > +++ b/scripts/lib/mic/imager/direct.py
> > @@ -262,10 +262,12 @@ class DirectImageCreator(BaseImageCreator):
> >              # when/if we need to actually do package selection we
> >              # should modify things to use those objects, but for now
> >              # we can avoid that.
> > +
> > +            fstab = self.__write_fstab(self.rootfs_dir.get("ROOTFS_DIR"))
> > +
> >              p.prepare(self, self.workdir, self.oe_builddir, self.rootfs_dir,
> >                        self.bootimg_dir, self.kernel_dir, self.native_sysroot)
> >  
> > -            fstab = self.__write_fstab(p.get_rootfs())
> >              self._restore_fstab(fstab)
> >  
> >              self.__instimage.add_partition(int(p.size),
> > @@ -278,6 +280,7 @@ class DirectImageCreator(BaseImageCreator):
> >                                             boot = p.active,
> >                                             align = p.align,
> >                                             part_type = p.part_type)
> > +
> >          self.__instimage.layout_partitions(self._ptable_format)
> >  
> >          self.__imgdir = self.workdir
>