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.

Sign up for GitHub

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

Jump to bottom

lpac: don't overwrite env variables #25258

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+15 −12
Open

lpac: don't overwrite env variables #25258

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions utils/lpac/files/lpac.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,29 @@ APDU_DEBUG="$(uci_get lpac global apdu_debug 0)"
HTTP_BACKEND="$(uci_get lpac global http_backend curl)"
HTTP_DEBUG="$(uci_get lpac global http_debug 0)"
Comment on lines 8 to 9
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Extra variables
  • Default values are unneeded and shouldn't be used in the script here (because it's a code)

Copy link
Member Author

Choose a reason for hiding this comment

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

Why are default values not needed?

I don't go with "extra variables, they do not propagate outside the script.

Copy link
Contributor

Choose a reason for hiding this comment

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

Separating code and data is a good practice.
Additionally, the default values are just not needed here.

Please pay attention to the alternative implementation which doesn't use the defaults in code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't go with "extra variables, they do not propagate outside the script.

Having a mix of the real Lpac's env vars and your extra (unneeded) ones makes the script harder to read.


export LPAC_HTTP="$HTTP_BACKEND"
function export_if_not_in_env {
eval "value=\${$1}"
if [ -z "$value" ]; then
export "$1=$2"
fi
}
Comment on lines +11 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, the function is overkill, see my approach which goes without:
https://github.com/openwrt/packages/pull/25046/files

Copy link
Member Author

Choose a reason for hiding this comment

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

It provides a clean way of achieving the common task of only overwriting set values from the outside script.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't name it "a clean way", sorry.


export_if_not_in_env LPAC_HTTP "$HTTP_BACKEND"
if [ "$HTTP_DEBUG" -eq 1 ]; then
export LIBEUICC_DEBUG_HTTP="1"
export_if_not_in_env LIBEUICC_DEBUG_HTTP "1"
Comment on lines +18 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

#25046 (comment)

The problem with your handling of the debug options, as I see it, is that you have to interpret their values, which our scripts shouldn't do anyway.

Not addressed.
See the alternative approach above, again.

Copy link
Member Author

Choose a reason for hiding this comment

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

So you mean to not set the debug env variables if they are not enabled? This would suffice if we remove UCI handling from them, which i am fine with.

Copy link
Contributor

@bam80 bam80 Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right now, the UCI handling is in place, so we could (and should) rely on that instead of trying to be smart and interfere with lpac's envs interpretation in this script.

fi

export LPAC_APDU="$APDU_BACKEND"
export_if_not_in_env LPAC_APDU "$APDU_BACKEND"
if [ "$APDU_DEBUG" -eq 1 ]; then
export LIBEUICC_DEBUG_APDU="1"
export_if_not_in_env LIBEUICC_DEBUG_APDU "1"
fi

if [ "$APDU_BACKEND" = "at" ]; then
AT_DEVICE="$(uci_get lpac at device /dev/ttyUSB2)"
AT_DEBUG="$(uci_get lpac at debug 0)"
export AT_DEVICE="$AT_DEVICE"
export AT_DEBUG="$AT_DEBUG"
export_if_not_in_env AT_DEVICE "$(uci_get lpac at device /dev/ttyUSB2)"
export_if_not_in_env AT_DEBUG "$(uci_get lpac at debug 0)"
elif [ "$APDU_BACKEND" = "uqmi" ]; then
UQMI_DEV="$(uci_get lpac uqmi device /dev/cdc-wdm0)"
UQMI_DEBUG="$(uci_get lpac uqmi debug 0)"
export LPAC_QMI_DEV="$UQMI_DEV"
export LPAC_QMI_DEBUG="$UQMI_DEBUG"
export_if_not_in_env LPAC_QMI_DEV "$(uci_get lpac uqmi device /dev/cdc-wdm0)"
export_if_not_in_env LPAC_QMI_DEBUG "$(uci_get lpac uqmi debug 0)"
fi

/usr/lib/lpac "$@"