On Fri, Jun 28, 2019 at 10:15 AM Gedare Bloom <ged...@rtems.org> wrote:
>
> On Fri, Jun 28, 2019 at 5:02 AM Ravindra Meena <rmeena...@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@rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
