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

daemon/syscore: push livefs introspection to Rust #2292

Merged
merged 1 commit into from
Oct 29, 2020
Merged

daemon/syscore: push livefs introspection to Rust #2292

merged 1 commit into from
Oct 29, 2020

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Oct 28, 2020

This starts bridging parts of the daemon syscore logic to Rust
plumbing, moving the livefs detection logic over there as a first
consumer. That was the simplest logic available for wiring, and
mostly meant as a sanity check.

@cgwalters
Copy link
Member

Nice! I had a side project in this area, just pushed up #2293 to claim the mutex around some of this a bit.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Definitely overall approve the idea.

pub enum RpmOstreeOrigin {}

extern "C" {
fn rpmostree_origin_get_live_state(
Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with this but we need to be careful because this would mean changing that API C side in the future would result in crashes/corruption on this side.

Let's at least add a comment to both sides warning to change the other; something like this in C:
// WARNING: This prototype is also redefined in Rust, if changing this please also update the Rust side
And here:
// NOTICE: The C header definition is canonical, please update that first then synchronize this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.
There is another one already bridged (rpmostree_get_repodata_chksum_repr), do you prefer to have all of them in a single place/module in a Rust and a single top-notice?

&mut out_livereplaced,
)
};
let in_progress = ffi_new_nullable_string(out_inprogress);
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 this might exist today via glib::translate; try something like this:
let in_progress: Option<String> = from_glib_full(out_inprogress);?

See http://gtk-rs.org/docs/src/glib/translate.rs.html#1347-1356

Copy link
Member

Choose a reason for hiding this comment

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

(And I suspect some of the other ffi_view_ things should just be from_glib_none but at the time I didn't know about everything in GLib Rust bindings)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lovely, if that works it will also spare the g_free below. I guess I can also drop the custom ffi_ helper I've introduced.

@jlebon
Copy link
Member

jlebon commented Oct 28, 2020

/approve

This starts bridging parts of the daemon syscore logic to Rust
plumbing, moving the livefs detection logic over there as a first
consumer. That was the simplest logic available for wiring, and
mostly meant as a sanity check.
@lucab
Copy link
Contributor Author

lucab commented Oct 29, 2020

I've switched to the glib-rs methods and re-grouped the C prototypes into its own includes.rs module, let me know if it looks better compared to the initial approach.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Nice!
/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon, lucab

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 87775cb into coreos:master Oct 29, 2020
@lucab lucab deleted the ups/rs-daemon-live branch October 29, 2020 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants