Re: [OE-core] Initial review of ChenQi/read-only-rootfs-in-live-images
For convenience, here's a link below in case people may wondering which
patchset we are talking about.
http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=ChenQi/read-only-rootfs-in-live-images
//Chen Qi
On 08/05/2013 10:18 AM, ChenQi wrote:
Hi Chris,
Thanks for your review and please see comments inline.
(You might be reviewing the old patchset, please see the new one if
you have time.)
On 08/02/2013 11:50 PM, Chris Larson wrote:
Greetings,
Nice work on this branch. The bind based approach seems clean. I do
have some comments:
- I think the read-only-rootfs-hook stuff, at least the bind mounting
piece at a minimum, belongs in a separate script in ${sbindir} rather
than within the startup script itself. This will ease adding support
for r/o to systemd images.
Although I still hold to the tenet we should make sysvinit and systemd
separate from each other whenever possible, this option is acceptable
if they do duplicate code and there's no side effect.
But for now, I would suggest we leave it in the initscripts, and when
you add read-only-rootfs support for systemd , you could send a patch
to do the change.
- I think we should consider having an additional script which
unmounts the binds in reverse order, in case the binds prevent
unmounting of the partitions in which they live, potentially causing
an unclean shutdown.
I figure the bind mounts are essentially using volatile storage, the
data are lost between reboots after all. So could you please give an
example of unclean shutdown?
Another problem with this unmounting mechanism is that it's hard to
determine whether a directory is bind mounted. Note that in the
read-only-rootfs-hook.sh script, a check has been added to see whether
the directory specified in config file is on a read-only partition,
and only if so, the directory is bind mounted with a directory on
volatile storage.
- The irda read only bits aren't necessary, it seems. The startup
script writes to /etc/sysconfig/ if its config file doesn't already
exist, as a mechanism of handling default configuration. This is a
terrible way to handle that, and sysconfig is a redhatism, not the
way we do things in our distros. I have a commit I'll submit a patch
for to resolve this by fixing up the startup script.
This one has been fixed in the new patchset.
- I think we should enhance the copy part of the bind script in two ways:
- cp from/. to/ -> this ensures it will copy hidden directories
as well, which the wildcard does not
Agree. Thanks for reminding me of this. I'll fix this.
- handle bind mounts of files as well as directories
Agree. Thanks. I'll send out a new patchset.
(Of course I'll first wait for your comments :) )
- I also think the mount script should support mount options, making
the config files essentially a subset of the filesystem table in
fstab. There are cases where there's value in using options with bind
mounts, and it'd make the scripts potentially more generally useful.
I know it might be useful, but I can't think of a use case. Could you
please give an example where the mount options are useful?
- I think we should split out the mount and unmount scripts, along
with default configuration, into a separate recipe which inherits
update-rc.d, and which gets pulled into IMAGE_INSTALL by
image.bbclass only when read-only-rootfs is in IMAGE_FEATURES. This
means we'd need less conditional "if this is r/o" code in the
scripts, as we could rely on it being installed in r/o images, and
not in others.
The r/o check is needed anyway, it's not dedicated to the
read-only-rootfs-hook.sh script. Other init scripts might also need
this. That's why I made it a common function in the functions script
both in initscripts and lsbinitscripts.
As an aside, I looked into libmount's previous support of fstab.d, it
appears using util-linux's libmount-based mount is supposed to
support passing a directory to its -T/--fstab= argument, which would
let us offload the work to it if we use a real mount binary, but it
seems that this directory argument support doesn't work anyway, and
of course by doing that we'd suck in additional deps, so maybe it's
for the best :)
Thoughts? I have prototype mount-binds and unmount-binds shell
scripts here which implement the aforementioned suggestions, but I
wanted to throw my thoughts out to the list for comments before
proceeding any further.
It would be great if you could share them, and let's have some further
discussions.
Best Regards,
Chen Qi
Thanks,
--
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core
___
Openembedded-core mailing list
Opene
Re: [OE-core] Initial review of ChenQi/read-only-rootfs-in-live-images
Hi Chris,
Thanks for your review and please see comments inline.
(You might be reviewing the old patchset, please see the new one if you
have time.)
On 08/02/2013 11:50 PM, Chris Larson wrote:
Greetings,
Nice work on this branch. The bind based approach seems clean. I do
have some comments:
- I think the read-only-rootfs-hook stuff, at least the bind mounting
piece at a minimum, belongs in a separate script in ${sbindir} rather
than within the startup script itself. This will ease adding support
for r/o to systemd images.
Although I still hold to the tenet we should make sysvinit and systemd
separate from each other whenever possible, this option is acceptable if
they do duplicate code and there's no side effect.
But for now, I would suggest we leave it in the initscripts, and when
you add read-only-rootfs support for systemd , you could send a patch to
do the change.
- I think we should consider having an additional script which
unmounts the binds in reverse order, in case the binds prevent
unmounting of the partitions in which they live, potentially causing
an unclean shutdown.
I figure the bind mounts are essentially using volatile storage, the
data are lost between reboots after all. So could you please give an
example of unclean shutdown?
Another problem with this unmounting mechanism is that it's hard to
determine whether a directory is bind mounted. Note that in the
read-only-rootfs-hook.sh script, a check has been added to see whether
the directory specified in config file is on a read-only partition, and
only if so, the directory is bind mounted with a directory on volatile
storage.
- The irda read only bits aren't necessary, it seems. The startup
script writes to /etc/sysconfig/ if its config file doesn't already
exist, as a mechanism of handling default configuration. This is a
terrible way to handle that, and sysconfig is a redhatism, not the way
we do things in our distros. I have a commit I'll submit a patch for
to resolve this by fixing up the startup script.
This one has been fixed in the new patchset.
- I think we should enhance the copy part of the bind script in two ways:
- cp from/. to/ -> this ensures it will copy hidden directories as
well, which the wildcard does not
Agree. Thanks for reminding me of this. I'll fix this.
- handle bind mounts of files as well as directories
Agree. Thanks. I'll send out a new patchset.
(Of course I'll first wait for your comments :) )
- I also think the mount script should support mount options, making
the config files essentially a subset of the filesystem table in
fstab. There are cases where there's value in using options with bind
mounts, and it'd make the scripts potentially more generally useful.
I know it might be useful, but I can't think of a use case. Could you
please give an example where the mount options are useful?
- I think we should split out the mount and unmount scripts, along
with default configuration, into a separate recipe which inherits
update-rc.d, and which gets pulled into IMAGE_INSTALL by image.bbclass
only when read-only-rootfs is in IMAGE_FEATURES. This means we'd need
less conditional "if this is r/o" code in the scripts, as we could
rely on it being installed in r/o images, and not in others.
The r/o check is needed anyway, it's not dedicated to the
read-only-rootfs-hook.sh script. Other init scripts might also need
this. That's why I made it a common function in the functions script
both in initscripts and lsbinitscripts.
As an aside, I looked into libmount's previous support of fstab.d, it
appears using util-linux's libmount-based mount is supposed to support
passing a directory to its -T/--fstab= argument, which would let us
offload the work to it if we use a real mount binary, but it seems
that this directory argument support doesn't work anyway, and of
course by doing that we'd suck in additional deps, so maybe it's for
the best :)
Thoughts? I have prototype mount-binds and unmount-binds shell scripts
here which implement the aforementioned suggestions, but I wanted to
throw my thoughts out to the list for comments before proceeding any
further.
It would be great if you could share them, and let's have some further
discussions.
Best Regards,
Chen Qi
Thanks,
--
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics
___
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core
