Re: Initial review of ChenQi/read-only-rootfs-in-live-images

[ChenQi <Qi.Chen@windriver.com>]

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

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


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