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[i]);
Loading *program = malloc ( sizeof(Loading) );
if (!program){
fprintf(stdout,"program %s
failed",tmp[i]);
return;
}
(*program).name = malloc (
strlen(tmp[i]) + 1 );
strcpy( (*program).name,tmp[i] );
(*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[i] , LINE_LENGTH-1 , stream )
!= NULL ){
size_t l = strlen(tmp[i]);
if (l > 0 && tmp[i][l-1] ==
'\n')
tmp[i][l-1] = '\0';
//printf("%zu
%s\n",strlen(tmp[i]),tmp[i]);
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;
} 6 1699
Clunixchit wrote: 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 ch******@gmail-dot-com.no-spam.invalid (Clunixchit) writes: 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[i]); Loading *program = malloc ( sizeof(Loading) ); if (!program){ fprintf(stdout,"program %s failed",tmp[i]); return; }
--
"You call this a *C* question? What the hell are you smoking?" --Kaz
On Wed, 1 Jun 2005 09:42:44 +0000 (UTC), ch******@gmail-dot-com.no-spam.invalid (Clunixchit) wrote: 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[i]); Loading *program = malloc ( sizeof(Loading) ); if (!program){ fprintf(stdout,"program %s failed",tmp[i]); 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[i]) + 1 ); strcpy( (*program).name,tmp[i] ); (*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[i] , LINE_LENGTH-1 , stream )
fgets already subtracts the 1 for you.
!= NULL ){ size_t l = strlen(tmp[i]);
C89 does not support declarations after executable code in a block.
if (l > 0 && tmp[i][l-1] == '\n') tmp[i][l-1] = '\0'; //printf("%zu %s\n",strlen(tmp[i]),tmp[i]); 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>>
Clunixchit wrote: 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[i]); Loading *program = malloc ( sizeof(Loading) ); if (!program){ fprintf(stdout,"program %s failed",tmp[i]); return; } (*program).name = malloc ( strlen(tmp[i]) + 1 ); strcpy( (*program).name,tmp[i] );
A faster and more memory efficient solution using the modified Loading type
would be
for(i = 0; i < n; ++i)
if (NULL != tmp[i]){
size_t len = strlen(tmp[i] + 1u);
program = malloc(sizeof(Loading) + len);
if (NULL == program) {
perror(tmp[i]);
break;
}
program->prec = *myLoad;
program->pid = -1;
(void)memcpy(program->name, tmp[i], 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[i], LINE_LENGTH-1, stream)) {
if (feof(stream)
break;
perror(file_name);
...
}
while( fgets( tmp[i] , LINE_LENGTH-1 , stream ) != NULL ){
Here you are doing the same thing twice better do it once
{
char *ptr =strchr(tmp[i], '\n');
if (ptr)
*ptr = '\000';
and maybe you want do drop empty lines
if ('\000' == tmp[i][0])
--i
} size_t l = strlen(tmp[i]); if (l > 0 && tmp[i][l-1] == '\n') tmp[i][l-1] = '\0'; //printf("%zu %s\n",strlen(tmp[i]),tmp[i]); 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" clear(&myLoad); printf("\n"); return 0; }
--
Michael Knaup
Barry Schwarz wrote: On Wed, 1 Jun 2005 09:42:44 +0000 (UTC), ch******@gmail-dot-com.no-spam.invalid (Clunixchit) wrote:!= NULL ){ size_t l = strlen(tmp[i]);
C89 does not support declarations after executable code in a block.
There is no declaration after executable code in the block.
Dietmar Schindler wrote: Barry Schwarz wrote: On Wed, 1 Jun 2005 09:42:44 +0000 (UTC), ch******@gmail-dot-com.no-spam.invalid (Clunixchit) wrote:!= NULL ){ size_t l = strlen(tmp[i]);
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.
--
pete This thread has been closed and replies have been disabled. Please start a new discussion. Similar topics
by: Shufen |
last post by:
Hi,
I'm a newbie that just started to learn python, html and etc. I have
some questions to ask and hope that someone can help me on.
I'm trying to code a python script (with HTML) to get...
|
by: rked |
last post by:
I get nameSPAN1 is undefined when I place cursor in comments box..
<%@ LANGUAGE="VBScript" %>
<% DIM ipAddress
ipAddress=Request.Servervariables("REMOTE_HOST")
%>
<html>
<head>
<meta...
|
by: x muzuo |
last post by:
Hi guys,
I have got a prob of javascript form validation which just doesnt work
with my ASP code. Can any one help me out please.
Here is the code:
{////<<head>
<title>IIBO Submit Page</title>...
|
by: Zellan |
last post by:
Hi All,
Have added a Strconv function to the AfterUpdate event of a field to give me
first letter capitalization (vbProperCase). I have added a msgbox that will
allow me to over-ride where...
|
by: Fuzzyman |
last post by:
Sorry for this hurried message - I've done a new implementation of out
ordered dict. This comes out of the discussion on this newsgroup (see
blog entry for link to archive of discussion).
See...
|
by: KiMcHeE |
last post by:
Hi! I'm having a real difficult time trying to figure out what is
wrong with my program code. I'm trying to make a "calculator" in the C
language and I only know basic stuff. Here's my code:
...
|
by: jd2007 |
last post by:
Why the Ajax code below in ajax.js is causing my form not to work ?
ajax.js:
var a=0;
var b=0;
var c=0;
var d=0;
var e=0;
var f=0;
|
by: =?Utf-8?B?cm9kY2hhcg==?= |
last post by:
hey all,
please reference: http://pastebin.com/mef78b3d
As shown in the reference I am dynamically building my controls in my class.
I'd like to be able to attach javascript calls as...
|
by: emmanuelkatto |
last post by:
Hi All, I am Emmanuel katto from Uganda. I want to ask what challenges you've faced while migrating a website to cloud.
Please let me know.
Thanks!
Emmanuel
|
by: BarryA |
last post by:
What are the essential steps and strategies outlined in the Data Structures and Algorithms (DSA) roadmap for aspiring data scientists? How can individuals effectively utilize this roadmap to progress...
|
by: Sonnysonu |
last post by:
This is the data of csv file
1 2 3
1 2 3
1 2 3
1 2 3
2 3
2 3
3
the lengths should be different i have to store the data by column-wise with in the specific length.
suppose the i have to...
|
by: Hystou |
last post by:
There are some requirements for setting up RAID:
1. The motherboard and BIOS support RAID configuration.
2. The motherboard has 2 or more available SATA protocol SSD/HDD slots (including MSATA, M.2...
|
by: Hystou |
last post by:
Most computers default to English, but sometimes we require a different language, especially when relocating. Forgot to request a specific language before your computer shipped? No problem! You can...
|
by: Oralloy |
last post by:
Hello folks,
I am unable to find appropriate documentation on the type promotion of bit-fields when using the generalised comparison operator "<=>".
The problem is that using the GNU compilers,...
|
by: Hystou |
last post by:
Overview:
Windows 11 and 10 have less user interface control over operating system update behaviour than previous versions of Windows. In Windows 11 and 10, there is no way to turn off the Windows...
|
by: tracyyun |
last post by:
Dear forum friends,
With the development of smart home technology, a variety of wireless communication protocols have appeared on the market, such as Zigbee, Z-Wave, Wi-Fi, Bluetooth, etc. Each...
|
by: agi2029 |
last post by:
Let's talk about the concept of autonomous AI software engineers and no-code agents. These AIs are designed to manage the entire lifecycle of a software development project—planning, coding, testing,...
| |