Wonky piece of code detected by clang

[nemerle]

  idle_buffer.c:135: warning: comparison of array 'idle_buffer.buffer[i + 1].packet' not equal to a null     pointer is always true [-Wtautological-pointer-compare]
            if(idle_buffer.buffer[i+1].packet!=NULL){
               ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~  ~~~~

This is caused by 'packet' member never being null, since it's pre-allocated array of size MAX_PROTOCOL_PACKET_SIZE. I'm not 100% sure how to approach this. In my 'cpp' version this code is using a dynamically resize-able vector thus the 'c' version ->

        //compress buffer
        i=0;
        for(i=1; i<=idle_buffer.buffer_count-1; i++){

            strcpy(idle_buffer.buffer[i].sql, idle_buffer.buffer[i+1].sql);

            idle_buffer.buffer[i].connection=idle_buffer.buffer[i+1].connection;
            idle_buffer.buffer[i].process_type=idle_buffer.buffer[i+1].process_type;

            if(idle_buffer.buffer[i+1].packet!=NULL) {

                //memcpy is unreliable hence copy data bytes across individually
                int j=0;
                for(j=0; j<packet_length; j++){
                    idle_buffer.buffer[i].packet[j]=idle_buffer.buffer[i+1].packet[j];
                }
            }
        }

        //clear last entry in database buffer
        memset(&idle_buffer.buffer[i+1], 0, sizeof(idle_buffer.buffer[i+1])) ;

        idle_buffer.buffer_count--;

looks like

    //compress buffer
    idle_buffer.pop_front();

:smile:

[themuntdregger]

Well done Clang. Even -pedantic didn't see that issue.

idle_buffer.pop_front(); certainly looks cleaner, so I think this is a good opportunity for some ++goodness; plus a good opportunity for me to work on my ++skills lol

[themuntdregger]

Ok, so my attempt ++skills has come up with this :

#include <stdio.h>
#include <string.h>

#include "logging.h"
#include "db/database_functions.h"
#include "server_start_stop.h"
#include "log_in.h"
#include "character_creation.h"

#include <iostream>
#include <queue>
#include <stdexcept>

#include "idle_buffer2.h"

#define IDLE_BUFFER2_MAX 100
#define MAX_PROTOCOL_PACKET_SIZE2 160

struct data_{

    char sql[1024];
    unsigned char packet[80];
    int connection;
    int process_type;
};

std::queue<data_> idle_buffer2;

void push_idle_buffer2(const char *sql, int connection, int process_type, unsigned char *packet){

    /** public function - see header **/

    data_ data;

    if(idle_buffer2.size() < IDLE_BUFFER2_MAX){

        strcpy(data.sql, sql);
        data.connection=connection;
        data.process_type=process_type;

        //find length of packet
        int packet_length=packet[1]+(packet[2]*256)-1+3;

        //if packet length > 0 then copy packet data to buffer
        if(packet_length > 0){

            if(packet_length > MAX_PROTOCOL_PACKET_SIZE2-1){

                log_event(EVENT_ERROR, "maximum protocol packet size exceeded in function %s: module %s: line %i", __func__, __FILE__, __LINE__);
                stop_server();
            }

            for(int i=0; i<packet_length; i++){

                data.packet[i]=packet[i];
            }
        }

        idle_buffer2.push(data);
    }
    else {

        //buffer overflow
        log_event(EVENT_ERROR, "database buffer overflow in function %s: module %s: line %i", __func__, __FILE__, __LINE__);
        stop_server();
    }
}

void process_idle_buffer2(){

    /** public function - see header **/

    //make sure we have something in the buffer to process
    if(idle_buffer2.size() > 0){

        int connection=idle_buffer2.front().connection;
        int packet_length=idle_buffer2.front().packet[1] + (idle_buffer2.front().packet[2] * 256) + 2;

        unsigned char packet[1024]={0};

        for(int i=0; i<packet_length; i++){
            packet[i]=idle_buffer2.front().packet[i];
        }

        //use else if structure rather than switch, as this allows us to encapsulate
        //variables within each if statement
 /**********************************************************************************************/

        if(idle_buffer2.front().process_type==IDLE_BUFFER2_PROCESS_CHECK_NEWCHAR){

            //Checks whether a character name exists in the character_table of the database. If the
            //name exists, character creation is aborted and a message sent to the client. If the
            //name does not exist, the character creation packet is placed in the idle buffer with
            //an instruction for IDLE_BUFFER_PROCESS_ADD_NEWCHAR so as the new character is added to
            //the database at the next idle event
            check_new_character(connection, packet);
        }
/**********************************************************************************************/

        else if(idle_buffer2.front().process_type==IDLE_BUFFER2_PROCESS_ADD_NEWCHAR){

            add_new_character(connection, packet);
        }
/**********************************************************************************************/

        else if(idle_buffer2.front().process_type==IDLE_BUFFER2_PROCESS_LOGIN){

            process_log_in(connection, packet);
         }
/**********************************************************************************************/

        else if(idle_buffer2.front().process_type==IDLE_BUFFER2_PROCESS_SQL){

            process_sql(idle_buffer2.front().sql);
        }
/**********************************************************************************************/

        else {

            log_event(EVENT_ERROR, "unknown process type in function %s: module %s: line %i", __func__, __FILE__, __LINE__);
            stop_server();
        }

        idle_buffer2.pop();
    }
}

what do you think ?

[themuntdregger]

Massive uncool bug in that last code...

int packet_length=idle_buffer2.front().packet[1] + (idle_buffer2.front().packet[2] * 256) + 2;

which seg faults when you try to pop a null packet. Have now corrected this by adding a packet_len value to the queue and using this to test for null packets.

Let me know what Clang says :)

[nemerle]

If you want to see my approach: https://github.com/nemerle/unoff-landz/blob/cpp/Server/idle_buffer.cpp https://github.com/nemerle/unoff-landz/blob/cpp/Server/idle_buffer.h

[themuntdregger]

Lmao. I'm happy that I managed to work out the basic technique. However, I slapped myself when I saw I was pushing to the front, rather than the back of the queue.

I've learned masses about C++ in the process, and it has given me confidence to try more. However, your version of the code looks far cleaner and nicer than mine, and, I can also see why it will be more extensible. I therefore think we should go with your version. Please go ahead and make the commit.

Apologies also. I should have placed my (trial) code in a pull request.

Question - why do you prefer a 'void *dat' to an 'unsigned char array' when handling packets ?

[nemerle]

Well 'my' version is using the unoff_packeteer library - so it will need some TLC before it can be merged. I'll work on moving towards the same processing flow though.

As for void *data instead of unsigned char array ? My version was putting pre-processed packets in there, so i could simply cast data to appropriate type.

process_log_in(*command.client, (const LogIn *)command.data);
// or
add_new_character(command,(CreateChar *)command.data);

and more importantly, queue and vector are storing the values, and given that data_ is holding a whole array any operation will have to actually move/copy the whole array contents. My approach only requires a single pointer copy ( so 4/8 bytes vs copying MAX_PROTOCOL_PACKET_SIZE2 of bytes )

[themuntdregger]

Nice :+1: