Problem with small piece of C code

Discussion in 'C Programming' started by kalyan.listsubs@yahoo.com, May 14, 2006.

  1. Guest

    Hi,

    I have the below program which will simply write struct employee to a
    file (binary mode). The problem here is empid is writen to the file but
    the name (char name[40]) is not written. I am using cygwin (gcc
    compiler) and use the following commands to compile & run respectively:

    gcc -o emp emp.c

    ../emp write -> this will create a file emp.out and write employee (Adam
    details) to the file
    ../emp read -> this will reproduce what has been written to the file.

    the command ./emp write creates and writes to the file emp.out, but
    doesn't includes name so ./emp read doesn't includes name variable in
    the employee struct.

    Please help me & let me know what is missing or wrong in this piece of
    code.

    Thank you.

    code foe emp.c
    ==================

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

    typedef struct employee_ {
    int empid;
    char name[40];
    } employee;

    FILE *fp;

    int main(int argc, char **argv)
    {
    employee *emp = (employee*) malloc(sizeof(employee)+sizeof(40));
    if(strcmp(argv[1],"write") == 0)
    {
    emp->empid = 1;
    char *pbuf = "Adam";
    strcpy(emp->name,pbuf);
    fp=fopen("emp.out","wb+");
    fwrite(emp,1,sizeof(emp),fp);
    fflush(fp);
    fclose(fp);
    }
    else
    {
    fp=fopen("emp.out","rb+");
    fread(emp,1,sizeof(emp),fp);
    fclose(fp);
    char *pbuf = (char*)malloc(41);
    strcpy(pbuf,emp->name);
    printf("%d,%s\n",emp->empid,pbuf);
    }
    return 0;
    }
     
    , May 14, 2006
    #1
    1. Advertising

  2. wrote:
    > typedef struct employee_ {
    > int empid;
    > char name[40];
    > } employee;
    >
    > FILE *fp;


    Why is this a global? As general advise, keep variables as local in scope
    as possible, it helps to not loose focus on how and where they are used.

    > int main(int argc, char **argv)
    > {
    > employee *emp = (employee*) malloc(sizeof(employee)+sizeof(40));


    1. You should read the FAQ before posting here, it explains about the
    dangers and uselessness of casting the returnvalue of malloc().
    2. 'sizeof' doesn't need brackets when used with an object, only when used
    with a type.
    3. 'employee' includes the forty bytes of its 'name' field, no need to
    allocate extra.
    4. How about checking the returnvalue?
    5. Looking at the rest of your program, there is no need at all to
    dynamically allocate this struct.

    > fp=fopen("emp.out","wb+");
    > fwrite(emp,1,sizeof(emp),fp);


    'emp' is a pointer, 'sizeof emp' (useless brackets removed) is the size of
    a pointer - probably not what you want.

    > fflush(fp);
    > fclose(fp);


    You're closing anyway, no need to flush. Might only degrade performance.

    > fp=fopen("emp.out","rb+");
    > fread(emp,1,sizeof(emp),fp);
    > fclose(fp);
    > char *pbuf = (char*)malloc(41);
    > strcpy(pbuf,emp->name);
    > printf("%d,%s\n",emp->empid,pbuf);


    Why? Why copy to a temporary buffer? Why allocate such a small, fixed-size
    buffer dynamically? Other than that, same problem as above.

    Uli
     
    Ulrich Eckhardt, May 14, 2006
    #2
    1. Advertising

  3. CBFalconer Guest

    wrote:
    >
    > I have the below program which will simply write struct employee to a
    > file (binary mode). The problem here is empid is writen to the file but
    > the name (char name[40]) is not written. I am using cygwin (gcc
    > compiler) and use the following commands to compile & run respectively:
    >
    > gcc -o emp emp.c
    >
    > ./emp write -> this will create a file emp.out and write employee (Adam
    > details) to the file
    > ./emp read -> this will reproduce what has been written to the file.
    >
    > the command ./emp write creates and writes to the file emp.out, but
    > doesn't includes name so ./emp read doesn't includes name variable in
    > the employee struct.
    >
    > code foe emp.c
    > ==================
    >
    > #include <stdio.h>
    > #include <stdlib.h>
    > #include <string.h>
    >
    > typedef struct employee_ {
    > int empid;
    > char name[40];
    > } employee;
    >
    > FILE *fp;
    >
    > int main(int argc, char **argv)
    > {
    > employee *emp = (employee*) malloc(sizeof(employee)+sizeof(40));
    > if(strcmp(argv[1],"write") == 0)
    > {
    > emp->empid = 1;
    > char *pbuf = "Adam";
    > strcpy(emp->name,pbuf);
    > fp=fopen("emp.out","wb+");
    > fwrite(emp,1,sizeof(emp),fp);
    > fflush(fp);
    > fclose(fp);
    > }
    > else
    > {
    > fp=fopen("emp.out","rb+");
    > fread(emp,1,sizeof(emp),fp);
    > fclose(fp);
    > char *pbuf = (char*)malloc(41);
    > strcpy(pbuf,emp->name);
    > printf("%d,%s\n",emp->empid,pbuf);
    > }
    > return 0;
    > }


    To start with, I get the following errors/warnings:

    junk.c: In function `main':
    junk.c:18: warning: initialization discards qualifiers from pointer
    target type
    junk.c:18: warning: ISO C89 forbids mixed declarations and code
    junk.c:30: warning: ISO C89 forbids mixed declarations and code
    junk.c:12: warning: unused parameter `argc'

    Now, you have no need for pbuf, because you have declared an
    employee such that the structure includes a 40 char field. There
    is also no need for fp to be global, and you should NEVER cast the
    return from malloc, but you should always test it for success. You
    should also test that argv[1] exists before using it, not to
    mention the success of a fopen call. In addition, the thing
    written (or read) should not be the pointer, but what the pointer
    points at. So the size written/read needs correction. So fixing
    some of these things we get:

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

    typedef struct employee_ {
    int empid;
    char name[40];
    } employee;

    int main(int argc, char **argv)
    {
    employee *emp;
    FILE *fp;

    if (argc > 1) {
    if (emp = malloc(sizeof *emp)) {
    if (strcmp(argv[1], "write") == 0) {
    emp->empid = 1;
    strcpy(emp->name, "Adam");
    if (fp = fopen("emp.out", "wb+")) {
    fwrite(emp, 1, sizeof(*emp), fp);
    fflush(fp);
    fclose(fp);
    }
    }
    else {
    if (fp = fopen("emp.out", "rb+")) {
    fread(emp, 1, sizeof(*emp), fp);
    fclose(fp);
    printf("%d, %s\n", emp->empid, emp->name);
    }
    }
    }
    }
    return 0;
    }

    and it shows signs of working.

    --
    "If you want to post a followup via groups.google.com, don't use
    the broken "Reply" link at the bottom of the article. Click on
    "show options" at the top of the article, then click on the
    "Reply" at the bottom of the article headers." - Keith Thompson
    More details at: <http://cfaj.freeshell.org/google/>
    Also see <http://www.safalra.com/special/googlegroupsreply/>
     
    CBFalconer, May 14, 2006
    #3
  4. Ulrich Eckhardt <> writes:
    > wrote:
    >> typedef struct employee_ {
    >> int empid;
    >> char name[40];
    >> } employee;
    >>
    >> FILE *fp;

    >
    > Why is this a global? As general advise, keep variables as local in scope
    > as possible, it helps to not loose focus on how and where they are used.
    >
    >> int main(int argc, char **argv)
    >> {
    >> employee *emp = (employee*) malloc(sizeof(employee)+sizeof(40));

    [...]
    > 3. 'employee' includes the forty bytes of its 'name' field, no need to
    > allocate extra.


    And sizeof(40) is just silly. 40 is an expression of type int, so
    sizeof(40) is equivalent to sizeof(int).

    --
    Keith Thompson (The_Other_Keith) <http://www.ghoti.net/~kst>
    San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
    We must do something. This is something. Therefore, we must do this.
     
    Keith Thompson, May 14, 2006
    #4
  5. Groovy hepcat CBFalconer was jivin' on Sun, 14 May 2006 09:47:55 -0400
    in comp.lang.c.
    Re: Problem with small piece of C code's a cool scene! Dig it!

    > wrote:
    >>
    >> I have the below program which will simply write struct employee to a
    >> file (binary mode). The problem here is empid is writen to the file but
    >> the name (char name[40]) is not written. I am using cygwin (gcc
    >> compiler) and use the following commands to compile & run respectively:
    >>
    >> gcc -o emp emp.c
    >>
    >> ./emp write -> this will create a file emp.out and write employee (Adam
    >> details) to the file
    >> ./emp read -> this will reproduce what has been written to the file.
    >>
    >> the command ./emp write creates and writes to the file emp.out, but
    >> doesn't includes name so ./emp read doesn't includes name variable in
    >> the employee struct.
    >>
    >> code foe emp.c
    >> ==================
    >>
    >> #include <stdio.h>
    >> #include <stdlib.h>
    >> #include <string.h>
    >>
    >> typedef struct employee_ {
    >> int empid;
    >> char name[40];
    >> } employee;
    >>
    >> FILE *fp;
    >>
    >> int main(int argc, char **argv)
    >> {
    >> employee *emp = (employee*) malloc(sizeof(employee)+sizeof(40));
    >> if(strcmp(argv[1],"write") == 0)
    >> {
    >> emp->empid = 1;
    >> char *pbuf = "Adam";
    >> strcpy(emp->name,pbuf);
    >> fp=fopen("emp.out","wb+");
    >> fwrite(emp,1,sizeof(emp),fp);
    >> fflush(fp);
    >> fclose(fp);
    >> }
    >> else
    >> {
    >> fp=fopen("emp.out","rb+");
    >> fread(emp,1,sizeof(emp),fp);
    >> fclose(fp);
    >> char *pbuf = (char*)malloc(41);
    >> strcpy(pbuf,emp->name);
    >> printf("%d,%s\n",emp->empid,pbuf);
    >> }
    >> return 0;
    >> }

    >
    >To start with, I get the following errors/warnings:
    >
    >junk.c: In function `main':
    >junk.c:18: warning: initialization discards qualifiers from pointer
    >target type


    Presumably that's for the line

    char *pbuf = "Adam";

    But the diagnostic message is spurious. A string literal has no
    qualifiers. It would be nice if they were const qualified. And you
    have presumably used some kind of compiler switch or something to make
    it so. However, this is an extention.

    --

    Dig the even newer still, yet more improved, sig!

    http://alphalink.com.au/~phaywood/
    "Ain't I'm a dog?" - Ronny Self, Ain't I'm a Dog, written by G. Sherry & W. Walker.
    I know it's not "technically correct" English; but since when was rock & roll "technically correct"?
     
    Peter Shaggy Haywood, May 17, 2006
    #5
  6. CBFalconer Guest

    Peter Shaggy Haywood wrote:
    > CBFalconer wrote:
    >

    .... snip ...
    >>
    >> To start with, I get the following errors/warnings:
    >>
    >> junk.c: In function `main':
    >> junk.c:18: warning: initialization discards qualifiers from pointer
    >> target type

    >
    > Presumably that's for the line
    >
    > char *pbuf = "Adam";
    >
    > But the diagnostic message is spurious. A string literal has no
    > qualifiers. It would be nice if they were const qualified. And you
    > have presumably used some kind of compiler switch or something to
    > make it so. However, this is an extention.


    Yup. I routinely run gcc with -Wwrite-strings, via an alias.

    --
    "If you want to post a followup via groups.google.com, don't use
    the broken "Reply" link at the bottom of the article. Click on
    "show options" at the top of the article, then click on the
    "Reply" at the bottom of the article headers." - Keith Thompson
    More details at: <http://cfaj.freeshell.org/google/>
    Also see <http://www.safalra.com/special/googlegroupsreply/>
     
    CBFalconer, May 17, 2006
    #6
  7. CBFalconer <> writes:
    > Peter Shaggy Haywood wrote:
    >> CBFalconer wrote:
    >>

    > ... snip ...
    >>>
    >>> To start with, I get the following errors/warnings:
    >>>
    >>> junk.c: In function `main':
    >>> junk.c:18: warning: initialization discards qualifiers from pointer
    >>> target type

    >>
    >> Presumably that's for the line
    >>
    >> char *pbuf = "Adam";
    >>
    >> But the diagnostic message is spurious. A string literal has no
    >> qualifiers. It would be nice if they were const qualified. And you
    >> have presumably used some kind of compiler switch or something to
    >> make it so. However, this is an extention.

    >
    > Yup. I routinely run gcc with -Wwrite-strings, via an alias.


    And the resulting warning message is incorrectly worded, but
    nevertheless useful.

    --
    Keith Thompson (The_Other_Keith) <http://www.ghoti.net/~kst>
    San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
    We must do something. This is something. Therefore, we must do this.
     
    Keith Thompson, May 17, 2006
    #7
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Jaco Naude
    Replies:
    9
    Views:
    784
  2. SM
    Replies:
    0
    Views:
    379
  3. Patrick Plattes

    Download a file piece by piece

    Patrick Plattes, Nov 30, 2006, in forum: Ruby
    Replies:
    2
    Views:
    224
    Patrick Plattes
    Nov 30, 2006
  4. Gaba Luschi

    question about a small piece of code

    Gaba Luschi, Feb 20, 2011, in forum: Ruby
    Replies:
    2
    Views:
    114
    masdel
    Feb 20, 2011
  5. SM
    Replies:
    2
    Views:
    188
Loading...

Share This Page