[OE-core] Initial review of ChenQi/read-only-rootfs-in-live-images
ChenQi
Qi.Chen at windriver.com
Mon Aug 5 03:30:33 UTC 2013
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 at lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openembedded.org/pipermail/openembedded-core/attachments/20130805/bce8c2a9/attachment-0002.html>
More information about the Openembedded-core
mailing list