2

The following is an excerpt from myprogram.c:

int main(int argc, char *argv[])
{
   UserOptions opt;
   DmtxEncode *enc;
   unsigned char codeBuffer[BUFFER_SIZE];
   int codeBufferSize;
   opt = GetDefaultOptions();


   /* Read input data into buffer */
   codeBufferSize = ReadInputData(codeBuffer, &opt);

   //do something with the input data

   exit(0);
}

static size_t ReadInputData(unsigned char *codeBuffer, UserOptions *opt)
{
   int fd;
   ssize_t bytesRead;
   size_t bytesReadTotal;

   /* Open file or stdin for reading */
   fd = (opt->inputPath == NULL) ? 0 : open(opt->inputPath, O_RDONLY);

   /* Read input contents into buffer */
   for(bytesReadTotal = 0;; bytesReadTotal += bytesRead) {
      bytesRead = read(fd, codeBuffer + bytesReadTotal, BUFFER_SIZE);
      if(bytesRead == 0)
         break;
   }

   return bytesReadTotal;
}

This takes the input in the form of echo 1234 | myprogram and the program generates an output and terminates.

I would like to make this program execute continuously and accept user inputs where two separate inputs are differentiated by a newline (\n) character. Say for example I would like to give 1234 \n 5678 \n 6363 \n as the input. The program should first take the input 1234, generate an output for it, then see that there is a newline and treat 5678 as the second input, generate an output and so on.

I have tried modifying the ReadInputData function as follows:

static size_t ReadInputData(unsigned char *codeBuffer, UserOptions *opt) {
    int fd;
    ssize_t bytesRead;
    size_t bytesReadTotal;
    size_t startOfBlock;
    unsigned char tmpBuffer[BUFFER_SIZE];

    /* Open file or stdin for reading */
    fd = (opt->inputPath == NULL) ? 0 : open(opt->inputPath, O_RDONLY);

    for (bytesReadTotal = 0;; bytesReadTotal += bytesRead) {

        startOfBlock = tmpBuffer + bytesReadTotal;
        bytesRead = read(fd, startOfBlock, BUFFER_SIZE);

        for (int i = startOfBlock; i < startOfBlock + bytesRead; i++) {
            if (tmpBuffer[i] != '\n')
                codeBuffer[i] = tmpBuffer[i];
            else
                break;
    }
    }


    return bytesReadTotal;
}

and enclosed the workings of the main within a while(1) loop but this just produces the output for a single input and terminates the program.

Can someone help me figure out what I am doing wrong here?

EDIT: According to the suggestions, I modified the ReadInputData function as follows:

static size_t
ReadInputData(unsigned char *codeBuffer, UserOptions *opt) {
    int fd;
    ssize_t bytesRead;
    size_t bytesReadTotal;
    unsigned char* startOfBlock;
    unsigned char tmpBuffer[BUFFER_SIZE];

    /* Open file or stdin for reading */
    fd = (opt->inputPath == NULL) ? 0 : open(opt->inputPath, O_RDONLY);

    for (bytesReadTotal = 0;; bytesReadTotal += bytesRead) {

        startOfBlock = tmpBuffer + bytesReadTotal;
        bytesRead = read(fd, startOfBlock, DMTXWRITE_BUFFER_SIZE);

        if (bytesRead == 0)
            break;
        if (bytesRead == -1)
            FatalError(EX_IOERR, _("Message read error"));
        else if (bytesReadTotal > DMTXWRITE_BUFFER_SIZE)
            FatalError(EX_DATAERR, _("Message to be encoded is too large"));

        for (int i = 0; i < bytesRead; i++) {
            if (startOfBlock[i] != '\n')
                codeBuffer[i] = startOfBlock[i];
            else
                break;
        }

    }

This solves the problem of newline characters but the program exits after taking one input instead of accepting continuous inputs. The contents of main are inside a while(1) loop.

EDIT: main with while loop. No exit(0) present in main.

int main(int argc, char *argv[])
{
   UserOptions opt;
   DmtxEncode *enc;
   unsigned char codeBuffer[BUFFER_SIZE];
   int codeBufferSize;
   opt = GetDefaultOptions();

   while(1){

   /* Read input data into buffer */
   codeBufferSize = ReadInputData(codeBuffer, &opt);

   //do something with the input data

   }

}
22
  • I am not sure you can read \n as part of the input. Normally it is an input terminator. You can let your program loop, reading input after each action. Commented Oct 23, 2019 at 14:13
  • 1
    @Paul of course they can read '\n'; it's a common source of error, actually. Commented Oct 23, 2019 at 14:22
  • 1
    @JL2210 There is no fp ;-). You'll have to check the return value of read() for 0. Commented Oct 23, 2019 at 14:37
  • 1
    Why not simply read the input line by line with fgets? Commented Oct 23, 2019 at 15:01
  • 2
    fgets works just fine with binary data if it's delimited by \n (which you say in your question). Though delimiting binary data with newlines is somewhat strange and limiting because the data itself won't be able to hold \n characters. Commented Oct 23, 2019 at 15:31

1 Answer 1

1

There are quite a lot of problems with the code in ReadInputData. The biggest one is that your input is not preserved between calls. read will get all the data available to it (up to the count you pass in). So if there happens to be two lines available, ReadInputData will get both of them into tmpBuffer. You save the first one and the second is discarded when tmpBuffer goes out of scope.

You need a buffer that is preserved across calls.

Another problem is this test:

else if (bytesReadTotal > DMTXWRITE_BUFFER_SIZE)

if DMTXWRITE_BUFFER_SIZE is quite small such that the first two lines added together are greater than this, there will be a fatal error. I'm pretty sure you really need something like:

if (bytesReadTotal == BUFFER_SIZE)
{
    FatalError(EX_DATAERR, _("Message to be encoded is too large"));
}
bytesRead = read(fd, startOfBlock, min(DMTXWRITE_BUFFER_SIZE BUFFER_SIZE, BUFFER_SIZE - bytesReadTotal));

i.e. you need to be sure there is space in the buffer before you do the read. Also, there's no point in reading more than you are allowed to.


Returning to the first problem, there are two ways you can solve this.

  1. Make tmpBuffer and bytesReadTotal static variables

  2. Put them in a struct and pass a pointer to it into the call

struct ReadBuffer
{
    char tmpBuffer[BUFFER_SIZE];
    size_t bytesReadTotal;
}

int main(int argc, char *argv[])
{
    struct ReadBuffer buffer;
    buffer.bytesReadTotal = 0;

    // stuff 

    codeBufferSize = ReadInputData(codeBuffer, &opt, &buffer);

    // stuff

}

static size_t ReadInputData(unsigned char *codeBuffer, UserOptions *opt, struct ReadBuffer *buffer)
{
    // stuff

    startOfBlock = buffer.tmpBuffer + buffer.bytesReadTotal;
    // and so on
}

Then you need to add in all your code to manage the buffer and so on, but such a thing already exists in the C library, it's called a FILE. If you used the standard library buffered FILE API instead of raw Unix system calls, your life will be much easier. There's even a function (that many in comments are telling you to use, which reads and returns input up to the next \n. It's called fgets().


Just spotted another problem. You open the file each time you call ReadInputData. It's not a problem for stdin because you just use file descriptor 0. However, for any other file, you'll just read the first line each time until your process runs out of file descriptors. In my solution above, you'd have to open the file in main and add the file descriptor to the ReadBuffer struct. If you do that, you are definitely reinventing the C library FILE.

Here's a rough reimplementation

int main(int argc, char *argv[])
{
   UserOptions opt;
   DmtxEncode *enc;
   unsigned char codeBuffer[BUFFER_SIZE];
   ssize_t codeBufferSize;
   opt = GetDefaultOptions();

   FILE* input = opt->inputPath == NULL ? stdin : open(opt->inputPath, "r");
   if (input == NULL)
   {
       // Handle the error
   } 

   while(1)
   {

       /* Read input data into buffer */
       codeBufferSize = fgets(codeBuffer, BUFFER_SIZE, input);
       if (codeBufferSize == -1)
       {
           // Handfle IO error
       }
       else if (codeBufferSize == 0) 
       { 
           // reached end of file
       }
       else if (codeBuffer[codeBufferSize - 1] != '\n')
       {
           // Handle input truncated because line was too long
       }
       else 
       { 
           //do something with the input dat
       }
   }
   if (input != NULL)  
   {
       fclose(input);
   }
}


Sign up to request clarification or add additional context in comments.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.