feature request: expose mbimcli connect parameter 'ip-type' in mbim-network utility

[Aleksander Morgado]

Hey Thijs,

>
> This is my first time contributing to an open source project and I'm not sure if this is the correct place to post this request, so please correct me if I'm wrong. I've read on the freedesktop wiki that I should create an issue and merge request in gitlab. But I can only select 'Issue' and 'Incident', that kind of feels wrong. And I'm not allowed to create an MR. That's basically why I decided to mail.

In order to create a merge request you should register in the
freedesktop gitlab instance, fork the repo, and push a branch into
your own forked repo, then you'll be able to create a merge request
against the official repo from the branch you pushed. Would you be
able to try that?

> Anyway, here goes;
>
> I would like to propose an addition to the `mbim-network` utility: since mbimcli has the option to specify the ip protocol configuration, it would be nice that mbim-network also exposes this. Attached is my patch.
>

Here's the review of that patch:

> diff --git a/utils/mbim-network.in b/utils/mbim-network.in
> index 17d92e4..12684fe 100755
> --- a/utils/mbim-network.in
> +++ b/utils/mbim-network.in
> @@ -70,7 +70,12 @@ help ()
>      echo "   in the profile:"
>      echo "      PROXY=yes"
>      echo
> -    echo "   7) Once the mbim-network script reports a successful connection"
> +    echo "   7) If you want to have the modem connect with a specific ip protocol"
> +    echo "   (one of ipv4, ipv6 or default to ipv4v6), you can do so by configuring"
> +    echo "   the following in the profile:"
> +    echo "      IP_TYPE=yes"

Please change that "yes" in the documentation to whatever the default
is in the MBIM protocol. Is it ipv4v6 the default?

> +    echo
> +    echo "   8) Once the mbim-network script reports a successful connection"
>      echo "   you still need to run a DHCP client on the associated WWAN network"
>      echo "   interface."
>      echo
> @@ -190,6 +195,12 @@ load_profile ()
>          else
>              echo "    mbim-proxy: no"
>          fi
> +
> +        if [ -n "$IP_TYPE" ]; then
> +            echo "    ip-type= $IP_TYPE"

Better print "IP type: $IP_TYPE" to be in line with the other fields printed.

> +        else
> +            echo "    ip-type= default (ipv4v6)"

Better print "IP type: unset" to be in line with the other fields printed.

> +        fi
>      else
>          echo "Profile at '$PROFILE_FILE' not found..."
>      fi
> @@ -299,6 +310,9 @@ connect ()
>      else
>          EXTRA_OPT="${PROXY_OPT}"
>      fi
> +    if [ -n "$IP_TYPE" ]; then
> +           CONNECT_ARGS="${CONNECT_ARGS},ip-type='$IP_TYPE'"
> +    fi
>

This is not the place to add this previous logic. You should modify
CONNECT_ARGS after it has been set at least once. You can move this if
block to just before CONNECT_CMD is built.

Also, the ip-type field has a very limited set of values allowed. We
should probably validate that the user provided value is one of the
ones accepted, before trying to use it (e.g. with a case statement)

>      ATTACH_CMD="mbimcli -d $DEVICE --attach-packet-service ${EXTRA_OPT}"
>      echo "Attaching to packet service with '$ATTACH_CMD'..."
>

Cheers!

-- 
Aleksander
https://aleksander.es