copy data from an array into a data structure

Go To Last Post
10 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

hi

i wrote this code to copy data from an array of bytes into an data structure

typedef struct {
	unsigned char data[512];
	unsigned short offset;
}TDataBlock;
unsigned char db_read(void* __ptr, size_t __size, size_t __nmember, TDataBlock* db)
{

	unsigned int __buffSize = __size * __nmember;
	if(__buffSize > sizeof(db->data)) return 0xFF; // check buffer size

	// cast both source and destination in to an array of bytes
	char* __surc = (char*) db->data;
	char* __dest = (char*) __ptr;

	unsigned int ii = 0;
	for(unsigned int i=db->offset; i<(db->offset+__buffSize); i++)
	{
		__dest[ii] = __surc[i]; // move data
		ii++;
	}

	return 0;
}

the results seems fine and the function works!

i just wanted to know your opinion about it and if there is a problem with this code, please let me know

thanks.

This topic has a solution.
Last Edited: Sun. Jun 18, 2017 - 02:44 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Don't use double underscore prefix in user code (apart from the fact that it is reserved for the compiler namespace it simply obfuscates the code!)

BTW why did you feel the need to reinvent memcpy()?

Last Edited: Sat. Jun 17, 2017 - 07:30 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

K-AVR wrote:
if there is a problem with this code
Besides the things already mentioned by Cliff:

if(__buffSize > sizeof(db->data)) return 0xFF; // check buffer size

Why is db->offset not taken into account here?

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

OK.

thank you all for good advises.

i rewrote the code and now its like this:

unsigned char db_read(void* ptr, unsigned int memb_size, unsigned int nmemb,TDataBlock* db)
{
	unsigned int buffsize = memb_size * nmemb;
	if(buffsize > sizeof(db->data)) return 0xFF;
	if((db->offset+buffsize) > sizeof(db->data)) return 0xFF;
	memmove(ptr, db->data, buffsize);
	return 0;
}

and i added a new function to change the offset member of TDatablock

unsigned char db_seek(TDataBlock* db, unsigned int _offset, unsigned char _origin)
{
	if(_offset > 511) return 0xFF;
	if(_offset < 0) return 0xFF;
	
	switch(_origin)
	{
		case SEEK_SET : db->offset = _offset; break; // move form start
		case SEEK_CUR : if((db->offset + _offset) > 511) return 0xFF; db->offset += _offset; break; // move from current location
	}
	
	return 0;
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
	if(buffsize > sizeof(db->data)) return 0xFF;
	if((db->offset+buffsize) > sizeof(db->data)) return 0xFF;

Why twice?

memmove(ptr, db->data, buffsize);

That is not equivalent to your former code. There the source pointer was data+offset.

And why memmove instead of memcpy?

	if(_offset > 511) return 0xFF;

I would not use a magic number here (and later in the function).

if(_offset < 0) return 0xFF;

Can't happen, _offset is unsigned.

Stefan Ernst

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

you right replacing my own code with memmove() completely ruined it.

so i am going to stick with my own code.

 

and about the first question : maybe you did not understand the role of offset member in TDatablock.

it holds current position in our data block.

its like a caret in a text file.

 

the first if statement checks the size of our destination buffer, it must not bigger that our source.

the second if statement prevents overflow in reading data form data block.

 

exp:

if offset member in TDatablock is 400

and size of data member in TDatablock is 512 bytes and you want to read data in to a buffer with a size of 250 bytes the code will overflow.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

K-AVR wrote:
you right replacing my own code with memmove() completely ruined it. so i am going to stick with my own code.

I already wrote what is wrong.

 

K-AVR wrote:
the first if statement checks the size of our destination buffer, it must not bigger that our source.

the second if statement prevents overflow in reading data form data block.

But the first if doesn't cover any case that is not also covered by the second. If A>B, then is A+C>B also true (for C being unsigned). (*)

 

Edit:

(*) except A is that much off that A+C overflows. Since your function setting C ensures C<B, it is better to use A>B-C. That truly covers both if statements.

Stefan Ernst

Last Edited: Sun. Jun 18, 2017 - 11:13 AM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

that was a really good explanation thank you.

now i understand what you mean and why first if statement was completely useless.

 

Here is the code :

unsigned char db_read(void* ptr, unsigned int memb_size, unsigned int nmemb,TDataBlock* db)
{
	unsigned int buffsize = memb_size * nmemb;
	if((buffsize+db->offset) > sizeof(db->data)) return 0xFF;
	
	char* _surc = (char*) db->data;
	char* _dest = (char*) db->ptr;
	
	int ii = 0;
	for(unsigned int i=db->offset; i<(db->offset+buffsize); i++)
	{
		_dest[ii] = _surc[i];
		ii++;
	}
	
	return 0;
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Why do you need the variable ii? You can take care of that when you assign your pointers. I'm not sure what the compiler would generate for your code, but i expect your code can be simplified so it would be easier to read and execute.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
Why do you need the variable ii? You can take care of that when you assign your pointers.

well.

that actually a very good point. thank you Kartman

i just solved that problem too

here is the code:

unsigned char db_read(void* ptr, unsigned int memb_size, unsigned int nmemb,TDataBlock* db)
{
	unsigned int buffsize = memb_size * nmemb;
	if((buffsize+db->offset) > sizeof(db->data)) return 0xFF;
	
	char* _surc = (char*) db->data;
	char* _dest = (char*) ptr;
	
	for(unsigned int i=db->offset; i<(db->offset+buffsize); i++)
	{
		*_dest = _surc[i];
		 _dest++;
	}
	
	return 0;
}