[PATCH v2] pwdgrp.c: removed assert causing runtime issues

[Joel Sherrill]

On Thu, Mar 18, 2021 at 3:11 PM Chris Johns <chrisj at rtems.org> wrote:

> On 19/3/21 6:19 am, Joel Sherrill wrote:
> > On Thu, Mar 18, 2021 at 2:14 PM Gedare Bloom <gedare at rtems.org
> > <mailto:gedare at rtems.org>> wrote:
> >     On Thu, Mar 18, 2021 at 11:40 AM Ryan Long <ryan.long at oarcorp.com
> >     <mailto:ryan.long at oarcorp.com>> wrote:
> >     >
> >     > Changed the _Assert_unused_variable_equals macro to just a (void)
> due to
> >     > /etc having already been created by the network stack
> initialization
> >     > or an initial filesystem image.
> >     >
> >     > Updates #4282
> >     > ---
> >     >  cpukit/libcsupport/src/pwdgrp.c | 9 ++++-----
> >     >  1 file changed, 4 insertions(+), 5 deletions(-)
> >     >
> >     > diff --git a/cpukit/libcsupport/src/pwdgrp.c
> b/cpukit/libcsupport/src/pwdgrp.c
> >     > index f4a10f7..edc8aff 100644
> >     > --- a/cpukit/libcsupport/src/pwdgrp.c
> >     > +++ b/cpukit/libcsupport/src/pwdgrp.c
> >     > @@ -36,7 +36,6 @@
> >     >  #include <stdint.h>
> >     >
> >     >  #include <rtems/seterr.h>
> >     > -#include <rtems/score/assert.h>
> >     >
> >     >  #include "pwdgrp.h"
> >     >
> >     > @@ -63,13 +62,13 @@ static void init_file(const char *name, const
> char
> >     *content)
> >     >   */
> >     >  static void pwdgrp_init(void)
> >     >  {
> >     > -  int sc;
> >     > -
> >     >    /*
> >     >     * Do the best to create this directory.
> >     > +   *
> >     > +   * /etc could be created by the network stack initialization or
> an initial
> >     > +   * filesystem image. Deliberately ignore the return value.
> >     >     */
> >     > -  sc = mkdir("/etc", S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH |
> S_IXOTH);
> >     > -  _Assert_Unused_variable_equals(sc, 0);
> >     > +  (void) mkdir("/etc", S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH |
> S_IXOTH);
> >     >
> >
> >     So this seems fine, but it got me thinking, does this issue affect
> all
> >     the other mkdir-related patches recently done?
> >
> >     Probably, we should use
> >     _Assert(sc == 0 || errno == EEXIST);
> >     (void) sc;
> >
> > If we are sure that it is OK for it to exist and that's the errno
> returned. :)
> >
> > But if it isn't, that's probably another bug.
> >
> > FWIW this seems typical of static analysis issues to me. FIx it wrong,
> then
> > fix it again a correct way, and then realize there was an even better
> way to
> > fix it. I presented a case at the Flight Software Workshop where I think
> I went
> > through multiple solutions before realizing B0 was special in POSIX to
> hang
> > up a line and the UART device driver had to say "not supported" instead
> of
> > an assert to avoid a divide by zero.
> >
> > These are often more subtle than they appear on first examination.
>
> This patch flip flop smells of deeper architecture issues. Ryan is doing a
> great
> job in handling these tough converity issues and he or anyone else can
> never
> know what is happening else where because nothing is specified. Every
> place /etc
> is needed has to add code to handle this and that is wrong. No where in
> RTEMS do
> we tolerate this type of coding.
>

The grlib case was quite unique to them. It wasn't creating /dev as I
recall, it was
creating a directory under /dev as I understood the code.  This is one of
them as
it is on the 5 branch.

       strcpy(priv->prefix, "/dev/rastatmtc0");
        priv->prefix[14] += dev->minor_drv;
        mkdir(priv->prefix, S_IRWXU | S_IRWXG | S_IRWXO);
        priv->prefix[15] = '/';
        priv->prefix[16] = '\0';

>
> I think I said before the real solution is to add a SYSINIT or what ever
> that
> creates "the RTEMS" root fs. We document it and make is standard. We make
> the
> initialise point something drivers and code can depend on and then these
> make
> dirs etc can be stripped away.
>

I'd tend to say a sysinit method for /etc. This will impact the libbsd code
but it would be better to put it in one place. No point in adding it to
every
configuration even if not needed.

/dev is already there in sysinit because it is already part of the base
filesystem image. I don't see a second mkdir() on just it.

src/base_fs.c:  rv = mkdir( "/dev", S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH |
S_IXOTH );

>
> Chris
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210318/b396a161/attachment.html>