crosutils: add scripts/.local_mounts to allow mounting additional (RO) folders in chroot

crosutils: add scripts/.local_mounts to allow mounting additional (RO) folders in chroot

It would be helpful if we can share some folders inside/outside chroot, for
example editor configuration / settings, or the default Downloads folder.

For safety concern and prevent user accidentally deleted their files in actual home, the mounts were made as read-only.

This CL reads .local_mounts (just like .default_boards) from scripts folder,
and mount the folders whenever you do enter_chroot.

.local_mounts has a very simple syntax:
    mount_path
 or source_path(outside chroot) destination_path(inside chroot)
 or # comments.

Examples:
# comment here
/usr/share/vim/google
/home/XXX/Downloads /outside

BUG=none
TEST=manually:
 1. Create ~/trunk/src/scripts/.local_mounts with following content:
 /usr/share/vim/google
 /home/XXX/Downloads /outside
 2. Create the folders: chroot/usr/share/vim/google, chroot/outside
 3. ./enter_chroot.sh
 4. ls -l /usr/share/vim/google # folder is mounted correctly
    ls -l /outside
 5. exit
 6. mount | grep chroot # nothing is left

Change-Id: I8901d8291e618cea493b3d74e7ef00e6830c660f

[kliegs, 2010-11-26 15:33:35]

I'm a little mixed on this. While this is a big help, there's been concern
in the past because a user forgetting to run 'make_chroot --delete' and then
doing a rm -rf on their checked out tree could delete those files from their
actual home directory.

If the intent is just to copy over editor or other configuration, would just
doing a copy of the relevant files work?  It does mean changes made inside
the chroot don't get propagated back to the main host environment but it
prevents the risk of deleting things.

Other comments:
Should you use FLAGS_trunk instead of GCLIENT_ROOT?

The die message needs to be cleaned up (It would parse as "<file>: Failed to
<path> by <source>".

And a mix of upper and lower case variables made it a bit confusing for me
to read (MOUNTED_PATH vs mount_source)

On Fri, Nov 26, 2010 at 7:02 AM, <hungte@chromium.org> wrote:

> Reviewers: adlr, robotboy,
>
> Description:
> crosutils: add scripts/.local_mounts to allow mounting additional folders
> in
> chroot
>
> It would be helpful if we can share some folders inside/outside chroot, for
> example editor configuration / settings, or the default Downloads folder.
>
> This CL reads .local_mounts (just like .default_boards) from scripts
> folder,
> and mount the folders whenever you do enter_chroot.
>
> .local_mounts has a very simple syntax:
>
> Example: /usr/share/vim/google /usr/share/vim/google
>
> BUG=none
> TEST=manually:
>  1. Create ~/trunk/src/scripts/.local_mounts with following content:
>  # src dest
>  /usr/share/vim/google /usr/share/vim/google
>  /home/XXX/Downloads /home/XXX/Downloads
>  2. Create the folders: chroot/usr/share/vim/google,
> chroot/home/XXX/Downloads
>  3. ./enter_chroot.sh
>  4. ls -l /usr/share/vim/google # folder is mounted correctly
>    ls -l /home/XXX/Downloads
>  5. exit
>  6. mount | grep chroot # nothing is left
>
> Change-Id: I8901d8291e618cea493b3d74e7ef00e6830c660f
>
> Please review this at http://codereview.chromium.org/5331009/
>
> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master
>
> Affected files:
>  M enter_chroot.sh
>
>
> Index: enter_chroot.sh
> diff --git a/enter_chroot.sh b/enter_chroot.sh
> index
>
606eaf51a6beef94cbca10047e282e08e2bf81de..cb4305e02c374fb46d38875d4fcedf90ba655bbf
> 100755
> --- a/enter_chroot.sh
> +++ b/enter_chroot.sh
> @@ -188,6 +188,24 @@ function setup_env {
>       fi
>     fi
>
> +    # Mount additional directories
> +    LOCAL_MOUNTS="${GCLIENT_ROOT}/src/scripts/.local_mounts"
> +    if [ -f "${LOCAL_MOUNTS}" ]; then
> +      # format: [#] mount_source mount_point
> +      info "Mounting local folders"
> +      cat "${LOCAL_MOUNTS}" | while read mount_source mount_point; do
> +        if [ "$(echo "$mount_source" | cut -b 1)" = "#" ]; then
> +          continue
> +        fi
> +        MOUNTED_PATH="$(readlink -f "${FLAGS_chroot}${mount_point}")"
> +        if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
> +        then
> +          sudo mount --bind "${mount_source}" "$MOUNTED_PATH" ||
> +            die "${LOCAL_MOUNTS}: Failed to ${MOUNTED_PATH} by
> ${mount_source}"
> +        fi
> +      done
> +    fi
> +
>   ) 200>>"$LOCKFILE" || die "setup_env failed"
>  }
>
>
>
>

[Hung-Te, 2010-11-26 16:22:25]

On 2010/11/26 15:33:35, kliegs wrote:
> a user doing a rm -rf could delete those files from their actual home
 That's an important issue. What about only allowing read-only mounts?

> If the intent is just to copy over editor or other configuration, would just
> doing a copy of the relevant files work?
 Not only the configurations. Sometimes I download images with my browser
outside chroot, and then need to copy the files into chroot.

> Should you use FLAGS_trunk instead of GCLIENT_ROOT?
 I was trying to follow the same rule as ".default_board", which (in common.sh)
uses GCLIENT_ROOT.

 Thanks for all your suggestions - I've also also fixed the die message and
variable upper/lower cases.

[kliegs, 2010-11-26 16:35:33]

On Fri, Nov 26, 2010 at 11:22 AM, <hungte@chromium.org> wrote:

> On 2010/11/26 15:33:35, kliegs wrote:
>
>> a user doing a rm -rf could delete those files from their actual home
>>
>  That's an important issue. What about only allowing read-only mounts

I don't think it actually happens that often (it hasn't happened to me and
I've forgotten to run make_chroot --delete) before but it is possible.

I don't know if read-only mounts would fix this and it might cause other
issues (not sure if editors will complain if their config file directories
are read-only).  The way you did it looks odd in your new patch - you should
be able to mount it read-only on the initial mount and not need to remount I
believe.


>
>  If the intent is just to copy over editor or other configuration, would
>> just
>> doing a copy of the relevant files work?
>>
>  Not only the configurations. Sometimes I download images with my browser
> outside chroot, and then need to copy the files into chroot


If you just want to download a file you can download directly into
.../<srctreeroot>/chroot/...  from outside the root when asked where to save
the file.  You can also make a symlink from ~/curroot or similar to find
easily in the future.



>
>  Should you use FLAGS_trunk instead of GCLIENT_ROOT?
>>
>  I was trying to follow the same rule as ".default_board", which (in
> common.sh)
> uses GCLIENT_ROOT.
>
>  Thanks for all your suggestions - I've also also fixed the die message and
> variable upper/lower cases.


Looking at the file, enter_chroot uses GCLIENT_ROOT as the default setting
for FLAGS_trunk.  This way if FLAGS_trunk gets overridden (not sure if
anyone does this but the option is there) your code will still work.


>
>
> http://codereview.chromium.org/5331009/
>

[Hung-Te, 2010-11-26 17:21:06]

On 2010/11/26 16:35:33, kliegs wrote:
> The way you did it looks odd in your new patch - you should be able to 
> mount it read-only on the initial mount and not need to remount I believe.
  In fact, --bind mounts can only be read-only by a remount.
  http://lwn.net/Articles/281157/

> enter_chroot uses GCLIENT_ROOT as the default setting for FLAGS_trunk.
  Fixed, thanks

> You can also make a symlink from ~/curroot or similar to find
> easily in the future.
  Another example is, I may have some files that I don't want to store inside
the
  chroot folder nor the source tree folder, or I have some files to be shared
  between two source tree/chroots, and I still want to access them inside the
chroot.

  For example, whenever I get a Chrome OS device, my first step is to backup 
  its image and firmware; so I'd put it somewhere outside the chroot to make
sure
  it won't be accidentally removed. But when I want to use it with some scripts,
  I need to copy it into chroot.

[Mandeep Singh Baines, 2010-11-29 18:23:23]

This seems useful.

But can you please document this on the wiki before commiting the CL. We seem
to be adding features like crazy to the build system without documenting
them anywhere.

hungte@chromium.org (hungte@chromium.org) wrote:
> Reviewers: adlr, robotboy,
> 
> Description:
> crosutils: add scripts/.local_mounts to allow mounting additional folders in
> chroot
> 
> It would be helpful if we can share some folders inside/outside chroot, for
> example editor configuration / settings, or the default Downloads folder.
> 
> This CL reads .local_mounts (just like .default_boards) from scripts folder,
> and mount the folders whenever you do enter_chroot.
> 
> .local_mounts has a very simple syntax:
> 
> Example: /usr/share/vim/google /usr/share/vim/google
> 
> BUG=none
> TEST=manually:
>  1. Create ~/trunk/src/scripts/.local_mounts with following content:
>  # src dest
>  /usr/share/vim/google /usr/share/vim/google
>  /home/XXX/Downloads /home/XXX/Downloads
>  2. Create the folders: chroot/usr/share/vim/google,
> chroot/home/XXX/Downloads
>  3. ./enter_chroot.sh
>  4. ls -l /usr/share/vim/google # folder is mounted correctly
>     ls -l /home/XXX/Downloads
>  5. exit
>  6. mount | grep chroot # nothing is left
> 
> Change-Id: I8901d8291e618cea493b3d74e7ef00e6830c660f
> 
> Please review this at http://codereview.chromium.org/5331009/
> 
> SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master
> 
> Affected files:
>   M enter_chroot.sh
> 
> 
> Index: enter_chroot.sh
> diff --git a/enter_chroot.sh b/enter_chroot.sh
> index
606eaf51a6beef94cbca10047e282e08e2bf81de..cb4305e02c374fb46d38875d4fcedf90ba655bbf
> 100755
> --- a/enter_chroot.sh
> +++ b/enter_chroot.sh
> @@ -188,6 +188,24 @@ function setup_env {
>        fi
>      fi
> 
> +    # Mount additional directories
> +    LOCAL_MOUNTS="${GCLIENT_ROOT}/src/scripts/.local_mounts"
> +    if [ -f "${LOCAL_MOUNTS}" ]; then
> +      # format: [#] mount_source mount_point
> +      info "Mounting local folders"
> +      cat "${LOCAL_MOUNTS}" | while read mount_source mount_point; do
> +        if [ "$(echo "$mount_source" | cut -b 1)" = "#" ]; then
> +          continue
> +        fi
> +        MOUNTED_PATH="$(readlink -f "${FLAGS_chroot}${mount_point}")"
> +        if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
> +        then
> +          sudo mount --bind "${mount_source}" "$MOUNTED_PATH" ||
> +            die "${LOCAL_MOUNTS}: Failed to ${MOUNTED_PATH} by
> ${mount_source}"
> +        fi
> +      done
> +    fi
> +
>    ) 200>>"$LOCKFILE" || die "setup_env failed"
>  }
> 
> 
> 

[sosa, 2010-11-29 19:31:54]

Some comments:

1)  local variables should have following cases:  blah_variable not
BLAH_VARIABLE ... that casing should be reserved for sh globals.

2)  You never unmount these ... ever. Add logic to the unmount similar to the
other unmounts.

3)  Please add this file to .gitignore

[Hung-Te, 2010-11-30 02:30:06]

On 2010/11/29 19:31:54, sosa wrote:
> 1)  local variables should have following cases:  blah_variable not
> BLAH_VARIABLE ... that casing should be reserved for sh globals.
   I used lower case at first, then kllegs suggested to use upper case  to fit
into existing style of enter_chroot.
   Since it's really confusing, I'm going to change all references to
mounted_path so that we can use lower case.

> 2)  You never unmount these ... ever. 
> Add logic to the unmount similar to the other unmounts.
   Currently the unmounting of others is made by a (mount|grep) which will
include all mounts under chroot, so no need to write extra unmounts.

> 3)  Please add this file to .gitignore
  Done, thanks.

[Hung-Te, 2010-11-30 02:34:08]

On 2010/11/29 18:23:23, Mandeep Singh Baines wrote:
> But can you please document this on the wiki before commiting the CL. We seem
> to be adding features like crazy to the build system without documenting
> them anywhere.
   http://dev.chromium.org/chromium-os/developer-guide  section "Tips and
Tricks" seems a good place for this. Thanks!

[kliegs, 2010-11-30 15:17:24]

Sorry for not keeping up on this as I got pulled into other things.  Was
commenting that you were alternating case and needed to settle on a style -
I hope I didn't actually say to make everything upper case.

On Mon, Nov 29, 2010 at 9:30 PM, <hungte@chromium.org> wrote:

> On 2010/11/29 19:31:54, sosa wrote:
>
>> 1)  local variables should have following cases:  blah_variable not
>> BLAH_VARIABLE ... that casing should be reserved for sh globals.
>>
>   I used lower case at first, then kllegs suggested to use upper case  to
> fit
> into existing style of enter_chroot.
>   Since it's really confusing, I'm going to change all references to
> mounted_path so that we can use lower case.
>
>
>  2)  You never unmount these ... ever.
>> Add logic to the unmount similar to the other unmounts.
>>
>   Currently the unmounting of others is made by a (mount|grep) which will
> include all mounts under chroot, so no need to write extra unmounts.
>
>
>  3)  Please add this file to .gitignore
>>
>  Done, thanks.
>
>
> http://codereview.chromium.org/5331009/
>

[Hung-Te, 2010-11-30 15:48:31]

On 2010/11/30 15:17:24, kliegs wrote:
> Was commenting that you were alternating case and needed to 
> settle on a style - I hope I didn't actually say to make 
> everything upper case.

My apologize for that - you're right. The original code uses upper case, and you
simply mentioned the consistency issue -- not how to fix it.
Really sorry for that.