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

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

[Chris Larson]

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

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.
- 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.
- 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.
- 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
    - handle bind mounts of files as well as directories
- 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 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.

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.

Thanks,
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

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

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

[ChenQi]

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

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


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

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

[ChenQi]

[-- 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 --]