annoying fgets problem <fgets> 
Author Message
 annoying fgets problem <fgets>

Can someone explain why when I do this:

int writePacketHdr(FILE *output, packetHdr *source )
{
 fseek(output, -1, SEEK_CUR);
 fputc('-', output);
 if( fputs(source->preamble, output) < 0) printf("shit");
 fputs(source->framedel, output);
 fputs(source->dmac, output);
 fputs(source->smac, output);
 fputs(source->plength, output);
 fputs(source->payloadEncoded, output);
 fputs(source->crc, output);
 fputc('\n', output);

 return 1;

Quote:
}

and call the function the only thing that actually gets printed are the calls to fputc, and NOT fputs ???

Thanks,
Tim



Sat, 26 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>

Quote:

>Can someone explain why when I do this:

>int writePacketHdr(FILE *output, packetHdr *source )
>{
> fseek(output, -1, SEEK_CUR);
> fputc('-', output);
> if( fputs(source->preamble, output) < 0) printf("shit");
> fputs(source->framedel, output);
> fputs(source->dmac, output);
> fputs(source->smac, output);
> fputs(source->plength, output);
> fputs(source->payloadEncoded, output);
> fputs(source->crc, output);
> fputc('\n', output);

> return 1;
>}

>and call the function the only thing that actually gets printed are the calls to fputc, and NOT fputs ???

The names of the things you are passing to fputs suggest that at least some of
them are not strings. I would suspect that source->crc is some unsigned
integer, as is source->plength, etc. Or are these elements marshalled into
text already?

Is it possible that you may be ignoring some serious compiler diagnostics like
warnings about conversions of integers to pointers?

--
#exclude <windows.h>



Sat, 26 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>

Quote:

>  fputs(source->plength, output);

This stuff sounds pretty much like numerical values, not char *. And I'd
rather use fprintf() instead of fputs(), because it allows formatting at
print phase.

--
Pasi Matilainen               Inf. Systems Science, Univ. of Jyvaskyla




Sat, 26 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>
ZeroRage a crit dans le message ...

Quote:
>Can someone explain why when I do this:

>int writePacketHdr(FILE *output, packetHdr *source )

What is packetHdr ?

Quote:
>{
> fseek(output, -1, SEEK_CUR);
> fputc('-', output);
> if( fputs(source->preamble, output) < 0) printf("shit");

ITYM
http://www.dinkum.com/htm_cl/stdio.html#fputs

 if( fputs(source->preamble, output) ==EOF)
 {
   puts("s**t");
 }
and maybe you should not execute the rest of the code... but I don't know your specs...

Quote:
> fputs(source->framedel, output);
> fputs(source->dmac, output);
> fputs(source->smac, output);
> fputs(source->plength, output);
> fputs(source->payloadEncoded, output);
> fputs(source->crc, output);
> fputc('\n', output);

> return 1;
>}

>and call the function the only thing that actually gets printed are the calls to fputc, and NOT fputs ???

How is your question related with your subject line ? I have found no fgets() in your code...

--
-hs- "Stove"
CLC-FAQ: http://www.eskimo.com/~scs/C-faq/top.html
ISO-C Library: http://www.dinkum.com/htm_cl
"Show us your source code, and let us, like
rapacious vultures half-crazed by starvation, rip it to shreds until
there's practically nothing left" --Richard Heathfield CLC



Sat, 26 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>
Thanks to everyone that replied, now let me clearify.... I am writing this
to read in a mock packet, however the file is supposed to be in ascii, so
all of these values are ascii hex, these variables like plength, crc, are
all strings that contain ascii hex, such as "002e" rather than the integer
2e. So I double checked and I don't have any warnings or errors when I
compile.

Quote:
> >int writePacketHdr(FILE *output, packetHdr *source ) {
> What is packetHdr ?

this is my structure
struct packetHeader {...};
typedef packetHeader packetHdr;

Quote:
> > fseek(output, -1, SEEK_CUR);
> > fputc('-', output);
> > if( fputs(source->preamble, output) < 0) printf("s**t");
> > fputs(source->framedel, output);
> > fputs(source->dmac, output);
> > fputs(source->smac, output);
> > fputs(source->plength, output);
> > fputs(source->payloadEncoded, output);
> > fputs(source->crc, output);
> > fputc('\n', output);

> > return 1;
> >}

> >and call the function the only thing that actually gets printed are the

calls to fputc, and NOT fputs ???
To put this into a context, I call this function after I have received input
for dmac, smac, and the message.  Perform error checking, convert the ascii
message entered into an ascii hex ( is this the right term??) and then print
it out on the screen to verify the info - which works and is correct.
Finally I call  writePacketHdr, which is suppose to write this all to the
file.
Quote:

> How is your question related with your subject line ? I have found no

fgets() in your code...
ya...i noticed that as well.....change that to RE: annoying fputs problem
<fputs> - sorry.

If you need more code to see exactly what I am doing just let me know, but I
don't understand why fputc() works, and fputs() doesn't.
Thanks again for your help,
Tim

ps: on a totally unrelated, probably-shouldn't-put-in-here note, this runs
perfectly under linux, and writes to the file, but not in windows



Sat, 26 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>

Quote:

> Thanks to everyone that replied, now let me clearify.... I am writing this
> to read in a mock packet, however the file is supposed to be in ascii, so
> all of these values are ascii hex, these variables like plength, crc, are
> all strings that contain ascii hex, such as "002e" rather than the integer
> 2e. So I double checked and I don't have any warnings or errors when I
> compile.

> > >int writePacketHdr(FILE *output, packetHdr *source ) {
> > What is packetHdr ?

> this is my structure
> struct packetHeader {...};
> typedef packetHeader packetHdr;

HaHa, that is what most of us suspected it to be. Please show us
the contents! Otherwise we still can't be sure.

Quote:

> > > fseek(output, -1, SEEK_CUR);
> > > fputc('-', output);
> > > if( fputs(source->preamble, output) < 0) printf("s**t");
> > > fputs(source->framedel, output);
> > > fputs(source->dmac, output);
> > > fputs(source->smac, output);
> > > fputs(source->plength, output);
> > > fputs(source->payloadEncoded, output);
> > > fputs(source->crc, output);
> > > fputc('\n', output);

> > > return 1;
> > >}

> > >and call the function the only thing that actually gets printed are the
> calls to fputc, and NOT fputs ???

> To put this into a context, I call this function after I have received input
> for dmac, smac, and the message.  Perform error checking, convert the ascii
> message entered into an ascii hex ( is this the right term??) and then print
> it out on the screen to verify the info - which works and is correct.
> Finally I call  writePacketHdr, which is suppose to write this all to the
> file.

> > How is your question related with your subject line ? I have found no
> fgets() in your code...
> ya...i noticed that as well.....change that to RE: annoying fputs problem
> <fputs> - sorry.

> If you need more code to see exactly what I am doing just let me know, but I
> don't understand why fputc() works, and fputs() doesn't.
> Thanks again for your help,
> Tim

> ps: on a totally unrelated, probably-shouldn't-put-in-here note, this runs
> perfectly under linux, and writes to the file, but not in windows

This is the way it ought to be ;-), I beg your pardon, but did you scan
for
ILOVYOU?? :))

        Z



Sat, 26 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>


Quote:
> Can someone explain why when I do this:

> int writePacketHdr(FILE *output, packetHdr *source )
> {
>  fseek(output, -1, SEEK_CUR);
>  fputc('-', output);
>  if( fputs(source->preamble, output) < 0) printf("shit");
>  fputs(source->framedel, output);
>  fputs(source->dmac, output);
>  fputs(source->smac, output);
>  fputs(source->plength, output);
>  fputs(source->payloadEncoded, output);
>  fputs(source->crc, output);
>  fputc('\n', output);

>  return 1;
> }

> and call the function the only thing that actually gets printed are
> the calls to fputc, and NOT fputs ???

Not without seeing the definition of the `packetHdr` (yuk) type, no.

However, since it seems implausible to me that things called `dmac`,
`plength`, and `crc` should all be strings, my guess is that they're
*not* strings, and treating them as such happens to make them look like
empty strings.

For example, if they happen to be declared as say `char [16]`, being a
series of bytes that hold (say) a dmac [whatever that is], if the first
byte is 0, then as far as C is concerned that's an empty string.

Motto: just because they're arrays-of-char doesn't mean that they're strings.

--
Chris "fractally nooked and crannied, C is" Dollin
C FAQs at: http://www.faqs.org/faqs/by-newsgroup/comp/comp.lang.c.html



Sat, 26 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>
****Warning: Large post *************

Quote:
> > > >int writePacketHdr(FILE *output, packetHdr *source ) {
> > > What is packetHdr ?

> > this is my structure
> > struct packetHeader {...};
> > typedef packetHeader packetHdr;

> HaHa, that is what most of us suspected it to be. Please show us
> the contents! Otherwise we still can't be sure.

doh....schedule one beating for Tim!  Hopefully i have included everything
you could possibly need

struct packetFrame
{ /* each contains an extra character for '\0' */
 char preamble[15];
 char framedel[3];
 char dmac[13];
 char smac[13];
 char plength[5];
 char crc[9];
 int msgLength;
 char *payloadEncoded;
 char *payloadDecoded;

Quote:
};

typedef struct packetFrame packetHdr;

int writePacketHdr(FILE *output, packetHdr *source )
{
 fseek(output, -1, SEEK_CUR); // to over write the control character???

 fputc('-', output);  // control char to display
 fputs(source->preamble, output);
 fputs(source->framedel, output);
 fputs(source->dmac, output);
 fputs(source->smac, output);
 fputs(source->plength, output);
 fputs(source->payloadEncoded, output);
 fputs(source->crc, output);
 fputc('\n', output);
 return 0;

Quote:
}

int inputPacketData(FILE *output, packetHdr *dest)
{
 char *tmpbuf, ch;
 int diff, x;

 strcpy(dest->preamble , "aaaaaaaaaaaaaa");
 strcpy(dest->framedel , "ab");
 strcpy(dest->crc , "11223344");

 do
 {
  printf("Enter in the dmac: ");
  scanf(" %12c", dest->dmac);
  while ( (ch = getchar() ) != '\n');  /* captures extra characters they write  */
  for ( x = 0; x<12; x++)
   if ( !isxdigit(dest->dmac[x]) )
   { ch = 'z';
    printf("DMAC invalid, try again.\n");
   }
 } while ( ch == 'z');

 do
 { printf("Enter in the smac: ");
  scanf(" %12c", dest->smac);
  while ( (ch = getchar() ) != '\n');  /* look above */
  for ( x = 0; x<12; x++)
   if ( !isxdigit(dest->smac[x]) )
   { ch = 'z';
    printf("SMAC invalid, try again.\n");
   }
 } while ( ch == 'z');

 if ( (tmpbuf = (char * ) malloc ( 15
01 ) ) == NULL )
 { printf("Error creating buffer for message: not enough memory.\n");
  return 1;
 }
 else
 { printf("Enter message: ");
  x = 0;
  scanf(" ");
  while ( ( ch = getchar() ) != '\n'  && x <=1500 )
   tmpbuf[x++] = ch;
  tmpbuf[x]='\0';

  if (x < 46)
  { diff = 46 - x;
   for ( ; diff > 0; diff--, x++)
    tmpbuf[x] = '.';
   tmpbuf[x] = '\0';
  }

  dest->msgLength = x;
  sprintf(dest->plength , "%04x", x);

  if ( (dest->payloadEncoded = (char *) malloc(x*2 + 1) ) != NULL )
  { txtStrngtohexStrng(tmpbuf, dest->payloadEncoded , x);  /* convert text
"abc" to ascii hex "616263" */
   printPacketHdr( dest);
   writePacketHdr(output, dest);
   free (dest->payloadEncoded);
  }
  else
  { printf("%i \n", dest->payloadEncoded);
   printf("Error: Unable to allocate payload.\n");
  }
  free(tmpbuf);
 }

Quote:
}

int main(int argc, char *argv[])
{
FILE *packetFile;
packetHdr *packetHeader;   /* Please don't complain, explain (...why this is
"yuk" :) */

    if (argc == 1)
 { printf("usage: %s <filename>\n", argv[0]);
  return 1;
 }
 else
 { if  ( (packetFile = fopen(*++argv, "r+")) == NULL )
   printf("Error opening %s.\n", *argv);
 }
....
  case '+':
   if(  (packetHeader = (packetHdr *) malloc ( sizeof(packetHdr))) == NULL )
    printf("Error: Unable to allocate buffer for input.\n");
   else
   {
    inputPacketData(packetFile,  packetHeader );
    free (packetHeader);
   }
   break;
  } // end of switch

 fclose (packetFile);

 return 0;

Quote:
}
> > ps: on a totally unrelated, probably-shouldn't-put-in-here note, this
runs
> > perfectly under linux, and writes to the file, but not in windows

> This is the way it ought to be ;-), I beg your pardon, but did you scan
> for
> ILOVYOU?? :))

whats ILOVYOU??? hehehe

Quote:

> Z

Thanks for all your help!
Tim


Sun, 27 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>


Quote:
> Thanks to everyone that replied, now let me clearify.... I am writing this
> to read in a mock packet, however the file is supposed to be in ascii, so
> all of these values are ascii hex, these variables like plength, crc, are
> all strings that contain ascii hex, such as "002e" rather than the integer
> 2e. So I double checked and I don't have any warnings or errors when I
> compile.

>> >int writePacketHdr(FILE *output, packetHdr *source ) {
>> What is packetHdr ?

> this is my structure
> struct packetHeader {...};

Er ... we'd rather like to see what the "..." is. We'd guessed that it
was a structure-with-fields already.

Quote:
> typedef packetHeader packetHdr;

Do you do anything different in the Windows version? *Should* you? [Like
set an I-want-p*ing-ANSI-C flag, or something.]

Post the smallest complete code you can write which shows up the problem.
(Doing that may *confront* you with the problem ...)

--
Chris "remembering some embarrassing cases" Dollin
C FAQs at: http://www.faqs.org/faqs/by-newsgroup/comp/comp.lang.c.html



Sun, 27 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>


Quote:

> struct packetFrame
> { /* each contains an extra character for '\0' */
>  char preamble[15];
>  char framedel[3];
>  char dmac[13];
>  char smac[13];
>  char plength[5];
>  char crc[9];
>  int msgLength;
>  char *payloadEncoded;
>  char *payloadDecoded;
> };

> typedef struct packetFrame packetHdr;

> int writePacketHdr(FILE *output, packetHdr *source )
> {
>  fseek(output, -1, SEEK_CUR); // to over write the control character???

What control character? By the way, "//" probably only works by accident
on your compiler; it wasn't official C until recently.

Quote:
>  fputc('-', output);  // control char to display

Why is `-` a "control" character?

Quote:
>  fputs(source->preamble, output);
>  fputs(source->framedel, output);
>  fputs(source->dmac, output);
>  fputs(source->smac, output);
>  fputs(source->plength, output);
>  fputs(source->payloadEncoded, output);
>  fputs(source->crc, output);
>  fputc('\n', output);
>  return 0;

Why? If it always returns 0, why have it return `int` rather than `void`?

Quote:
> }

> int inputPacketData(FILE *output, packetHdr *dest)
> {
>  char *tmpbuf, ch;
>  int diff, x;

>  strcpy(dest->preamble , "aaaaaaaaaaaaaa");
>  strcpy(dest->framedel , "ab");
>  strcpy(dest->crc , "11223344");

Why those particular strings? (Presumably they're obvious on output, and
later versions will have different versions.)

Quote:
>  do
>  {
>   printf("Enter in the dmac: ");

You should `fflush(stdout)` at this point, to ensure the prompt will be
displayed. (It's also usual for prompts to go to `stderr` rather than
`stdout`.)

Quote:
>   scanf(" %12c", dest->dmac);

Avoid scanf. See the FAQ.

Quote:
>   while ( (ch = getchar() ) != '\n');  /* captures extra characters they write  */

Good man!

Quote:
>   for ( x = 0; x<12; x++)
>    if ( !isxdigit(dest->dmac[x]) )
>    { ch = 'z';
>     printf("DMAC invalid, try again.\n");
>    }
>  } while ( ch == 'z');

I'd suggest reading in the line with `fgets`, not `scanf`. In fact (in view
of the repetition of the code below) I'd suggest writing a function

    int readHexField( FILE *source, char *buffer, size_t size )

to read a hex field into `buffer`, which is `size` characters long,
delivering 0 if it fails (ie reaches EOF) and 1 otherwise.

You can add a `char *prompt` to the argument list if you like. Note that
you can then use `sizeof dest->dmac` (for example) for the `size` argument,
rather than couting yourself. (Note: this only works for array fields,
*not* pointer fields, for which the sizeof is *not* the amount of room
available).

Quote:

>  do
>  { printf("Enter in the smac: ");
>   scanf(" %12c", dest->smac);
>   while ( (ch = getchar() ) != '\n');  /* look above */
>   for ( x = 0; x<12; x++)
>    if ( !isxdigit(dest->smac[x]) )
>    { ch = 'z';
>     printf("SMAC invalid, try again.\n");
>    }
>  } while ( ch == 'z');

>  if ( (tmpbuf = (char * ) malloc ( 15
> 01 ) ) == NULL )

Was that `malloc(1501)` before something sliced it?

I'd have put the malloc at the top of the function, not buried it here.

Don't cast the return value from `malloc`, and make sure you #include
<stdlib.h>.

Full marks for checking for NULL.

Quote:
>  { printf("Error creating buffer for message: not enough memory.\n");
>   return 1;
>  }
>  else
>  { printf("Enter message: ");
>   x = 0;
>   scanf(" ");

What's that `scanf` for?

Quote:
>   while ( ( ch = getchar() ) != '\n'  && x <=1500 )
>    tmpbuf[x++] = ch;
>   tmpbuf[x]='\0';

*definitely* use `fgets` here.

Quote:

>   if (x < 46)
>   { diff = 46 - x;
>    for ( ; diff > 0; diff--, x++)
>     tmpbuf[x] = '.';
>    tmpbuf[x] = '\0';
>   }

Eh? What's this for? Padding out with some dots? What's special about 46?
Why not write this as

    while (x < 46) tmpbuf[x++] = '.';

and inject it just before the previous `tmpbuf[x] = '\0';`?

Quote:
>   dest->msgLength = x;
>   sprintf(dest->plength , "%04x", x);
>   if ( (dest->payloadEncoded = (char *) malloc(x*2 + 1) ) != NULL )

I'd have done this at the top of the function, where it's not buried.

Quote:
>   { txtStrngtohexStrng(tmpbuf, dest->payloadEncoded , x);  /* convert text
> "abc" to ascii hex "616263" */

We don't know what that does: it might trash something. (Since it's not here,
I can't save-and-compile the code. Although I'm running Linux; you had
your problems with Windows, yes?)

Quote:
>    printPacketHdr( dest);
>    writePacketHdr(output, dest);
>    free (dest->payloadEncoded);

I'm unhappy with a packet reader that trashes part of the data it's read!

Quote:
>   }
>   else
>   { printf("%i \n", dest->payloadEncoded);
>    printf("Error: Unable to allocate payload.\n");
>   }
>   free(tmpbuf);
>  }

You don't return a value here, and you should. (Although you don't test
for the value you *do* return ...)

Quote:
> }

> int main(int argc, char *argv[])
> {
> FILE *packetFile;
> packetHdr *packetHeader;   /* Please don't complain, explain (...why this is
> "yuk" :) */

I called it "yuk" because I detest type names that don't start with a capital
letter; I find them hard to read. (Yes, that includes `int` etc). You may
ignore it as a religious belief if you wish.

Quote:
>     if (argc == 1)
>  { printf("usage: %s <filename>\n", argv[0]);
>   return 1;
>  }
>  else
>  { if  ( (packetFile = fopen(*++argv, "r+")) == NULL )
>    printf("Error opening %s.\n", *argv);
>  }
> ....
>   case '+':
>    if(  (packetHeader = (packetHdr *) malloc ( sizeof(packetHdr))) == NULL )

Don't cast `malloc`, etc.

Quote:
>     printf("Error: Unable to allocate buffer for input.\n");
>    else
>    {
>     inputPacketData(packetFile,  packetHeader );
>     free (packetHeader);

Note: *this* would be the point to `free` the allocated char* fields of
the header (I'd write a function `freePacketHeader` for just this
purpose and park it near the structure definition.)

Quote:
>    }
>    break;
>   } // end of switch

>  fclose (packetFile);

>  return 0;
> }

Well, I'd hoped that going through and commenting on the code would also
highlight your problem, but it hasn't!

I hope the other comments are useful to you ...

--
Chris "electric hedgehog" Dollin
C FAQs at: http://www.faqs.org/faqs/by-newsgroup/comp/comp.lang.c.html



Sun, 27 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>
...

Quote:
> char *tmpbuf, ch;
...
>  while ( (ch = getchar() ) != '\n');  /* captures extra characters they write  */

Sorry, I have no insight into the problem you're actually asking about, but
I did want to point out that you should get into the habit of always
declaring as "int" any variables that receive the result of getchar(),
because you should be checking the getchar() result against EOF, to check
for errors and end of input.

--

Kenan Systems Corp., a wholly-owned subsidiary of Lucent Technologies



Sun, 27 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>

Quote:


> ...
> > char *tmpbuf, ch;
> ...
> >  while ( (ch = getchar() ) != '\n');  /* captures extra characters they write  */

> Sorry, I have no insight into the problem you're actually asking about, but
> I did want to point out that you should get into the habit of always
> declaring as "int" any variables that receive the result of getchar(),
> because you should be checking the getchar() result against EOF, to check
> for errors and end of input.

> --

> Kenan Systems Corp., a wholly-owned subsidiary of Lucent Technologies

ah... -1 on a char doesn't work to well then I take it...Thanks, and
this may be the answer to some other problems I was seeing.

Thanks Again - Tim



Sun, 27 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>

Quote:
> What control character? By the way, "//" probably only works by accident
> on your compiler; it wasn't official C until recently.

> >  fputc('-', output);  // control char to display

> Why is `-` a "control" character?

The plan was to read in the first char, if '-', decode the ascii-hex
into an actual message, such as "61626365..." into "abce" so people
could read what the message was.  the other control was '+', so the
could add a new message....all from a file, except the '+' part gets
keyboard input..

Quote:

> >  fputs(source->preamble, output);
> >  ....
> >  fputc('\n', output);
> >  return 0;

> Why? If it always returns 0, why have it return `int` rather than `void`?

I was...err..am going to put in some error checking so I know if it
prints, just got sidetrack with it not working in windows.

Quote:

> > }

> > int inputPacketData(FILE *output, packetHdr *dest)
> > {
> >  char *tmpbuf, ch;
> >  int diff, x;

> >  strcpy(dest->preamble , "aaaaaaaaaaaaaa");
> >  strcpy(dest->framedel , "ab");
> >  strcpy(dest->crc , "11223344");

> Why those particular strings? (Presumably they're obvious on output, and
> later versions will have different versions.)

Actually, the 'preamble' and 'framedel' are ALWAYS the same , but the
'crc' is supposed to be calculated.  The confusing part was I would have
put this as a binary file, and had all the values actually *values* so I
didn't have to do as much conversion, but another criteria is to be able
to edit the files, so I went ascii.

Quote:

> >  do
> >  {
> >   printf("Enter in the dmac: ");

> You should `fflush(stdout)` at this point, to ensure the prompt will be
> displayed. (It's also usual for prompts to go to `stderr` rather than
> `stdout`.)

will add, never though it wouldn't be displayed...and if I wanted to
make sure it went to stand out, would I have to do 'fprintf("message",
stdout);' ?

Quote:
> >   scanf(" %12c", dest->dmac);

> Avoid scanf. See the FAQ.

eh..i see 12.17 - 12.20...so it is just better to parse the input
yourself..got it!

Quote:

> >   while ( (ch = getchar() ) != '\n');  /* captures extra characters they write  */

> Good man!

WOOHOO!!
> >   for ( x = 0; x<12; x++)
> >    if ( !isxdigit(dest->dmac[x]) )
> >    { ch = 'z';
> >     printf("DMAC invalid, try again.\n");
> >    }
> >  } while ( ch == 'z');

> I'd suggest reading in the line with `fgets`, not `scanf`. In fact (in view
> of the repetition of the code below) I'd suggest writing a function

>     int readHexField( FILE *source, char *buffer, size_t size )

> to read a hex field into `buffer`, which is `size` characters long,
> delivering 0 if it fails (ie reaches EOF) and 1 otherwise.

> You can add a `char *prompt` to the argument list if you like. Note that
> you can then use `sizeof dest->dmac` (for example) for the `size` argument,
> rather than couting yourself. (Note: this only works for array fields,
> *not* pointer fields, for which the sizeof is *not* the amount of room
> available).

I'll put that in and test how it works.

- Show quoted text -

Quote:

> >  do
> >  { printf("Enter in the smac: ");
> >   scanf(" %12c", dest->smac);
> >   while ( (ch = getchar() ) != '\n');  /* look above */
> >   for ( x = 0; x<12; x++)
> >    if ( !isxdigit(dest->smac[x]) )
> >    { ch = 'z';
> >     printf("SMAC invalid, try again.\n");
> >    }
> >  } while ( ch == 'z');

> >  if ( (tmpbuf = (char * ) malloc ( 15
> > 01 ) ) == NULL )

> Was that `malloc(1501)` before something sliced it?

yes

Quote:

> I'd have put the malloc at the top of the function, not buried it here.

> Don't cast the return value from `malloc`, and make sure you #include
> <stdlib.h>.

doh..found it in the FAQ, don't need cast and hides warnings, thanks for
pointing that out!

Quote:

> Full marks for checking for NULL.

> >  { printf("Error creating buffer for message: not enough memory.\n");
> >   return 1;
> >  }
> >  else
> >  { printf("Enter message: ");
> >   x = 0;
> >   scanf(" ");

> What's that `scanf` for?

ehhh...it was a lazy attempt to skip white spaces, but if I am going to
parse the input myself, no need anymore.

- Show quoted text -

Quote:

> >   while ( ( ch = getchar() ) != '\n'  && x <=1500 )
> >    tmpbuf[x++] = ch;
> >   tmpbuf[x]='\0';

> *definitely* use `fgets` here.

> >   if (x < 46)
> >   { diff = 46 - x;
> >    for ( ; diff > 0; diff--, x++)
> >     tmpbuf[x] = '.';
> >    tmpbuf[x] = '\0';
> >   }

> Eh? What's this for? Padding out with some dots? What's special about 46?
> Why not write this as

>     while (x < 46) tmpbuf[x++] = '.';

> and inject it just before the previous `tmpbuf[x] = '\0';`?

now that I look at it...that loop is pretty useless.  As for 46, it is
the minimun size of the message.

Quote:

> >   dest->msgLength = x;
> >   sprintf(dest->plength , "%04x", x);
> >   if ( (dest->payloadEncoded = (char *) malloc(x*2 + 1) ) != NULL )

> I'd have done this at the top of the function, where it's not buried.

but I have to input the message and determine the size, I don't think I
can do that above??

Quote:

> >   { txtStrngtohexStrng(tmpbuf, dest->payloadEncoded , x);  /* convert text
> > "abc" to ascii hex "616263" */

> We don't know what that does: it might trash something. (Since it's not here,
> I can't save-and-compile the code. Although I'm running Linux; you had
> your problems with Windows, yes?)

Here it is....let me know what you think.

const char asciiHex[16] = {'0', '1', '2', '3', '4', '5', '6', '7', '8',
'9', 'a', 'b', 'c', 'd',                'e', 'f'};
void txtStrngtohexStrng(char *source, char *dest, int length)
{
        int x, j= 0;
        for ( x= 0; x < length; x++)
        {      
                dest[j++] = asciiHex[source[x] / 16 ];
                dest[j++] = asciiHex[source[x] % 16 ];
        }
        dest[j] = '\0';

- Show quoted text -

Quote:
}

> >    printPacketHdr( dest);
> >    writePacketHdr(output, dest);
> >    free (dest->payloadEncoded);

> I'm unhappy with a packet reader that trashes part of the data it's read!

> >   }
> >   else
> >   { printf("%i \n", dest->payloadEncoded);
> >    printf("Error: Unable to allocate payload.\n");
> >   }
> >   free(tmpbuf);
> >  }

> You don't return a value here, and you should. (Although you don't test
> for the value you *do* return ...)

> > }

> > int main(int argc, char *argv[])
> > {
> > FILE *packetFile;
> > packetHdr *packetHeader;   /* Please don't complain, explain (...why this is
> > "yuk" :) */

> I called it "yuk" because I detest type names that don't start with a capital
> letter; I find them hard to read. (Yes, that includes `int` etc). You may
> ignore it as a religious belief if you wish.

> >     if (argc == 1)
> >  { printf("usage: %s <filename>\n", argv[0]);
> >   return 1;
> >  }
> >  else
> >  { if  ( (packetFile = fopen(*++argv, "r+")) == NULL )
> >    printf("Error opening %s.\n", *argv);

> >  }
> > ....
> >   case '+':
> >    if(  (packetHeader = (packetHdr *) malloc ( sizeof(packetHdr))) == NULL )

> Don't cast `malloc`, etc.

> >     printf("Error: Unable to allocate buffer for input.\n");
> >    else
> >    {
> >     inputPacketData(packetFile,  packetHeader );
> >     free (packetHeader);

> Note: *this* would be the point to `free` the allocated char* fields of
> the header (I'd write a function `freePacketHeader` for just this
> purpose and park it near the structure definition.)

I thought i would want to 'free' the memory as soon as I was done, as
was the case above, is there any other advantage to putting them all in
the same function other than it is easier to find out whats going on. (
which is definitely better, but just curious )

- Show quoted text -

Quote:

> >    }
> >    break;
> >   } // end of switch

> >  fclose (packetFile);

> >  return 0;
> > }

> Well, I'd hoped that going through and commenting on the code would also
> highlight your problem, but it hasn't!

> I hope the other comments are useful to you ...

Extremely...I just picked up more with an actual critique of my code
than I have in the past 3hrs of lecture!


Sun, 27 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>


Quote:
> The plan was to read in the first char, if '-', decode the ascii-hex
> into an actual message, such as "61626365..." into "abce" so people
> could read what the message was.  the other control was '+', so the
> could add a new message....all from a file, except the '+' part gets
> keyboard input..

OK; be aware that the term `control character` usually refers to ascii
characters in the range 0..31, including such things as line feed aka newline,
carriage return, tab, form feed, etc & co.

Quote:
>> >  return 0;

>> Why? If it always returns 0, why have it return `int` rather than `void`?

> I was...err..am going to put in some error checking so I know if it
> prints, just got sidetrack with it not working in windows.

That's OK then -- in fact it's an excellent idea.

Quote:
>> >   printf("Enter in the dmac: ");

>> You should `fflush(stdout)` at this point, to ensure the prompt will be
>> displayed. (It's also usual for prompts to go to `stderr` rather than
>> `stdout`.)

> will add, never though it wouldn't be displayed...

Some implementations may not force the output to, er, the output, until
either a newline is written or fflush is called on the stream.

Quote:
> and if I wanted to
> make sure it went to stand out, would I have to do 'fprintf("message",
> stdout);' ?

`printf` goes to `stdout` anyway; to send it to `stderr`,

    fprintf( stderr, "message" );
or
    fputs( "message", stderr );

Some people object to `*printf` formats that don't do any formatting; I
don't mind (if you have a sequence of output statements, it's often clearer
to have them all use the same output routine, especially if that makes
common arguments, eg stderr, line up & look right).

Quote:
>> >   dest->msgLength = x;
>> >   sprintf(dest->plength , "%04x", x);
>> >   if ( (dest->payloadEncoded = (char *) malloc(x*2 + 1) ) != NULL )

>> I'd have done this at the top of the function, where it's not buried.

> but I have to input the message and determine the size, I don't think I
> can do that above??

Oops, quite right. Mumble. Well, I'd see what happened after I'd fiddled
with the rest of the code!

Quote:
>> >   { txtStrngtohexStrng(tmpbuf, dest->payloadEncoded , x);  /* convert text
>> > "abc" to ascii hex "616263" */

>> We don't know what that does: it might trash something. (Since it's not here,
>> I can't save-and-compile the code. Although I'm running Linux; you had
>> your problems with Windows, yes?)
> Here it is....let me know what you think.

> const char asciiHex[16] = {'0', '1', '2', '3', '4', '5', '6', '7', '8',
> '9', 'a', 'b', 'c', 'd',           'e', 'f'};
> void txtStrngtohexStrng(char *source, char *dest, int length)
> {
>    int x, j= 0;
>    for ( x= 0; x < length; x++)
>    {      
>            dest[j++] = asciiHex[source[x] / 16 ];
>            dest[j++] = asciiHex[source[x] % 16 ];
>    }
>    dest[j] = '\0';
> }

Looks OK, so long as `dest` is big enough (I think it was). Warning: in
this kind of function, the destination is often (as in, it is in the C
library and in lots of things that mimic it) the first argument, ie, the
same relative position as an assignment. (You may wish to copy this
convention, or not; just be aware that it exists.)

Quote:
>> >    {
>> >     inputPacketData(packetFile,  packetHeader );
>> >     free (packetHeader);

>> Note: *this* would be the point to `free` the allocated char* fields of
>> the header (I'd write a function `freePacketHeader` for just this
>> purpose and park it near the structure definition.)

> I thought i would want to 'free' the memory as soon as I was done, as
> was the case above, is there any other advantage to putting them all in
> the same function other than it is easier to find out whats going on. (
> which is definitely better, but just curious )

It's often useful to think of a data-type as a "thing" with a life-cycle and
related operations (this is part of the whole object-oriented attitude).
In which case, you try to think of nice re-usable operations. Make a Foo,
read a Foo, write a Foo, finish with a Foo ... are all things you'd like to
have, and be able to do more-or-less independantly.

You had a `read a Foo` (inputPacketData) that reads the data but doesn't
leave the complete packet around for further use, and you *don't* have a
`finish with Foo` operation that takes a packet and tidies it away. In the
longer term (next week), this will probably bite you.

Quote:
>> Well, I'd hoped that going through and commenting on the code would also
>> highlight your problem, but it hasn't!

>> I hope the other comments are useful to you ...

> Extremely...I just picked up more with an actual critique of my code
> than I have in the past 3hrs of lecture!

Oh, good.

--
Chris "I didn't do so well faced with an unruly bunch of 16-year-olds" Dollin
C FAQs at: http://www.faqs.org/faqs/by-newsgroup/comp/comp.lang.c.html



Mon, 28 Oct 2002 03:00:00 GMT  
 annoying fgets problem <fgets>

...

Quote:
> > > fseek(output, -1, SEEK_CUR);
> > > fputc('-', output);
...
> > >and call the function the only thing that actually gets printed are
the
> calls to fputc, and NOT fputs ???
...
> ps: on a totally unrelated, probably-shouldn't-put-in-here note, this
runs
> perfectly under linux, and writes to the file, but not in windows

I agree with most of the other suggestions about your code,
but this is your immediate problem.  You are opening a file
(with r+ though really used only for output) which leaves
the fileposn at beginning-of-file (0 if binary, which this isn't)
then trying to fseek to one byte before the current posn.
This is undefined, and in this case probably is affected
by whatever the underlying OS does, which only for linux
is accidentally "correct".

--
- David.Thompson 1 now at worldnet.att.net



Sun, 17 Nov 2002 03:00:00 GMT  
 
 [ 15 post ] 

 Relevant Pages 

1. <<<<<<<Parsing help, please>>>>>>>>

2. File Format conversion, ascii freeform -->.csv <-->.wk1<-->dbf<-->?HELP

3. <<<>>>Need C code advice with functions and sorting.<<<>>>

4. <><><>HELP<><><> PCMCIA Motorola Montana 33.6

5. >>>Windows Service<<<

6. <<Borland C/C++ 5.0 Problem >>!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!1

7. Problems with fgets and reading in a number

8. fgets() problem (is it a bug?)

9. fgets problem

10. fgets() and then fscanf() possible problems?

11. Problem with fgets()

12. fgets & sscanf problem

 

 
Powered by phpBB® Forum Software