New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

F/csvparser refinements #741

Merged

lbudai merged 10 commits into master from f/csvparser-refinements

Oct 21, 2015

Collaborator

bazsi commented Oct 19, 2015

This is the first chunk of my work to clean up the code for csv-parser. I only wanted to add a single and simple option, but the code doesn't really help, rather it became rather convoluted.

So instead of adding the new option, I started refactoring what we have there.

There is at least one important bugfix here, one that fixes the clone() method. That should probably be backported to earlier versions as well.

I also have yet another batch of patches, but that still needs some cleanup, so I submit this, hopefully reviewing this in smaller steps is easier.

Cheers,
Bazsi

bazsi added the in progress label

bazsi added 10 commits

October 20, 2015 09:28


          lib: add test coverage for string_list related functions

7090b85

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>


          csvparser: remove log prefix from all identifiers

02705df

It really doesn't have an added value and makes identifiers overly long.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>


          csvparser: merge LogColumnParser back into CSVParser

a795a64

This is the only derived class that uses it, originally I wanted
to use LogColumnParser to be used for stuff like a fixed-position based
parser, but

1) that didn't happen in the past,
2) I can't see that happening in the foreseeable future
3) LogColumnParser will probably not be applicable anyway

And removing this makes the code much easier to follow.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>


          csvparser: move csv_parser_get_flags() to be close to set_flags()

46dc258

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>


          csvparser: extract clone_string_list() function instead of open codin…

b248cfd

…g it

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>


          csvparser: use string_list_free() to free string_delimiters in clone()

dbe1b61

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>


          csvparser: properly clone string_delimiters

d24fb82

Previously only the pointer was borrowed, which is of course not correct
and can cause use-after-free conditions if the original CSVParser is
destroyed.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>


          csvparser: add test coverage to csv_parser_clone()

2f2555a

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>


          csvparser: use string_list to parse delimiter(strings()) option

5fe3c15

Instead of the open-coded, csv-parser specific constructs, use the one
provided by the core, making csvparser simpler and easier to read.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>


          csvparser: allow chars() and strings() within delimiters in any order

4e1d0a7

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>

bazsi force-pushed the f/csvparser-refinements branch from e767128 to 4e1d0a7 Compare

October 20, 2015 15:44

Collaborator Author

bazsi commented Oct 20, 2015

I think the latest update of this branch addresses all review comments.

lbudai added a commit that referenced this pull request


          Merge pull request #741 from balabit/f/csvparser-refinements

43a689d

F/csvparser refinements

lbudai merged commit 43a689d into master

lbudai removed the in progress label

Contributor

u2yg commented Oct 21, 2015

@lbudai Did you review the new force push before merging? He has many code changes since the last reviews.

Collaborator

lbudai commented Oct 21, 2015

yep (Bazsi mentioned in his last comment that he made modifications)

( sorry for not waiting to react your own review notes but my plan was to starting releasing the 3.7.2 on today morning, and this PR fixes a possible double-free issue and I wanted to include in 3.7.2 )

Collaborator Author

bazsi commented Oct 21, 2015

Sorry for the force push. I didn't know that github removes the comments in this case (or I knew I just forgot ;).

Next time, I am going to commit followup patches until they are reviewed as well, and then merge back to the base.

I only changed the string-list related stuff for two reasons:

addressing review comments
I noticed that the string_list may contain non-pointer elements and added handling those to string_list_clone()

I also added some (but still pretty limited) coverage for these functions.

Contributor

u2yg commented Oct 22, 2015

That strategy sounds nice, thank you. Actually, I had asked Laci in person
sometimes later and he told me that he did review the complete diff. The
situation was only unclear to me from the online conversation, but all is
well now.

On Wed, Oct 21, 2015 at 5:43 PM, Balazs Scheidler notifications@github.com
wrote:

Sorry for the force push. I didn't know that github removes the comments
in this case (or I knew I just forgot ;).

Next time, I am going to commit followup patches until they are reviewed
as well, and then merge back to the base.

I only changed the string-list related stuff for two reasons:

addressing review comments

I noticed that the string_list may contain non-pointer elements and
added handling those to string_list_clone()

I also added some (but still pretty limited) coverage for these functions.

—
Reply to this email directly or view it on GitHub
#741 (comment).

bazsi deleted the f/csvparser-refinements branch

January 14, 2016 06:54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment