Skip to content

serde serializer + deserializer for NvList#75

Draft
ahrens wants to merge 1 commit into
codyps:masterfrom
ahrens:serde-upstream
Draft

serde serializer + deserializer for NvList#75
ahrens wants to merge 1 commit into
codyps:masterfrom
ahrens:serde-upstream

Conversation

@ahrens

@ahrens ahrens commented Sep 21, 2021

Copy link
Copy Markdown
Contributor

Add a serde serializer that can serialize structs to NvList's.

Add a serde deserializer that can deserialize NvList's into structs.

I'd consider this prototype-quality. I'm looking for feedback on if this would be useful, and how much additional functionality would be needed to get it integrated (or make it useful for folks). Things that are known to be missing:

  • error handling (return Errors rather than todo!() on unimplemented functionality)
  • finer-grained Error variants (not always falling back on Error::Message)
  • deserializer: option to fail on unconsumed pairs?
  • deserializer: handle nvlist_array-type pairs
  • serializer: handle sequences (arrays, vec's, etc) of types other than Byte and NvList
  • serializer: handle tuples
  • serializer: handle enums (variants)
  • automated testing

@ahrens
ahrens marked this pull request as draft September 21, 2021 21:28
Add a serde serializer that can serialize structs to NvList's.

Add a serde deserializer that can deserialize NvList's into structs.
@ahrens

ahrens commented Oct 13, 2021

Copy link
Copy Markdown
Contributor Author

@ahl do you have any thoughts on which of the missing items should be required before integration?

@codyps

codyps commented Oct 16, 2021

Copy link
Copy Markdown
Owner

I'm generally in favor of having serde support for NvList. Would make working with some NvList content a lot quicker. Might be worth considering hiding behind a feature because serde is a bit heavy (could be default enabled), but that wouldn't block merging this.

@ahrens

ahrens commented Oct 18, 2021

Copy link
Copy Markdown
Contributor Author

Thanks! I'll look into having serde be a configurable feature, and improving the error handling at least.

@sempervictus

Copy link
Copy Markdown

@ahrens: does draft status mean this shouldn't be used because its somehow deficient/dangerous, or just that it hasn't been thoroughly used/QA'd?

@ahrens

ahrens commented Mar 16, 2023

Copy link
Copy Markdown
Contributor Author

@sempervictus We are using this in production (this version to be specific). However, there may be some corner cases where it doesn't work or has confusing results where the nvpair data model doesn't match up with serde's. I would want to document those more thoroughly, and ideally add some unit tests, before this is integrated.

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