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

[Chris Johns]

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.

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.

Chris