Thread: Quick Problem Check

  1. #1
    Registered User awsdert's Avatar
    Join Date
    Jan 2015
    Posts
    1,733

    Quick Problem Check

    Before I go onto splitting this into files for gasp I'd like to ask anyone willing to just take a quick glance through the code and see if they see any problems (compiles and runs fine, the mm just stands for memory manager - gonna leave all allocations and deallocations to that one thread for the sake of stability in gasp as gasp will be sharing some memory across threads), the main problems to look for - if any - is if anything will cause calls to C within lua any problems.

    Code:
    #define _GNU_SOURCE
    #include <unistd.h>
    #include <errno.h>
    #include <stdbool.h>
    #include <inttypes.h>
    #include <stdlib.h>
    #include <stdio.h>
    #include <signal.h>
    #include <poll.h>
    #include <pthread.h>
    #ifdef _WIN32
    #include <namedpipeapi.h>
    #endif
    
    enum
    {
    	ALL_MSG_NIL = 0
    	, ALL_MSG_DIE
    	, MM_MSG_NEW
    	, MM_MSG_DEL
    	, MM_MSG_ALT
    	, MM_MSG_INC
    	, MM_MSG_DEC
    	, MM_MSG_DIE
    	, MM_MSG_WAIT
    	, MM_MSG_READY
    	, MM_MSG_COUNT
    };
    
    int smem_msg_num[MM_MSG_COUNT] = {-1};
    char *smem_msg_txt[MM_MSG_COUNT] = {NULL};
    
    void init_smem_msg_txt();
    
    struct memory_block
    {
    	void *block;
    	size_t bytes;
    };
    
    #define get_memory_block_bytes( block ) ((block)->bytes)
    #define get_memory_block_block( T, block ) ((T)((block)->block))
    
    /* Initialises memory block structure
     * 
     * @memory_block Pointer to structure you want to initialise, will segfault on
     * invalid pointer
     * 
     * @bytes Number of bytes you want to start with
     * 
     * Will attempt to allocate block of bytes to point the structure to
     * 
     * @return NULL on failure, valid pointer on success
    */
    void * new_memory_block( struct memory_block *memory_block, size_t bytes )
    {
    	memory_block->block = malloc( bytes );
    	memory_block->bytes = !!(memory_block) * bytes;
    	return memory_block->block;
    }
    
    /* Reallocates memory pointed by structure
     * 
     * @memory_block Pointer to structure you want to reallocate pointed memory of,
     * will segfault on invalid pointer
     * 
     * @bytes New number of bytes to allocate
     * 
     * Will attempt to reallocate block pointed to by structure and update the
     * members on success, on failure it is left as is, attempts with invalid
     * pointer in memory_block->block can lead to undefined behaviour, currently
     * whatever realloc() does in this situation is what happens here as it is
     * passed directly onto the function
     * 
     * @return NULL on failure, memory_block->block on success
    */
    void * alt_memory_block( struct memory_block *memory_block, size_t bytes )
    {
    	void * block = realloc( memory_block->block, bytes );
    	uintptr_t old = (uintptr_t)(memory_block->block), nxt = (uintptr_t)block;
    	memory_block->block = (void*)(nxt + (!nxt * old));
    	memory_block->bytes = (!!nxt * bytes) + (!nxt * memory_block->bytes);
    	return block;
    }
    
    /* Reallocates memory pointed by structure
     * 
     * @memory_block Pointer to structure you want to reallocate pointed memory of,
     * will segfault on invalid pointer
     * 
     * @bytes New number of bytes to allocate
     * 
     * Will attempt to reallocate block pointed to by structure ONLY if the wanted
     * bytes are more than the current amount allocated and update the
     * members on success, on failure it is left as is, attempts with invalid
     * pointer in memory_block->block can lead to undefined behaviour, currently
     * whatever realloc() does in this situation is what happens here as it is
     * passed directly onto the function
     * 
     * @return NULL on failure, memory_block->block on success
    */
    void * inc_memory_block( struct memory_block *memory_block, size_t bytes )
    {
    	if ( bytes > memory_block->bytes )
    	{
    		void * block = realloc( memory_block->block, bytes );
    		uintptr_t old = (uintptr_t)(memory_block->block), nxt = (uintptr_t)block;
    		memory_block->block = (void*)(nxt + (!nxt * old));
    		memory_block->bytes = (!!nxt * bytes) + (!nxt * memory_block->bytes);
    		return block;
    	}
    	return NULL;
    }
    
    /* Reallocates memory pointed by structure
     * 
     * @memory_block Pointer to structure you want to reallocate pointed memory of,
     * will segfault on invalid pointer
     * 
     * @bytes New number of bytes to allocate
     * 
     * Will attempt to reallocate block pointed to by structure ONLY if the wanted
     * bytes are less than the current amount allocated and update the
     * members on success, on failure it is left as is, attempts with invalid
     * pointer in memory_block->block can lead to undefined behaviour, currently
     * whatever realloc() does in this situation is what happens here as it is
     * passed directly onto the function
     * 
     * @return NULL on failure, memory_block->block on success
    */
    void * dec_memory_block( struct memory_block *memory_block, size_t bytes )
    {
    	if ( bytes > memory_block->bytes )
    	{
    		void * block = realloc( memory_block->block, bytes );
    		uintptr_t old = (uintptr_t)(memory_block->block), nxt = (uintptr_t)block;
    		memory_block->block = (void*)(nxt + (!nxt * old));
    		memory_block->bytes = (!!nxt * bytes) + (!nxt * memory_block->bytes);
    		return block;
    	}
    	return NULL;
    }
    
    /* Releases pointed memory
     * 
     * @memory_block Pointer to memory block structure holding a pointer to the
     * block to be released, will segfault on invalid pointer
     * 
     * Will attempt to release memory pointed to by memory_block->block, afterwards
     * it will reinitialise the members to 0 & NULL, invalid pointers can lead to
     * undefined behaviour, currently whatever free() does in this situation is
     * what happens here as the pointer is directly passed on to the function
    */
    void del_memory_block( struct memory_block *memory_block )
    {
    	free( memory_block->block );
    	memory_block->block = NULL;
    	memory_block->bytes = 0;
    }
    
    struct memory_group
    {
    	struct memory_block memory_block;
    	int total, count, focus;
    };
    
    #define get_memory_group_total( group ) ((group)->total)
    #define get_memory_group_count( group ) ((group)->count)
    #define get_memory_group_focus( group ) ((group)->focus)
    #define get_memory_group_bytes( group ) \
    	get_memory_block_bytes( &((group)->memory_block) )
    #define get_memory_group_group( T, group ) \
    	get_memory_block_block( T*, &((group)->memory_block) )
    #define get_memory_group_entry( T, group, index ) \
    	(get_memory_group_group( T, group )[index])
    
    struct memory_group shared_blocks = {0};
    
    #define get_shared_blocks_total() get_memory_group_total( &shared_blocks )
    #define get_shared_blocks_count() get_memory_group_count( &shared_blocks )
    #define get_shared_blocks_focus() get_memory_group_focus( &shared_blocks )
    #define get_shared_blocks_bytes() get_memory_group_bytes( &shared_blocks )
    #define get_shared_blocks_group() \
    	get_memory_group_group( struct memory_block, &shared_blocks )
    #define get_shared_blocks_entry( index ) \
    	(&(get_memory_group_entry( struct memory_block, &shared_blocks, index )))
    
    #define PIPE_RD 0
    #define PIPE_WR 1
    #define PIPE_COUNT 2
    
    #ifdef _WIN32
    typedef HANDLE pipe_t;
    #define INVALID_PIPE NULL
    #else
    typedef int pipe_t;
    #define INVALID_PIPE -1
    #endif
    
    int open_pipes( pipe_t *pipes );
    void shut_pipes( pipe_t *pipes );
    int pipe_err( pipe_t pipe );
    ssize_t rdpipe( pipe_t pipe, void *data, size_t size );
    ssize_t wrpipe( pipe_t pipe, void *data, size_t size );
    
    struct shared_block
    {
    	size_t want;
    	struct memory_block *memory_block;
    };
    
    struct worker_msg
    {
    	int type;
    	void *data;
    };
    
    #define INVALID_TID -1
    
    typedef struct worker
    {
    	// Thread reference
    	int num;
    	// Thread ID
    	pthread_t tid;
    	// Memory Manager
    	pipe_t mmi_pipes[PIPE_COUNT], mmo_pipes[PIPE_COUNT];
    	// Will an attempt to join be made?
    	bool join;
    	// Should an attempt to close member attr be made by main() during cleanup?
    	bool attr_created;
    	// Attributes
    	pthread_attr_t attr;
    } worker_t;
    
    size_t size;
    int have_workers;
    worker_t *workerv;
    
    int new_worker( char const * const name );
    #define get_worker( index ) ((index >= 0 && index )
    void del_worker( int index );
    
    typedef void * (*Worker_t)( void *arg );
    
    void * mmworker( worker_t *worker );
    void * fooworker( worker_t *worker );
    
    int main()
    {
    	int i;
    	enum
    	{
    		WORKER_NULL = 0,
    		WORKER_MM,
    		WORKER_FOO,
    		WORKER_COUNT
    	};
    	pipe_t pipes[PIPE_COUNT] = {INVALID_PIPE,INVALID_PIPE};
    	worker_t workers[WORKER_COUNT] = {{0}}, *worker, *worker_ret[WORKER_COUNT];
    	Worker_t Worker[WORKER_COUNT] = { NULL };
    	
    	Worker[WORKER_MM] = (Worker_t)mmworker;
    	Worker[WORKER_FOO] = (Worker_t)fooworker;
    #define JOIN
    #ifdef JOIN
    	workers[WORKER_MM].join = true;
    	workers[WORKER_FOO].join = true;
    #endif
    	init_smem_msg_txt();
    	
    	printf("Hello World!\n");
    	
    	if ( open_pipes( pipes ) != 0 )
    		goto cleanup;
    	
    	for ( i = 1; i < WORKER_COUNT; ++i )
    	{
    		printf( "Creating worker %d...\n", i );
    		
    		worker = &(workers[i]);
    		worker->num = i;
    		worker->tid = INVALID_TID;
    		worker->mmi_pipes[PIPE_RD] = pipes[PIPE_RD];
    		worker->mmi_pipes[PIPE_WR] = pipes[PIPE_WR];
    		worker->attr_created = false;
    		
    		if ( pthread_attr_init( &(worker->attr) ) != 0 )
    			goto cleanup;
    		
    		worker->attr_created = true;
    		
    		if
    		( pthread_create(
    				&(worker->tid), &(worker->attr), Worker[i], worker ) != 0
    		) goto cleanup;
    		
    		if ( !(worker->join) && pthread_detach( worker->tid ) != 0 )
    			goto cleanup;
    	}
    
    #ifdef JOIN
    	pthread_join( workers[WORKER_MM].tid, (void**)&(worker_ret[WORKER_MM]) );
    	pthread_join( workers[WORKER_FOO].tid, (void**)&(worker_ret[WORKER_FOO]) );
    #endif
    	
    	cleanup:
    	
    	while ( i > WORKER_MM )
    	{
    		--i;
    		worker = &(workers[i]);
    		
    		printf( "Cleaning worker %d\n", i );
    		
    		if ( worker->num >= 0 )
    			pthread_cancel( worker->tid );
    		
    		if ( worker->attr_created )
    			pthread_attr_destroy( &(worker->attr) );
    		
    		worker->attr_created = false;
    		worker->mmi_pipes[PIPE_WR] = INVALID_PIPE;
    		worker->mmi_pipes[PIPE_RD] = INVALID_PIPE;
    		worker->tid = INVALID_TID;
    	}
    	
    	shut_pipes( pipes );
    	
    	return 0;
    }
    
    int open_pipes( pipe_t *pipes )
    {
    #ifdef _WIN32
    	SECURITY_ATTRIBUTES sa;
    	sa.nLength = sizeof(sa);
    	sa.lpSecurityDescriptor = NULL;
    	sa.bInheritHandle = true;
    #endif
    	pipes[0] = pipes[1] = INVALID_PIPE;
    #ifdef _WIN32
    	int ret = (CreatePipe( &pipes[PIPE_RD], &pipes[PIPE_WR], 0 ) != 0 );
    	if ( ret != 0 )
    	{
    		pipes[PIPE_RD] = pipes[PIPE_WR] = INVALID_PIPE;
    	}
    #else
    	return pipe( pipes );
    #endif
    }
    void shut_pipes( pipe_t *pipes )
    {
    #ifdef _WIN32
    	CloseHandle( pipes[PIPE_WR] );
    	CloseHandle( pipes[PIPE_RD] );
    #else
    	close( pipes[PIPE_WR] );
    	close( pipes[PIPE_RD] );
    #endif
    	pipes[PIPE_RD] = pipes[PIPE_WR] = INVALID_PIPE;
    }
    int pipe_err( pipe_t pipe )
    {
    	return errno;
    }
    ssize_t rdpipe( pipe_t pipe, void *data, size_t size )
    {
    #ifdef _WIN32
    	DWORD bytes = 0;
    	ReadFile( pipe, data, size, &bytes, NULL );
    	return bytes;
    #else
    	return read( pipe, data, size );
    #endif
    }
    ssize_t wrpipe( pipe_t pipe, void *data, size_t size )
    {
    #ifdef _WIN32
    	DWORD bytes = 0;
    	WriteFile( pipe, data, size, &bytes, NULL );
    	return bytes;
    #else
    	return write( pipe, data, size );
    #endif
    }
    
    void * mmworker( worker_t *worker )
    {
    	int ret;
    	struct pollfd fds = {0};
    	pipe_t mm_pipes[PIPE_COUNT] =
    	{
    		worker->mmi_pipes[PIPE_RD],
    		worker->mmo_pipes[PIPE_WR]
    	};
    	
    	fds.fd = mm_pipes[PIPE_RD];
    	fds.events = POLLIN | POLLERR | POLLHUP | POLLNVAL;
    	
    	printf( "Worker %d started\n", worker->num );
    	
    	while ( (ret = poll( &fds, 1, 1 )) >= 0 )
    	{
    		if ( ret == 1 )
    		{
    			struct worker_msg *worker_msg = NULL;
    			ssize_t bytes;
    			
    			bytes = rdpipe(
    				mm_pipes[PIPE_RD], &worker_msg, sizeof(struct worker_msg) );
    			
    			printf( "Worker %d: Read %zd bytes\n", worker->num, bytes );
    		}
    	}
    	
    	/* Indicate the thread exited */
    	worker->num = -1;
    	return worker;
    }
    
    void * fooworker( worker_t *worker )
    {
    	int ret;
    	struct pollfd fds = {0};
    	pipe_t mm_pipes[PIPE_COUNT] =
    	{
    		worker->mmo_pipes[PIPE_RD],
    		worker->mmi_pipes[PIPE_WR]
    	};
    	ssize_t bytes;
    	struct worker_msg *worker_msg = NULL;
    	
    	printf( "Worker %d started\n", worker->num );
    	
    #if 0
    	while ( (ret = poll( &fds, 1, 1 )) >= 0 )
    	{
    		if ( ret == 1 )
    		{
    			struct worker_msg *worker_msg = NULL;
    			ssize_t bytes;
    			
    			bytes = rdpipe
    			(
    				mm_pipes[PIPE_RD]
    				, &worker_msg
    				, sizeof(struct worker_msg)
    			);
    			
    			printf( "Worker %d: Read %zd bytes\n", worker->num, bytes );
    		}
    	}
    #endif
    	
    	bytes = wrpipe(
    		mm_pipes[PIPE_WR], &worker_msg, sizeof(struct worker_msg) );
    	
    	printf( "Worker %d: Wrote %zd bytes\n", worker->num, bytes );
    	
    	/* Indicate the thread exited */
    	worker->num = -1;
    	return worker;
    }
    
    void init_smem_msg_txt()
    {
    	smem_msg_txt[ALL_MSG_NIL] = "ALL_MSG_NIL";
    	smem_msg_txt[ALL_MSG_DIE] = "ALL_MSG_DIE";
    	smem_msg_txt[MM_MSG_NEW] = "MM_MSG_NEW";
    	smem_msg_txt[MM_MSG_DEL] = "MM_MSG_DEL";
    	smem_msg_txt[MM_MSG_ALT] = "MM_MSG_ALT";
    	smem_msg_txt[MM_MSG_INC] = "MM_MSG_INC";
    	smem_msg_txt[MM_MSG_DEC] = "MM_MSG_DEC";
    	smem_msg_txt[MM_MSG_DIE] = "MM_MSG_DIE";
    	smem_msg_txt[MM_MSG_WAIT] = "MM_MSG_WAIT";
    	smem_msg_txt[MM_MSG_READY] = "MM_MSG_READY";
    }

  2. #2
    Registered User awsdert's Avatar
    Join Date
    Jan 2015
    Posts
    1,733
    Well an hour is long enough for people to see and do their quick check, the lack of responses tells me noone saw any problems jumping out at them so I will begin converting this to something gasp can use

  3. #3
    Registered User
    Join Date
    Sep 2020
    Posts
    425
    Line 406: You ask for four different poll events, then assume there is data to be read.

    Having 'worker_t' and 'Worker_t' seems problematic.

    This code is problematic
    Code:
                            struct worker_msg *worker_msg = NULL;
                            ssize_t bytes;
    
    
                            bytes = rdpipe(
                                    mm_pipes[PIPE_RD], &worker_msg, sizeof(struct worker_msg) );
    Also, about 200 lines of code you have asked people to review is not used - all the memory allocation stuff at the front.

  4. #4
    Registered User
    Join Date
    Sep 2020
    Posts
    425
    You call close() on the pipe fds even when open() fails.

    Using gotos to get out of the creation loop is a somewhat flawed pattern. Consider this instead:

    Code:
            for ( i = 1; i < WORKER_COUNT; ++i )
            {
                    printf( "Creating worker %d...\n", i );
    
                    worker = &(workers[i]);
                    worker->num = i;
                    worker->tid = INVALID_TID;
                    worker->mmi_pipes[PIPE_RD] = pipes[PIPE_RD];
                    worker->mmi_pipes[PIPE_WR] = pipes[PIPE_WR];
                    worker->attr_created = false;
    
                    if ( pthread_attr_init( &(worker->attr) ) != 0 )
                        break;
    
                    worker->attr_created = true;
    
                    if ( pthread_create( &(worker->tid), &(worker->attr), Worker[i], worker ) != 0)
                        break;
    
                    if ( !(worker->join) && pthread_detach( worker->tid ) != 0 )
                        break;
            }
    
    
    #ifdef JOIN
            if(i == WORKER_COUNT) {
               pthread_join( workers[WORKER_MM].tid, (void**)&(worker_ret[WORKER_MM]) );
               pthread_join( workers[WORKER_FOO].tid, (void**)&(worker_ret[WORKER_FOO]) );
            }
    #endif
    
            while ( i > WORKER_MM )
            {
    And WORKER_COUNT isn't a count of workers, which is pretty odd.

  5. #5
    Registered User
    Join Date
    Sep 2020
    Posts
    425
    You want to close the pipes in the works when done, so the reader gets a POLLHUP to react to.

    This is also broken:
    Code:
                struct worker_msg *worker_msg = NULL;
                ssize_t bytes;
                 
                bytes = rdpipe(
                    mm_pipes[PIPE_RD], &worker_msg, sizeof(struct worker_msg) );
    Last edited by hamster_nz; 03-15-2021 at 02:26 AM.

  6. #6
    Registered User
    Join Date
    Sep 2020
    Posts
    425
    Your use of the write() and read() system calls is broken.

  7. #7
    Registered User awsdert's Avatar
    Join Date
    Jan 2015
    Posts
    1,733
    Quote Originally Posted by hamster_nz View Post
    Line 406: You ask for four different poll events, then assume there is data to be read.
    Hmm, don't quite follow there but the method has slightly changed since the transfer anyways, if you care to take a look then here's the link:
    GitHub - awsdert/gasp at 366c971dec0a53760493334209f4079de6708acf

    Quote Originally Posted by hamster_nz View Post
    Having 'worker_t' and 'Worker_t' seems problematic.
    I think I get what you mean here and am considering changing the 'worker_t' to just plain "struct worker_data" for more clarity

    Quote Originally Posted by hamster_nz View Post
    This code is problematic
    Code:
                            struct worker_msg *worker_msg = NULL;
                            ssize_t bytes;
    
    
                            bytes = rdpipe(
                                    mm_pipes[PIPE_RD], &worker_msg, sizeof(struct worker_msg) );
    I think I caught that in the process of editing things after the transfer, thx anyways

    Quote Originally Posted by hamster_nz View Post
    Also, about 200 lines of code you have asked people to review is not used - all the memory allocation stuff at the front.
    Yeah, by the time I considered actually using that I thought f**k it, just transfer and use it after the editing (sorta done that now, just gotta finish that section off after getting the linker to link stuff).

  8. #8
    Registered User awsdert's Avatar
    Join Date
    Jan 2015
    Posts
    1,733
    Quote Originally Posted by hamster_nz View Post
    You call close() on the pipe fds even when open() fails.

    Using gotos to get out of the creation loop is a somewhat flawed pattern. Consider this instead:

    Code:
            for ( i = 1; i < WORKER_COUNT; ++i )
            {
                    printf( "Creating worker %d...\n", i );
    
                    worker = &(workers[i]);
                    worker->num = i;
                    worker->tid = INVALID_TID;
                    worker->mmi_pipes[PIPE_RD] = pipes[PIPE_RD];
                    worker->mmi_pipes[PIPE_WR] = pipes[PIPE_WR];
                    worker->attr_created = false;
    
                    if ( pthread_attr_init( &(worker->attr) ) != 0 )
                        break;
    
                    worker->attr_created = true;
    
                    if ( pthread_create( &(worker->tid), &(worker->attr), Worker[i], worker ) != 0)
                        break;
    
                    if ( !(worker->join) && pthread_detach( worker->tid ) != 0 )
                        break;
            }
    
    
    #ifdef JOIN
            if(i == WORKER_COUNT) {
               pthread_join( workers[WORKER_MM].tid, (void**)&(worker_ret[WORKER_MM]) );
               pthread_join( workers[WORKER_FOO].tid, (void**)&(worker_ret[WORKER_FOO]) );
            }
    #endif
    
            while ( i > WORKER_MM )
            {
    And WORKER_COUNT isn't a count of workers, which is pretty odd.
    Ah that WORKER_COUNT got dropped after the transfer, it was only for clarity in the test code (and I like to always treat 0 as a null member to catch faulty code out with), also the goto got dropped as well anyways, I normally use goto just to ensure I skip past code that could crash the app when it attempts to use pointers etc that are invalid, having a goto by default in that situation just ensure stability until I get round to thinking up a way that doesn't use it. Dunno what you mean with the "even when open() fails" part though, if you get round to mind highlighting the code in another post

  9. #9
    Registered User awsdert's Avatar
    Join Date
    Jan 2015
    Posts
    1,733
    Quote Originally Posted by hamster_nz View Post
    You want to close the pipes in the works when done, so the reader gets a POLLHUP to react to.

    This is also broken:
    Code:
                struct worker_msg *worker_msg = NULL;
                ssize_t bytes;
                 
                bytes = rdpipe(
                    mm_pipes[PIPE_RD], &worker_msg, sizeof(struct worker_msg) );
    The point of the pipes is to keep them open until the worker owning them dies, originally I was gonna have all the workers talk to each other via the pipes, I've since realised that was a partially bad idea as it would lead to faulty code all over the place, I now have the main thread acting as a middle man where every worker gets a reference to the main thread's pipes and a reference to pipes created for the main thread to talk to them, the main thread is now in charge of memory allocations too so there'll be no need for the risk of a memory managing worker suddenly dieing, at least if the main thread dies then all threads die with it. The main thread also handles the closer of pipes and other worker related stuff linked by the worker structure, although any internal stuff is still the responsibility of the worker itself to cleanup if it needs anything more than a deallocate (which again the main thread can act as a garbage handler for)

  10. #10
    Registered User
    Join Date
    Sep 2020
    Posts
    425
    I had a look in the repo, your usage of read() and write() is still broken.

    Code:
    ssize_t rdpipe( pipe_t pipe, void *data )
    {
        return read( pipe, data, sizeof(void*) );
    }
    
    
    ssize_t wrpipe( pipe_t pipe, void *data )
    {
        return write( pipe, data, sizeof(void*) );
    }

  11. #11
    Registered User awsdert's Avatar
    Join Date
    Jan 2015
    Posts
    1,733
    Quote Originally Posted by hamster_nz View Post
    I had a look in the repo, your usage of read() and write() is still broken.

    Code:
    ssize_t rdpipe( pipe_t pipe, void *data )
    {
        return read( pipe, data, sizeof(void*) );
    }
    
    
    ssize_t wrpipe( pipe_t pipe, void *data )
    {
        return write( pipe, data, sizeof(void*) );
    }
    This doesn't seem to agree with you, please explain since I'm not seeing it
    read(2) - Linux manual page

  12. #12
    Registered User
    Join Date
    Sep 2020
    Posts
    425
    Write does not have to write all the data requested, it can write just the first byte.

    On success, the number of bytes written is returned (zero indicates nothing was written). It is not an error if this number is smaller than the number of bytes requested;
    This is doubly important with pipes, where the pipe can only limited amount of data. It is possible that you might be dealing with this in higher-up layers... but you are most likely expecting the writes to be an atomic "all or nothing" deal.

    read() and write() can fail with retriable/recoverable errors - e.g. SIGINT:

    If a write() is interrupted by a signal handler before any bytes are written, then the call fails with the
    error EINTR; if it is interrupted after at least one byte has been written, the call succeeds, and returns the
    number of bytes written.
    The way the code at the start of the thread was written exposed you to these behaviors. This is very different to how fread()/fwrite() handle things.
    Last edited by hamster_nz; 03-15-2021 at 03:53 PM.

  13. #13
    Registered User awsdert's Avatar
    Join Date
    Jan 2015
    Posts
    1,733
    Quote Originally Posted by hamster_nz View Post
    Write does not have to write all the data requested, it can write just the first byte.



    This is doubly important with pipes, where the pipe can only limited amount of data. It is possible that you might be dealing with this in higher-up layers... but you are most likely expecting the writes to be an atomic "all or nothing" deal.

    read() and write() can fail with retriable/recoverable errors - e.g. SIGINT:



    The way the code at the start of the thread was written exposed you to these behaviors. This is very different to how fread()/fwrite() handle things.
    Ah so you meant that I wasn't checking the amount written/read, then yeah I get what you mean now, how 'bout this then?
    Code:
    			memset( rd2, 0, sizeof(void*) );
    			for ( bytes = 0, rdbytes = 0; rdbytes < sizeof(void*); )
    			{
    				bytes = rdpipe( pipes[PIPE_RD], rd2, sizeof(void*) - rdbytes );
    				if ( bytes >= 0 ) 
    				{
    					for ( byte = 0; byte < bytes; ++byte, ++rdbytes )
    						ptr2ptr[rdbytes] = rd2[byte];
    				}
    				else break;
    			}
    			
    			if ( bytes < 0 )
    				continue;
    I temporarily re-added the size parameter, depending on whether this method is a good idea I'll just move it inside the functions and remove the size parameter again
    Last edited by awsdert; 03-15-2021 at 05:17 PM. Reason: Forgot a parameter in my own code

  14. #14
    Registered User awsdert's Avatar
    Join Date
    Jan 2015
    Posts
    1,733
    Quote Originally Posted by awsdert View Post
    Ah so you meant that I wasn't checking the amount written/read, then yeah I get what you mean now, how 'bout this then?
    Code:
    			memset( rd2, 0, sizeof(void*) );
    			for ( bytes = 0, rdbytes = 0; rdbytes < sizeof(void*); )
    			{
    				bytes = rdpipe( pipes[PIPE_RD], rd2, sizeof(void*) - rdbytes );
    				if ( bytes >= 0 ) 
    				{
    					for ( byte = 0; byte < bytes; ++byte, ++rdbytes )
    						ptr2ptr[rdbytes] = rd2[byte];
    				}
    				else break;
    			}
    			
    			if ( bytes < 0 )
    				continue;
    I temporarily re-added the size parameter, depending on whether this method is a good idea I'll just move it inside the functions and remove the size parameter again
    Edit: Every example I've found treats this as a non-issue under PIPE_BUF or whatever it was called, it's often 4096 bytes apparently which is significantly more than the measly 4 or 8 bytes that is used for simple pointers, I'm gonna switch back the code and only revisit it if it ever actually causes a problem

  15. #15
    Registered User
    Join Date
    Sep 2020
    Posts
    425
    One day, after many hard nights of debugging where a few bytes have gone missing in the middle of a stream, or your program has terminated with a write error when you have plenty of disk space left, or your user suspends your program with CTRL+Z you will look back on this with fond memories...

    For write, it is common to just keep on trying the remaining buffer, until either all bytes are written, or a different error is received. Something like:

    Code:
       size_t written = 0;
       while(length > 0) {
          size_t bytes = write(df, buffer, length);
    
          if(bytes == -1) {
             if(errno == EINTR)
                continue;
             return -1;  // Or however you want to handle error
          } else {
             buffer  += bytes;
             length  -= bytes;  
             written += bytes;
          }
       }
       return written;
    Here's part of the "safe-read.c", that is part of GNU coreutils, which just checks for EINTR:

    Code:
    
    
    size_t
    safe_rw (int fd, void const *buf, size_t count)
    {
      for (;;)
        {
          ssize_t result = rw (fd, buf, count);
    
    
          if (0 <= result)
            return result;
          else if (IS_EINTR (errno))
            continue;
          else if (errno == EINVAL && SYS_BUFSIZE_MAX < count)
            count = SYS_BUFSIZE_MAX;
          else
            return result;
        }
    }

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Quick check for potential problems
    By awsdert in forum C Programming
    Replies: 28
    Last Post: 08-13-2020, 07:55 AM
  2. Quick program check
    By Nik635 in forum C Programming
    Replies: 1
    Last Post: 09-17-2015, 08:20 AM
  3. Quick check
    By SofaWolf in forum C Programming
    Replies: 6
    Last Post: 06-26-2012, 12:53 PM
  4. Need a quick check
    By Aliaks in forum C++ Programming
    Replies: 7
    Last Post: 06-05-2009, 04:57 AM
  5. Quick input check question
    By Jozrael in forum C++ Programming
    Replies: 3
    Last Post: 01-20-2009, 07:23 AM

Tags for this Thread