search

luigirizzo / netmap

Automatically exported from code.google.com/p/netmap
BSD 2-Clause "Simplified" License
1.85k stars 534 forks source link

libnetmap: remove interface name validation #940

Closed fichtner closed 10 months ago

fichtner commented 10 months ago

When trying to use a VLAN device (e.g. "em0.123") with a dot the library fails to parse the interface correctly. The former pattern is much too restrictive given that almost all characters can be coerced into a device name via ifconfig.

Remove the particularly restrictive validation. Some characters still cannot be used as an interface name as they are used as delimiters in the syntax, but this allows to be able to use most of them without an issue.

Note that the dotted VLAN naming is the default in FreeBSD now and a patch was proposed here https://reviews.freebsd.org/D42485 as well.

fichtner commented 10 months ago

Any thoughts on this? For default VLANs this is basically broken in FreeBSD 13.2 and 14.0 at the moment.

giuseppelettieri commented 10 months ago

Hi @fichtner, sorry but I have not had time to look at this until now. nm_is_identifier() is used in three places for three different purposes: 1) check the part after a vale prefix; 2) check the interface name; 3) check the pipe name after a curly bracket. I don't think we really need to change 1 and 3, so we should either create another nm_is_valid_interface() just for 2, or maybe just remove the check in 2. I think the latter is preferable, if there is really nothing to check in interface names. What's your opinion?

fichtner commented 10 months ago

@giuseppelettieri thanks for responding. You mean this one?

https://github.com/luigirizzo/netmap/blob/6055e3b0e20b7d93d92cc983fd08155d4078f3ac/libnetmap/nmreq.c#L159-L163

I haven't tested but it looks reasonable to remove it in order to fix this. The special character situation still applies, but I reckon it will complain later anyway when the "port" is not found because it had a special character like "}". But I also think that in that case people should reevaluate the device name policy. :)

giuseppelettieri commented 10 months ago

yes, that one. If you want to create a new pull request I will merge it.

fichtner commented 10 months ago

Ok, let me test this today and update the PR then.

fichtner commented 10 months ago

@giuseppelettieri already confirmed... updated the PR accordingly