On May 25, 2006, at 2:15 AM, Chinki wrote: Hi Micheal, Thanks a lot for your suggestion!! I really appreciate the time you took to look into my code and sent your suggestions. Below is the updated code which communicates well with the USB bulk device (it might be useful to other users too!). Again, please correct me for my mistakes in the code if any,
I think it's about time for you to learn about a programming technique called "single exit". In the case of errors your function leaks a lot of memory and this technique makes it a little easier to keep track of things like that.
Typically, a 'single-exit' function looks a bit like this:
foo() { var a = NULL, b = NULL, c = NULL;
if ( !(a = alloc()) error = a_failed, goto out; if ( !(b = alloc()) error = b_failed, goto out;
error = do_stuff(a, b, &c); if (error) goto out;
do_other_stuff(c);
out: if (a != NULL) free(a); if (b != NULL) free(b); if (error && c != NULL) { free(c); c = NULL;
return(c); }
The general idea is that because there is only one entry point and one exit point for the function, you can control the set-up and tear-down of all the local variable state. Every time you add a new local variable to the top, you add initialisation code at the top, and cleanup code at the bottom. Any time you need to exit the function, even if you're heavily nested inside multiple loops, you can just jump to the exit label.
You'll find this technique most useful for functions that allocate more than one or two objects, or which set up a lot of state that must be torn down in the event of an intermediate error; it's not something to use everywhere, but your example below cries out for it. ///////////////////////////// bulkDeviceCommuncation()
{
IOReturn status = kIOReturnNoDevice;
UInt32 size = 0;
IOMemoryDescriptor *memDescOut = NULL;
IOMemoryDescriptor *memDescIn = NULL;
// Allocate the memory descriptor needed
memDescOut = IOMemoryDescriptor::withAddress(
(void *)&Command,
sizeof(COMMAND),
kIODirectionOut);
if ( memDescOut == NULL )
{
return kIOReturnNoResources;
}
if(!myDataStructure->fOutPipe)
{
DBLog(("\nOutpipe not valid\n"));
return kIOReturnNoDevice;
You leak memDescOut here. }
// Prepare the memory for I/O Transfer
status = memDescOut->prepare(memDescOut->getDirection());
It is not necessary to specify a direction here if it would be the same as the direction specified when you allocate the descriptor.
if(status != kIOReturnSuccess){
return status;
And here. }
status = (myDataStructure->fOutPipe)->Write( memDescOut,
(UInt32)1000, // Use the client's timeout for both
(UInt32)2000,
sizof(COMMAND),
(IOUSBCompletion*)NULL );
if(status == kIOReturnSuccess) {
// Complete process of memory after i/o transaction finsishes
status = memDescOut->complete(memDescOut->getDirection());
You need to call ->complete() regardless of whether the operation was successful or not. if(status != kIOReturnSuccess){
return status;
You leak memDescOut here. It's probably OK to ignore the return from ->complete() here, as there's nothing you can do about it and your operation itself has completed successfully. }
memDescIn = IOMemoryDescriptor::withAddress( (void*)&Response,
sizof(RESPONSE),
kIODirectionIn);
if ( memDescIn == NULL )
{
return kIOReturnNoResources;
You leak memDescOut here. }
// Prepare the memory for I/O Transfer
status = memDescIn->prepare(memDescIn->getDirection());
if(status != kIOReturnSuccess){
return status;
You leak memDescOut and memDescIn here. }
status = (myDataStructure->fInPipe)->Read(memDescIn,
(UInt32)1000,
(UInt32)2000,
size,
(IOUSBCompletion*)NULL,
&size);
} else {
return status;
You leak memDescOut and memDescIn here, and you seem to be missing a matching 'if'.
}
if(status != kIOReturnSuccess){
return status;
You leak memDescOut and memDescIn here, with the latter prepared. Bad. }
// Complete process of memory after i/o transaction finsishes
status = memDescIn->complete(memDescIn->getDirection());
It's probably OK to disregard the result of ->complete(), as there isn't much you can do about it and your operation has already completed successfully. if(status != kIOReturnSuccess){
return status;
}
//free
memDescIn = NULL;
memDescOut = NULL;
I'm not sure who taught you that setting a variable to NULL frees whatever it points to, but let me assure you that it doesn't in the kernel; there is no background GC.
You need to call ->release() on your IOMemoryDescriptors in order to cause them to be freed.
(please forgive me for the 4-space hard tabs here, Mail has some funny ideas)
bulkDeviceCommuncation()
{
IOReturn status;
UInt32 size;
IOMemoryDescriptor *memDescOut = NULL;
IOMemoryDescriptor *memDescIn = NULL;
// Validate and allocate if (myDataStructure->fOutPipe == NULL) { status = kIOReturnNoDevice; goto out; } if ((memDescOut = IOMemoryDescriptor::withAddress(&Command, sizeof(COMMAND), kIODirectionOut) == NULL) { status = kIOReturnNoMemory; goto out; } if ((memDescIn = IOMemoryDescriptor::withAddress(&Response, sizeof(RESPONSE), kIODirectinIn) == NULL) { status = kIOReturnNoMemory; goto out; }
// do outbound command if ((status = memDescOut->prepare()) != kIOReturnSuccess) goto out;
status = myDataStructure->fOutPipe->Write(memDescOut, 1000, 2000, sizeof(COMMAND), NULL); memDescOut->complete();
if (status != kIOReturnSuccess) goto out;
// do inbound response if ((status = memDescIn->prepare()) != kIOReturnSuccess) goto out;
status = myDataStructure->fInPipe->Read(memDescIn, 1000, 2000, size, NULL, &size); memDescIn->complete();
out: if (memDescOut) memDescOut->release(); if (memDescIn) memDescIn->release();
return(status); }
= Mike
|