[PATCH] Record-client program to read from a file for --input=<FILE> flag

Gedare Bloom gedare at rtems.org
Fri Jun 28 16:20:02 UTC 2019


On Fri, Jun 28, 2019 at 10:15 AM Gedare Bloom <gedare at rtems.org> wrote:
>
> On Fri, Jun 28, 2019 at 5:02 AM Ravindra Meena <rmeena840 at gmail.com> wrote:
> >
> > ---
> >  misc/record/record-main.c | 35 ++++++++++++++++++++++++-----------
> >  1 file changed, 24 insertions(+), 11 deletions(-)
> >
> > diff --git a/misc/record/record-main.c b/misc/record/record-main.c
> > index b4591e3..14023e2 100644
> > --- a/misc/record/record-main.c
> > +++ b/misc/record/record-main.c
> > @@ -39,6 +39,8 @@
> >  #include <string.h>
> >  #include <unistd.h>
> >  #include <inttypes.h>
> > +#include<fcntl.h>
> > +#include<errno.h>
> >
> add space before <
>
> when writing code, aim to be consistent with surrounding code, and
> with coding style rules
>
> >  #include <netinet/in.h>
> >  #include <arpa/inet.h>
> > @@ -52,6 +54,7 @@ static const struct option longopts[] = {
> >    { "host", 1, NULL, 'H' },
> >    { "port", 1, NULL, 'p' },
> >    { "items", 1, NULL, 'i' },
> > +  { "input", 1, NULL, 'f' },
>
> I know you were asked to add --input, however, since 'i' is already
> used, it might be sensible to use --file= and 'f'?
>
> >    { NULL, 0, NULL, 0 }
> >  };
> >
> > @@ -111,32 +114,35 @@ RB_GENERATE_INTERNAL( active, client_item, active_node, item_cmp, static inline
> >  static void usage( char **argv )
> >  {
> >    printf(
> > -    "%s [--host=HOST] [--port=PORT] [--items=ITEMS]\n"
> > +    "%s [--host=HOST] [--port=PORT] [--items=ITEMS] [--input=INPUT]\n"
> >      "\n"
> >      "Mandatory arguments to long options are mandatory for short options too.\n"
> > -    "  -h, --help                 print this help text\n"
> > -    "  -H, --host=HOST            the host IPv4 address of the record server\n"
> > -    "  -p, --port=PORT            the TCP port of the record server\n"
> > -    "  -i, --items=ITEMS          the maximum count of active record items\n",
> > +    "  -h,      --help             print this help text\n"
> > +    "  -H,      --host=HOST        the host IPv4 address of the record server\n"
> > +    "  -p,      --port=PORT        the TCP port of the record server\n"
> > +    "  -i,      --items=ITEMS      the maximum count of active record items\n"
> Why is this reformatting the existing strings?
>
> > +    "  -input,  --input=INPUT      the file input\n",
> >      argv[ 0 ]
> >    );
> >  }
> >
> > -static int connect_client( const char *host, uint16_t port )
> > +static int connect_client( const char *host, uint16_t port, const char *input_file  ,bool input_file_flag )
>
> This is > 80 characters.
> https://devel.rtems.org/wiki/Developer/Coding/Conventions#Formatting
>
I guess this is not RTEMS code, so you should refer to your mentor
what would be the coding standard in use.

> The space should go after the comma, not before it, following
> input_file parameter name.
>
> Depending what variant C this is, bool may not be a defined type.
>
> Regarding input_file_flag, if it is 'false', then what is the value of
> 'input_file'? If it is 'true', is the value of 'input_file' possibly
> the same as when the flag is 'false'? If there is a distinct
> difference between 'input_value', can you just check for that
> difference instead of the flag?  Think about it for a bit.
>
> I don't think it makes sense to expand this function to add the file
> processing. You're trying to make this function do too much.
>
> >  {
> >    struct sockaddr_in in_addr;
> >    int fd;
> >    int rv;
> >
> > -  fd = socket( PF_INET, SOCK_STREAM, 0 );
> > +  fd = ( input_file_flag ) ? open( input_file, O_RDONLY ) : socket( PF_INET, SOCK_STREAM, 0 );
> This is also > 80 characters
>
> >    assert( fd >= 0 );
> >
> >    memset( &in_addr, 0, sizeof( in_addr ) );
> >    in_addr.sin_family = AF_INET;
> >    in_addr.sin_port = htons( port );
> >    in_addr.sin_addr.s_addr = inet_addr( host );
> > +  if( !input_file_flag ){
> >    rv = connect( fd, (struct sockaddr *) &in_addr, sizeof( in_addr ) );
> >    assert( rv == 0 );
> > +  }
> This code is superfluous when there is not an input file.
>
> >
> >    return fd;
> >  }
> > @@ -268,6 +274,8 @@ int main( int argc, char **argv )
> >    client_item *items;
> >    const char *host;
> >    uint16_t port;
> > +  const char *input_file;
> > +  bool input_file_flag = false;
> >    int fd;
> >    int rv;
> >    int opt;
> > @@ -280,7 +288,7 @@ int main( int argc, char **argv )
> >    n = RTEMS_RECORD_CLIENT_MAXIMUM_CPU_COUNT * 1024 * 1024;
> >
> >    while (
> > -    ( opt = getopt_long( argc, argv, "hH:p:i:", &longopts[0], &longindex ) )
> > +    ( opt = getopt_long( argc, argv, "hH:p:i:f", &longopts[0], &longindex ) )
> >        != -1
> >    ) {
> >      switch ( opt ) {
> > @@ -297,6 +305,11 @@ int main( int argc, char **argv )
> >        case 'i':
> >          n = (size_t) strtoul( optarg, NULL, 10 );
> >          break;
> > +      case 'f':
> > +        input_file = optarg;
> > +        assert( input_file != NULL );
> > +        input_file_flag = true;
> another hint for you: (input_file != NULL) == input_file_flag.
>
> > +        break;
> >        default:
> >          exit( EXIT_FAILURE );
> >          break;
> > @@ -320,15 +333,15 @@ int main( int argc, char **argv )
> >      SLIST_INSERT_HEAD( &cctx.free_items, &items[ i ], free_node );
> >    }
> >
> > -  fd = connect_client( host, port );
> > +  fd = connect_client( host, port , input_file, input_file_flag );
> You should split this here, based on whether it is host/port
> connection or opening a file.
>
> >    rtems_record_client_init( &ctx, handler, &cctx );
> >
> >    while ( true ) {
> >      int buf[ 8192 ];
> >      ssize_t n;
> >
> > -    n = recv( fd, buf, sizeof( buf ), 0 );
> > -    if ( n >= 0 ) {
> > +    n = ( input_file_flag ) ? read(fd, buf, 10) : recv( fd, buf, sizeof( buf ), 0 );
> What is this '10' for?
>
> > +    if ( n > 0 ) {
> >        rtems_record_client_run( &ctx, buf, (size_t) n );
> >      } else {
> >        break;
> > --
> > 2.7.4
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel



More information about the devel mailing list