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.

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

journal: fix error handling when compressing journal objects #1663

Merged
merged 2 commits into from Oct 25, 2015

Conversation

3 participants
@poettering
Member

poettering commented Oct 24, 2015

Let's make sure we handle compression errors properly, and don't
misunderstand an error for success.

Also, let's actually compress things if lz4 is enabled.

Fixes #1662.

journal: fix error handling when compressing journal objects
Let's make sure we handle compression errors properly, and don't
misunderstand an error for success.

Also, let's actually compress things if lz4 is enabled.

Fixes #1662.

@poettering poettering added the journal label Oct 24, 2015

@poettering

This comment has been minimized.

Show comment
Hide comment
@poettering

poettering Oct 24, 2015

Member

@vcaputo, @keszybz this is a reworked fix for #1662. Please have a look.

Member

poettering commented Oct 24, 2015

@vcaputo, @keszybz this is a reworked fix for #1662. Please have a look.

size_t rsize = 0;
compression = compress_blob(data, size, o->data.payload, &rsize);
if (compression) {
if (compression >= 0) {

This comment has been minimized.

@crawford

crawford Oct 24, 2015

Contributor

Why not strictly greater than? This is checking against the bitfield containing OBJECT_COMPRESSED_LZ4 and OBJECT_COMPRESSED_XZ, right?

@crawford

crawford Oct 24, 2015

Contributor

Why not strictly greater than? This is checking against the bitfield containing OBJECT_COMPRESSED_LZ4 and OBJECT_COMPRESSED_XZ, right?

This comment has been minimized.

@poettering

poettering Oct 24, 2015

Member

Well, it's customary in systemd to indicate success with return values >= 0, and errors with return values < 0... Now, it's true that the two OBJECT_COMPRESSED_XYZ values are actually != 0, so we could write > 0, but I mean they actually aren't 3 or 4 either, so I'd rather not get into comparing with too specific values, but stick to the simple decision between "is this an error" or "is this not an error"... Hope that makes some sense...

@poettering

poettering Oct 24, 2015

Member

Well, it's customary in systemd to indicate success with return values >= 0, and errors with return values < 0... Now, it's true that the two OBJECT_COMPRESSED_XYZ values are actually != 0, so we could write > 0, but I mean they actually aren't 3 or 4 either, so I'd rather not get into comparing with too specific values, but stick to the simple decision between "is this an error" or "is this not an error"... Hope that makes some sense...

This comment has been minimized.

@crawford

crawford Oct 24, 2015

Contributor

Yes, I actually misread o->object.flags |= compression; and thought that it was always setting that to be non-zero (which wouldn't make sense of compress_blob() returned 0). This makes sense.

@crawford

crawford Oct 24, 2015

Contributor

Yes, I actually misread o->object.flags |= compression; and thought that it was always setting that to be non-zero (which wouldn't make sense of compress_blob() returned 0). This makes sense.

@keszybz

This comment has been minimized.

Show comment
Hide comment
@keszybz

keszybz Oct 25, 2015

Member

Tested with:

src/journal-remote/log-generator.py --data-size=200000 --data-type=simple 1000 > input
./systemd-journal-remote -o output.journal --split-mode=none input
Member

keszybz commented Oct 25, 2015

Tested with:

src/journal-remote/log-generator.py --data-size=200000 --data-type=simple 1000 > input
./systemd-journal-remote -o output.journal --split-mode=none input

keszybz added a commit that referenced this pull request Oct 25, 2015

Merge pull request #1663 from poettering/journal-compress-fix
journal: fix error handling when compressing journal objects

@keszybz keszybz merged commit 8e0dfb6 into systemd:master Oct 25, 2015

1 check passed

semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment