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

[Gedare Bloom]

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

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