Re: [dpdk-dev] [PATCH] port: add file descriptor SWX port

[Thomas Monjalon <thomas@monjalon.net>]

23/03/2021 19:07, P, Venkata Suresh Kumar:
> Thanks a lot for reviewing the code and providing your comments.
> 
> I have addressed below comments in V2 patch.

OK thanks.

What about the question about rte_trace?
Opinions?



> From: Thomas Monjalon <thomas@monjalon.net> 
> 19/03/2021 13:02, Venkata Suresh Kumar P:
> > Add the file descriptor input/output port type for the SWX pipeline.
> 
> I think it deserves a bit more explanation about what is FD I/O port. -- [Suresh] - Addressed in V2 patch
> 
> >  /*
> > + * tap
> > + */
> > +#define TAP_DEV                                            "/dev/net/tun"
> 
> Spaces are free :) -- [Suresh] - Addressed in V2 patch
> 
> 
> > +#ifndef TRACE_LEVEL
> > +#define TRACE_LEVEL 0
> > +#endif
> > +
> > +#if TRACE_LEVEL
> > +#define TRACE(...) printf(__VA_ARGS__) #else #define TRACE(...) 
> > +#endif
> 
> Would you consider rte_trace?
> 
> > --- /dev/null
> > +++ b/lib/librte_port/rte_swx_port_fd.h
> > @@ -0,0 +1,57 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2016 Intel Corporation
> 
> I guess you did not create it in 2016. -- [Suresh] - Addressed in V2 patch
> 
> > + */
> > +
> > +#ifndef __INCLUDE_RTE_SWX_PORT_FD_H__ #define 
> > +__INCLUDE_RTE_SWX_PORT_FD_H__
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/**
> > + * @file
> > + * RTE SWX FD Input and Output Ports
> > + *
> > + ***/
> 
> Useless blank line. -- [Suresh] - Addressed in V2 patch
> 
> [...]
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif
> 
> A comment after such a far #endif is better: -- [Suresh] - Addressed in V2 patch
> 	/* __INCLUDE_RTE_SWX_PORT_FD_H__ */
> 
> > --- a/lib/librte_port/version.map
> > +++ b/lib/librte_port/version.map
> > @@ -48,4 +48,6 @@ EXPERIMENTAL {
> >         #added in 21.02
> 
> In 21.05
> 
> >         rte_swx_port_ring_reader_ops;
> >         rte_swx_port_ring_writer_ops;
> > +       rte_swx_port_fd_reader_ops;
> > +       rte_swx_port_fd_writer_ops;
> 
> Please sort in alphabetical order. -- [Suresh] - Addressed in V2 patch