From: mjn@pseudo.uucp (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


In article <1992Jul24.204944.18516@cbfsb.cb.att.com> rajeev@cbnewsf.cb.att.com (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".
Xref: sparky comp.unix.programmer:3997 comp.unix.questions:9541 comp.unix.wizards:3349 comp.sys.sun.misc:3386