Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

Bind API to convert from/to bytes to/from Variant #588

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

zeenix
Copy link
Contributor

@zeenix zeenix commented Feb 11, 2020

Bind Variant::new_from_bytes & Variant::get_data_as_bytes().

Related: #113.

src/variant.rs Outdated Show resolved Hide resolved
@sdroege
Copy link
Member

sdroege commented Feb 11, 2020

Maybe also add the to_bytes() binding while you're at it? Then we keep it symmetric :)

@zeenix
Copy link
Contributor Author

zeenix commented Feb 11, 2020

Maybe also add the to_bytes() binding while you're at it? Then we keep it symmetric :)

Yeah, i intend to. I just wanted to get early feedback for learning purposes. :)

@zeenix zeenix changed the title Bind Variant::new_from_bytes Bind API to convert from/to bytes to/from Variant Feb 12, 2020
@zeenix
Copy link
Contributor Author

zeenix commented Feb 12, 2020

Maybe also add the to_bytes() binding while you're at it? Then we keep it symmetric :)

Done. I've not tested that one yet. I'll report it here once done.

@sdroege
Copy link
Member

sdroege commented Feb 12, 2020

Done. I've not tested that one yet. I'll report it here once done.

Looks good to me as long as that can never return NULL :)

@zeenix
Copy link
Contributor Author

zeenix commented Feb 12, 2020

Looks good to me as long as that can never return NULL :)

Actually it can. :(

If value is a fixed-sized value that was deserialised from a corrupted serialised container then NULL may be returned. In this case, the proper thing to do is typically to use the appropriate number of nul bytes in place of value . If value is not fixed-sized then NULL is never returned.

This is from docs of g_variant_get_data but g_variant_get_data_as_bytes docs say the semantics are identical.

@sdroege
Copy link
Member

sdroege commented Feb 12, 2020

Actually it can. :(

Option it is then :)

@zeenix
Copy link
Contributor Author

zeenix commented Feb 12, 2020

All done.

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

👍

@EPashkin
Copy link
Member

From https:/GNOME/glib/blob/mainline/glib/gvariant-core.c#L938 g_variant_get_data_as_bytes always return Bytes,
even in case NULL from g_variant_get_data
it doing something strange, but returns Bytes anyway.

@EPashkin
Copy link
Member

@zeenix Sorry, I was wrong, g_bytes_new_from_bytes return NULL if it get wrong parameters, so Option is needed.
Thanks, 👍

@sdroege
Copy link
Member

sdroege commented Feb 12, 2020

@zeenix Sorry, I was wrong, g_bytes_new_from_bytes return NULL if it get wrong parameters, so Option is needed.

When?

@EPashkin
Copy link
Member

If bytes_data==NULL, then data - bytes_data=>data so g_bytes_new_from_bytes get pointer as offset parameter and it not pass check in https:/GNOME/glib/blob/mainline/glib/gbytes.c#L227

@sdroege
Copy link
Member

sdroege commented Feb 12, 2020

That's a g_return_val_if_fail(), those are basically like panics. Nothing we need to take into account (other than trying to not make it possible to call C API in ways that end up in there).

Bind Variant::new_from_bytes & Variant::get_data_as_bytes().
@zeenix
Copy link
Contributor Author

zeenix commented Feb 12, 2020

I removed the Option.

@sdroege
Copy link
Member

sdroege commented Feb 12, 2020

Nice

process didn't exit successfully: /home/travis/build/gtk-rs/glib/target/debug/deps/glib-b0a38ca36f7dfe57 (signal: 11, SIGSEGV: invalid memory reference)

On all nightly builds 🙈

@sdroege
Copy link
Member

sdroege commented Feb 12, 2020

I can confirm, it's the subclass::object::test::test_create test that explodes. Unrelated to this PR though.

@sdroege sdroege merged commit 67caba9 into gtk-rs:master Feb 12, 2020
@sdroege
Copy link
Member

sdroege commented Feb 12, 2020

Regression caused by rust-lang/rust@fc23a81, hm!

@sdroege
Copy link
Member

sdroege commented Feb 12, 2020

rust-lang/rust#69102

@GuillaumeGomez
Copy link
Member

Daily breaking changes time is back!

@sdroege
Copy link
Member

sdroege commented Feb 12, 2020

Don't be so dramatic :) It's just a bug, and might even be one on our side that just happened to not trigger for a long long time.

@GuillaumeGomez
Copy link
Member

My bad... I have memories from the past where every day was a whole new language haha.

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

Successfully merging this pull request may close these issues.

4 participants