🚀 go-pugleaf

"Remy Lebeau \(TeamB\)" <no.spam@no.spam.com> wrote:
>
>"Peter" <piotrtrzeciak@yahoo.com> wrote in message 
>news:479f7070$1@newsgroups.borland.com...
>
>> Here is the code:
>
>Other than error checking that is missing, I see nothing else in that code 
>that would cause the kind of problems you are describing.
>
>> ImageData *imageData;
>
>What does the declaration of ImageData actually look like?
>
>> imageData = (ImageData *)malloc(sizeof(ImageData));
>
>malloc() can return NULL, which you are not checking for.  In fact, since 
>you always free() the ImageData instance before exiting, there is no point 
>in dynamically allocating it at all.  You could declare it on the stack 
>instead and let the compiler manage it for you.
>
>> imageData->INITC = RawToBytes("\x00\xA1", 2);
>> imageData->DISPLAY = RawToBytes("\x00\xA9", 2);
>> imageData->EVN_MESSAGE = RawToBytes("\x00\xA7", 2);
>
>Why are you treating the command IDs as 2-byte buffers instead of 2-byte 
>integers?  That would make your processing much cleaner.  You could get rid 
>of the INITC, DISPLAY, and EVN_MESSAGE members altogether and just use 
>compile-time constants instead.
>
>> imageData->requested_length = imageData->length - 2;
>
>You are not checking to make sure that requested_length is greater than 0 
>before reading and processing the buffer.
>
>> // This return is here only for testing to exit without reading more data.
>
>That would be a memory leak since you don't free the ImageData instance 
>before returning.
>
>> if ( (imageData->rcv_buffer[0] == imageData->INITC[0]) && 
>> (imageData->rcv_buffer[1] == imageData->INITC[1]) && (imageData->length > 
>> 4) ) {
>> // INITC as OINT, no reads, just write a response to the socket
>> processOint(AContext, imageData);
>> }
>> else {
>> if ( (imageData->rcv_buffer[0] == imageData->INITC[0]) && 
>> (imageData->rcv_buffer[1] == imageData->INITC[1]) && (imageData->length 
>> <=4) ) {
>> // INITC as INIT, no reads, just write a response to the socket
>> processInit(AContext, imageData);
>> }
>
>Those if statements are a bit redundant.  You could clean that up as follows 
>(more so if you get rid of the INITC buffer and use integers instead):
>
>    if( (imageData->rcv_buffer[0] == imageData->INITC[0]) && 
>(imageData->rcv_buffer[1] == imageData->INITC[1]) )
>    {
>        if( imageData->length > 4 )
>        {
>            // INITC as OINT, no reads, just write a response to the socket
>            processOint(AContext, imageData);
>        }
>        else
>        {
>            // INITC as INIT, no reads, just write a response to the socket
>            processInit(AContext, imageData);
>        }
>    }
>
>> Now, I put in a return statement in the middle, just to see if I can get
>> more than one connection, but it still fails.
>
>You need to be more specific about what "it fails" actually means.
>
>> For any "command" other than DISPLAY, it works fine, because they
>> only come as single connections. If I get a series of DISPLAY commands,
>> the first one usually goes ok (depends on the timing), but the second 
>> client
>> connection kills the service.
>
>Then most likely your processing and responding code, which you have no 
>shown yet, is not managing the data/connection properly and corrupting the 
>service in some way.
>
>> The only other thing I can think of, is that I'm adding the TIdTCPServer
>> component to a wrong place (TService), and that's why it gets "clobbered".
>
>Putting a TIdTCPServer inside the TService works perfectly fine when used 
>properly.
>
>
>Gambit 
>
>

Well... I stripped all the code that's not relevant (there is error checking and other things there :-)). And certain code "ugliness" comes from the initial version that was written based on network traces.
The only thing I can improve is to get rid of memory management (malloc) and let it do it automatically, but to be honest, I do not think that's the fix here.
I'll keep looking.
Thanks again!