Criticise my code please.

[Clunixchit]

I would appreciate if you can criticise my code as well as if possible
give advice on how to improve it.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
#include <fcntl.h>

#define LINE_LENGTH 20
#define nLINES 10
#define MYAIRC "myAIc"

typedef struct Loading {
char *name; // Name of the program
int pid; // current pid of the program
struct Loading *prec;
} Loading ;

/* ----array_to_structure
* n = number of lines in the file
*/

void array_to_structure ( Loading **myLoad , char
tmp[nLINES][LINE_LENGTH] , int n ) {
int i=0;
Loading *program;

while ( i < n )
{ //printf("1.%s\n",tmp);
Loading *program = malloc ( sizeof(Loading) );
if (!program){
fprintf(stdout,"program %s
failed",tmp);
return;
}
(*program).name = malloc (
strlen(tmp) + 1 );
strcpy( (*program).name,tmp );
(*program).pid = -1;
(*program).prec = *myLoad;
*myLoad=program;
i++;
}

}
void find_comment(char *tline) {
char *comment;

comment = strchr(tline, '\n');/* Replace \n with \0 for
consistency. */
if (comment != NULL) {
*comment = '\0';
}
}

void config_file ( Loading **myLoad , char *config_file
){
int i=0,j=0;
char tmp[nLINES][LINE_LENGTH];

FILE *stream = fopen( config_file , "r" );
if ( stream == NULL ) {
fprintf (stdout , "fopen %s failed" , *config_file
);
return;
}

while( fgets( tmp , LINE_LENGTH-1 , stream )
!= NULL ){
size_t l = strlen(tmp);
if (l > 0 && tmp[l-1] ==
'\n')
tmp[l-1] = '\0';
//printf("%zu
%s\n",strlen(tmp),tmp);
i++;
}
fclose (stream);
j=i;
while(j>=0) {
find_comment(tmp[j]);
j--;
}

array_to_structure(myLoad,tmp,i);
}

void view( Loading *p ) {
char *arg_list[]={(*p).name,NULL};

if ( fork() == 0 ) {
(*p).pid = getpid();
printf("Loading: %s
(%d)\n",(*p).name,(*p).pid);
if (execvp((*p).name,arg_list)) {
perror("execvp");
return;
}

}
}

void view_structure( Loading *T ) {
while ( T ){
view(T);
T=T->prec;
}
}

void clear ( Loading **T) {
Loading *tmp;
while(*T){
tmp=(**T).prec;
free(*T);
*T=tmp;
}
}

int main () {
Loading *myLoad = NULL;

printf("\n---- myAI mainboard:
%d\n",getpid());
config_file(&myLoad,MYAIRC);
view_structure(myLoad);
wait(NULL);
clear(&myLoad);
printf("\n");
return 0;
}

 

[Robert Gamble]

I would appreciate if you can criticise my code as well as if possible
give advice on how to improve it.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/types.h>

Not a Standard C header

#include <sys/wait.h>

Not a Standard C header

#include <unistd.h>

Not a Standard C header

[snip]

Your code utilizes non-Standard functionality which makes it off-topic
here. comp.unix.programmer would be more appropriate, but before you
run over there and post your code, you might want to explain what your
code is supposed to do and clearly articulate what your questions are.
Most people won't be too receptive to "Please figure out what my code
does and QA it for me for free".

Robert Gamble

 

[Ben Pfaff]

I would appreciate if you can criticise my code as well as if possible
give advice on how to improve it.

As a first comment, your indentation style seems completely random:

[...]

void array_to_structure ( Loading **myLoad , char
tmp[nLINES][LINE_LENGTH] , int n ) {
int i=0;
Loading *program;

while ( i < n )
{ //printf("1.%s\n",tmp);
Loading *program = malloc ( sizeof(Loading) );
if (!program){
fprintf(stdout,"program %s
failed",tmp);
return;
}

 

[Barry Schwarz]

On Wed, 1 Jun 2005 09:42:44 +0000 (UTC),

I would appreciate if you can criticise my code as well as if possible
give advice on how to improve it.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
#include <fcntl.h>

#define LINE_LENGTH 20
#define nLINES 10
#define MYAIRC "myAIc"

typedef struct Loading {
char *name; // Name of the program
int pid; // current pid of the program
struct Loading *prec;
} Loading ;

/* ----array_to_structure
* n = number of lines in the file
*/

void array_to_structure ( Loading **myLoad , char
tmp[nLINES][LINE_LENGTH] , int n ) {
int i=0;
Loading *program;

while ( i < n )

Is there a reason you didn't use the more natural for loop?

{ //printf("1.%s\n",tmp);
Loading *program = malloc ( sizeof(Loading) );
if (!program){
fprintf(stdout,"program %s
failed",tmp);
return;

At this point, how is the calling function supposed to know the
program failed? Should you set *myLoad to NULL? Then again, how will
the calling function know on which iteration this function failed?

}
(*program).name = malloc (
strlen(tmp) + 1 );
strcpy( (*program).name,tmp );
(*program).pid = -1;
(*program).prec = *myLoad;
*myLoad=program;
i++;
}

}
void find_comment(char *tline) {

A strange title for a function that doesn't look for comments.

char *comment;

comment = strchr(tline, '\n');/* Replace \n with \0 for
consistency. */
if (comment != NULL) {
*comment = '\0';
}
}

void config_file ( Loading **myLoad , char *config_file
){
int i=0,j=0;
char tmp[nLINES][LINE_LENGTH];

FILE *stream = fopen( config_file , "r" );
if ( stream == NULL ) {
fprintf (stdout , "fopen %s failed" , *config_file
);
return;
}

while( fgets( tmp , LINE_LENGTH-1 , stream )

fgets already subtracts the 1 for you.

!= NULL ){
size_t l = strlen(tmp);

C89 does not support declarations after executable code in a block.

if (l > 0 && tmp[l-1] ==
'\n')
tmp[l-1] = '\0';
//printf("%zu
%s\n",strlen(tmp),tmp);
i++;
}
fclose (stream);
j=i;
while(j>=0) {

A for loop would be simpler.

find_comment(tmp[j]);

You already removed the \n above. Why are you trying to remove it
again?

j--;
}

array_to_structure(myLoad,tmp,i);
}

void view( Loading *p ) {
char *arg_list[]={(*p).name,NULL};

if ( fork() == 0 ) {
(*p).pid = getpid();
printf("Loading: %s
(%d)\n",(*p).name,(*p).pid);
if (execvp((*p).name,arg_list)) {
perror("execvp");
return;
}

}
}

void view_structure( Loading *T ) {
while ( T ){
view(T);
T=T->prec;
}
}

void clear ( Loading **T) {
Loading *tmp;
while(*T){
tmp=(**T).prec;
free(*T);

You forgot to free (*T).name first.

*T=tmp;
}
}

int main () {
Loading *myLoad = NULL;

printf("\n---- myAI mainboard:
%d\n",getpid());
config_file(&myLoad,MYAIRC);
view_structure(myLoad);
wait(NULL);
clear(&myLoad);
printf("\n");
return 0;
}

<<Remove the del for email>>

 

[Michael Knaup]

I would appreciate if you can criticise my code as well as if possible
give advice on how to improve it.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
#include <fcntl.h>

#define LINE_LENGTH 20
#define nLINES 10
#define MYAIRC "myAIc"

typedef struct Loading {
char *name; // Name of the program
int pid; // current pid of the program
struct Loading *prec;
} Loading ;

using C99 better
typedef struct Loading {
struct Loading *prec;
pid_t pid;
char name[];
} Loading;

/* ----array_to_structure
* n = number of lines in the file
*/

void array_to_structure ( Loading **myLoad , char
tmp[nLINES][LINE_LENGTH] , int n ) {

Better return the number of generated list items, so the caller
can check for success
n == array_to_structure(*,*,n)

int i=0;
Loading *program;

What happens if myLoad == NULL ?

while ( i < n )

use for(i = 0; i < n; ++i) instead

{//printf("1.%s\n",tmp);
Loading *program = malloc ( sizeof(Loading) );
if (!program){
fprintf(stdout,"program %s
failed",tmp);
return;
}
(*program).name = malloc (
strlen(tmp) + 1 );
strcpy( (*program).name,tmp );

A faster and more memory efficient solution using the modified Loading type
would be

for(i = 0; i < n; ++i)
if (NULL != tmp){
size_t len = strlen(tmp + 1u);
program = malloc(sizeof(Loading) + len);
if (NULL == program) {
perror(tmp);
break;
}
program->prec = *myLoad;
program->pid = -1;
(void)memcpy(program->name, tmp, len);

*myLoad = program;
}
return i;
}

(*program).pid = -1;
(*program).prec = *myLoad;

use program->prec instead

*myLoad=program;
i++;
}

}
void find_comment(char *tline) {
char *comment;

comment = strchr(tline, '\n');/* Replace \n with \0 for
consistency. */
if (comment != NULL) {
*comment = '\0';
}
}

drop the whole function

void config_file ( Loading **myLoad , char *config_file
){

double use of config_file use filename instead

How can the caller check if there were no error ?

int i=0,j=0;
char tmp[nLINES][LINE_LENGTH];

What happens if myLoad == NULL or config_file == NULL ?

FILE *stream = fopen( config_file , "r" );
if ( stream == NULL ) {
fprintf (stdout , "fopen %s failed" , *config_file
);

Write to stderr instead or just use
perror(file_name);

return;
}

This whole loop leads to a memory access failure if more then
nLINES are in the file. So again better

for (i = 0; i < nLINES; ++i) {
if (NULL == fgets(tmp, LINE_LENGTH-1, stream)) {
if (feof(stream)
break;
perror(file_name);
...
}

while( fgets( tmp , LINE_LENGTH-1 , stream )
!= NULL ){

Here you are doing the same thing twice better do it once
{
char *ptr =strchr(tmp, '\n');
if (ptr)
*ptr = '\000';
and maybe you want do drop empty lines
if ('\000' == tmp[0])
--i
}

size_t l = strlen(tmp);
if (l > 0 && tmp[l-1] ==
'\n')
tmp[l-1] = '\0';
//printf("%zu
%s\n",strlen(tmp),tmp);
i++;
}
fclose (stream);
j=i;
while(j>=0) {
find_comment(tmp[j]);
j--;
}

array_to_structure(myLoad,tmp,i);
}

void view( Loading *p ) {
char *arg_list[]={(*p).name,NULL};

if ( fork() == 0 ) {

fork may fail and nobody gets a message

(*p).pid = getpid();
printf("Loading: %s
(%d)\n",(*p).name,(*p).pid);

The check here is not necessary if execvp was successfull the code is
"overridden". On the other side if it fails you should exit the
childprocess here exit(EXIT_FAILURE) otherwise you will have your programm
running twice

if (execvp((*p).name,arg_list)) {
perror("execvp");
return;
}

}
}

void view_structure( Loading *T ) {
while ( T ){
view(T);
T=T->prec;
}
}

void clear ( Loading **T) {
Loading *tmp;
while(*T){
tmp=(**T).prec;
free(*T);

With the above change using C99 this is correct otherwise you will have to
add free((*T)->name) above your call to free

*T=tmp;
}
}

int main () { int main(void) is correct
Loading *myLoad = NULL;

printf("\n---- myAI mainboard:
%d\n",getpid());
config_file(&myLoad,MYAIRC);
view_structure(myLoad);
wait(NULL);

If any of your subtask finished the wait will "come back"

 

[Dietmar Schindler]

On Wed, 1 Jun 2005 09:42:44 +0000 (UTC),

!= NULL ){
size_t l = strlen(tmp);

C89 does not support declarations after executable code in a block.

There is no declaration after executable code in the block.

 

[pete]

On Wed, 1 Jun 2005 09:42:44 +0000 (UTC),

!= NULL ){
size_t l = strlen(tmp);

C89 does not support declarations after executable code in a block.

There is no declaration after executable code in the block.

.... and
C89 doesn't support declarations after *statements* in a block,
but object definitions, especially ones with initialization,
are executable code themselves
and may be followed by various declarations.