Bug 101268

Comment on attachment 131633 [details] [review]
PIN.patch

Review of attachment 131633 [details] [review]:
-----------------------------------------------------------------

Hey, thanks for the patch, see comments inline.

::: /usr/bin/mbim-network
@@ +70,5 @@
>      echo "   in the profile:"
>      echo "      PROXY=yes"
>      echo
> +    echo "   7) If your SIM card is PIN locked, you can supply the pin"
> +    echo "   like this in the config file"

in the *profile:*

@@ +195,5 @@
>          else
>              echo "    mbim-proxy: no"
>          fi
> +
> +	if [ -n "$PIN" ]; then

Looks like there's a TAB before the if []; we are indenting with spaces here.

@@ +266,4 @@
>          clear_state
>      fi
>  
> +    if [ -n "$PIN" ]; then

how about storing the command to run in a variable so that we can print it as well in a message, as done in e.g. SUBSCRIBER_READY_CMD below?

@@ +266,5 @@
>          clear_state
>      fi
>  
> +    if [ -n "$PIN" ]; then
> +        mbimcli -d $DEVICE --enter-pin=$PIN

missing: --no-close $PROXY_OPT

Created attachment 131639 [details] [review]
patch v2

better like this? sorry for the tab, vim got it wrong:-(

Comment on attachment 131639 [details] [review]
patch v2

Review of attachment 131639 [details] [review]:
-----------------------------------------------------------------

I see the diff is done against /usr/bin/mbim-network... would you be willing to provide a proper git patch against the libmbim git repository instead? If you cannot or you don't want, let me know and I'll manually apply the changes myself.

::: /usr/bin/mbim-network
@@ +269,5 @@
> +    if [ -n "$PIN" ]; then
> +        UNLOCK_SIM_CMD="mbimcli -d $DEVICE --enter-pin=$PIN --no-close $PROXY_OPT"
> +        echo "Unlocking SIM card with PIN code '$UNLOCK_SIM_CMD'..."
> +        UNLOCK_SIM_OUT=`$UNLOCK_SIM_CMD`
> +        echo $UNLOCK_SIM_OUT

Instead of always printing the UNLOCK_SIM_OUT, we should probably detect whether the unlock attempt succeeded or not, and print the full response only if it failed, e.g.:

    UNLOCK_SIM_OUT=`$UNLOCK_SIM_CMD`
    if [ $? -eq 0 ]; then
        echo "SIM-PIN unlocked successfully"
    else
        echo "SIM-PIN unlock failed"
        echo $UNLOCK_SIM_OUT
    fi

What do you think?

I will post a proper git patch, just have to RTFM how as I've never done it before...

Same for the other patch for the help switch.

As for the proposed changes to the pin change detection, I am all for it.

I didn't like the way how the libmbim commands are always sent so I created a refactored version using a dispatcher function. I is much nicer to use, but I am still ironing out some edge use cases. Will post it as soon as it is ready.

> I didn't like the way how the libmbim commands are always sent so I created
> a refactored version using a dispatcher function. I is much nicer to use,
> but I am still ironing out some edge use cases. Will post it as soon as it
> is ready.

I'm not sure what you mean, but I'd suggest you post it either in bugzilla or in the libmbim-mailing list first as a RFC to see what everyone says about those changes ;)

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mobile-broadband/libmbim/issues/2.