Help the helpless.. code read

[Mark Richards]

Re: Help the helpless.. code read

I'm so greatful for all the help, encouragement, and kicks in the arse
that I've received here. As a result, I tried to apply all the good
advice, and worked through the problem as carefully as my ability and
experience has enabled. It was especially helpful to try and build a
test application which duplicates the functions in the application I'm
attempting to understand. I found that this is a really great way to
learn and breaks things down into understandable segments.

I'd really appreciate it if you might have a look at this code and offer
up criticisms, or reiterate those that I unintentionally overlooked.
I'm certain that better minds than mine will have a few.

Thanks!


(To generate the executable:
gcc -o testapp testapp.c && ./testapp && rm -f testapp.o
Save the source as testapp.c)



// BEGIN testapp.c

//This generates a small file called file.txt. file.txt is a replica of
//a segment of the file that the larger application uses to store
//various data, including the ROM address.

#include <strings.h>
#include <string.h>
#include <stdio.h>
#include <stdlib.h>

struct _roms
{
unsigned char *roms;
int max;
};


int initlist(struct _roms *sensor_list)
{

int sensors;
sensors = 1;

sensor_list->roms = malloc( sensors * 8 );
sensor_list->max = sensors;

return 0;
}

int loadstring(char *fname, struct _roms *sensor_list)
{
int x;
char astring[17];
char *ptrstring;
char *ptr;
int sensors=0;
char temp[1024];
FILE *fp;


// create the file that contains the string address
if( ( fp = fopen( fname, "w" ) ) == NULL )
{
printf("Error creating file\n");
return 1;
}

// write string address to file
if (fputs("0x12 0xBA 0xA9 0x1C 0x00 0x00 0x00 0xA3",fp) == 0)
{
printf("Error writing file\n");
return 1;
}
fclose(fp);

// re-open file and read contents into temp
if( ( fp = fopen( fname, "r" ) ) == NULL )
{
printf("Error opening file\n");
return 1;
}

while( fgets( temp, 80, fp ) != 0 )
{

if( (temp[0] == '\n') || (temp[0] == '#') ) continue;
ptr = strtok(temp, " \t\n");

sensors = 0;
for(x = 0; x < 8; x++)
{
sensor_list->roms[(sensors * 8) + x] = strtol(ptr, (char **) NULL,0);
ptr = strtok(NULL, " \t\n");
}
sensor_list->max = 1;
}
fclose(fp);

return 0;
}


int printit(struct _roms *sensor_list)
{
unsigned char *ptr;
int x,y;
unsigned char cstring[17];


for( x = 0; x < sensor_list->max; x++ )
{
printf("ROM %d = ", x );

for( y = 0; y < 8; y++ )
{
printf( "0x%02X ", sensor_list->roms[(x * 8) + y] );
}
printf( "\n" );
}

for( x = 0; x < sensor_list->max; x++ )
{
printf("ROM %d = ", x );

for( y = 0; y < 8; y++ )
{
printf( "%d ", sensor_list->roms[(x * 8) + y] );
}
printf( "\n" );
}


return 0;
}


int main(int argc, char *argv[])
{

// *roms is an array of 8 bytes
// max is the maximum number of entries
struct _roms sensor_list;

int sensors;
int x;
char conf_file[1024];
char *ptr;

printf("Start of test application\n") ;

strcpy(conf_fil e, "file.txt") ;

bzero(&sensor_l ist, sizeof(struct _roms));

x = initlist(&senso r_list);
x = loadstring(conf _file,&sensor_l ist);
x = printit(&sensor _list);

printf("End of test application\n") ;
return 0;
}

// END testapp.c

[Allin Cottrell]

Re: Help the helpless.. code read

Mark Richards wrote:
[color=blue]
> // BEGIN testapp.c
>
> //This generates a small file called file.txt. file.txt is a replica of
> //a segment of the file that the larger application uses to store
> //various data, including the ROM address.
>
> #include <strings.h>[/color]

non-standard header: dump it.
[color=blue]
> #include <string.h>
> #include <stdio.h>
> #include <stdlib.h>
>
> struct _roms
> {
> unsigned char *roms;
> int max;
> };
>
>
> int initlist(struct _roms *sensor_list)
> {
>
> int sensors;
> sensors = 1;
>
> sensor_list->roms = malloc( sensors * 8 );
> sensor_list->max = sensors;
>
> return 0;
> }[/color]

You'd be better off passing in "sensors" (the number of sensors, which
I would suggest renaming as "n_sensors" ) as a parameter to this
function. You should also check that malloc didn't return NULL
(and return an error code to the caller if it did).
[color=blue]
>
> int loadstring(char *fname, struct _roms *sensor_list)
> {
> int x;
> char astring[17];
> char *ptrstring;[/color]

Unused variables astring, ptrstring.
[color=blue]
> char *ptr;
> int sensors=0;
> char temp[1024];
> FILE *fp;
>
>
> // create the file that contains the string address
> if( ( fp = fopen( fname, "w" ) ) == NULL )
> {
> printf("Error creating file\n");
> return 1;
> }
>
> // write string address to file
> if (fputs("0x12 0xBA 0xA9 0x1C 0x00 0x00 0x00 0xA3",fp) == 0)[/color]

Better to put '\n' to terminate the line.
[color=blue]
> {
> printf("Error writing file\n");
> return 1;
> }
> fclose(fp);
>
> // re-open file and read contents into temp
> if( ( fp = fopen( fname, "r" ) ) == NULL )
> {
> printf("Error opening file\n");
> return 1;
> }
>
> while( fgets( temp, 80, fp ) != 0 )[/color]

plain "while (fgets(temp, 80, fp))" is more idiomatic. Or
"while (fgets(temp, sizeof temp, fp))" to avoid magic numbers.
[color=blue]
> {
>
> if( (temp[0] == '\n') || (temp[0] == '#') ) continue;
> ptr = strtok(temp, " \t\n");[/color]

strtok is deprecated hereabouts, because it messes up the original
string (sticks in '\0' characters). Better to use strcspn or
(better still) sscanf to pick out the numeric fields in the line.
[color=blue]
>
> sensors = 0;
> for(x = 0; x < 8; x++)
> {
> sensor_list->roms[(sensors * 8) + x] = strtol(ptr, (char **) NULL,0);[/color]

strtol is regarded as "better" than plain atoi on account of the error-
checking it affords. But you're doing no error checking here, so atoi
would do the same job. As mentioned above, sscanf would be better
anyway (check that you get 8 valid ints in the line).
[color=blue]
> ptr = strtok(NULL, " \t\n");
> }
> sensor_list->max = 1;[/color]

You mean, sensor_list->max += 1 (with the value having been initialized
to zero prior to reading)?
[color=blue]
> }
> fclose(fp);
>
> return 0;
> }
>
>
> int printit(struct _roms *sensor_list)
> {
> unsigned char *ptr;
> int x,y;
> unsigned char cstring[17];[/color]

unused variables ptr and cstring.[color=blue]
>
>
> for( x = 0; x < sensor_list->max; x++ )
> {
> printf("ROM %d = ", x );
>
> for( y = 0; y < 8; y++ )
> {
> printf( "0x%02X ", sensor_list->roms[(x * 8) + y] );
> }
> printf( "\n" );
> }
>
> for( x = 0; x < sensor_list->max; x++ )
> {
> printf("ROM %d = ", x );
>
> for( y = 0; y < 8; y++ )
> {
> printf( "%d ", sensor_list->roms[(x * 8) + y] );
> }
> printf( "\n" );
> }
>
>
> return 0;
> }
>
>
> int main(int argc, char *argv[])
> {
>
> // *roms is an array of 8 bytes
> // max is the maximum number of entries
> struct _roms sensor_list;
>
> int sensors;
> int x;
> char conf_file[1024];
> char *ptr;[/color]

Unused variable ptr.
[color=blue]
>
> printf("Start of test application\n") ;
>
> strcpy(conf_fil e, "file.txt") ;
>
> bzero(&sensor_l ist, sizeof(struct _roms));[/color]

Don't use the non-standard bzero. Use an intializer:

struct _roms sensor_list = {0};
[color=blue]
>
> x = initlist(&senso r_list);[/color]

Rename "x" to "err" (or similar), and react appropriately
if one of the functions returns a non-zero value.
[color=blue]
> x = loadstring(conf _file,&sensor_l ist);
> x = printit(&sensor _list);
>
> printf("End of test application\n") ;
> return 0;
> }
>
> // END testapp.c[/color]

[Barry Schwarz]

Re: Help the helpless.. code read

On Fri, 18 Jun 2004 15:36:38 -0400, Mark Richards
<nospam@massmic ro.com> wrote:
[color=blue]
>I'm so greatful for all the help, encouragement, and kicks in the arse
>that I've received here. As a result, I tried to apply all the good
>advice, and worked through the problem as carefully as my ability and
>experience has enabled. It was especially helpful to try and build a
>test application which duplicates the functions in the application I'm
>attempting to understand. I found that this is a really great way to
>learn and breaks things down into understandable segments.
>
>I'd really appreciate it if you might have a look at this code and offer
>up criticisms, or reiterate those that I unintentionally overlooked.
>I'm certain that better minds than mine will have a few.
>
>Thanks!
>
>
>(To generate the executable:
>gcc -o testapp testapp.c && ./testapp && rm -f testapp.o
>Save the source as testapp.c)
>
>
>
>// BEGIN testapp.c
>
>//This generates a small file called file.txt. file.txt is a replica of
>//a segment of the file that the larger application uses to store
>//various data, including the ROM address.
>
>#include <strings.h>
>#include <string.h>
>#include <stdio.h>
>#include <stdlib.h>
>
>struct _roms
> {
> unsigned char *roms;
> int max;
> };
>
>
>int initlist(struct _roms *sensor_list)[/color]

There doesn't seem to be any point to the return value of this
function. It is always 0.
[color=blue]
>{
>
> int sensors;
> sensors = 1;
>
> sensor_list->roms = malloc( sensors * 8 );
> sensor_list->max = sensors;
>
> return 0;
>}
>
>int loadstring(char *fname, struct _roms *sensor_list)
>{
> int x;
> char astring[17];
> char *ptrstring;
> char *ptr;
> int sensors=0;
> char temp[1024];
> FILE *fp;
>
>
> // create the file that contains the string address
> if( ( fp = fopen( fname, "w" ) ) == NULL )
> {
> printf("Error creating file\n");
> return 1;
> }
>
> // write string address to file
> if (fputs("0x12 0xBA 0xA9 0x1C 0x00 0x00 0x00 0xA3",fp) == 0)[/color]

fputs returns EOF on error. A return of 0, or any other non-negative
value, indicates success.
[color=blue]
> {
> printf("Error writing file\n");
> return 1;
> }
> fclose(fp);
>
> // re-open file and read contents into temp
> if( ( fp = fopen( fname, "r" ) ) == NULL )
> {
> printf("Error opening file\n");
> return 1;
> }
>
> while( fgets( temp, 80, fp ) != 0 )[/color]

fgets returns a pointer. While comparing a pointer to 0 is legal, it
is stylistically more informative to use NULL.

You wrote exactly 40 characters to the file. The data you wrote did
not include a '\n'. Telling fgets to read 80 characters is guaranteed
to cause end of file to occur. This will cause fgets to return NULL
and the while expression can never evaluate to true.
[color=blue]
> {
>
> if( (temp[0] == '\n') || (temp[0] == '#') ) continue;[/color]

These characters do not appear in your sample string.
[color=blue]
> ptr = strtok(temp, " \t\n");[/color]

Only the space appears in your sample.
[color=blue]
>
> sensors = 0;
> for(x = 0; x < 8; x++)
> {
> sensor_list->roms[(sensors * 8) + x] = strtol(ptr, (char **) NULL,0);[/color]

The cast is superfluous. Since there is a prototype in scope, the
compiler will automatically convert NULL from its defined type to
char**.
[color=blue]
> ptr = strtok(NULL, " \t\n");
> }
> sensor_list->max = 1;
> }
> fclose(fp);
>
> return 0;
>}
>
>
>int printit(struct _roms *sensor_list)[/color]

There doesn't seem to be any point to the return value of this
function. It is always 0.
[color=blue]
>{
> unsigned char *ptr;
> int x,y;
> unsigned char cstring[17];
>
>
> for( x = 0; x < sensor_list->max; x++ )
> {
> printf("ROM %d = ", x );
>
> for( y = 0; y < 8; y++ )
> {
> printf( "0x%02X ", sensor_list->roms[(x * 8) + y] );
> }
> printf( "\n" );
> }
>
> for( x = 0; x < sensor_list->max; x++ )
> {
> printf("ROM %d = ", x );
>
> for( y = 0; y < 8; y++ )
> {
> printf( "%d ", sensor_list->roms[(x * 8) + y] );
> }
> printf( "\n" );
> }
>
>
> return 0;
>}
>
>
>int main(int argc, char *argv[])[/color]

Since you don't use either parameter, you can get rid of an annoying
diagnostic that many compilers produce by using
int main(void)
[color=blue]
>{
>
> // *roms is an array of 8 bytes[/color]

As several of us have already stated, neither roms nor *roms is an
array.
[color=blue]
> // max is the maximum number of entries
> struct _roms sensor_list;
>
> int sensors;
> int x;
> char conf_file[1024];
> char *ptr;
>
> printf("Start of test application\n") ;
>
> strcpy(conf_fil e, "file.txt") ;
>
> bzero(&sensor_l ist, sizeof(struct _roms));[/color]

This does not do anything useful and the resulting value in
sensor_list.rom s may not be legal. Of course, it's also not a
standard function so maybe it does do something useful.
[color=blue]
>
> x = initlist(&senso r_list);[/color]

Since you don't use the return value, there doesn't seem to be any
point in assigning it to x.
[color=blue]
> x = loadstring(conf _file,&sensor_l ist);[/color]

You should be checking this for success.
[color=blue]
> x = printit(&sensor _list);
>
> printf("End of test application\n") ;
> return 0;
>}
>
>// END testapp.c[/color]



<<Remove the del for email>>

[Al Bowers]

Re: Help the helpless.. code read



Barry Schwarz wrote:

[color=blue][color=green]
>> // write string address to file
>> if (fputs("0x12 0xBA 0xA9 0x1C 0x00 0x00 0x00 0xA3",fp) == 0)[/color]
>[/color]
[color=blue][color=green]
>>
>> while( fgets( temp, 80, fp ) != 0 )[/color]
>[/color]
[color=blue]
> You wrote exactly 40 characters to the file. The data you wrote did
> not include a '\n'. Telling fgets to read 80 characters is guaranteed
> to cause end of file to occur. This will cause fgets to return NULL
> and the while expression can never evaluate to true.
>[/color]

That is not correct. The Standard says

char *fgets(char * restrict s, int n,
FILE * restrict stream);


The fgets function returns s if successful. If end-of-file is
encountered and NO characters have been read into the array,
^^
the contents of the array remain unchanged and a null pointer
is returned.

--
Al Bowers
Tampa, Fl USA
mailto: xabowers@myrapi dsys.com (remove the x to send email)

http://www.geocities.com/abowers822/

[Barry Schwarz]

Re: Help the helpless.. code read

On Sat, 19 Jun 2004 09:41:28 -0400, Al Bowers <xabowers@rapid sys.com>
wrote:
[color=blue]
>
>
>Barry Schwarz wrote:
>
>[color=green][color=darkred]
>>> // write string address to file
>>> if (fputs("0x12 0xBA 0xA9 0x1C 0x00 0x00 0x00 0xA3",fp) == 0)[/color]
>>[/color]
>[color=green][color=darkred]
>>>
>>> while( fgets( temp, 80, fp ) != 0 )[/color]
>>[/color]
>[color=green]
>> You wrote exactly 40 characters to the file. The data you wrote did
>> not include a '\n'. Telling fgets to read 80 characters is guaranteed
>> to cause end of file to occur. This will cause fgets to return NULL
>> and the while expression can never evaluate to true.
>>[/color]
>
>That is not correct. The Standard says
>
>char *fgets(char * restrict s, int n,
> FILE * restrict stream);
>
>
>The fgets function returns s if successful. If end-of-file is
>encountered and NO characters have been read into the array,
> ^^
>the contents of the array remain unchanged and a null pointer
>is returned.[/color]

I should have checked the standard rather than depend on the brief
description in K&R, pg 247.


<<Remove the del for email>>