Re: [PATCH] corrected setenv|putenv rc checking (bin)

[Alexey Slynko <slynko@xxxxxxxxxxxxxxxxxx>]

On Fri, 30 Sep 2005, Simon 'corecode' Schubert wrote:

> Date: Fri, 30 Sep 2005 12:22:48 +0200
> From: Simon 'corecode' Schubert <corecode@xxxxxxxxxxxx>
> Newsgroups: dragonfly.submit
> Subject: Re: [PATCH] corrected setenv|putenv rc checking (bin)
>
> Alexey Slynko wrote:
> > > > Index: ls/ls.c
> > > > ===================================================================
> > > > RCS file: /mnt/share/dragonfly-cvs/src/bin/ls/ls.c,v
> > > > retrieving revision 1.14
> > > > diff -u -r1.14 ls.c
> > > > --- ls/ls.c    18 Sep 2005 18:01:49 -0000    1.14
> > > > +++ ls/ls.c    29 Sep 2005 00:48:06 -0000
> > > > @@ -221,7 +221,8 @@
> > > >              fts_options |= FTS_COMFOLLOW;
> > > >              break;
> > > >          case 'G':
> > > > -            setenv("CLICOLOR", "", 1);
> > > > +            if (setenv("CLICOLOR", "", 1) != 0)
> > > > +                err(1, "setenv: cannot set CLICOLOR");
> > > >              break;
> > > >          case 'L':
> > > >              fts_options &= ~FTS_PHYSICAL;
> > >
> > >
> > > I still think this shouldn't abort.
> >
> > I still think that ls is not an exception of rule
>
> Abort ls because it can't color the output?
Because it's an unpredictable behaviuor

> This is absolutely ridiculous.
> Maybe invent a global int clicolor?
Can we discuss it?

> > > > Index: sh/var.c
> > > > ===================================================================
> > > > RCS file: /mnt/share/dragonfly-cvs/src/bin/sh/var.c,v
> > > > retrieving revision 1.8
> > > > diff -u -r1.8 var.c
> > > > --- sh/var.c    19 Apr 2005 05:18:19 -0000    1.8
> > > > +++ sh/var.c    29 Sep 2005 01:02:35 -0000
> > > > @@ -316,7 +317,8 @@
> > > >              if (vp == &vmpath || (vp == &vmail && ! mpathset()))
> > > >                  chkmail(1);
> > > >              if ((vp->flags & VEXPORT) && localevar(s)) {
> > > > -                putenv(s);
> > > > +                if (putenv(s) != 0)
> > > > +                    err(1, "putenv: cannot set %s", s);
> > > >                  setlocale(LC_ALL, "");
> > > >              }
> > > >              INTON;
> > > > @@ -332,7 +334,8 @@
> > > >      INTOFF;
> > > >      *vpp = vp;
> > > >      if ((vp->flags & VEXPORT) && localevar(s)) {
> > > > -        putenv(s);
> > > > +        if (putenv(s) != 0)
> > > > +            err(1, "putenv: cannot set %s", s);
> > > >          setlocale(LC_ALL, "");
> > > >      }
> > > >      INTON;
> > > > @@ -553,7 +556,8 @@
> > > >                           vp->flags |= flag;
> > > >                          if ((vp->flags & VEXPORT) && 
> > > > localevar(vp->text)) {
> > > > -                            putenv(vp->text);
> > > > +                            if (putenv(vp->text) != 0)
> > > > +                                err(1, "putenv: cannot set %s", 
> > > > vp->text);
> > > >                              setlocale(LC_ALL, "");
> > > >                          }
> > > >                          goto found;
> > >
> > >
> > > no!  don't collapse my shell if it can't allocate an env var!  set $? and 
> > > output a warning.  You don't expect the shell to collapse if it can't exec 
> > > a binary, right?
> > >
> > > $ /bin/ehco foo
> > > /bin/ehco: Command not found.
> > >
> > > Login: _
> > >
> > > WTF?
> >
> > Can't reproduce it. Can you explain, how it can be reproduced?
>
> This is an example.  You wouldn't want sh to close itself if it can't run the 
> binary you are asking it to run.  Following this reasoning you'd not want the 
> shell to close itself if it can't set the env var you are asking it to set.
Well, it's an argument. I'l try to correct it.