[OE-core] Initial review of ChenQi/read-only-rootfs-in-live-images

[ChenQi]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openembedded.org/pipermail/openembedded-core/attachments/20130805/389ced1c/attachment-0002.html>