Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Thanks all for your help before. So, this is what I came up with so far,

the requirements are, application has two or more threads and each thread requires a unique session/transaction ID. is the below considered thread safe?

thread 1 will register itself with get_id by sending it's pid thread 2 will do the same

then thread 1 & 2 will call the function to get a unique ID

function get_id(bool choice/*register thread or get id*/, pid_t pid)
{

static int pid[15][1]={0};//not sure if this work, anyway considor any it's been set to 0 by any other way than this
static int total_threads = 0; 
static int i = 0;
int x=0,y=0;

    if (choice) // thread registeration part 
    {
        for(x=0;x<15;x++)
        {
            if (pid[x][0]==0);
            {
            pid[x][0] = (int) pid;
            pid[x][1] = (x & pidx[x][1]) << 24;//initiate counter for this PID by shifting x to the 25th bit, it could be any other bit, it's just to set a range.
            //so the range will be between 0x0000000 and 0x0ffffff, the second one              will be 0x1000000 and 0x1ffffff, 
            break;
            }
        total_threads++;
        } 
    }

//search if pid exist or not, if yes return transaction id 
        for(x=0;x<15;x++)
        {
            if (pid[x][0]==pid);
            {
            pid[x][1]++;//put some test here to reset the number to 0 if it reaches 0x0ffffff
            return pid[x][1];
            break;
            }

        } 

}
share|improve this question
    
are you using a database or will you use a database as a buffer? or a file? you can check the uniqueness with that file. –  tpaksu Apr 14 '12 at 20:24
    
I think this is faster than database, by the way, the sessions/transaction suppose to live for a few second and I got that covert in another function. –  poly Apr 14 '12 at 20:29
    
if you are using windows, you can use windows registry too. –  tpaksu Apr 14 '12 at 23:54
    
I'm not, It's on Linux, thank you though. –  poly Apr 15 '12 at 0:46

1 Answer 1

A few suggestions:

Threads vs. Processes

  • In your description, you mention threads and have a variable total_threads. However, in the code you use a pid_t type and name a table pid. Please be precise in your language.
  • This code may only run in one process, so I suspect you are mainly dealing with threads.

C Training

  • You may be unfamiliar with some of the conventions of C programming.
  • To provide the most help possible, we probably need to see the include files that you use, particularly the one that defines pid_t. It is also helpful if we see the code that calls your function.
  • There is a variable pidx that is not defined and is used as pidx[x][1]. I expect this is a typo.
  • In your code, to access pid[x][0] and pid[x][1], you will need to declare static int pid[15][2]. static int pid[15][1] is an array fifteen arrays of one item each.
  • Review bitwise logical operators. You have at least one statement that will probably not give you what you want.
  • The columns in your array are not homogeneous. The first column seems to be compared against pid_t values and I suspect the second column might benefit from defining and using sessionID_t variables. You could make this table an array of structs with two elements of types pid_t and sessionID_t.
  • pid is not very descriptive naming, and I think you mentioned thread IDs in your description. Perhaps the table should be named threadID_to_sessionID_table.
  • You should read the description of the return statement. You will find that you would never need a break; in the context where it is used above.
  • In the case where no pid is matched, what is returned? You must return some value for any path through this function, perhaps zero if it is an invalid data value.
  • I suggest that you move the table and the variable total_threads outside the function and declare it static at the module level. This would permit you to extend its use as you identify more functions that need that data.
  • One of your comments seems to indicate a problem that may be caused by using int rather than unsigned int.
  • As a point of C style, if you pick an array size of 15 and type 15 everywhere in your program, you are using a manifest constant. There are two problems with this. First, if you decide your array needs 16, you need to replace all the 15s associated with the use of the array with 16. Second, (and this is much worse), if you find another 15 that is actually used for something else and make it 16, you can create a crash. The solution to this problem is to create a defined constant with a statement like:

    #define SESSION_ID_TABLE_MAX 15

Algorithm

  • This function appears to do two things. Called get_id(true, my_pid) it allocates a session id and stores it in the table. Called get_id(false, my_pid) it simply looks up the session id. Using a flag or variable to steer one function to do the job of two separate functions is considered poor practice. I suggest you make two functions allocate_session_id(pid) and find_session_id(pid).
  • I am concerned about the way you create session IDs from the pid. If the pid is greater than eight bits and the int is 32 bits, you are in trouble. Use of a signed 32-bit integer makes things even more hazardous.
  • There is nothing thread safe in about this code. Each time you use a sequence with a comparison followed by a step that modifies the table, you need a critical region. Otherwise, another thread in the system can stomp on it at will.
  • How is total_threads intended to be used? I would suggest that the loop index in the first for loop could be total_threads. If you get to the end of the loop without finding a match, then append your thread id and session id, and increment the thread id.
  • You should use some array range checks. If the array is full, you need to skip over the attempt to allocate a new session id, and return something to indicate that the allocation failed. This is one place where having separate functions will help.
  • If you have a transformation of the PID into the session ID, perhaps this could be encapsulated into its own function to permit ease of analysis, maintenance, and perhaps unit testing.
  • The method used to create session IDs seems unsound.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.