Mellanox / bfscripts

Collection of scripts used for BlueField SoC system management.
BSD 2-Clause "Simplified" License
20 stars 27 forks source link

[bfpxe] bfmac param not accepting MACs delimited with ":" #232

Open pawel-baldysiak-dell opened 3 months ago

pawel-baldysiak-dell commented 3 months ago

Hi, Currently "bfmac" param used in bfpxe is expecting to get: bfmac=<mac>:<ip>:<netmask> where MAC is in "aa-bb-cc-dd-ee-ff" format. https://github.com/Mellanox/bfscripts/blob/master/bfpxe#L64C1-L67C43

I'm trying to have a dynamic grub.cfg file, where I can pass mac addresses via this variable. The problem that I see is that grub is storing macs in "aa:bb:cc:dd:ee:ff" format - so if I create a param as: _bfmac=$net_default_mac:$net_defaultip:255.255.255.255 it would confuse the script due to too many ":" delimiters.

I was thinking about how to improve it and keep it backward compatible - came up with 3 ideas: 1) Change the way of paring each param: treat first 17 chars as MAC - change "-" to ":" no matter what is there Get param as second to last after ":" Get param as last after ":"

Code snippet for this:

realbfmac=$(echo $bfmac | cut -b1-17 | tr '-' ':')
ifname=$(grep $realbfmac /sys/class/net/*/address | awk -F\/ {'print $5'})
ifaddr=$(echo $bfmac | awk -F: '{print $(NF-1) }')
ifnet=$(echo $bfmac |  awk -F: '{print $(NF) }')

2) call tr to change potential ":" to "-" in first 17 bytes of bfmac:

Something like below + append rest of input

bfmac=$(echo $bfmac | cut -b1-17 | tr ':' '-')

3) get MAC as first 17 chars, then trim it from bfmac string

realbfmac=$(echo $bfmac | cut -b1-17 | tr '-' ':')
ifname=$(grep $realbfmac /sys/class/net/*/address | awk -F\/ {'print $5'})
bfmac=$(echo $bfmc | cut -b 18-)
ifaddr=$(echo $bfmac | cut -d: -f1)
ifnet=$(echo $bfmac |  cut -d: -f2)

Number 1 seems to be most elegant for me, while number 2 keeps the readability of the code - but calls /tr/ back and forth.

What's your opinion about those approaches? I could create a PR with fix once we agree on the options.

mahantesh-nvidia commented 2 months ago

I believe option 1 above is a better candidate to address backward compatibility and also handle grub format. This is efficient because the mac is required to be in the same format as grub stores is it, for finding the interface name.

pawel-baldysiak-dell commented 2 months ago

ok, thanks! Will create a patch for that shortly

pawel-baldysiak-dell commented 2 months ago

Created PR to address this issue

235

mahantesh-nvidia commented 2 months ago

Please assign reviewers (lsun100 and mahantesh-nvidia) to the PR. Thanks

pawel-baldysiak-dell commented 2 months ago

I'm afraid I can't do that :) I don't have rights to assign reviewers