Better way for parsing trait-marked constants in struct fields

[jbaublitz]

The current way I get around the many constants that can be passed into certain fields like Nlmsghdr.nl_type is separating each set of constants into its own finite enum and marking the enum as compatible with this field by implementing the NlType trait for that enum. This is a good first step but a use case that came up in this pull request made it clear that currently the best way to differentiate between different enums when parsing a stream where for example nl_type could be one of multiple enums is to use a u16 as the type there and then manually try parsing as both enum types. This is not an ideal way to do this and I'm interested in figuring out a better way to do this.

[jbaublitz]

I've been considering this and it may make the most sense to implement methods in the marker trait. Do you have any thoughts on this @gluxon?

[gluxon]

I've been considering this and it may make the most sense to implement methods in the marker trait.

Were you thinking of adding a deserialization method, or something else?

[jbaublitz]

What I'm considering is some sort of approach that takes advantage of the fact that every enum that implements a given marker trait will have variants such that when each variant is converted to an integer, it will have a unique value across all variants in the set of variants for enums implementing the marker trait. This is guaranteed because each integer value must be unique in fields that are not bitwise flags which will never have marker traits. How to implement this is a bit trickier as doing it automatically sounds a bit more complicated but it may be possible to do manually with some TryInto-type code implemented for each marker trait. Does that sound reasonable?

[gluxon]

So I was thinking of something like this, but it doesn't work since type ids are defined at runtime.

#[derive(neli)]
enum GenlResponse {
  #[type(3,4)] Nlmsg(Nlmsg),
  #[type(16)] CtrlCmd(CtrlCmd),
}

How to implement this is a bit trickier as doing it automatically sounds a bit more complicated but it may be possible to do manually with some TryInto-type code implemented for each marker trait.

Yeah, I'm leaning towards this as well. Maybe a generic NlPayload type that has a vector of attributes. Other payloads then implement a TryFrom for this type.

[jbaublitz]

Hi Brandon, I've been thinking on this for a bit and I'm pretty sure I have a solution. It's going to take some reworking of the macros but I'd like to do this now rather than down the road.

Proposed solution

I'd like to remove the macro impl_var_trait and rework impl_var and impl_trait as follows.

impl_var would accept the same parameters but also implement TryFrom and TryInto as well as From and Into.

impl_trait would now accept the names of all of the traits that the marker trait is implemented for and the name of the wrapper enum. This would allow me to generate two additional pieces of information in the macro. One additional data structure to generate would be an enum that has a variant wrapping every type that implements the trait. Additionally, I could add an implementation of the marker trait for the enum wrapping all of the other enums that implement the marker trait. For example, if Nla and AnotherConst both implemented MarkerTrait, the invocation of impl_trait would be as follows:

impl_trait!(MarkerTrait, WrapperName, u16, Nla, AnotherConst);

This would generate MarkerTrait implementations for Nla, AnotherConst, and WrapperName which would be defined as follows:

pub enum WrapperName {
    Nla(Nla),
    AnotherConst(AnotherConst),
    UnrecognizedVariant(u16),
}

In addition, Nl would be implemented for WrapperName which would return the number as a single datatype but would allow differentiation between all of the subtypes being wrapped.

Benefits

Dealing with returning one of a few types in neli is impossible it seems even with trait objects because of how the Nl trait is constrained. This leaves us with the option of returning only one type from dserialize() which seems to be best addressed using enums with new type variants. In addition, this would allow WrapperName to be used as a type parameter to deserialize any data type recognized by neli for a given marker trait. This should ease some of the type parameter problems you addressed earlier when we were discussing the example you contributed.

How does this sound?

[gluxon]

How does this sound?

Pretty good so far.

From my understanding, the proposed reworkings of impl_var and impl_trait address the type aspect of a Netlink message. Does it extend somehow to the payload? How would I tell neli to parse a response as a Nlmsg payload versus a Genlmsg payload?

[jbaublitz]

Hi Brandon, I'm not sure I understand. Can you clarify a little bit? Nlmsghdr already does take a type parameter to specify the type of payloads. Nlmsghdr also takes a type parameter that you could provide to parse Nlmsghdr.nl_type as an Nlmsg, a GenlId, or even with this feature, you could parse as any type that is marked as NlType. Does that clear things up?

[gluxon]

Hi Brandon, I'm not sure I understand. Can you clarify a little bit?

Sure. From my perspective, there's 2 (heavily related) improvements that the ctrl-list.rs example would benefit from. Mainly in the following section:

neli/examples/ctrl-list.rs

Lines 32 to 42 in 899a5c5

	let mut iter = socket.iter::<Nlmsg, Genlmsghdr<CtrlCmd, CtrlAttr>>();
	while let Some(Ok(response)) = iter.next() {
	match response.nl_type {
	// This example could be improved by reinterpreting the payload as an Nlmsgerr struct
	// and printing the specific error encountered.
	Nlmsg::Error => return Err(
	NlError::new("An error occurred while retrieving available families"
	)),
	Nlmsg::Done => break,
	_ => (),
	};

1. The first (which I think this issue is more heavily focused on) is that the first type argument to socket.iter is currently Nlmsg. Ideally it should be some combination of Nlmsg and GenlId since both message types can be returned from the kernel.

2. The second is that it doesn't properly print the error that the kernel returned. The example is capable of detecting an error, but doesn't know any information beyond that. This is because the 2nd argument to socket.iter is Genlmsghdr<CtrlCmd, CtrlAttr>, but sometimes the response payload can be Nlmsgerr. At the moment there's no possible (or easy way I've found) that takes the response and reinterprets it as Nlmsgerr.

I was hoping we could address the 2nd point in a more general way, but the reality is that this issue probably only comes up with error responses. Meaning we could just have the socket iterator spit out a Result<V, E> instead.

[jbaublitz]

Hi Brandon, finally getting back around to this! I'm going to try to resolve some of these older issues for the v0.5.0 release now that I have a little more time to attend to neli again.

I think you'll be very interested in #50 as this directly ties into some of what you mentioned here. Feel free to give feedback there if you have any thoughts.

I do think when parsing based on a marker trait my proposed solution makes the most sense for a limited scope item to resolve. I think #50 is a much larger project that is equally or more important but will need a different approach. This can be handled by reworking types a bit while issue 50 seems like it will require some ability to try parsing and then reparsing as something else if it fails, all supported by the ability to rewind the buffer.

How does it sound if I go ahead with the macro changes for now and leave #31 and #50 to cover your other concerns?

[jbaublitz]

Take a look at the linked commit above. I think that should resolve this.

[gluxon]

Take a look at the linked commit above. I think that should resolve this.

Hey, sorry for the late response. I won't have time for an in-depth code review but if it implements what you proposed above I'm definitely excited for it. 🙂

I think you'll be very interested in #50 as this directly ties into some of what you mentioned here. Feel free to give feedback there if you have any thoughts.

Will do. The poster there is asking the question I was.

[jbaublitz]

This should be fixed via e868974. Closing.