Figuring out Write in Unix

Question

We've been assigned an ASCII compression project for Systems Programming, and I'm having a hard time with one specific line in my code.

I asked a question about compressing, and I adapted the array code to my program after working through the first dozen letters of a sample file on paper. In ddd, the values of the packed[] array are what I worked out on paper, but the values aren't being written to the file.

    unsigned char packed[7]; //compression storage
    int i, jCount;
    int j;

    int bufferLength= sizeof(unpacked)/sizeof(char);
    //loop through the buffer array
    for (i=0; i< bufferLength-1; i++){
        j= i%7;
        jCount++;

        //fill up the compressed array
        packed[i]= packer(unpacked[i], unpacked[i+1], j);

        //compressed array is full, write to file, then clear
        if ((j%7==6) && (j > 1)){
            int writeLoop;
            for (writeLoop=0; writeLoop < 8; writeLoop++){
                //printf("%X", packed[writeLoop]);  //write to screen
                write(openWriteFile, &packed[writeLoop], 1);//this is my trouble, write to file
            }

            memset(&packed[0], 0, sizeof(packed)); //clear array
        }
//more code down here for padding the end of short bytes.

The write function expects a const void * as the second argument, which is why I'm referencing the value of that particular array slot, but nothing is written to the file.

When I delete the &, I get a compile warning.

Any suggestions to get me down the right path are appreciated.

For starters, `packed` only has 7 bytes but your output loop iterates 8 times. Also, you shouldn't need a loop anyway; `write(openWriteFile, packed, 8)` should work. — chrisaycock, Mar 02 '11 at 05:22
If a system call appears to fail, it is generally a good idea to check the return value, and potentially errno in turn. And for Systems Programming homework, doing that and reporting it consistently might earn you extra marks. — Keith, Mar 02 '11 at 05:32
@ chrisaycock, no change at all. I have a `write(openWriteFile, &fileSize, 4)` declaration to write the original file size to the compressed file, and it writes the `int fileSize` to the file perfectly. @Keith, when I open the write file, it is created with the proper permissions, since rw show up in the `ls -l` output. I have error handling built in, but errors only show up as negative numbers, and `writeOpenFile` is a positive int. — Jason, Mar 02 '11 at 05:36
@Jason Your comments are confused/confusing. I don't think you understood what either chrisaycock or Keith wrote. In particular, write can fail, e.g., you could run out of space on the output device, but you don't check for failure. — Jim Balter, Mar 02 '11 at 05:47
@jason. Your test using an `int` is good. Not sure I follow your comment re. positive ,negative and `writeOpenFile`. The call to `::write` should be returning `1`. Is it? — Keith, Mar 02 '11 at 05:47
@ Keith, how do I tell what `write` returns? in `ddd`, I get `` when checking `write`. @Jim Balter, I definitely understand about running out of space in write, but considering the input file is only 63 bytes long, I doubt that's what's happening. Your comment about checking for errors is understood, and that's next on my list once the program functionality is assured. — Jason, Mar 02 '11 at 05:56
@Jason To find out what write returns, see the API documentation. Mine says "On success, the number of bytes written is returned (zero indicates nothing was written). On error, -1 is returned, and errno is set appropriately." I didn't say that running out of space is what's happening ... I wrote "e.g.", which means "for example". It's good practice to check for errors as a matter of course -- adding it later makes it more likely that you'll miss some instance ... or never even get around to it. — Jim Balter, Mar 02 '11 at 06:31

score 1 · Answer 1 · answered Mar 02 '11 at 05:24

1

You're reading past the end of the array. You declare packed[7] then access elements 0 to 7 (8 elements) in your write loop.

Typically, you would use write to write a series of bytes, not just one. In your case that means replacing

    int writeLoop;
    for (writeLoop=0; writeLoop < 8; writeLoop++){
        write(openWriteFile, &packed[writeLoop], 1);
    }

with

    write(openWriteFile, packed, 8);

and changing your declaration to

unsigned char packed[8]; //compression storage

Or maybe it should be a 7 byte write? In which case replace the 8s with 7s.

answered Mar 02 '11 at 05:24

Mike DeSimone

41,631
10
72
96

I was working under the impression that to write the contents of an array, a loop was needed for each element. Are you saying its not true? Also, it is a 7 byte write, not 8, as I'm shrinking 8 bytes to 7. – Jason Mar 02 '11 at 05:34
@Jason No, that's not true -- write has an argument that says how many bytes to write. And your writeLoop=0; writeLoop < 8 iterates 8 times, not 7. – Jim Balter Mar 02 '11 at 05:41
@Jason: The per-byte loop is inside the `write()` function. – Mike DeSimone Mar 02 '11 at 13:51

Jim Balter · Accepted Answer · 2011-03-02T05:54:01.727

0

I would think that should be packed[j] = ..., not packed[i]. And as chrisaycock notes, you can just write the whole packed array, you don't need a loop: write(openWriteFile, packed, sizeof packed); And you should avoid all those literals ... you have 3 instances of 7, one instance of 8, and one instance of 6, when you should have a single defined constant and use that + or - 1 as appropriate. Also jCount is uninitialized.

EDIT: Here's some code without those problems:

#define PACKEDSIZE 7 // Why 7?

    unsigned char packed[PACKEDSIZE]; //compression storage
    int i, jCount = 0;
    int j = 0;

    int bufferLength = sizeof(unpacked)/* /sizeof(char) ... not needed because sizeof(char) is 1 in all conforming implementations */;
    //loop through the buffer array
    for( i = 0; i < bufferLength-1; i++ ){
        jCount++;

        //fill up the compressed array
        packed[j]= packer(unpacked[i], unpacked[i+1], j);

        if( ++j == sizeof packed ){
            if( write(openWriteFile, packed, sizeof packed) != sizeof packed )
                reportWriteError(); // stub -- do real error handling here
            //memset(packed 0, sizeof packed); // this isn't necessary
            j = 0;
        }
    }

edited Mar 02 '11 at 05:54

answered Mar 02 '11 at 05:22

Jim Balter

16,163
3
43
66

Ah, good catch! The OP should also reset `j` to 0 when writing the buffer rather than rely on mod operations. – chrisaycock Mar 02 '11 at 05:27
I tried your suggestions, but the results were unchanged as the file size is showing 4 bytes after writing the file header. As for why 7 in packed, it was easier for me to understand bit shifting when using an 8 byte input array to 7 byte output. – Jason Mar 02 '11 at 05:49
@Jason Then you're doing something else wrong ... does this code even get executed? And how is unpacked defined? BTW, sizeof(char) is always 1, so you don't need that divide. – Jim Balter Mar 02 '11 at 05:51
You were right, it was something else. I had my end of array logic following this section, and it was triggering true on the first pass. unpacked is defined as a global variable above the main function. Thanks for the help! – Jason Mar 02 '11 at 06:09
`sizeof(char)` is always 1... even when `char` is 16 bits. ^_- – Mike DeSimone Mar 02 '11 at 13:52

Figuring out Write in Unix

2 Answers2