Device Drivers Which Include mkdir("/dev")

[Joel Sherrill]

On Thu, Feb 18, 2021 at 3:28 PM Gedare Bloom <gedare at rtems.org> wrote:

> On Thu, Feb 18, 2021 at 2:18 PM Joel Sherrill <joel at rtems.org> wrote:
> >
> >
> >
> > On Thu, Feb 18, 2021 at 2:08 PM Gedare Bloom <gedare at rtems.org> wrote:
> >>
> >> On Thu, Feb 18, 2021 at 12:52 PM Joel Sherrill <joel at rtems.org> wrote:
> >> >
> >> >
> >> >
> >> > On Thu, Feb 18, 2021 at 11:52 AM Gedare Bloom <gedare at rtems.org>
> wrote:
> >> >>
> >> >> On Thu, Feb 18, 2021 at 10:20 AM Joel Sherrill <joel at rtems.org>
> wrote:
> >> >> >
> >> >> >
> >> >> >
> >> >> > On Thu, Feb 18, 2021 at 11:06 AM Gedare Bloom <gedare at rtems.org>
> wrote:
> >> >> >>
> >> >> >> On Thu, Feb 18, 2021 at 8:56 AM Joel Sherrill <joel at rtems.org>
> wrote:
> >> >> >> >
> >> >> >> > Hi
> >> >> >> >
> >> >> >> > There are a lot of Coverity issues related to device drivers
> which call mkdir("/dev") and ignore the return value. My first thought was
> that they should have (void) added since /dev could have been created by an
> earlier driver.
> >> >> >> >
> >> >> >> > Then it occurred to me that libcsupport/src/base_fs.c includes
> mkdir("/dev") and a fatal error if it cannot create it.
> >> >> >> >
> >> >> >> > Doesn't this mean that every call to mkdir("/dev") in a BSP or
> device driver is redundant? They should be removed since the base FS
> contents are always in place before any device drivers are called.
> >> >> >> >
> >> >> >> > Thoughts?
> >> >> >> >
> >> >> >>
> >> >> >> Seems reasonable. You could probably add an assert that /dev
> exists.
> >> >> >
> >> >> >
> >> >> > The creation of the device node will fail if /dev does not exist
> so I think that
> >> >> > is covered.
> >> >> >
> >> >> > I think the call is pointless and changing it to stat() to make
> sure it exists
> >> >> > just adds bulk to the driver/BSP dependencies.
> >> >> >
> >> >> > I think just deleting it seems prudent. There is an error path if
> it doesn't
> >> >> > exist.
> >> >> >
> >> >>
> >> >> ok. I was just suggesting conversion to an assert since that is some
> >> >> good practice when you have a belief/assumption but aren't totally
> >> >> convinced. It's fine with me either way. assert(mkdir("/dev") == -1)
> >> >> or just delete it.
> >> >
> >> >
> >> > Grrr.. I've looked again at the code and it is all Gaisler code doing
> something
> >> > like mkdir("/dev/leonXXX"). It really could fail. This should be a
> fatal error
> >> > and would seem to indicate that we need a grlib category of fatal
> BSP/driver
> >> > errors.
> >>
> >> Given the complexity of grlib, that would make some sense. Although
> >> maybe we can make it a little more generic... FATAL_THIRD_PARTY_DRIVER
> >> errors might be nicer on them, and can be reused
> >
> >
> > Good suggestion. But I think this particular error is potentially even
> more generic.
> > Any device driver wanting to make a subdirectory /dev/XXX could generate
> this.
> > This seems like the right place to add this but I'm open for better
> names:
> >
> > diff --git a/bsps/include/bsp/fatal.h b/bsps/include/bsp/fatal.h
> > index ec5902755e..0d8507e398 100644
> > --- a/bsps/include/bsp/fatal.h
> > +++ b/bsps/include/bsp/fatal.h
> > @@ -41,6 +41,7 @@ typedef enum {
> >    BSP_FATAL_CONSOLE_INSTALL_0,
> >    BSP_FATAL_CONSOLE_INSTALL_1,
> >    BSP_FATAL_CONSOLE_REGISTER_DEV_2,
> > +  BSP_FATAL_MAKE_SUBDIR_OF_DEV,
> >
> Please try to come up with a name that is consistent with the other
> patterns. If it can be any driver, I guess
> BSP_FATAL_DEVFS_INVALID_NAME
> or something?
>

That sounds like the device filesystem.

BSP_FATAL_CANNOT_MAKE_DIRECTORY?


>
> >    /* ARM fatal codes */
> >    BSP_ARM_A9MPCORE_FATAL_CLOCK_IRQ_INSTALL = BSP_FATAL_CODE_BLOCK(1),
> >
> >>
> >>
> >> >>
> >> >>
> >> >> > --joel
> >> >> >>
> >> >> >>
> >> >> >> > --joel
> >> >> >> > _______________________________________________
> >> >> >> > devel mailing list
> >> >> >> > devel at rtems.org
> >> >> >> > http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210218/687e11e4/attachment.html>