Skip to content
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

lib: Add a lighter weight internal checksum wrapper #1256

Conversation

cgwalters
Copy link
Member

The faster (OpenSSL/GnuTLS) code lived in a GInputStream wrapper, and that
adds a lot of weight (GObject + vtable calls). Move it into a simple
autoptr-struct wrapper, and use it in the metadata path, so we're
now using the faster checksums there too.

This also drops a malloc there as the new API does hexdigest in place to a
buffer.

Prep for more work in the commit path to avoid GInputStream for local file
commits, and "adopting" files.

The faster (OpenSSL/GnuTLS) code lived in a `GInputStream` wrapper, and that
adds a lot of weight (GObject + vtable calls). Move it into a simple
autoptr-struct wrapper, and use it in the metadata path, so we're
now using the faster checksums there too.

This also drops a malloc there as the new API does hexdigest in place to a
buffer.

Prep for more work in the commit path to avoid `GInputStream` for local file
commits, and ["adopting" files](ostreedev#1255).
@cgwalters cgwalters force-pushed the abstract-singleton-proxy-factory-checksum-bean branch from dfc9079 to 69c898f Compare October 7, 2017 13:50
@cgwalters cgwalters mentioned this pull request Oct 9, 2017
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane overall, just some minor nits.

guint8 digest[_OSTREE_SHA256_DIGEST_LEN];
ot_checksum_get_digest (&checksum, digest, sizeof(digest));
g_autofree guchar *ret_csum = g_malloc (sizeof (digest));
memcpy (ret_csum, digest, sizeof (digest));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: we're not very consistent with space between the sizeof operator and the opening parenthesis.

};
typedef struct OtChecksum OtChecksum;

/* Same as OSTREE_SHA256_DIGEST_LEN, but this header can't depend on that */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I missed the part about why we can't do that. Can you expand on this comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but it feels like a layering violation since conceptually src/libostree depends on src/libotutil (which depends on libglnx/), etc.

Copy link
Member Author

@cgwalters cgwalters Oct 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came up with something that'll at least error out if we somehow make them different ⬇️

@jlebon
Copy link
Member

jlebon commented Oct 10, 2017

Time for a ⚡️! @rh-atomic-bot r+ 36c8169

@rh-atomic-bot
Copy link

⚡ Test exempted: merge already tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants