Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm learning C and today I wrote a program that displays info about my hardware on ubuntu.

#include <stdio.h>
#include <stdlib.h>
int main()
{
   char ch, file_name[25] = "/proc/scsi/scsi";
   FILE *fp; 
   fp = fopen(file_name,"r"); // read mode 
   if( fp == NULL )
   {
      perror("Error while opening the file.\n");
      exit(EXIT_FAILURE);
   } 
   printf("The contents of %s file are :\n", file_name); 
   while( ( ch = fgetc(fp) ) != EOF )
      printf("%c",ch); 
   fclose(fp);
   return 0;
}

For me it looked like this

$ cc driveinfo.c;./a.out 
The contents of /proc/scsi/scsi file are :
Attached devices:
Host: scsi0 Channel: 00 Id: 00 Lun: 00
  Vendor: ATA      Model: WDC WD2500JS-75N Rev: 10.0
  Type:   Direct-Access                    ANSI  SCSI revision: 05
Host: scsi1 Channel: 00 Id: 00 Lun: 00
  Vendor: ATA      Model: ST3250824AS      Rev: 3.AD
  Type:   Direct-Access                    ANSI  SCSI revision: 05
Host: scsi2 Channel: 00 Id: 00 Lun: 00
  Vendor: TSSTcorp Model: DVD+-RW TS-H653A Rev: D300
  Type:   CD-ROM                           ANSI  SCSI revision: 05
Host: scsi3 Channel: 00 Id: 00 Lun: 00
  Vendor: Optiarc  Model: DVD-ROM DDU1681S Rev: 102A
  Type:   CD-ROM                           ANSI  SCSI revision: 05
Host: scsi4 Channel: 00 Id: 00 Lun: 00
  Vendor: Lexar    Model: USB Flash Drive  Rev: 1100
  Type:   Direct-Access                    ANSI  SCSI revision: 00
Host: scsi5 Channel: 00 Id: 00 Lun: 00
  Vendor: WD       Model: 5000AAKB Externa Rev: l108
  Type:   Direct-Access                    ANSI  SCSI revision: 00

Can it run on other unices e.g. bsd? How can I make it run on ms-windows? Can I query the hardware directly instead of the file /proc/scsi/scsi ?

Update

Thanks a lot. Here are my updates.

#include <stdio.h>
#include <stdlib.h> 
int main(int argc, char **argv)
{
   int ch;
   char file_name[25] = "/proc/scsi/scsi";
   FILE *fp; 
   fp = fopen(file_name,"r"); // read mode 
   if (fp == NULL)
   {
      perror(file_name);
      exit(EXIT_FAILURE);
   } 
   printf("The contents of %s file are :\n", file_name); 
   while ((ch = fgetc(fp)) != EOF)
      putchar(ch); 
   fclose(fp);
   return 0;
}

Test run

$ cc driveinfo.c;./a.out 
The contents of /proc/scsi/scsi file are :
Attached devices:
Host: scsi0 Channel: 00 Id: 00 Lun: 00
  Vendor: ATA      Model: WDC WD2500JS-75N Rev: 10.0
  Type:   Direct-Access                    ANSI  SCSI revision: 05
Host: scsi1 Channel: 00 Id: 00 Lun: 00
  Vendor: ATA      Model: ST3250824AS      Rev: 3.AD
  Type:   Direct-Access                    ANSI  SCSI revision: 05
Host: scsi2 Channel: 00 Id: 00 Lun: 00
  Vendor: TSSTcorp Model: DVD+-RW TS-H653A Rev: D300
  Type:   CD-ROM                           ANSI  SCSI revision: 05
Host: scsi3 Channel: 00 Id: 00 Lun: 00
  Vendor: Optiarc  Model: DVD-ROM DDU1681S Rev: 102A
  Type:   CD-ROM                           ANSI  SCSI revision: 05
Host: scsi4 Channel: 00 Id: 00 Lun: 00
  Vendor: Lexar    Model: USB Flash Drive  Rev: 1100
  Type:   Direct-Access                    ANSI  SCSI revision: 00
Host: scsi5 Channel: 00 Id: 00 Lun: 00
  Vendor: WD       Model: 5000AAKB Externa Rev: l108
  Type:   Direct-Access                    ANSI  SCSI revision: 00

Latest version

#include <stdio.h>
#include <stdlib.h> 
int main(int argc, char **argv)
{
   int ch;
   char file_name[] = "/proc/scsi/scsi";
   char cpu_file_name[] = "/proc/cpuinfo";
   FILE *fp; 
   FILE *cpu_fp; 
   fp = fopen(file_name,"r"); // read mode 
   cpu_fp = fopen(cpu_file_name,"r"); // read mode 
   if (fp == NULL)
   {
      perror(file_name);
      exit(EXIT_FAILURE);
   } 
   while ((ch = fgetc(fp)) != EOF)
   {
      putchar(ch); 
   }
   fclose(fp);
   if (cpu_fp == NULL)
   {
      perror(cpu_file_name);
      exit(EXIT_FAILURE);
   } 
   while ((ch = fgetc(cpu_fp)) != EOF)
   {
      putchar(ch); 
   }
   fclose(cpu_fp);
   return 0;
}

Test

$ cat cpu-disk-info.c;clang -Wconversion cpu-disk-info.c;./a.out
#include <stdio.h>
#include <stdlib.h> 
int main(int argc, char **argv)
{
   int ch;
   char file_name[] = "/proc/scsi/scsi";
   char cpu_file_name[] = "/proc/cpuinfo";
   FILE *fp; 
   FILE *cpu_fp; 
   fp = fopen(file_name,"r"); // read mode 
   cpu_fp = fopen(cpu_file_name,"r"); // read mode 
   if (fp == NULL)
   {
      perror(file_name);
      exit(EXIT_FAILURE);
   } 
   while ((ch = fgetc(fp)) != EOF)
   {
      putchar(ch); 
   }
   fclose(fp);
   if (cpu_fp == NULL)
   {
      perror(cpu_file_name);
      exit(EXIT_FAILURE);
   } 
   while ((ch = fgetc(cpu_fp)) != EOF)
   {
      putchar(ch); 
   }
   fclose(cpu_fp);
   return 0;
}
Attached devices:
Host: scsi0 Channel: 00 Id: 00 Lun: 00
  Vendor: ATA      Model: WDC WD2500JS-75N Rev: 10.0
  Type:   Direct-Access                    ANSI  SCSI revision: 05
Host: scsi1 Channel: 00 Id: 00 Lun: 00
  Vendor: ATA      Model: ST3250824AS      Rev: 3.AD
  Type:   Direct-Access                    ANSI  SCSI revision: 05
Host: scsi2 Channel: 00 Id: 00 Lun: 00
  Vendor: TSSTcorp Model: DVD+-RW TS-H653A Rev: D300
  Type:   CD-ROM                           ANSI  SCSI revision: 05
Host: scsi3 Channel: 00 Id: 00 Lun: 00
  Vendor: Optiarc  Model: DVD-ROM DDU1681S Rev: 102A
  Type:   CD-ROM                           ANSI  SCSI revision: 05
Host: scsi4 Channel: 00 Id: 00 Lun: 00
  Vendor: Lexar    Model: USB Flash Drive  Rev: 1100
  Type:   Direct-Access                    ANSI  SCSI revision: 00
Host: scsi5 Channel: 00 Id: 00 Lun: 00
  Vendor: WD       Model: 5000AAKB Externa Rev: l108
  Type:   Direct-Access                    ANSI  SCSI revision: 00
processor   : 0
vendor_id   : GenuineIntel
cpu family  : 6
model       : 15
model name  : Intel(R) Core(TM)2 CPU          6400  @ 2.13GHz
stepping    : 6
microcode   : 0xc6
cpu MHz     : 2128.073
cache size  : 2048 KB
physical id : 0
siblings    : 2
core id     : 0
cpu cores   : 2
apicid      : 0
initial apicid  : 0
fpu     : yes
fpu_exception   : yes
cpuid level : 10
wp      : yes
flags       : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts nopl aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm dtherm tpr_shadow
bogomips    : 4256.14
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor   : 1
vendor_id   : GenuineIntel
cpu family  : 6
model       : 15
model name  : Intel(R) Core(TM)2 CPU          6400  @ 2.13GHz
stepping    : 6
microcode   : 0xc6
cpu MHz     : 2128.073
cache size  : 2048 KB
physical id : 0
siblings    : 2
core id     : 1
cpu cores   : 2
apicid      : 1
initial apicid  : 1
fpu     : yes
fpu_exception   : yes
cpuid level : 10
wp      : yes
flags       : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts nopl aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm dtherm tpr_shadow
bogomips    : 4256.00
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

dev@dev-OptiPlex-745:~$ 

Very latest version with new function

#include <stdio.h>
#include <stdlib.h> 
void info(char file_name[])
{
    int ch;
    FILE *fp;
    fp = fopen(file_name,"r");
    // read mode  
    if (fp == NULL)
    {
        perror(file_name);
        exit(EXIT_FAILURE);
    }
    while ((ch = fgetc(fp)) != EOF)
    {
        putchar(ch);
    }
    fclose(fp);
}
int main(int argc, char **argv)
{
    info("/proc/scsi/scsi");
    info("/proc/cpuinfo");
    return 0;
}

Test

$ cat cpu-disk-info.c;clang -Wconversion cpu-disk-info.c;./a.out 
#include <stdio.h>
#include <stdlib.h> 
void info(char file_name[])
{
    int ch;
    FILE *fp;
    fp = fopen(file_name,"r");
    // read mode  
    if (fp == NULL)
    {
        perror(file_name);
        exit(EXIT_FAILURE);
    }
    while ((ch = fgetc(fp)) != EOF)
    {
        putchar(ch);
    }
    fclose(fp);
}
int main(int argc, char **argv)
{
    info("/proc/scsi/scsi");
    info("/proc/cpuinfo");
    return 0;
}
Attached devices:
Host: scsi0 Channel: 00 Id: 00 Lun: 00
  Vendor: ATA      Model: WDC WD2500JS-75N Rev: 10.0
  Type:   Direct-Access                    ANSI  SCSI revision: 05
Host: scsi1 Channel: 00 Id: 00 Lun: 00
  Vendor: ATA      Model: ST3250824AS      Rev: 3.AD
  Type:   Direct-Access                    ANSI  SCSI revision: 05
Host: scsi2 Channel: 00 Id: 00 Lun: 00
  Vendor: TSSTcorp Model: DVD+-RW TS-H653A Rev: D300
  Type:   CD-ROM                           ANSI  SCSI revision: 05
Host: scsi3 Channel: 00 Id: 00 Lun: 00
  Vendor: Optiarc  Model: DVD-ROM DDU1681S Rev: 102A
  Type:   CD-ROM                           ANSI  SCSI revision: 05
Host: scsi4 Channel: 00 Id: 00 Lun: 00
  Vendor: Lexar    Model: USB Flash Drive  Rev: 1100
  Type:   Direct-Access                    ANSI  SCSI revision: 00
Host: scsi5 Channel: 00 Id: 00 Lun: 00
  Vendor: WD       Model: 5000AAKB Externa Rev: l108
  Type:   Direct-Access                    ANSI  SCSI revision: 00
processor   : 0
vendor_id   : GenuineIntel
cpu family  : 6
model       : 15
model name  : Intel(R) Core(TM)2 CPU          6400  @ 2.13GHz
stepping    : 6
microcode   : 0xc6
cpu MHz     : 2128.073
cache size  : 2048 KB
physical id : 0
siblings    : 2
core id     : 0
cpu cores   : 2
apicid      : 0
initial apicid  : 0
fpu     : yes
fpu_exception   : yes
cpuid level : 10
wp      : yes
flags       : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts nopl aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm dtherm tpr_shadow
bogomips    : 4256.14
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

processor   : 1
vendor_id   : GenuineIntel
cpu family  : 6
model       : 15
model name  : Intel(R) Core(TM)2 CPU          6400  @ 2.13GHz
stepping    : 6
microcode   : 0xc6
cpu MHz     : 2128.073
cache size  : 2048 KB
physical id : 0
siblings    : 2
core id     : 1
cpu cores   : 2
apicid      : 1
initial apicid  : 1
fpu     : yes
fpu_exception   : yes
cpuid level : 10
wp      : yes
flags       : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts nopl aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm lahf_lm dtherm tpr_shadow
bogomips    : 4256.00
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

2 strategies implemented

I tried implementing 1024 bytes i/o for 2 strategies. I'm not sure but the chunkinfo function can be better. I didn't find the source to the unix program cat but I try something similar.

#include <stdio.h>
#include <stdlib.h> 
#define CHUNK 1024 /* read 1024 bytes at a time */
void info(char file_name[])
{
    int ch;
    FILE *fp;
    fp = fopen(file_name,"r");
    // read mode  
    if (fp == NULL)
    {
        perror(file_name);
        exit(EXIT_FAILURE);
    }
    while ((ch = fgetc(fp)) != EOF)
    {
        putchar(ch);
    }
    fclose(fp);
}

void chunkinfo(char file_name[])
{
    char buf[CHUNK];
    FILE *file;
    size_t nread;

    file = fopen(file_name, "r");
    if (file) {
        while ((nread = fread(buf, 1, sizeof buf, file)) > 0)
        fwrite(buf, 1, nread, stdout);
        if (ferror(file)) {
            /* deal with error */
        }
        fclose(file);
    }
}

int main(int argc, char **argv)
{
    info("/proc/scsi/scsi");
    chunkinfo("/proc/cpuinfo");
    return 0;
}

For the record I also review how a cat.c implementation does it:

#include <sys/param.h>
#include <sys/stat.h>
#include <ctype.h>
#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <locale.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

extern char *__progname;
int bflag, eflag, nflag, sflag, tflag, vflag;
int rval;
char *filename;
void cook_args(char *argv[]);
void cook_buf(FILE *);
void raw_args(char *argv[]);
void raw_cat(int);

int main(int argc, char *argv[])
{
    int ch;
    setlocale(LC_ALL, "");

    while ((ch = getopt(argc, argv, "benstuv")) != -1)
    switch (ch) {
        case 'b':
        bflag = nflag = 1;    /* -b implies -n */
        break;
        case 'e':
        eflag = vflag = 1;    /* -e implies -v */
        break;
        case 'n':
        nflag = 1;
        break;
        case 's':
        sflag = 1;
        break;
        case 't':
        tflag = vflag = 1;    /* -t implies -v */
        break;
        case 'u':
        setbuf(stdout, NULL);
        break;
        case 'v':
        vflag = 1;
        break;
        default:
        (void)fprintf(stderr,
        "usage: %s [-benstuv] [-] [file ...]\n", __progname);
        exit(1);
        /* NOTREACHED */
    }
    argv += optind;

    if (bflag || eflag || nflag || sflag || tflag || vflag)
    cook_args(argv);
    else
    raw_args(argv);
    if (fclose(stdout))
    err(1, "stdout");
    exit(rval);
    /* NOTREACHED */
}

void cook_args(char **argv)
{
    FILE *fp;    
    fp = stdin;
    filename = "stdin";
    do {
        if (*argv) {
            if (!strcmp(*argv, "-"))
            fp = stdin;
            else if ((fp = fopen(*argv, "r")) == NULL) {
                warn("%s", *argv);
                rval = 1;
                ++argv;
                continue;
            }
            filename = *argv++;
        }
        cook_buf(fp);
        if (fp != stdin)
        (void)fclose(fp);
    } while (*argv);
}

void cook_buf(FILE *fp)
{
    int ch, gobble, line, prev;

    line = gobble = 0;
    for (prev = '\n'; (ch = getc(fp)) != EOF; prev = ch) {
        if (prev == '\n') {
            if (sflag) {
                if (ch == '\n') {
                    if (gobble)
                    continue;
                    gobble = 1;
                } else
                gobble = 0;
            }
            if (nflag && (!bflag || ch != '\n')) {
                (void)fprintf(stdout, "%6d\t", ++line);
                if (ferror(stdout))
                break;
            }
        }
        if (ch == '\n') {
            if (eflag && putchar('$') == EOF)
            break;
            } else if (ch == '\t') {
            if (tflag) {
                if (putchar('^') == EOF || putchar('I') == EOF)
                break;
                continue;
            }
            } else if (vflag) {
            if (!isascii(ch)) {
                if (putchar('M') == EOF || putchar('-') == EOF)
                break;
                ch = toascii(ch);
            }
            if (iscntrl(ch)) {
                if (putchar('^') == EOF ||
                putchar(ch == '\177' ? '?' :
                ch | 0100) == EOF)
                break;
                continue;
            }
        }
        if (putchar(ch) == EOF)
        break;
    }
    if (ferror(fp)) {
        warn("%s", filename);
        rval = 1;
        clearerr(fp);
    }
    if (ferror(stdout))
    err(1, "stdout");
}

void raw_args(char **argv)
{
    int fd;

    fd = fileno(stdin);
    filename = "stdin";
    do {
        if (*argv) {
            if (!strcmp(*argv, "-"))
            fd = fileno(stdin);
            else if ((fd = open(*argv, O_RDONLY, 0)) < 0) {
                warn("%s", *argv);
                rval = 1;
                ++argv;
                continue;
            }
            filename = *argv++;
        }
        raw_cat(fd);
        if (fd != fileno(stdin))
        (void)close(fd);
    } while (*argv);
}

void raw_cat(int rfd)
{
    int wfd;
    ssize_t nr, nw, off;
    static size_t bsize;
    static char *buf = NULL;
    struct stat sbuf;

    wfd = fileno(stdout);
    if (buf == NULL) {
        if (fstat(wfd, &sbuf))
        err(1, "stdout");
        bsize = MAX(sbuf.st_blksize, BUFSIZ);
        if ((buf = malloc(bsize)) == NULL)
        err(1, "malloc");
    }
    while ((nr = read(rfd, buf, bsize)) != -1 && nr != 0)
    for (off = 0; nr; nr -= nw, off += nw)
    if ((nw = write(wfd, buf + off, (size_t)nr)) == 0 ||
    nw == -1)
    err(1, "stdout");
    if (nr < 0) {
        warn("%s", filename);
        rval = 1;
    }
}
share|improve this question
    
It may be better to ask a new question when you have an updated version of the code, instead of editing it into an existing question. That way it will get a more attention and a more specific review. –  syb0rg Jan 14 at 4:59
add comment

3 Answers 3

up vote 2 down vote accepted

It can run on BSD since you wrote a cross-platform code and /proc/scsi/scsi is present on BSD as well. Yes, you can list the installed hardware using ioctl calls, see http://stackoverflow.com/questions/2698465/how-to-find-out-if-scsi-device-say-etc-sda-is-a-disk-or-not-via-ioctl-calls-o It seems you can use IOCTL on Windows too. (http://msdn.microsoft.com/en-us/library/aa363219%28VS.85%29.aspx).

After a few researches, I found that might correspond to your needs http://www.qtfr.org/viewtopic.php?id=13210

I just found this guide on SCSI that deals with programming

share|improve this answer
add comment

Nick, a few comments on the code:

  • define only one variable per line
  • ch should be an int not char
  • I prefer to see a space after if, while etc and no spaces inside the brackets

    if (fp == NULL)
    
  • pass the filename to perror, not your own text (and certainly don't add a \n). Here's what it prints for me:

    Error while opening the file.
    : No such file or directory
    

    It doesn't tell me which file. Here's what I would expect:

    /proc/scsi/scsi: No such file or directory
    
  • add braces in the while, even if strictly unnecessary.

  • use putchar instead of printf for single chars

  • As ch is the wrong type it cannot really hold 'EOF'. It appears to work because EOF is converted to (char) -1 and then converted to (int) -1 for the comparison. If the file happened to hold a value 255 the loop would exit there. Try it by running this code and redirecting to a file, then reading it with your program:

    int main(int argc, char **argv)
    {
        printf("hello\n");
        putchar(255);
        printf("world\n");
        return 0;
    }
    
share|improve this answer
    
Many thanks. I updated, compiled and ran code with perror(file_name); posting the complete program the the question so that you can view that the updates you suggest appear to work. –  909 Niklas Mar 6 '13 at 17:56
2  
Looks better - you forgot the braces on the while {}. BTW, the compiler (clang compiler anyway) would have warned you about the conversion error if you had included -Wconversion to the warnings. BTW2, you can omit the 25 in file_name[] –  William Morris Mar 6 '13 at 20:45
    
Yes I can confirm that the clang compiler will warn about this but not gcc. –  909 Niklas Mar 6 '13 at 21:03
add comment

Based on your latest version, have a look at this section of the code:

fp = fopen(file_name,"r"); // read mode 
cpu_fp = fopen(cpu_file_name,"r"); // read mode 
if (fp == NULL)
{
   perror(file_name);
   exit(EXIT_FAILURE);
} 
while ((ch = fgetc(fp)) != EOF)
{
   putchar(ch); 
}
fclose(fp);
if (cpu_fp == NULL)
{
   perror(cpu_file_name);
   exit(EXIT_FAILURE);
} 
while ((ch = fgetc(cpu_fp)) != EOF)
{
   putchar(ch); 
}
fclose(cpu_fp);

That's essentially two identical sections of code, if you disregard the filename. Try to make a function instead that you can call twice. Something like:

show_hardware_info("/proc/scsi/scsi");
show_hardware_info("/proc/cpuinfo");

Also you should be able to read more than one char at the time from the file. Reading line by line, or even a fixed size buffer at the time should be more efficient.

Update:

Nice that you tried different approaches with the functions. One thing that I would do differently, though, is to not call the exit() function from within the functions of your program. Even if reading the scsi-info fails, it may make sence to read the cpuinfo. Instead return some value from your function to indicate if it was successful or not. In this way the caller can determine if it's worthwhile to continue, to propagate the error further or simply to terminate the program.

Something like this is a common idiom in C, where the function will reuturn 0 if successful and some negative value to indicate an error:

int myfunction(const char * arg)
{
    if (!valid_args(arg))
        return -1;

    do_some_work();

    return 0;
}

Also, you should pay attention to the indentation. The while loop in chunkinfo() seems off. This makes it difficult to know if you want the if-statement to execute for each iteration or only after the loop is finished. As it is now, the latter is the case.

share|improve this answer
    
Thanks. I've also been scanning the net for the source to the unix program cat that displays a text file. I'll make a function like you mention. –  909 Niklas Mar 6 '13 at 22:35
1  
I added 1024 bytes buffering if you like to have another look. I didn't add a function that reads line by line but I might. –  909 Niklas Mar 7 '13 at 1:47
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.