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

[WIP] Added subclass for recognize InfiniBand cards (bsc#903190) #383

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
5 participants
@teclator
Copy link
Contributor

commented Feb 12, 2016

No description provided.

Fri Feb 12 17:31:48 UTC 2016 - knut.anderssen@suse.com

- bsc#903190
- Recognized netcards with subclass 02 as InfiniBand (ib) which

This comment has been minimized.

Copy link
@mchf

mchf Feb 12, 2016

Member
  1. AFAIK it should be 06
  2. I'd go for more "user's" friendly description here. Something like "Improved detection of InfiniBand cards".
@@ -324,7 +324,9 @@ def ControllerType(hwdevice)
return "atm"
elsif subclass_id == 4
return "isdn"
elsif subclass_id == 6

This comment has been minimized.

Copy link
@mchf

mchf Feb 12, 2016

Member

what about simplifying this terrible "elseif" beast ... I mean sometinhg like

elseif infiniband_subclass?(subclass_id)
  return "ib"

@teclator teclator changed the title Added subclass for recognize InfiniBand cards (bsc#903190) [WIP] Added subclass for recognize InfiniBand cards (bsc#903190) Feb 15, 2016

subclass_id = Ops.get_integer(hwdevice, "sub_class_id", -1)
return "modem" if hwdevice["subclass"] == "Modem" || modem_controller?(class_id)
return "isdn" if hwdevice["subclass"] == "ISDN" || isdn_controller?(class_id)
return "dsl" if hwdevice["subclass"] == "DSL" || dsl_controller?(class_id)

This comment has been minimized.

Copy link
@mchf

mchf Feb 17, 2016

Member

I think, when refactoring this you should check if these returns are needed ... modem / isdn / dsl support was dropped from yast some time ago

This comment has been minimized.

Copy link
@teclator

teclator Feb 17, 2016

Author Contributor

So if it was dropped i could drop all references for it and for example only support ReadHardware("netcard")

@@ -450,7 +412,7 @@ def ControllerType(hwdevice)
def ReadHardware(hwtype)
hardware = []

Builtins.y2debug("hwtype=%1", hwtype)
log.debug "hwtype=#{hwtype}"

This comment has been minimized.

Copy link
@teclator

teclator Feb 17, 2016

Author Contributor

Uh .. i will revert this, to not change only parts of the method

if Ops.get_integer(hwdevice, "class_id", -1) == 12
return "ib" if subclass_id == 6
end
return "ib" if infiniband_controller?(class_id) && subclass_id == 6

This comment has been minimized.

Copy link
@mchf

mchf Feb 17, 2016

Member

why is not this integrated into network_controller? method? Infiniband is kind of network controller ....

elsif Ops.get_integer(hwdevice, "class_id", -1) == 276
return "dsl"
if interface_controller?(class_id)
log.info "CLASS 0x107" # this should happen rarely

This comment has been minimized.

Copy link
@mchf

mchf Feb 17, 2016

Member

the log message should be improved or dropped (?)

12 => "usb",
129 => "sit"
}
}

This comment has been minimized.

Copy link
@mchf

mchf Feb 17, 2016

Member

I'm thinking if these constants belongs to a yaml file. These constants are defined by hw standards and putting it into the code makes the file unnecessarily long.

This comment has been minimized.

Copy link
@teclator

teclator Feb 19, 2016

Author Contributor

It could be splitted in other module that you can import. Is preferable a yaml file?

This comment has been minimized.

Copy link
@mchf

mchf Feb 22, 2016

Member

I was thinking of yaml file bcs you put it there and forgot of it. That is exactly what I wanted to achieve. However, if you have any other idea, I've no problem with that.

# Read HW information
# @param [String] hwtype type of devices to read (netcard|modem|isdn)
# Read HW netcards information
# @param [String] not needed anymore

This comment has been minimized.

Copy link
@jreidinger

jreidinger Feb 23, 2016

Member

I think correct comment is do nothing unless empty or "netcard".

# only drivers that are not marked as broken (#97540)
drivers = card["drivers"].select do |d|
# ignoring more modules per driver...
first_module = d.fetch("modules", []).fetch(0, []).fetch(0, []) # [module, options]

This comment has been minimized.

Copy link
@jreidinger

jreidinger Feb 23, 2016

Member

For me the last fetch looks like bug as include? below looks like expect module name as it use split on broken_modules. so it never find [] in broken_modules. so it actually works, but it is confusing.

This comment has been minimized.

Copy link
@teclator

teclator Feb 24, 2016

Author Contributor

Yeah, it is confusing i could avoid the last one, but also i will replace it with a method like get_first_module_name to maintain this more legible.

This comment has been minimized.

Copy link
@jreidinger

jreidinger Feb 24, 2016

Member

not confusing but last fetch should be .fetch(0, "") as first_module is string and not array. So bug, which is not shown as "" nor [] is in broken modules

This comment has been minimized.

Copy link
@teclator

teclator Feb 24, 2016

Author Contributor

Yeah, this is already fixed in local, i had to commit it, with your other sugerences, i realized when you did the first review.

!brk
end

if drivers == []

This comment has been minimized.

Copy link
@jreidinger

jreidinger Feb 23, 2016

Member

drivers from above is for sure array and not nil, so if drivers.empty? is better

bus = "usb"
elsif bus == "Virtual IO"
bus = "vio"
end

This comment has been minimized.

Copy link
@jreidinger

jreidinger Feb 23, 2016

Member

btw for such translation using constant map is nicer like

BUS_ID_TO_NAME = {
  "PCI" => "pci",
  "USB" => "usb",
  "Virtual IO" => "vio"
}
...
one["bus"] = BUS_ID_TO_NAME[bus] || bus

This comment has been minimized.

Copy link
@teclator

teclator Feb 23, 2016

Author Contributor

+1

This comment has been minimized.

Copy link
@teclator

teclator Feb 24, 2016

Author Contributor

A recurrent comment? xD

#227


one["bus"] = bus
one["busid"] = card["sysfs_bus_id"] || ""
one["mac"] = resource["hwaddr"][0]["addr"] || ""

This comment has been minimized.

Copy link
@jreidinger

jreidinger Feb 23, 2016

Member

this can raise exception if hwaddr is for example empty.

This comment has been minimized.

Copy link
@teclator

teclator Feb 23, 2016

Author Contributor

Yeah, maybe i was too optimistic. I tried to replace the Ops dependency

Ops.get_string(resource, ["hwaddr", 0, "addr"], "")

And it take cares of nil values, so i not realized this.

Ruby 2.3 #dig wille be nice here ;)

one["busid"] = card["sysfs_bus_id"] || ""
one["mac"] = resource["hwaddr"][0]["addr"] || ""
# is the cable plugged in? nil = don't know
one["link"] = resource["link"][0]["state"]

This comment has been minimized.

Copy link
@jreidinger

jreidinger Feb 23, 2016

Member

same here, are we sure "link" is always there with at least one element?

This comment has been minimized.

Copy link
@teclator

teclator Feb 23, 2016

Author Contributor

Same as above.

@teclator teclator force-pushed the teclator:mellanox_support branch 3 times, most recently from 58a5a1b to 573e659 Feb 23, 2016


- bsc#903190
- Recognized netcards with subclass 07 as InfiniBand (ib) which
now are not filtered out.

This comment has been minimized.

Copy link
@mvidner

mvidner Mar 18, 2016

Member

This is the same description as for 3.1.143 below?!

This comment has been minimized.

Copy link
@mvidner

mvidner Mar 18, 2016

Member

I see, apparently #385 was merged because this PR was taking too long?

This comment has been minimized.

Copy link
@teclator

teclator Mar 18, 2016

Author Contributor

Exactly, the fix was merged but i will continue with the clean up, just after i finish with yast/yast-snapper#40 and yast/yast-snapper#39 i'm working on it currently.

@@ -0,0 +1,35 @@
---

This comment has been minimized.

Copy link
@mvidner

mvidner Mar 18, 2016

Member

Nice, this is much more readable!
And so the problem gets exposed: where does the magic mapping come from, and what do the strings mean? We should add a comment about that.
Is it a PCI standard? A kernel thing? A hwinfo thing?

This comment has been minimized.

Copy link
@teclator

teclator Mar 18, 2016

Author Contributor

We read it from hwinfo and subclasses so for network PCI class 0x02 ("network") those are the recognized ones.

https://pci-ids.ucw.cz/read/PD

@teclator teclator force-pushed the teclator:mellanox_support branch from 2d8d406 to fdfa1e7 Jun 9, 2016

@coveralls

This comment has been minimized.

Copy link

commented Jun 9, 2016

Coverage Status

Coverage increased (+0.6%) to 46.147% when pulling fdfa1e7 on teclator:mellanox_support into a5698c2 on yast:master.

@teclator teclator closed this Nov 14, 2016

@teclator

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2016

I will create a new PR that will supersede this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.