NetNews Usenet Archive 1992 #16/NN_1992_16.iso/spool/comp/unix/programm/4033

Text File | 1992-07-31 | 5.1 KB | 198 lines

Newsgroups: comp.unix.programmer
Path: sparky!uunet!uunet.ca!geac!r-node!pseudo!mjn
From: [e-mail] (Murray Nesbitt)
Subject: Re: I am stumped on this one!!!
Organization: Private system in Toronto, ON
Date: Fri, 31 Jul 1992 12:19:58 GMT
Message-ID: <[e-mail]>
In-Reply-To: [e-mail]'s message of Fri, 24 Jul 1992 20:49:44 GMT
References: <[e-mail]>
Sender: [e-mail] (Murray Nesbitt)
Lines: 185


In article <[e-mail]> [e-mail] (rajeev.dolas) writes:

>    I have included a function from my code which doesn't work on a Sun
>    (690 w/ 4.1.2) but works on an Amdahl and I can't figure out why.

I can't figure out why it works on the Amdahl either. :-)

>#include <stdio.h>

You should also #include <string.h> and <stdlib.h> for the string and
memory functions you call below.

>
>/* I have deleted lots of stuff to keep it simple for netters */
>parse_conf(f2)
>int f2;
>{
>
>    char fname[80], ch, cmp[80];
>    char pars_wbuf[128];
>    char *pbuf;
>    char *cp_line;
>    char *tmp_line;
>    char *var, *getenv();
>    int comp, he;
>    int parse_ret;
>    int fd;
>    FILE *fp1, *fp2;
>
>    strcpy(fname, "/qsun/h5/raj/progs/acs/config/Star3/072392");

You should probably just

    #define FNAME "/qsun/h5/raj/progs/acs/config/Star3/072392"

then your compiler would have prevented you from making the horrific
mistake you make below (and it's much cheaper than calling strcpy()
each time).

>
>    umask(0);
>    umask(066);

Just out of curiosity, why two calls to umask()?  You should also
provide a declaration for umask().

>    if((fp1 = fopen(fname, "w")) == NULL)
>    {
>        if((fp1 = fopen(fname, "w+")) == NULL)

If fopen() fails with mode "w", why would you think it might be
successful with mode "w+"?

>        printf("Cannot open fp1 in parse_conf()\n");
>        return(-78);
>    }
>
>    /** f2 was opened in another function with "w" and data was written */
>    if((fp2 = fdopen(f2, "r")) == NULL)
>    {
>        printf("Cannot open fp2 in parse_conf()\n");
>        return(-78);
>    }
>
>    /* As data was written to this file before, I must do a fseek */
>    if((he = fseek(fp2, 0L, 0)) < 0)

fseek() can return a *positive* value on failure so the comparison
should be against zero.  Also, the mode argument to fseek() should be
SEEK_SET.  Personally, I'd use rewind() if you actually *needed* to
position the file pointer here, which you don't.

>    {
>        printf("fseek error in parse_conf()\n");
>        return(-78);
>    }
>
>    /* str_comp is my function which returns -1 if match is not found */
>    while((fgets(pars_wbuf, 80, fp2) != NULL) &&

Rather than hard-coding the value 80 into fgets(), use sizeof() unless
you actually *want* fewer characters than your array can hold.

>        ((parse_ret = str_comp(cmp, pars_wbuf)) != 0))
>    {
>        pars_wbuf[strlen(pars_wbuf)] = '\0';

Rather than having multiple calls to strlen(), call it once and store
its return value in a variable of type size_t for later use.

>        pbuf = pars_wbuf;
>        cp_line = (char *)malloc(sizeof(strlen(pars_wbuf)));

Check for failure of the malloc(), and replace
``sizeof(strlen(pars_wbuf))'' (which is not what you want anyways)
with (the size_t you saved above + 1).

>        strcpy(cp_line, "\0");

Use *cp_line = '\0' to achieve the same effect more efficiently.

>        tmp_line = cp_line;
>
>        if((comp = line_comp(pars_wbuf)) == 0)
>            ;
>        else
>        {
>            /* Traverse thru pbuf, skip "
>            while(*pbuf != '\0')
>            {
>                if(*pbuf != 0x0D)
>                {
>                    /* I was writing a char at a time to
>                       the file before. This part works
>                       on Amdahl but not on a sun.    */
>
>                    /**
>                    ch = *pbuf;
>                    fprintf(stdout, "%c", ch);
>                    if((he = putc(ch, fp1)) < 0)
>                    {
>                         printf("pars_conf: Write error\n");
>                        return(-13);
>                    }
>                    **/
>
>                    /* Now I save it to a buffer */
>                    *cp_line++ = *pbuf;
>                }
>                *pbuf++;

I doubt that *pbuf++ is what you intended - it should be pbuf++;

>            }
>            /* If I do a fputs to stdout, it works fine */
>            /**
>            fputs(tmp_line, stdout);
>            fflush(stdout);
>            **/
>
>            /* dbxtool shows the following error on this instr -
>             signal SEGV in malloc at 0xf773634
>             malloc+0x150:    st    %o0, [%15]
>             */
>            fputs(tmp_line, fp1);
>        }
>        free(cp_line);

Rather than calling malloc() and free() every time you read a line,
you should just declare another array of char and have cp_line
manipulate it.

>    }
>
>    fclose(fp1);
>
>
>    if(fname)
>        free(*fname);

Ouch!  Trying to free a local array will always get you bitten in the
arse.  And even if it pointed to a free()able area, you definitely
would not want to free *fname.

>    if(parse_ret != 0)
>        return(-89);
>

Finally, if you're going to return all those wacky values on failure,
you should at least return something meaningful (probably 0) on
success.

>}

I wouldn't be surprised if your code worked after taking care of these
problems and potential problems, but no guarantees.  The most likely
culprits are 1) the incorrect malloc() of cp_line (which probably
resulted in the corruption of the file pointer fp1, a hypothesis that
is backed up by your ability to fputs() to stderr), and/or 2) the
incorrect free()ing of fname.

Murray
--
Until my site's in the maps, please reply to "druid!pseudo!mjn".