Merge lp:~tmaesaka/drizzle/drizzle-fix-drizzleslap into lp:~drizzle-trunk/drizzle/development

Proposed by Toru Maesaka on 2009-05-18
Status: Merged
Merged at revision: not available
Proposed branch: lp:~tmaesaka/drizzle/drizzle-fix-drizzleslap
Merge into: lp:~drizzle-trunk/drizzle/development
Diff against target: None lines
To merge this branch: bzr merge lp:~tmaesaka/drizzle/drizzle-fix-drizzleslap
Reviewer Review Type Date Requested Status
Brian Aker 2009-05-19 Resubmit on 2009-06-01
Jay Pipes (community) 2009-05-18 Approve on 2009-05-19
Stewart Smith (community) Approve on 2009-05-19
Review via email: mp+6668@code.launchpad.net
To post a comment you must log in.
Toru Maesaka (tmaesaka) wrote :

This merge involves three fixes:

(1) There was a block of code that assumed memory allocation
    is always successful so a check is added.

(2) Fix a segmentation fault that is caused due to
    trying to free a invalid pointer in statement_clean().

    Note: This fix is not perfect.

    Ideally, we should move the query_statement list to
    a proper container like std::vector and create a
    nice cleanup function for it (which I'm planning to
    do next). This way, we can get rid of the small
    memory leak that has been in drizzleslap for a while.

(3) Fix a memory leak due to not freeing the string
    obtained inside my_open(). So, use my_close() instead
    of directly closing the descriptor with close(2).

Cheers,
Toru

Toru Maesaka (tmaesaka) wrote :

Err, sorry I forgot to mention that all these fixes are for the drizzleslap tool :)

Jay Pipes (jaypipes) wrote :

Hi Toru!

I think we've been trying to get rid of calls to my_open() and my_close() in favour of the standard open()/close() methods... so I'd prefer to see the calls to my_open() disappear than going back to my_close() :)

Toru, I can tell you didn't want to go back to my_close()...is there another reason you did that?

-jay

review: Needs Fixing
Stewart Smith (stewart) wrote :

Would be great if we removed my_open instead

review: Needs Fixing
Toru Maesaka (tmaesaka) wrote :

Jay, Stewart

Hi!

Fair enough I'll get rid of my_open() and resubmit the patch.

> Toru, I can tell you didn't want to go back to my_close()...is there another
> reason you did that?

What I had in mind was to refactor drizzleslap later on and get rid of mysys dependencies then . But I guess I was just being lazy :(

Cheers,
Toru

> Hi Toru!
>
> I think we've been trying to get rid of calls to my_open() and my_close() in
> favour of the standard open()/close() methods... so I'd prefer to see the
> calls to my_open() disappear than going back to my_close() :)
>
> Toru, I can tell you didn't want to go back to my_close()...is there another
> reason you did that?
>
> -jay

1018. By Toru Maesaka on 2009-05-19

Replace occrrences of my_(open|close) with straight open() and close() system calls

Toru Maesaka (tmaesaka) wrote :

Hi!

As suggested in the review, I've replaced all occurrences of my_open() and my_close() in drizzleslap with open(2) and close(2) system calls.

Next stop: Use a real list container for query statements.

Cheers,
Toru

Stewart Smith (stewart) :
review: Approve
Jay Pipes (jaypipes) wrote :

Approved :) Much better :)

review: Approve
Brian Aker (brianaker) wrote :

Hi!

I've asked Toru to take the patch, and apply it to trunk. This tree came off of some tree that must have been abandoned (lots of bad stuff in the merge).

Cheers,
   -Brian

review: Resubmit
Wesley Batista (w-le-y) wrote :

hi

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'client/drizzleslap.cc'
2--- client/drizzleslap.cc 2009-03-26 09:22:55 +0000
3+++ client/drizzleslap.cc 2009-05-18 06:43:07 +0000
4@@ -1563,7 +1563,7 @@
5 bytes_read= read(data_file, (unsigned char*) tmp_string,
6 (size_t)sbuf.st_size);
7 tmp_string[sbuf.st_size]= '\0';
8- close(data_file);
9+ my_close(data_file, MYF(0));
10 if (bytes_read != sbuf.st_size)
11 {
12 fprintf(stderr, "Problem reading file: read less bytes than requested\n");
13@@ -1620,7 +1620,7 @@
14 bytes_read= read(data_file, (unsigned char*) tmp_string,
15 (size_t)sbuf.st_size);
16 tmp_string[sbuf.st_size]= '\0';
17- close(data_file);
18+ my_close(data_file, MYF(0));
19 if (bytes_read != sbuf.st_size)
20 {
21 fprintf(stderr, "Problem reading file: read less bytes than requested\n");
22@@ -1667,7 +1667,7 @@
23 bytes_read= read(data_file, (unsigned char*) tmp_string,
24 (size_t)sbuf.st_size);
25 tmp_string[sbuf.st_size]= '\0';
26- close(data_file);
27+ my_close(data_file, MYF(0));
28 if (bytes_read != sbuf.st_size)
29 {
30 fprintf(stderr, "Problem reading file: read less bytes than requested\n");
31@@ -1716,7 +1716,7 @@
32 bytes_read= read(data_file, (unsigned char*) tmp_string,
33 (size_t)(sbuf.st_size));
34 tmp_string[sbuf.st_size]= '\0';
35- close(data_file);
36+ my_close(data_file, MYF(0));
37 if (bytes_read != sbuf.st_size)
38 {
39 fprintf(stderr, "Problem reading file: read less bytes than requested\n");
40@@ -2417,20 +2417,27 @@
41 uint32_t length= strlen(script);
42 uint32_t count= 0; /* We know that there is always one */
43
44- for (tmp= *sptr= (statement *)malloc(sizeof(statement));
45+ for (tmp= *sptr= (statement *)calloc(1, sizeof(statement));
46 (retstr= strchr(ptr, delm));
47- tmp->next= (statement *)malloc(sizeof(statement)),
48+ tmp->next= (statement *)calloc(1, sizeof(statement)),
49 tmp= tmp->next)
50 {
51- memset(tmp, 0, sizeof(statement));
52+ if (tmp == NULL)
53+ {
54+ fprintf(stderr,"Error allocating memory while parsing delimiter\n");
55+ exit(1);
56+ }
57+
58 count++;
59 tmp->length= (size_t)(retstr - ptr);
60 tmp->string= (char *)malloc(tmp->length + 1);
61+
62 if (tmp->string == NULL)
63 {
64 fprintf(stderr,"Error allocating memory while parsing delimiter\n");
65 exit(1);
66 }
67+
68 memcpy(tmp->string, ptr, tmp->length);
69 tmp->string[tmp->length]= 0;
70 ptr+= retstr - ptr + 1;