Implement gio::File::set_attribute #1713
Conversation
gio/src/file.rs
Outdated
| &self, | ||
| attribute: &str, | ||
| type_: FileAttributeType, | ||
| value: FileAttributeTypeValue<T>, |
There was a problem hiding this comment.
There should be type-system matching from the file attribute to the value type probably. If you e.g. pass a u32 where a u64 was expected this will read random stack memory.
There was a problem hiding this comment.
ok I changed the implementation: an enum ensures value is coherent with type
gio/src/file.rs
Outdated
|
|
||
| impl From<i32> for FileAttributeTypeValue<i32> { | ||
| fn from(value: i32) -> Self { | ||
| Self(&value as *const _ as _, value) |
There was a problem hiding this comment.
This and the others are unsound. You store a pointer to some random, temporary stack frame of this function in the FileAttributeValue.
There was a problem hiding this comment.
hope the new version is ok
fbrouille
force-pushed
the
g_file_set_attribute
branch
3 times, most recently
from
91b8c29 to
2deef12
Compare
gio/src/file.rs
Outdated
| .1, | ||
| ), | ||
| }; | ||
| Stash(s.as_ptr(), s) |
There was a problem hiding this comment.
Here you get the pointer into s (stored on this functions stack), then return (i.e. move) s and afterwards the pointer points to this stack frame still while s is somewhere else
There was a problem hiding this comment.
Fixed: return FileAttributeTypeValueStorage instead of Stash and then caller calls as_ptr() on the object stored on stack (no move)
| #[derive(Debug)] | ||
| // checker-ignore-item | ||
| pub enum FileAttributeTypeValue<'a> { | ||
| Pointer(FileAttributeType, glib::ffi::gpointer), |
There was a problem hiding this comment.
This allows creating values of arbitrary pointers from safe code, which is probably not a good idea
There was a problem hiding this comment.
Fixed: FileAttributeTypeValue is now a public structure with a private field of type FileAttributeTypeValueInner which is crate private. FileAttributeTypeValue provides associated methods to create instances for different types. All are public except new_pointer() which is only accessible by the crate.
gio/src/file.rs
Outdated
| Self::ByteString(s) => s.as_ptr() as _, | ||
| Self::Boolean(s) => s as *const _ as _, | ||
| Self::Uint32(s) => *s as *const _ as _, | ||
| Self::Int32(s) => *s as *const _ as _, |
There was a problem hiding this comment.
Some more types instead of _ would make it clearer whether this is correct or not
| Self::Int32(s) => *s as *const _ as _, | |
| Self::Int32(s) => *s as *const i32 as _, |
if that doesn't compile then the code is wrong
gio/src/file.rs
Outdated
|
|
||
| #[derive(Debug)] | ||
| // checker-ignore-item | ||
| pub enum FileAttributeTypeValue<'a> { |
There was a problem hiding this comment.
I think I would gone with a trait but this also works. Should probably have From<T> impls to easily convert from plain types to this
There was a problem hiding this comment.
Could then also use impl Into<FileAttributeTypeValue> in the function.
Also I wonder if the same type could be used for the other file attribute API
There was a problem hiding this comment.
I'm not sure: From<T> would imply guessing the attribute type from T. If a developer pass a value of wrong type, code will compile but inferring a wrong attribute type.
I think that developers should explicitly specify the attribute type: e.g. file.set_attribute("name", FileAttributeTypeValue::new_uint64(1), ...) which is less error prone.
And I think also that this type is useless for the other file attribute API because their names are very explicit (type is clearly indicated) and their signatures already constrain the value.
There was a problem hiding this comment.
Not very different from object.set_property() and other such API though
There was a problem hiding this comment.
ok, new implementation like in object.set_property()
|
i guess you could also easily add https://docs.gtk.org/gio/method.FileInfo.set_attribute.html ? |
fbrouille
force-pushed
the
g_file_set_attribute
branch
from
2deef12 to
ef65bb7
Compare
|
sure, when we'll be aligned |
gio/src/file.rs
Outdated
| } | ||
|
|
||
| impl<O: IsA<File>> FileExtManual for O {} | ||
|
|
||
| #[derive(Debug)] | ||
| // checker-ignore-item |
gio/src/file.rs
Outdated
| // new_pointer is private to gio crate to not allow creating values of arbitrary pointers from safe code | ||
| #[allow(dead_code)] // TODO remove this allow attribute when new_pointer will be used by this crate | ||
| pub(crate) fn new_pointer(type_: FileAttributeType, ptr: glib::ffi::gpointer) -> Self { | ||
| Self(FileAttributeTypeValueInner::Pointer(type_, ptr)) | ||
| } |
There was a problem hiding this comment.
i would remove this and move the dead_code to the enum if clippy complains, can be added later if needed
fbrouille
force-pushed
the
g_file_set_attribute
branch
from
ef65bb7 to
e53d619
Compare
gio/src/file.rs
Outdated
| } | ||
|
|
||
| impl<O: IsA<File>> FileExtManual for O {} | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct FileAttributeTypeValue<'a>(FileAttributeTypeValueInner<'a>); |
There was a problem hiding this comment.
| pub struct FileAttributeTypeValue<'a>(FileAttributeTypeValueInner<'a>); | |
| pub struct FileAttributeValue<'a>(FileAttributeValueInner<'a>); |
the type seems a bit redundant to me, but no strong opinion
gio/src/file.rs
Outdated
| Self(FileAttributeTypeValueInner::Object(value.as_ref())) | ||
| } | ||
|
|
||
| pub fn new_stringv(value: &'a Vec<&'a str>) -> Self { |
There was a problem hiding this comment.
There's also glib::collections::StrV or &[&str] or so that would work here
There was a problem hiding this comment.
Changed it to &[&str] (cannot use &StrV because From<&StrV> conflicts with From<&Object>
gio/src/file.rs
Outdated
| Stringv(&'a Vec<&'a str>), | ||
| } | ||
|
|
||
| impl<'a> FileAttributeTypeValueInner<'a> { |
There was a problem hiding this comment.
You could merge the Inner and Storage, and on construction of the Inner just create whatever you need for passing it to C directly
Signed-off-by: fbrouille <[email protected]>
fbrouille
force-pushed
the
g_file_set_attribute
branch
from
e53d619 to
7520fd3
Compare
No description provided.