Better handling of fuse in chroot

Better handling of fuse in chroot

[Will Drewry, 2010-04-05 19:19:37]

LGTM but I wonder if it could be simpler :)

http://codereview.chromium.org/1582011/diff/1/2
File src/scripts/enter_chroot.sh (right):

http://codereview.chromium.org/1582011/diff/1/2#newcode154
src/scripts/enter_chroot.sh:154: INNER_MOD_PATH="$(readlink -m
"${FLAGS_chroot}${KERNEL_MODULES_ROOT}")"
Aside from adding the fuse device, does it really need all the modules and
modules.dep in there?  I missed the first round of reviews, but I was just
surprised that we'd need that in the chroot once it's loaded and we have the dev
node.

I wonder if this could just be a mount check followed by a device check, but I'd
guess I am not quite understanding what the is being done to setup the fuse fs.
(Maybe it always walks the modules?)

Maybe something like:

if ! mount | grep -Fq "on ${MOUNTED_PATH} "; then 
  if ! test -c "${FUSE_DEVICE}"; then
    modprobe fuse
  fi
  if test -c "${FUSE_DEVICE}"; then
    sudo touch "${MOUNTED_PATH}"
    sudo mount --bind "${FUSE_DEVICE}" "${MOUNTED_PATH}"
  else
    echo Warning: gmergefs will not work without fuse support
  fi
fi

http://codereview.chromium.org/1582011/diff/1/2#newcode155
src/scripts/enter_chroot.sh:155: if [ ! -f "${INNER_MOD_PATH}/modules.dep" ];
then
Is this line too long (and line 161) or is it weird codereview rendering?

http://codereview.chromium.org/1582011/diff/1/2#newcode165
src/scripts/enter_chroot.sh:165: echo "Warning:  Fuse found without loadable
module" \
extra space after Warning: ?

http://codereview.chromium.org/1582011/diff/1/2#newcode166
src/scripts/enter_chroot.sh:166: "gmergefs will only work if fuse built into
kernel"
if fuse /is/ built

http://codereview.chromium.org/1582011/diff/1/2#newcode169
src/scripts/enter_chroot.sh:169: echo "Warning:  Fuse found but no modules.dep
found for your kernel"
No more complaining if the device isn't there?

[sosa, 2010-04-05 22:37:44]

PTAL

I've simplified this a lot

[Will Drewry, 2010-04-05 22:44:05]

LGTM

Thanks for making it less noisy :)

[adlr, 2010-04-05 23:31:31]

http://codereview.chromium.org/1582011/diff/13001/14001
File src/scripts/enter_chroot.sh (right):

http://codereview.chromium.org/1582011/diff/13001/14001#newcode147
src/scripts/enter_chroot.sh:147: MOUNTED_PATH="$(readlink -f
"${FLAGS_chroot}${FUSE_DEVICE}")"
can you remove the call to readlink? readlink will return empty string if the
path doesn't exist.

also, should you mkdir -p the path?

[sosa, 2010-04-05 23:41:40]

http://codereview.chromium.org/1582011/diff/13001/14001
File src/scripts/enter_chroot.sh (right):

http://codereview.chromium.org/1582011/diff/13001/14001#newcode147
src/scripts/enter_chroot.sh:147: MOUNTED_PATH="$(readlink -f
"${FLAGS_chroot}${FUSE_DEVICE}")"
readlink -f only checks the directory, not the last part ... and it /dev is
missing, the chroot is broken anyway

On 2010/04/05 23:31:31, adlr wrote:
> can you remove the call to readlink? readlink will return empty string if the
> path doesn't exist.
> 
> also, should you mkdir -p the path?

[adlr, 2010-04-05 23:43:08]

LGTM my mistake

[sosa, 2010-04-05 23:46:47]

Thanks for stepping in with your fix though.  Sorry I dropped the ball a bit.

On Mon, Apr 5, 2010 at 4:43 PM,  <adlr@chromium.org> wrote:
> LGTM my mistake
>
> http://codereview.chromium.org/1582011
>