Solved

memcpy a struct to an array

Posted on 2009-04-01
18
587 Views
Last Modified: 2012-05-06
I'll try to keep this as short as I can. I am basically implementing a mailbox system, which basically can store (send/receive) any type of messages, which can be a struct, char, int.. or whatever. However just now I realize that it doesn't work for sending over a struct. I've attached my mailbox code and as well as the test code I have. I believe the problem is with memcpy... but please let me know what I should do to fix it.

Some information regarding the code:
1. arg->arg3 is equal to 40 here, I got that from sizeof(DiskStruct), I don't know why it's 40
2. arg->arg2 here is the pointer to the DiskBuffer ds
3. boxToSend->box[i] is where I want to copy the struct to, it's basically just some space on the
    memory. I've malloced this before, I just didn't show it on the code below. This will be used to read back from the  
    mailbox. The size of box[i] is set to be bigger than the size of the struct as I don't know what kind of data will be
    put there, it's up to the user who uses the mailbox
4. In the struct I have void* buffer, this is basically because I can put/send anything to the
    mailbox, a char, struct, or whatever I want. Therefore I don't know the type and how big it
    will be so I declared it as a void*

The problem is actually because after I did the memcpy below and tried to read the struct back at boxToSend->box[i], the result I get is totally different. I tried to extract the value of unit, track, first, etc and it prints out some random weird numbers.  Which implies that the memcpy doesn't work. Please help me on this. And let me know if you need further information


typedef struct {
	P1_Semaphore sem;
	int unit;
	int track;
	int first;
	int sectors;
	int action; /* DISK_READ for read, DISK_WRITE for write */
	void* buffer;
} DiskStruct;
 
DiskStruct* ds = (DiskStruct*) malloc(sizeof(DiskStruct));
ds->sem = P1_SemCreate(0);
ds->unit = unit;
ds->track = track;
ds->first = first;
ds->sectors = sectors;
ds->action = USLOSS_DISK_WRITE;
ds->buffer = buffer;
 
 
 
/*ACTUAL CODE*/
 
memcpy((void*) &(boxToSend->box[i]), arg->arg2, arg->arg3);

Open in new window

0
Comment
Question by:kuntilanak
  • 9
  • 7
18 Comments
 
LVL 40

Expert Comment

by:mrjoltcola
ID: 24046079
I don't think you posted enough code for me to help. So far it looks ok, albeit dangerous, since you admit DiskStruct->buffer can point to anything, so I'm not sure how you can tell what it points to unless you save the size in the mailbox as well. But that might be a different issue.

Post more code?
0
 

Author Comment

by:kuntilanak
ID: 24046285
here's some more code.. I hope you understand, as I don't know where to start explaining.. USLOSS_Args is just a struct.. The arg is just as I explained above for the send, so I hope you can trace it back
void MBoxCreate(USLOSS_Sysargs* arg)
{
	if (numboxes != P2_MAX_MBOX) {
		MBox* newBox = (MBox*) malloc(sizeof(MBox));
		newBox->sender = P1_SemCreate(1);
		newBox->receiver = P1_SemCreate(0);
		newBox->receipt = P1_SemCreate(0);
		newBox->numslots = arg->arg1;
		newBox->size = arg->arg2;
		newBox->numMessages = 0;
		newBox->flag = -1;
 
		/* CREATE THE MAILBOX */
		void** newArray = (void**) malloc(newBox->size * newBox->numslots);
		int i;
 
		newBox->box = newArray;
 
		for (i = 0; i < P2_MAX_MBOX; i++)
			if (MBoxes[i] == NULL)
				break;
 
		MBoxes[i] = newBox;
 
		if (arg->arg1 < 0 || arg->arg2 < 0)
			arg->arg4 = -1;
		else
			arg->arg4 = 0;
 
		arg->arg1 = i;
		numboxes++;
 
		newBox->messSizes = (int*) malloc(sizeof(int) * newBox->numslots);
	} else {
		arg->arg1 = -1;
	}
 
}
 
 
void MBoxSend(USLOSS_Sysargs* arg)
{
	int id = (int) arg->arg1;
	if (id < 0 || id >= P2_MAX_MBOX || MBoxes[id] == NULL) {
		arg->arg4 = -1;
		return;
	}
 
	MBox* boxToSend = MBoxes[id];
 
	if (arg->arg3 > boxToSend->size) {
		arg->arg4 = -1;
		return;
	}
 
	P1_P(boxToSend->sender);
 
	// send the message
	int i = 0;
	for (; i < boxToSend->numslots; i++)
		if (boxToSend->messSizes[i] == 0) {
			break;
		}
 
 
	/* CHECK IF MB IS FULL */
	if (i == boxToSend->numslots) {
		P1_V(boxToSend->receiver);
		if (boxToSend->flag == 1) {
			P1_V(boxToSend->receipt);
		}
		boxToSend->flag = 0;
		P1_P(boxToSend->receipt);
		for (i = 0; i < boxToSend->numslots; i++)
			if (boxToSend->messSizes[i] == 0) {
				break;
			}
	} else {
		P1_V(boxToSend->receiver);
 
	}
 
	memcpy((void*) &(boxToSend->box[i]), arg->arg2, arg->arg3);
 
	
	boxToSend->messSizes[i] = arg->arg3;
	arg->arg2 = (void*) boxToSend->messSizes[i];
	arg->arg4 = 0;
	boxToSend->numMessages++;
 
	P1_V(boxToSend->sender);
	if (boxToSend->flag == 1) {
		P1_V(boxToSend->receipt);
		boxToSend->flag = -1;
	}
 
}

Open in new window

0
 

Author Comment

by:kuntilanak
ID: 24046313
The problem might be in here:

void** newArray = (void**) malloc(newBox->size * newBox->numslots);

that's actually where the actual mailbox is and where I actually send/do a memcpy if I send something to the mailbox.

I declared it as a void** as I don't know what type of array that is, besides that array could contain different kind of things, index 0 can be a struct, index 1 can be an int, etc..etc.. so that's why it's a void**
0
ScreenConnect 6.0 Free Trial

Check out the updates in one game-changing release, ScreenConnect 6.0, based on partner feedback. New features include a redesigned UI that improves session organization and overall user experience. See the enhancements for yourself!

 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 24047661
>>>> memcpy((void*) &(boxToSend->box[i]), arg->arg2, arg->arg3);

What has this statement to do with the DiskStruct structure above?

>>>> ds->buffer = buffer;

You prrobably need to copy the buffer to newly allocated memory rather than assigning a pointer. That would mean you either need to know the max size of the buffer or have to add an additional member which specifies the size of the buffer:

(A)

typedef struct {
      P1_Semaphore sem;
      int unit;
      int track;
      int first;
      int sectors;
      int action; /* DISK_READ for read, DISK_WRITE for write */
      unsigned char buffer[MAX_BUF_SIZE];
} DiskStruct;

or

(B)

typedef struct {
      P1_Semaphore sem;
      int unit;
      int track;
      int first;
      int sectors;
      int action; /* DISK_READ for read, DISK_WRITE for write */
      void* buffer;
                int      bufsiz;
} DiskStruct;



For (A) you would have

    memcpy(ds->buffer, buffer, min(MAX_BUF_SIZ, sizBuf));

where sizBuf would be an additional argument passed to your function with buffer.

For (B) you would have


    ds->buffer = (unsigned char*)malloc(sizBuf);
    memcpy(ds->buffer, buffer, sizBuf);

where gain sizBuf would be an additional argument passed to your function with buffer.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 24047800
>>>> void** newArray = (void**) malloc(newBox->size * newBox->numslots);
Here you were allocating an array of pointers (only), but not the memory for the data.  But the 'newBox->size * newBox->numslots´' expression seems to be the size of data and not the number of slots times size of pointer.

I wonder why you want to go the stony way of dynamic allocation when you are so much unexperienced in pointers and allocation issues. For example the

    memcpy((void*) &(boxToSend->box[i]), arg->arg2, arg->arg3);

most probably would crash as I can't see a proper allocation for boxToSend->box[i].

You better would use fixed-sized arrays and structures on the stack which are much easier to handle.

typedef struct
{
     int length;
     int senderId;
     int receiverId;
     int status;      
     unsigned char message[MAX_MSG_SIZ];
} MailBoxSlot;

typedef struct
{
     int numslotsUsed;
     int ownerId;
     MailBoxSlot[MAX_SLOTS];
} MailBox;

then with

   MailBox mbox = { 0 };

you have a fully allocated mailbox with all members set to zero.

BTW, it seems it is a .cpp source (or you wouldn't be able to create new variables in the middle of a scope). Why don't using more comfortable and safer C++, e, g, container classes or string class?



   
0
 

Author Comment

by:kuntilanak
ID: 24050209
if I do it your way above with the structs and array... how can I say copy the diskstruct to the array mailbox?
0
 

Author Comment

by:kuntilanak
ID: 24050318
>>BTW, it seems it is a .cpp source (or you wouldn't be able to create new variables in the middle of a >>scope). Why don't using more comfortable and safer C++, e, g, container classes or string class?

I am, working on C, not C++
0
 

Author Comment

by:kuntilanak
ID: 24050419
>>But the 'newBox->size * newBox->numslots´' expression seems to be the size of data and not the >>number of slots times size of pointer.

is that how C interprets it? I was thinking that if I did a void** that would mean to be an array of (void)* objects...

is there a way to fix this without using the struct approach you showed us, as I'll need to change a lot of stuffs that way... I already have an array of mailboxes, so treating each slot as a struct and a mailbox as a struct would make it more complex
0
 
LVL 39

Accepted Solution

by:
itsmeandnobodyelse earned 500 total points
ID: 24052805
>>>> as I'll need to change a lot of stuffs that way...
But your mallocs don't work in the moment and as you have not posted the structures it is impossible to say where you missed an allocation and where not. And a void** is nothing where I could make any analysis from.

My structure was only a sample. I would suggest to take your structure(s) and exchange pointer members by fixed-sized arrays (taht way you corrected the biggest flaws and could use all following statements (beside of malloc).

Instead of an array of pointers use a fixed-sized array of structures. You only have to change the arrow -> by a period .

If you need to pass the structure to a function use the & to pass it as pointer.

>>>> I already have an array of mailboxes

No, you have an array of unitialized pointers pointing to nowhere. The malloc only reserves space for the pointers not for the data.

>>>>  I was thinking that if I did a void** that would mean to be an array of (void)* objects...

indeed, but a void* object is a pointer and nothing else. Worse, it is an uninitialized pointer and you can be happy if it is NULL by accident. The malloc doesn't know what you have left of the assignment operator. It simply allocates memory for the size you pass and returns the start address of that memory. If you request

     newBox->size * newBox->numslots

memory and the size is 64 and numslots is 100 you got 6400 bytes allocated. Dividing that by pointer size it is 1600 for 32bit pointers and 800 for 64bit pointers. Let us assume the latter, then you may access

 newArray[0] to newArray[799]

without accessing beyond the allocation made by malloc. I assume you don't actually need 800 slots for your mailbox but only numslots slots. Then it would have been

   void** newArray = malloc(newBox->numslots * sizeof(void*));

And to get an allocation for each slot you need

   for (i = 0; i < newBox->numslots; ++i)
         newArray[i] = malloc(newBox->size);  

if the size is in bytes. But the slots still were not initialized. So it is

   for (i = 0; i < newBox->numslots; ++i)
   {
         newArray[i] = malloc(newBox->size);  
         memset(newArray[i], 0, newBox->size);
    }

or using calloc instead of malloc.

You could get rid of all that by using a fixed sized array of your structure for one slot. And it automatically would be zero if you do like

   MailboxSlot   newArray[MAX_SLOTS]  = { 0 };

Define it at top of your function and get rid of the malloc.

   



0
 

Author Comment

by:kuntilanak
ID: 24053804
ok, I've changed my code of initializing the array to the following:

however now about having problems to copy the buffer to the struct, I wanted to use option B that you showed on the second post. However how do I get the bufSize, below is part of the function signatures I have


/*code*/
int P2_DiskWrite(int unit, int track, int first, int sectors, void* buffer)
{
	if (unit < 0 || unit >= USLOSS_DISK_UNITS)
		return -1;
 
	DiskStruct* ds = (DiskStruct*) malloc(sizeof(DiskStruct));
	ds->sem = P1_SemCreate(0);
	ds->unit = unit;
	ds->track = track;
	ds->first = first;
	ds->sectors = sectors;
	ds->action = USLOSS_DISK_WRITE;
	ds->buffer = malloc(/*what is the size supposed to be here*/);
	memcpy(ds->buffer, buffer, /*what is the size of void* */);
        ................................
/**********************/
 
 
void** newArray = (void**) malloc(sizeof(void*) * newBox->numslots);
int i;
for (i = 0; i < newBox->numslots; i++){
	newArray[i] = malloc(sizeof(newBox->size));
	memset(newArray[i], 0, newBox->size);
}

Open in new window

0
 

Author Comment

by:kuntilanak
ID: 24054221
ok I think I got it to work now, however I need to understand what this is... so void** is an array of void*, and then when I do the:

newArray[i] = malloc(sizeof(newBox->size));

for each slots I have, is this basically allocating space for how big each slot is, newArray[i] type is a void* right? and then when I do a memcpy(newArray[i] , ds, structsize);

am I actually copying a pointer to another pointer, and not actually copying the real value that ds is pointing to? or am I actually copying the value (i.e the struct) to newArray[i] ?? If I am just copying a pointer to another place then, I'd say this is useless right as the data pointed by the pointer might be changed and the results when I tried to receive from would be different from what I originally send.
I hope this makes sense... I know I am not experienced enough with pointers, especially with void*, but I am trying  to understand what I am doing here and I hope what I am doing does what I want to do.. which is copying the actual data (struct) to the array slots
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 24054864
>>>> newArray[i] type is a void* right?

Yes, a pointer to unknown (or arbitrary) data.

>>>> then when I do a memcpy(newArray[i] , ds, structsize);

Then, you were copying data ...

   newArray[i] = ds;

would be copying the pointer.

>>>> I know I am not experienced enough with pointers, especially with void*

You must not use void* or even void**

If you have a structure which maps your mailbox slot, say  typedef struct {  ... } MailboxSlot;

Then you could/should do

   MailboxSlot* newArray = (MailboxSlot*)malloc(newBox->numSlots*sizeof(MailboxSlot*));

That is much better than to using a void** which actually is nothing else but a 'unsigned char**' a pointer to an array of pointers to byte arrays, or less. You can every array of pointers look at as an array of pointers to sequences of bytes but actually it doesn't help much if you couldn't decide. It only prevents the compiler from being able to check type integrity and hence it is less secure and more error-prone. Note, the void* type is much less then a baseclass pointer in C++. It actually is only for defining an argument type so that the argument can take any (address of) data. Using a void* for non argument types simply is bad programming.  
0
 

Author Comment

by:kuntilanak
ID: 24056316
ok.. I am going to bear with the void** as of now.. as it already works and I thought of it that it would need to take a lot of work to change it to the struct implementation you said... also say that  newArray[i]  is already occupied by something and I want to use it for some other storage do I need to free it first?
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 24057118
>>>> also say that  newArray[i]  is already occupied by something and I want to use it for some other storage do I need to free it first?

Yes for any malloc you need a free. If you allocated memory in a loop for each slot you need to free it in a loop again.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 24060724
Why did you give a C grade?

A C grade is appropriate when you got no (more) answers though the initial question was not yet answered.

A B grade is appropriate if you got valid answers but there either were things left open or you had to work out most parts of the solution yourself.

An A grade should be given if one or more comments do answer the initial question sufficiently.

So, here you could choose between B or A as you got a lot of answers even if you did not like them all.
0
 

Author Comment

by:kuntilanak
ID: 24061477
I am sorry, I didn't mean to give you a C grade.. I meant to give you A since the first time as you explained everything clearly.. must have pressed the wrong button. I am sorry once again, however I just want to let you know that I really appreciate your help.. how can I change it to A now
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
ID: 24061705
You could check the 'Request Attention' Link at the top of that thread and the bottom of your question.

That way you can get a moderator to reopen the question.

Thanks for responding. I already assumed that it happened by accident.

Regards
0

Featured Post

Netscaler Common Configuration How To guides

If you use NetScaler you will want to see these guides. The NetScaler How To Guides show administrators how to get NetScaler up and configured by providing instructions for common scenarios and some not so common ones.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

Title # Comments Views Activity
C++ to C# code conversion issue 4 107
How to gracefully close the c++ 11 thread? 3 95
Error creating a new C++ project in ,net 20 36
macro vba to pivot table 1 20
Templates For Beginners Or How To Encourage The Compiler To Work For You Introduction This tutorial is targeted at the reader who is, perhaps, familiar with the basics of C++ but would prefer a little slower introduction to the more ad…
IntroductionThis article is the second in a three part article series on the Visual Studio 2008 Debugger.  It provides tips in setting and using breakpoints. If not familiar with this debugger, you can find a basic introduction in the EE article loc…
The goal of the tutorial is to teach the user how to use functions in C++. The video will cover how to define functions, how to call functions and how to create functions prototypes. Microsoft Visual C++ 2010 Express will be used as a text editor an…
The viewer will learn how to pass data into a function in C++. This is one step further in using functions. Instead of only printing text onto the console, the function will be able to perform calculations with argumentents given by the user.

810 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question