|
Home > Archive > Unix Programming > March 2006 > I was told this line of code would get me fired
You are viewing an archived Text-only version of the thread.
To view this thread in it's original format and/or if you want to reply to
this thread please [click here]
| Author |
I was told this line of code would get me fired
|
|
|
| Given the following lines of code:
#include <stdio.h>
#include <utmp.h>
#include <string.h>
#include <unistd.h>
#include <stdlib.h>
#include <time.h>
#define LOG "/var/run/utmp"
static int process_type(struct utmp *user) {
setutent();
if(user->ut_type == UT_UNKNOWN)
printf("%12s:\t Unknown Process Type\n",user->ut_user);
if(user->ut_type == RUN_LVL)
printf("%12s:\t Run Level \n",user->ut_user);
if(user->ut_type == BOOT_TIME)
printf("%12s:\t Boot Time \n",user->ut_user);
if(user->ut_type == NEW_TIME)
printf("%12s:\t New Time \n",user->ut_user);
if(user->ut_type == OLD_TIME)
printf("%12s:\t Old Time \n",user->ut_user);
if(user->ut_type == INIT_PROCESS)
printf("%12s:\t Init \n",user->ut_user);
if(user->ut_type == LOGIN_PROCESS)
printf("%12s:\t Automatic Login \n",user->ut_user);
if(user->ut_type == USER_PROCESS)
printf("%12s:\t User account \n",user->ut_user);
if(user->ut_type == DEAD_PROCESS)
printf("%12s:\t Dead Process \n",user->ut_user);
if(user->ut_type == ACCOUNTING)
printf("%12s:\t Accounting \n",user->ut_user);
endutent();
return 0;
}
static int user_info(struct utmp *user){
setutent();
printf("%12s %8s %22s %14s",
user->ut_user,
user->ut_line,
user->ut_host,
ctime(&user->ut_time)
);
endutent();
return 0;
}
static int print_banner(void) {
const char *name = "Name";
const char *term = "Terminal";
const char *host = "Hostname";
const char *time = "Time";
printf("%12s %10s %20s %3s\n",name,term,host,time);
return 0;
}
static int print_process_type_banner(void) {
int i;
for(i=0;i<70;i++) {
printf("-");
}
printf("\n");
const char *name ="Name";
const char *type="Process Description";
printf("%12s \t %12s\n",name,type);
return 0;
}
int main(int argc, char **argv) {
struct utmp log_file;
FILE *fp;
print_banner();
if ((fp = fopen(LOG,"r")) != 0){
for(;fread(&log_file,sizeof(struct utmp),1,fp) !=0;) {
user_info(&log_file);
}
if(!feof(fp)){
perror(argv[0]);
exit(EXIT_FAILURE);
}
fclose(fp);
}
print_process_type_banner();
if(fp == NULL) {
perror(argv[0]);
exit(EXIT_FAILURE);
}
if ((fp = fopen(LOG,"r")) != 0) {
for(;fread(&log_file,sizeof(struct utmp),1,fp) !=0;) {
process_type(&log_file);
}
if(!feof(fp)){
perror(argv[0]);
exit(EXIT_FAILURE);
}
fclose(fp);
}
if(fp == NULL) {
perror(argv[0]);
exit(EXIT_FAILURE);
}
return 0;
I was told the follwing line of code:
for(;fread(&log_file,sizeof(struct utmp),1,fp) !=0;)
would have gotten me fired had I used this out in the workforce. Can
someone clue me on what is going wrong here? Just for the record, I'm
not a computer programmer.
Thanks in advance
Chad
| |
|
| Chad wrote:
> Given the following lines of code:
> ...snip...
> I was told the follwing line of code:
>
> for(;fread(&log_file,sizeof(struct utmp),1,fp) !=0;)
>
> would have gotten me fired had I used this out in the workforce. Can
> someone clue me on what is going wrong here? Just for the record, I'm
> not a computer programmer.
Others better than I will complete the prosecution's arguments, but
even a casual inspection tells me that on any error in fread(), the
loop will not exit (likely to spin endlessly); the error code is
discarded; and short reads are likewise not detected.
If you're not a programmer, why would you care? Are you the boss of the
guy who wrote this? 
>
> Thanks in advance
> Chad
| |
| Måns Rullgård 2006-03-11, 5:51 pm |
| "toby" <toby@telegraphics.com.au> writes:
> Chad wrote:
>
> Others better than I will complete the prosecution's arguments, but
> even a casual inspection tells me that on any error in fread(), the
> loop will not exit (likely to spin endlessly); the error code is
> discarded;
And why use for(;foo;) when while(foo) would be much more readable?
> and short reads are likewise not detected.
In this case they are, since he is only requesting one unit to be
read.
> If you're not a programmer, why would you care? Are you the boss of the
> guy who wrote this?
If he is, he should indeed fire the guy.
--
Måns Rullgård
mru@inprovide.com
| |
|
|
toby wrote:[vbcol=seagreen]
> Chad wrote:
>
> Others better than I will complete the prosecution's arguments, but
> even a casual inspection tells me that on any error in fread(), the
> loop will not exit (likely to spin endlessly); the error code is
> discarded; and short reads are likewise not detected.
>
> If you're not a programmer, why would you care? Are you the boss of the
> guy who wrote this?
>
No, I'm a self-taught computer programmer. I was one day, maybe when
I'm old and gray, hoping to go to school for computer science.
| |
|
|
M=E5ns Rullg=E5rd wrote:
> "toby" <toby@telegraphics.com.au> writes:
>
>
> And why use for(;foo;) when while(foo) would be much more readable?
>
>
> In this case they are, since he is only requesting one unit to be
> read.
Yep. You're always catching me out :-)
>
>
> If he is, he should indeed fire the guy.
>=20
> --=20
> M=E5ns Rullg=E5rd
> mru@inprovide.com
| |
| David Schwartz 2006-03-11, 8:47 pm |
|
"Chad" <cdalten@gmail.com> wrote in message
news:1142107515.250125.292970@i39g2000cwa.googlegroups.com...
> I was told the follwing line of code:
>
> for(;fread(&log_file,sizeof(struct utmp),1,fp) !=0;)
Along with the other complaint, it's just plain senseless. There is no
reason to abuse a 'for' loop this way.
The 'for' loop is a complex loop structure, including an initialization
statement, a test statement, and a per-loop statement. In this case only one
of those statements is used. This makes the 'for' loop the worst possible
choice. Arguably a 'while' loop is best in the case, but a 'do' loop would
be fine too.
DS
| |
|
|
David Schwartz wrote:
> "Chad" <cdalten@gmail.com> wrote in message
> news:1142107515.250125.292970@i39g2000cwa.googlegroups.com...
>
>
> Along with the other complaint, it's just plain senseless. There is no
> reason to abuse a 'for' loop this way.
>
> The 'for' loop is a complex loop structure, including an initialization
> statement, a test statement, and a per-loop statement. In this case only one
> of those statements is used. This makes the 'for' loop the worst possible
> choice. Arguably a 'while' loop is best in the case, but a 'do' loop would
> be fine too.
>
> DS
I was always under the impression a 'for' loop and a 'while' loop were
nearly the same.
Chad
| |
| Barry Margolin 2006-03-12, 2:47 am |
| In article <1142132080.599943.251110@j52g2000cwj.googlegroups.com>,
"Chad" <cdalten@gmail.com> wrote:
> David Schwartz wrote:
>
> I was always under the impression a 'for' loop and a 'while' loop were
> nearly the same.
A for loop is like a while loop with built-in initialization and update
steps. If you're not using either of those features, you might as well
use a while loop -- it's confusing to use a statement without using its
distinguishing features.
Code like that is appropriate for obfuscated C contests, but not
production applications.
--
Barry Margolin, barmar@alum.mit.edu
Arlington, MA
*** PLEASE post questions in newsgroups, not directly to me ***
*** PLEASE don't copy me on replies, I'll read them in the group ***
| |
|
|
toby wrote:
> Chad wrote:
>
> Others better than I will complete the prosecution's arguments, but
> even a casual inspection tells me that on any error in fread(), the
> loop will not exit (likely to spin endlessly); the error code is
> discarded; and short reads are likewise not detected.
>
Would something like the following possibly work:
while(fread(&log_file,sizeof(struct utmp),1,fp) >=0){
if( (ferror(stdin)) || feof(stdin) )
exit(1);
}
Chad
| |
| Chuck Dillon 2006-03-13, 5:54 pm |
| toby wrote:
> Chad wrote:
>
>
>
> Others better than I will complete the prosecution's arguments, but
> even a casual inspection tells me that on any error in fread(), the
> loop will not exit (likely to spin endlessly); the error code is
> discarded; and short reads are likewise not detected.
My fread usage is a bit stale but I think you are thinking of read(2).
From the fread/fwrite manpage (SunOS 5.8)...
<Paste>
RETURN VALUES
The fread() function returns the number of items read. The
fwrite() function returns the number of items written.
If size or nitems is 0, then fread() and fwrite() return 0
and do not effect the state of stream.
If an error occurs, fread() and fwrite() return 0 and set
the error indicator for stream.
</Paste>
So why would it get into an endless loop? Since the item count is 1
the conditional is equivalent to 'while (fread(...) == 1)' which is the
same as 'while (I read a record without error)'.
-- ced
>
> If you're not a programmer, why would you care? Are you the boss of the
> guy who wrote this?
>
>
>
>
--
Chuck Dillon
Senior Software Engineer
NimbleGen Systems Inc.
| |
| Ralf Fassel 2006-03-13, 5:54 pm |
| * "Chad" <cdalten@gmail.com>
| I was told the follwing line of code:
|
| for(;fread(&log_file,sizeof(struct utmp),1,fp) !=0;)
|
| would have gotten me fired had I used this out in the workforce. Can
| someone clue me on what is going wrong here?
If the number of bytes read by fread() is less than the requested
number of bytes but greater than zero, the code will work on an
incomplete utmp struct.
R'
| |
|
|
Chuck Dillon wrote:
> toby wrote:
>
> My fread usage is a bit stale but I think you are thinking of read(2).
> From the fread/fwrite manpage (SunOS 5.8)...
>
> <Paste>
> RETURN VALUES
> The fread() function returns the number of items read. The
> fwrite() function returns the number of items written.
>
> If size or nitems is 0, then fread() and fwrite() return 0
> and do not effect the state of stream.
>
> If an error occurs, fread() and fwrite() return 0 and set
> the error indicator for stream.
>
> </Paste>
>
> So why would it get into an endless loop? Since the item count is 1
> the conditional is equivalent to 'while (fread(...) == 1)' which is the
> same as 'while (I read a record without error)'.
Yes, my mistake, I was thinking of the error-returning calls. Thanks,
--T
>
> -- ced
>
>
>
>
> --
> Chuck Dillon
> Senior Software Engineer
> NimbleGen Systems Inc.
| |
| Måns Rullgård 2006-03-13, 5:54 pm |
| Ralf Fassel <ralfixx@gmx.de> writes:
> * "Chad" <cdalten@gmail.com>
> | I was told the follwing line of code:
> |
> | for(;fread(&log_file,sizeof(struct utmp),1,fp) !=0;)
> |
> | would have gotten me fired had I used this out in the workforce. Can
> | someone clue me on what is going wrong here?
>
> If the number of bytes read by fread() is less than the requested
> number of bytes but greater than zero,
fread() will return 0.
> the code will work on an incomplete utmp struct.
No, that's not the problem here.
--
Måns Rullgård
mru@inprovide.com
| |
| Wayne C. Morris 2006-03-13, 5:54 pm |
| In article <ygaoe0auzbi.fsf@panther.akutech-local.de>,
Ralf Fassel <ralfixx@gmx.de> wrote:
> * "Chad" <cdalten@gmail.com>
> | I was told the follwing line of code:
> |
> | for(;fread(&log_file,sizeof(struct utmp),1,fp) !=0;)
> |
> | would have gotten me fired had I used this out in the workforce. Can
> | someone clue me on what is going wrong here?
>
> If the number of bytes read by fread() is less than the requested
> number of bytes but greater than zero, the code will work on an
> incomplete utmp struct.
No it won't, because in that instance fread() will return zero. fread()
returns (n/size), where 'n' is the number of bytes read and 'size' is the
second parameter.
| |
| Ralf Fassel 2006-03-13, 5:54 pm |
| * "Wayne C. Morris" <wayne.morris@this.is.invalid>
| No it won't, because in that instance fread() will return zero.
| fread() returns (n/size), where 'n' is the number of bytes read and
| 'size' is the second parameter.
Arghh, had mixed up parms 2 and 3. Sorry.
R'
| |
| Ralf Fassel 2006-03-13, 5:54 pm |
| * Ralf Fassel <ralfixx@gmx.de>
| [more nonsense]
More precisely, I was thinking in terms of read() instead of fread().
R'
| |
| Giorgos Keramidas 2006-03-14, 2:49 am |
| On 12 Mar 2006 05:27:07 -0800, "Chad" <cdalten@gmail.com> wrote:
>toby wrote:
>
> Would something like the following possibly work:
>
> while(fread(&log_file,sizeof(struct utmp),1,fp) >=0){
> if( (ferror(stdin)) || feof(stdin) )
> exit(1);
>
> }
Two comments:
(1) Why not check explicitly *EXACTLY* for one object read? The return
value of fread() should *always* be 1 if it succeeded, as you are
only asking it to read one entry.
| while (fread(&log_file, sizeof(struct utmp), 1, fp) == 1) {
| /* Process record */
| }
| if (feof(fp) != 0) /* All done */
| return 0; /* Or whatever is appropriate */
| if (ferror(fp) != 0) /* Something went very wrong */
| return -1;
(2) Why are you testing `stdin' for feof() or ferror() instead of `fp'?
| |
|
|
Chuck Dillon wrote:
> toby wrote:
>
> My fread usage is a bit stale but I think you are thinking of read(2).
> From the fread/fwrite manpage (SunOS 5.8)...
>
> <Paste>
> RETURN VALUES
> The fread() function returns the number of items read. The
> fwrite() function returns the number of items written.
>
> If size or nitems is 0, then fread() and fwrite() return 0
> and do not effect the state of stream.
>
> If an error occurs, fread() and fwrite() return 0 and set
> the error indicator for stream.
>
> </Paste>
>
> So why would it get into an endless loop? Since the item count is 1
> the conditional is equivalent to 'while (fread(...) == 1)' which is the
> same as 'while (I read a record without error)'.
>
> -- ced
Why would it go into an endless loop? I have no idea. Okay, I need
think think about how the item count is one.
>
>
>
>
> --
> Chuck Dillon
> Senior Software Engineer
> NimbleGen Systems Inc.
| |
|
|
Giorgos Keramidas wrote:
> On 12 Mar 2006 05:27:07 -0800, "Chad" <cdalten@gmail.com> wrote:
>
> Two comments:
>
> (1) Why not check explicitly *EXACTLY* for one object read? The return
> value of fread() should *always* be 1 if it succeeded, as you are
> only asking it to read one entry.
>
> | while (fread(&log_file, sizeof(struct utmp), 1, fp) == 1) {
> | /* Process record */
> | }
> | if (feof(fp) != 0) /* All done */
> | return 0; /* Or whatever is appropriate */
> | if (ferror(fp) != 0) /* Something went very wrong */
> | return -1;
>
> (2) Why are you testing `stdin' for feof() or ferror() instead of `fp'?
Ahhh.... I knew someone would catch it. The 'stdin' was an error. It
was only AFTER I posted the speculated loop I realized I screwed up.
Chad
| |
|
|
> Why would it go into an endless loop? I have no idea. Okay, I need
> think think about how the item count is one.
>
Okay, disregard what I just said. I didn't read all the posts close
enough. I'm just going to shut up and go to bed.
Thanks again,
Chad
| |
|
| On 2006-03-11, Chad <cdalten@gmail.com> wrote:
> Given the following lines of code:
> for(;fread(&log_file,sizeof(struct utmp),1,fp) !=0;)
> would have gotten me fired had I used this out in the workforce. Can
> someone clue me on what is going wrong here?
The other complaint about this I don't think anyone's mentioned unless I
just didn't notice it is that it looks like you're relying on the image
of struct utmp in the file looking just like the one in memory.
If the data was written into the file originally with a similar fwrite
statement but by a program compiled with a different compiler, or on a
different platform, or a different version of the same compiler, etc., the
struct image in the file might be padded differently from the one you're
reading into.
| |
|
|
Ben C wrote:
> On 2006-03-11, Chad <cdalten@gmail.com> wrote:
>
>
>
> The other complaint about this I don't think anyone's mentioned unless I
> just didn't notice it is that it looks like you're relying on the image
> of struct utmp in the file looking just like the one in memory.
>
> If the data was written into the file originally with a similar fwrite
> statement but by a program compiled with a different compiler, or on a
> different platform, or a different version of the same compiler, etc., the
> struct image in the file might be padded differently from the one you're
> reading into.
So in other words, I would have do something like:
#ifdef LINUX
/*put linux specific fields for utmp here
#endif
And/or figure out some way to have struct utmp portable across
different versions of Linux?
Chad
| |
|
|
Chad wrote:
> Ben C wrote:
>
> So in other words, I would have do something like:
>
> #ifdef LINUX
> /*put linux specific fields for utmp here
> #endif
>
> And/or figure out some way to have struct utmp portable across
> different versions of Linux?
>
> Chad
Would maybe like accessing the fields at runtime work?
| |
|
| On 2006-03-15, Chad <cdalten@gmail.com> wrote:
>
> Ben C wrote:
>
> So in other words, I would have do something like:
>
> #ifdef LINUX
> /*put linux specific fields for utmp here
> #endif
>
> And/or figure out some way to have struct utmp portable across
> different versions of Linux?
No, rather than require the sources to track versions of everything
else, it's much better to write code that will work with everything.
You have to write out and read in fields one at a time, something like
this, although you could organize it better:
struct utmp
{
int32_t width; /* for example */
uint8_t height;
};
struct utmp my_struct;
fwrite(&my_struct.width, sizeof (int32_t), 1, fp);
fwrite(&my_struct.height, sizeof (uint8_t), 1, fp);
and the same sort of thing to read it back in. This way the file is
always packed with no gaps; the structs in memory can be packed how they
like.
Use types like int32_t, not types like int. If necessary typedef your
own, but int32_t etc. are to be found in stdint.h and are standard in
ISO C99.
More tedious than writing out the whole struct in one go, but the only
safe way to do it.
| |
| Giorgos Keramidas 2006-03-15, 5:55 pm |
| On 15 Mar 2006 06:30:02 -0800, "Chad" <cdalten@gmail.com> wrote:
>Ben C wrote:
>
> So in other words, I would have do something like:
>
> #ifdef LINUX
> /*put linux specific fields for utmp here
> #endif
>
> And/or figure out some way to have struct utmp portable across
> different versions of Linux?
Or have an architecture-independent "external representation" format for
data saved in files. This may not apply to `utmp' files though...
| |
| Bjorn Reese 2006-03-15, 5:55 pm |
| Chad wrote:
> So in other words, I would have do something like:
>
> #ifdef LINUX
> /*put linux specific fields for utmp here
> #endif
You may consider getutent() and/or getxutent().
--
mail1dotstofanetdotdk
| |
|
| > >> The other complaint about this I don't think anyone's mentioned unless I
>
> No, rather than require the sources to track versions of everything
> else, it's much better to write code that will work with everything.
>
> You have to write out and read in fields one at a time, something like
> this, although you could organize it better:
>
> struct utmp
> {
> int32_t width; /* for example */
> uint8_t height;
> };
>
> struct utmp my_struct;
>
> fwrite(&my_struct.width, sizeof (int32_t), 1, fp);
> fwrite(&my_struct.height, sizeof (uint8_t), 1, fp);
>
> and the same sort of thing to read it back in. This way the file is
> always packed with no gaps; the structs in memory can be packed how they
> like.
>
> Use types like int32_t, not types like int. If necessary typedef your
> own, but int32_t etc. are to be found in stdint.h and are standard in
> ISO C99.
>
> More tedious than writing out the whole struct in one go, but the only
> safe way to do it.
This might be going a bit off topic. If you have the book "Advanced
Programming in the Unix Environment", the second edition by Stevens &
Rago, Look at figure 8.29 on page 254. In the example, he prints some
fields from the system's accounting file. However, he doesn't write
in/or out of the file or access's the fields at runtime. What's the
reasoning behind it? Was it just to serve as an example and not
intended to be fully functional code?
Chad
| |
| Wayne C. Morris 2006-03-16, 5:53 pm |
| In article <slrne1ge3i.1lu.spamspam@bowser.marioworld>,
Ben C <spamspam@spam.eggs> wrote:
[snip][vbcol=seagreen]
>
> You have to write out and read in fields one at a time, something like
> this, although you could organize it better:
>
> struct utmp
> {
> int32_t width; /* for example */
> uint8_t height;
> };
>
> struct utmp my_struct;
>
> fwrite(&my_struct.width, sizeof (int32_t), 1, fp);
> fwrite(&my_struct.height, sizeof (uint8_t), 1, fp);
>
> and the same sort of thing to read it back in. This way the file is
> always packed with no gaps; the structs in memory can be packed how they
> like.
Close, but the file's byte order may also differ from the processor's (for
example, if it was copied from a Pentium PC to a PowerPC machine), so you
should convert each field to a platform-neutral format before writing, and
convert them back after reading.
One method is to use the ntohl, htonl, ntohs, and htons functions, like so:
int32_t x;
x = htonl( my_struct.width );
fwrite(&x, sizeof(x), 1, fp);
...
fread(&x, sizeof(x), 1, fp);
my_struct.width = ntohl(x);
Another method is to write the fields to the file as ASCII strings (fixed
length or delimited).
| |
|
|
>
> Close, but the file's byte order may also differ from the processor's (for
> example, if it was copied from a Pentium PC to a PowerPC machine), so you
> should convert each field to a platform-neutral format before writing, and
> convert them back after reading.
>
> One method is to use the ntohl, htonl, ntohs, and htons functions, like so:
>
> int32_t x;
>
> x = htonl( my_struct.width );
> fwrite(&x, sizeof(x), 1, fp);
>
> ...
>
> fread(&x, sizeof(x), 1, fp);
> my_struct.width = ntohl(x);
>
> Another method is to write the fields to the file as ASCII strings (fixed
> length or delimited).
Then why would the late Dr. Stevens omit such a thing in section 8.14
(the process accounting example) in the book "Advanced programming in
the Unix Environment?
Chad
| |
|
| On 2006-03-17, Chad <cdalten@gmail.com> wrote:
[vbcol=seagreen]
> Then why would the late Dr. Stevens omit such a thing in section 8.14
> (the process accounting example) in the book "Advanced programming in
> the Unix Environment?
Maybe he was trying to illustrate something else.
I haven't seen the example because I don't have that edition of that
book.
What people have been saying about struct packing and byte order is the
truth, there's no doubt about that. But draw your own conclusions from
the facts. In particular circumstances-- a temporary file that's always
read and written by the same program instance for example-- there may be
no need to bother with all this extra complexity, because all the
reasons why you need it wouldn't apply.
Of course merely having good reasons is not a cast-iron guarantee you
wouldn't get fired for it
|
|
|
|
|