[dpdk-dev] [PATCH 2/7] Move EAL common functions

[Neil Horman]

On Thu, Dec 25, 2014 at 11:23:12AM -0800, Ravi Kerur wrote:
> Thanks Neil for reviews. Inline <rk>
> 
> On Thu, Dec 25, 2014 at 9:30 AM, Neil Horman <nhorman at tuxdriver.com> wrote:
> 
> > On Thu, Dec 25, 2014 at 10:33:12AM -0500, Ravi Kerur wrote:
> > > eal_debug.c has no difference between Linux and BSD, move
> > > into common directory.
> > > Rename eal_debug.c to eal_common_debug.c
> > > Makefile changes to reflect file move and name change.
> > > Fix checkpatch warnings.
> > >
> > > Signed-off-by: Ravi Kerur <rkerur at gmail.com>
> > > +
> > > +/* not implemented in this environment */
> > > +void rte_dump_registers(void)
> > > +{
> > > +}
> > Clearly this function has no use, instead of keeping it around, can you
> > please
> > remove it until someone works up the gumption to make it do something.
> > We're
> > just wasting an extra call instruction here so someone doesn't have to
> > write a
> > prototype in the future.  I don't see the value.
> >
> 
> <rk> This is existing code, I just removed "return" statement as per
> checkpatch. Should I make it "inline" and add a comment indicating to
> revisit whether to make it inline/no inline when the function is
> implemented?
> 
I understand its existing code, I'm saying that, while you're moving it around,
clean it up.  Don't make it inline, just remove it, since it does nothing.  If
you feel its important to keep around, I suppose you can make it inline, but I
don't really think its needed at all.

Neil

> >
> > > +/*
> > > + * Like rte_panic this terminates the application. However, no
> > traceback is
> > > + * provided and no core-dump is generated.
> > > + */
> > > +void
> > > +rte_exit(int exit_code, const char *format, ...)
> > > +{
> > > +     va_list ap;
> > > +
> > > +     /* disable history */
> > > +     rte_log_set_history(0);
> > > +
> > > +     if (exit_code != 0)
> > > +             RTE_LOG(CRIT, EAL, "Error - exiting with code: %d\n"
> > > +                             "  Cause: ", exit_code);
> > > +
> > > +     va_start(ap, format);
> > > +     rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
> > > +     va_end(ap);
> > > +
> > > +#ifndef RTE_EAL_ALWAYS_PANIC_ON_ERROR
> > > +     exit(exit_code);
> > > +#else
> > > +     rte_dump_stack();
> > > +     rte_dump_registers();
> > > +     abort();
> > > +#endif
> > This doesn't match with the commentary above.  If rte_exit isn't meant to
> > provide a traceback, it shouldn't do so.  If an application wants that to
> > happen, then they need to use rte_panic.
> >
> > <rk> This is again existing code. I can change the comment which matches
> the function, will it work?