Date: Mon, 1 Mar 1999 11:28:40 +0200 (IST)
From: Eli Zaretskii <eliz AT is DOT elta DOT co DOT il>
X-Sender: eliz AT is
To: "Mark E." <snowball3 AT usa DOT net>
cc: djgpp-workers AT delorie DOT com
Subject: Re: chroot patches r3
In-Reply-To: <199902281957.TAA94214@out5.ibm.net>
Message-ID: <Pine.SUN.3.91.990301112814.2578N-100000@is>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Reply-To: djgpp-workers AT delorie DOT com


On Sun, 28 Feb 1999, Mark E. wrote:

> Below are my latest patches to implement chroot.

Thanks!

> +      __CHROOT_NO_SETENV_FLAG is set at startup to avoid
> +      wasting time setting the same environmental variables
> +      that resulted in chroot being called in the first place. */
> +   if (!(__chroot_flags & __CHROOT_NO_SETENV_FLAG))
> +     setenv(ROOT_ENV, __djgpp_root, 1);

I'm not sure this is worth the hassle: putenv (called by setenv)
already avoids most of the work when the old and the new values are
identical.

> + int
> + fchroot (int fd)
> + {
> +   unsigned int flags = __chroot_flags;
> + 
> +   if (fd < 0)
> +   {
> +     errno = EINVAL;
> +     return -1;
> +   }

I think errno should be set here to EBADF, not EINVAL.

> *** src/libc/compat/unistd/chroot.txh.orig	Sun Feb 28 14:25:02 1999
> --- src/libc/compat/unistd/chroot.txh	Sat Feb 27 18:01:34 1999

Please resend the diffs for .txh files, they are garbled in strange
ways (some + marks are missing, and some line beginnings too).

> + Causes @var{new_root_directory} to become the root directory, or 
> starting point, for path name searches beginning with '/' or '\'. The 
> current working directory is unaffected.

Here's an example of a garbled part.

Please put all file names or parts thereof (like '/') inside @file{}.

> {ROOT} is not set, but @var{SYSROOT} is, then @var{SYSROOT} is 
> used to set the root directory, and @code{chroot} is set to permissive 
> mode, and @var{CHROOT_UNIX} is ignored.

Another example of garbled lines.

Environment variables should use @code{}, not @var{}.  The latter is
for something that stands for another piece of text, like parameters
passed to a function.

Also, I think the permissive as opposed to restricted mode should be
explained a bit more, possibly with an example.  It seems to me that
the current text assumes people are familiar with Unix restrictions on
`chroot'.

> + The global variable @var{__chroot_flags} can be set to include the

Same here: use @code{}, since __chroot_flags is the actual name of the
variable.

> + Zero if successful, else nonzero and @var{errno} set if error.

@code{errno}

> +   if (unix_mode)
> +   {
> +     if (*unix_mode == 'Y' || *unix_mode == 'y')
> +       __chroot_flags = __CHROOT_UNIX_MODE_FLAG;
> +     else
> +       __chroot_flags &= ~__CHROOT_UNIX_MODE_FLAG;
> +   }
> +   else if (sysroot)
> +     __chroot_flags &= ~__CHROOT_UNIX_MODE_FLAG;

I think it's better to assign zero to __chroot_flags instead of ANDing
it with ~__CHROOT_UNIX_MODE_FLAG, because otherwise __chroot_flags
with which Emacs was dumped will leak into the restarted program.

Why did you need to use &=, anyway?  __chroot_flags isn't supposed to
have any useful value before this code runs, right?

Also, it seems to me that this code doesn't set __chroot_flags to
anything if none of the environment variables is defined.  This means
that __djgpp_root[] and __djgpp_root_len retain their previous values
(I'm thinking Emacs again).

> +   if (root)
> +   {
> +     __chroot_flags |= __CHROOT_NO_SETENV_FLAG;
> +     /* What should happen when chroot fails? */
> +     ret_code = chroot(root);

I think if chroot fails, you need to abort the program (actually call
`abort').

Where is the call to __crt0_setup_chroot from startup code?  I don't
see it in the diffs.