<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 18, 2021 at 3:11 PM Chris Johns <<a href="mailto:chrisj@rtems.org">chrisj@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 19/3/21 6:19 am, Joel Sherrill wrote:<br>
> On Thu, Mar 18, 2021 at 2:14 PM Gedare Bloom <<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a><br>
> <mailto:<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>>> wrote:<br>
> On Thu, Mar 18, 2021 at 11:40 AM Ryan Long <<a href="mailto:ryan.long@oarcorp.com" target="_blank">ryan.long@oarcorp.com</a><br>
> <mailto:<a href="mailto:ryan.long@oarcorp.com" target="_blank">ryan.long@oarcorp.com</a>>> wrote:<br>
> ><br>
> > Changed the _Assert_unused_variable_equals macro to just a (void) due to<br>
> > /etc having already been created by the network stack initialization<br>
> > or an initial filesystem image.<br>
> ><br>
> > Updates #4282<br>
> > ---<br>
> > cpukit/libcsupport/src/pwdgrp.c | 9 ++++-----<br>
> > 1 file changed, 4 insertions(+), 5 deletions(-)<br>
> ><br>
> > diff --git a/cpukit/libcsupport/src/pwdgrp.c b/cpukit/libcsupport/src/pwdgrp.c<br>
> > index f4a10f7..edc8aff 100644<br>
> > --- a/cpukit/libcsupport/src/pwdgrp.c<br>
> > +++ b/cpukit/libcsupport/src/pwdgrp.c<br>
> > @@ -36,7 +36,6 @@<br>
> > #include <stdint.h><br>
> ><br>
> > #include <rtems/seterr.h><br>
> > -#include <rtems/score/assert.h><br>
> ><br>
> > #include "pwdgrp.h"<br>
> ><br>
> > @@ -63,13 +62,13 @@ static void init_file(const char *name, const char<br>
> *content)<br>
> > */<br>
> > static void pwdgrp_init(void)<br>
> > {<br>
> > - int sc;<br>
> > -<br>
> > /*<br>
> > * Do the best to create this directory.<br>
> > + *<br>
> > + * /etc could be created by the network stack initialization or an initial<br>
> > + * filesystem image. Deliberately ignore the return value.<br>
> > */<br>
> > - sc = mkdir("/etc", S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);<br>
> > - _Assert_Unused_variable_equals(sc, 0);<br>
> > + (void) mkdir("/etc", S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);<br>
> ><br>
> <br>
> So this seems fine, but it got me thinking, does this issue affect all<br>
> the other mkdir-related patches recently done?<br>
> <br>
> Probably, we should use<br>
> _Assert(sc == 0 || errno == EEXIST);<br>
> (void) sc;<br>
> <br>
> If we are sure that it is OK for it to exist and that's the errno returned. :)<br>
> <br>
> But if it isn't, that's probably another bug.<br>
> <br>
> FWIW this seems typical of static analysis issues to me. FIx it wrong, then <br>
> fix it again a correct way, and then realize there was an even better way to<br>
> fix it. I presented a case at the Flight Software Workshop where I think I went<br>
> through multiple solutions before realizing B0 was special in POSIX to hang<br>
> up a line and the UART device driver had to say "not supported" instead of<br>
> an assert to avoid a divide by zero.<br>
> <br>
> These are often more subtle than they appear on first examination.<br>
<br>
This patch flip flop smells of deeper architecture issues. Ryan is doing a great<br>
job in handling these tough converity issues and he or anyone else can never<br>
know what is happening else where because nothing is specified. Every place /etc<br>
is needed has to add code to handle this and that is wrong. No where in RTEMS do<br>
we tolerate this type of coding.<br></blockquote><div><br></div><div>The grlib case was quite unique to them. It wasn't creating /dev as I recall, it was </div><div>creating a directory under /dev as I understood the code. This is one of them as</div><div>it is on the 5 branch.</div><div><br></div><div> strcpy(priv->prefix, "/dev/rastatmtc0");<br> priv->prefix[14] += dev->minor_drv;<br> mkdir(priv->prefix, S_IRWXU | S_IRWXG | S_IRWXO);<br> priv->prefix[15] = '/';<br> priv->prefix[16] = '\0';<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I think I said before the real solution is to add a SYSINIT or what ever that<br>
creates "the RTEMS" root fs. We document it and make is standard. We make the<br>
initialise point something drivers and code can depend on and then these make<br>
dirs etc can be stripped away.<br></blockquote><div><br></div><div>I'd tend to say a sysinit method for /etc. This will impact the libbsd code</div><div>but it would be better to put it in one place. No point in adding it to every</div><div>configuration even if not needed.</div><div><br></div><div>/dev is already there in sysinit because it is already part of the base</div><div>filesystem image. I don't see a second mkdir() on just it.<br><br>src/base_fs.c: rv = mkdir( "/dev", S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH );<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Chris<br>
</blockquote></div></div>