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

compat-suse: fix infiniband type detection from ifname #1015

Conversation

cfconrad
Copy link
Collaborator

@cfconrad cfconrad commented May 22, 2024

  • Restrict Infiniband configuration detection by ifcfg filename to /^ib[0-9]+(\.[a-fA-F0-9]+)?$/
  • Decouple the ifcfg name from the Infiniband parent interface name and allow to set parent device via IPOIB_DEVICE and the pkey via IPOIB_PKEY separately. These variables take precedence over the pkey and device in the ifcfg filename.

@cfconrad cfconrad marked this pull request as draft May 22, 2024 12:57
Copy link
Member

@mtomaschewski mtomaschewski left a comment

Choose a reason for hiding this comment

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

I'm not sure if the code makes the variable vs. from ifname parsing the right way,
when IPOIB variables are set or not... e.g. for foo.bar with IPOIB=yes only...

As we're cleaning up here, I'd also cover the pkey range (excl. vs incl.) in validate.

client/suse/compat-suse.c Outdated Show resolved Hide resolved
@cfconrad cfconrad force-pushed the fix_infiniband_name_based_type_detection branch from 99413df to e533cdc Compare May 24, 2024 10:07
client/suse/compat-suse.c Outdated Show resolved Hide resolved
Decouple the ifcfg name from the infiniband phys parent interface
name and allow to set parent device via `IPOIB_DEVICE` and
pkey via `IPOIB_PKEY` separately. These variables take precedence
over the pkey and device, which was taken from the ifcfg filename.
@mtomaschewski
Copy link
Member

mtomaschewski commented May 24, 2024

I'd change the "foo or bar, I don't know" + checking pkey & device twice and use instead (pasted diff):

-	if ((pkey == NULL) != (device == NULL)) {

-		ni_error("ifcfg-%s: Missing one of IPOIB_DEVICE or IPOIB_PKEY variable",

-				dev->name);

-		ni_string_free(&device);

-		return -1;

-	}

-

-	if (pkey) {

+	if (pkey || device) {

 		unsigned long tmp = ~0UL;

 

 		if (ni_parse_ulong(pkey, &tmp, 16) < 0 || tmp > 0xffff) {

-			ni_error("ifcfg-%s: Cannot parse infiniband child key number",

-				dev->name);

+			ni_error("ifcfg-%s: %s %s partition key (IPOIB_PKEY)", dev->name,

+					pkey ? "Cannot parse" : "Missing",

+					ni_linktype_type_to_name(NI_IFTYPE_INFINIBAND_CHILD));

 			ni_string_free(&device);

 			return -1;

 		}

 

 		if (!ni_netdev_name_is_valid(device)) {

-			ni_error("ifcfg-%s: %s parent device name is not valid",

-					dev->name, ni_linktype_type_to_name(NI_IFTYPE_INFINIBAND_CHILD));

+			ni_error("ifcfg-%s: %s %s parent device name (IPOIB_DEVICE)", dev->name,

+					device ? "Invalid" : "Missing",

+					ni_linktype_type_to_name(NI_IFTYPE_INFINIBAND_CHILD));

 			ni_string_free(&device);

 			return -1;

 		}

@cfconrad cfconrad force-pushed the fix_infiniband_name_based_type_detection branch from e533cdc to b9a3780 Compare May 24, 2024 11:09
@cfconrad cfconrad marked this pull request as ready for review May 24, 2024 11:11
Copy link
Member

@mtomaschewski mtomaschewski left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mtomaschewski mtomaschewski merged commit de65ba0 into openSUSE:master May 24, 2024
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.

2 participants