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

udev/net_id: avoid slot based names only for single function devices #22844

Merged
merged 1 commit into from Apr 6, 2022

Conversation

msekletar
Copy link
Contributor

If we have two or more devices that share the same slot but they are
also multifunction then it is OK to use the slot information even if it
is the same for all of them. Name conflict will be avoided because we
will append function number and form names like, ens1f1, ens1f2...

@yuwata
Copy link
Member

yuwata commented Mar 24, 2022

I think it is better to add a new NAMING_ flag for this change.

@msekletar
Copy link
Contributor Author

I think it is better to add a new NAMING_ flag for this change.

Ok, I will add that.

@poettering
Copy link
Member

patch looks ok, but please add a new naming scheme flag for that. Also please update the log message a bit according to the new conditionalization.

@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Mar 24, 2022
@msekletar msekletar force-pushed the net-id-pci-multifunction-fix branch from 184ff44 to 3ea076d Compare March 24, 2022 18:10
@msekletar msekletar removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Mar 24, 2022
@msekletar
Copy link
Contributor Author

Updated. @yuwata @poettering PTAL.

@poettering poettering added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Mar 24, 2022
src/shared/netif-naming-scheme.h Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
@keszybz keszybz added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Mar 25, 2022
@keszybz
Copy link
Member

keszybz commented Mar 25, 2022

I think it'll be easier to grok if the name is reversed. But if you disagree, ignore my comment.

@yuwata yuwata added this to the v251 milestone Mar 25, 2022
@msekletar
Copy link
Contributor Author

@keszybz, OK, so let's rename the option. But I am adding dont-merge label because I got some reports from Red Hat QE folks about issues with this change while they were doing testing internally.

@msekletar
Copy link
Contributor Author

This is now updated and should be ready to merge but please wait until we figure out issues regarding testing of this change. I will drop "dont-merge" label once this is fully ready.

@msekletar msekletar force-pushed the net-id-pci-multifunction-fix branch from 4fbc670 to 01737dc Compare March 30, 2022 08:56
@msekletar
Copy link
Contributor Author

I've force pushed my branch with small update to manual page (removed erroneous dot at the end of added paragraph which was causing man rendering problem).

If we have two or more devices that share the same slot but they are
also multifunction then it is OK to use the slot information even if it
is the same for all of them. Name conflict will be avoided because we
will append function number and form names like, ens1f1, ens1f2...
@msekletar msekletar force-pushed the net-id-pci-multifunction-fix branch from 01737dc to 0aea77e Compare April 1, 2022 10:57
@msekletar
Copy link
Contributor Author

@poettering I've fixed the typo and marked the comment from @keszybz as addressed.

@yuwata
Copy link
Member

yuwata commented Apr 1, 2022

@msekletar Is the tag 'dont-merge' still needed?

@msekletar
Copy link
Contributor Author

RH networking QE folks got back to me and are reporting that change is fine.

@keszybz keszybz merged commit 66425da into systemd:main Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants