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

Add the json-sysctl demo from the LSSNA talk #122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

geofft
Copy link
Collaborator

@geofft geofft commented Aug 18, 2019

Depends on a byteorder crate locally patched to make std not a default
feature, see #121 for why.

@geofft geofft marked this pull request as ready for review August 18, 2019 02:21
src/chrdev.rs Outdated Show resolved Hide resolved
@geofft
Copy link
Collaborator Author

geofft commented Aug 19, 2019

Another possible idea: write a custom derive https://serde.rs/deserialize-map.html to create sysctls based on a JSON map that's passed in. Probably won't get to implementing that.

tests/json-sysctl/src/lib.rs Outdated Show resolved Hide resolved
@geofft geofft changed the base branch from fadvise-4.19 to master August 19, 2019 17:09
@geofft
Copy link
Collaborator Author

geofft commented Aug 21, 2019

@alex please see the most recent commit, which should actually fix small reads. I'll rebase later today

geofft added a commit that referenced this pull request Aug 25, 2019
Motivation is json-sysctl (#122) where we need this to return things
like ENOMEM.
@geofft geofft mentioned this pull request Aug 25, 2019
@geofft geofft changed the title WIP: json-sysctl Add the json-sysctl demo from the LSSNA talk Aug 25, 2019
@geofft geofft requested a review from alex August 25, 2019 21:01

[dependencies]
linux-kernel-module = { path = "../.." }
serde = { version = "*", default-features = false , features = ["derive"] }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
serde = { version = "*", default-features = false , features = ["derive"] }
serde = { version = "1.0", default-features = false , features = ["derive"] }

linux-kernel-module = { path = "../.." }
serde = { version = "*", default-features = false , features = ["derive"] }
serde-json-core = { git = "https://github.com/japaric/serde-json-core" }
typenum = "*"
Copy link
Member

Choose a reason for hiding this comment

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

Put a real version here please

# all features, so even though our runtime dependency serde-json-core
# indirectly depends on byteorder without default features, Cargo
# confuses the two. See https://github.com/rust-lang/cargo/issues/5730
#byteorder = { path = "/root/byteorder" }
Copy link
Member

Choose a reason for hiding this comment

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

Ooof, this is painful, does this even build in CI as a result?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not, but it's skipped in CI since no tests directory exists.

I considered adding something to .travis.yml to make it build, happy to do that and make an empty test directory (or even write some tests) if you'd like.


linux_kernel_module::kernel_module!(
JsonSysctlModule,
author: "Alex Gaynor and Geoffrey Thomas",
Copy link
Member

Choose a reason for hiding this comment

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

FiaB auhors

@geofft geofft force-pushed the json-sysctl branch 3 times, most recently from 3406d91 to 9dd5bfb Compare August 19, 2020 06:24
@geofft
Copy link
Collaborator Author

geofft commented Aug 19, 2020

So, the cargo bug no longer seems to affect us, tests pass without a locally-hacked byteorder crate.

There's some future work, including

  • using real serde_json's new no_std support
  • using serde in the tests (probably related to that cargo bug?)

but I'd like to do that in a separate commit.

@stappersg
Copy link

I wonder what should be done to get this json systemctl demo merged.

@geofft
Copy link
Collaborator Author

geofft commented Oct 25, 2020

@stappersg I need to have a little bit of time to polish off the last few things. Maybe I will this week.

@stappersg
Copy link

The good thing of merging demos is that it attracts more demos.

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