fix(fcoe-uefi): exit early on empty vlan

[pvalena]

Exit early in case get_fcoe_boot_vlan exits with error or just an empty string, instead of producing invalid config entry.

Changes

Just handling a corner-case when vlan is empty; this should skip to another vlan if present.

Checklist

• I have tested it locally
• I have reviewed and updated any documentation if relevant
• I am providing new code and test(s) for it

[pvalena]

I have tested the snippet, but I'm unable to test in dracut, as I lack the HW / setup.

[LaszloGombos]

CC @mwilck

[stale]

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

[mwilck]

The code in question is obviously broken. But the fix is broken, too. Instead of combining several if statements and a case statement, the handling of $vlan should be done in a single case clause.

@LaszloGombos, sorry I saw this too late. Please reopen. I have no permission to do this.

[mwilck]

@pvalena, could you update the PR as indicated above?

[pvalena]

@pvalena, could you update the PR as indicated above?

Will do. Not sure why I didn't cover it in the case...

[pvalena]

@mwilck PTAL.

[mwilck]

Thanks! Can you elmininate the if statement entirely, please?

[pvalena]

Sure thing... this should have identical behaviour.

[LaszloGombos]

@pvalena please check the lint error. Thanks !

[pvalena]

Just shfmt whitespace change... fixed.

[mwilck]

Looks good, thanks!

[aafeijoo-suse]

Can get_fcoe_boot_vlan return 1

dracut/modules.d/99uefi-lib/uefi-lib.sh

Lines 141 to 144 in 6acfeca

	while :; do
	type=$(getbyte) || return 1
	subtype=$(getbyte) || return 1
	len=$(getword) || return 1

after printing some value?

dracut/modules.d/99uefi-lib/uefi-lib.sh

Lines 147 to 150 in 6acfeca

	0314)
	# VLAN
	printf "%d" "$(getword)"
	;;

i.e., in the next loop iteration. If the answer is "no", the patch is ok.

[mwilck]

Can get_fcoe_boot_vlan return 1

@aafeijoo-suse, I don't understand the question. Neither the current code nor the patched code check the return value of get_fcoe_boot_vlan(), so the case that you are concerned about has never been taken into account. The empty string is not a valid VLAN identifier and as such, exiting early is justified.

I can't see that Pavel's patch would cause a regression.

If your intention is to improve this code further, you might as well stumble upon the [0-9]*) clause in the case statement, which is unclean because it would accept something like 5foo, which is not a valid VLAN number.

[aafeijoo-suse]

Can get_fcoe_boot_vlan return 1

@aafeijoo-suse, I don't understand the question. Neither the current code nor the patched code check the return value of get_fcoe_boot_vlan(), so the case that you are concerned about has never been taken into account. The empty string is not a valid VLAN identifier and as such, exiting early is justified.

I just read what he tries to solve in the commit message: "Exit early in case get_fcoe_boot_vlan exits with error or just an empty string".
Of course, he is already improving the current code.

[mwilck]

I guess we could write

    vlan=$(get_fcoe_boot_vlan "$1") || return 1
    case $vlan in
    ...

I feel uncomfortable wrt the return value of assignments in general, but this should work.

@aafeijoo-suse, would you be fine with that?

[aafeijoo-suse]

I guess we could write

    vlan=$(get_fcoe_boot_vlan "$1") || return 1
    case $vlan in
    ...

I feel uncomfortable wrt the return value of assignments in general, but this should work.

@aafeijoo-suse, would you be fine with that?

Sure. Or just changing the commit message would be enough :)

[mwilck]

@pvalena would you change the code as indicated above?

[pvalena]

Sure, let's enhance it further. Sorry about the forgotten commit message :) will clear things up.

[pvalena]

Rebased, fixed as suggested. Sorry it has taken so long :).

[aafeijoo-suse]

Rebased, fixed as suggested. Sorry it has taken so long :).

No problem, it would have been enough to amend the commit message.

[mwilck]

LGTM