Improve batadv_vlan.sh for LuCI integration

[wififreedom]

Maintainer: Simon Wunderlich sw@simonwunderlich.de
Compile tested: N/A, this is a modification to an existing shell script that is not compiled.
Run tested: aarch64_cortex-a53, ARMv8 Processor rev 4, OpenWrt 25.12.0 r32713-f919e7899d.
Tests done with /etc/config/network:

config interface 'bat0_40'
        option device 'bat0.40'
        option proto 'batadv_vlan'
        option ap_isolation '1'

• added interface configuration as above with ap_isolation '1'; service network reload
• changed value to '0'; service network reload
• changed value to '1'; service network reload
• removed option (expect value 0 to be applied); service network reload
• re-added option with value '1'; service network reload
• rebooted
  Confirmed correct value with batctl vlan bat0.40 ap_isolation after every action.
  Checked log files with logread, nothing to see.

Description:
Apply ap_isolation default value '0' if option ap_isolation is not present in the batadv_vlan interface configuration.

Default value '0' should be applied for the use case where "option ap_isolation '1'" was present, is removed, and 'service network reload' is executed.

This is required for proper LuCI integration, because if an option is set to the default value, LuCI removes the option.

Also take into account $INCLUDE_ONLY as in other /lib/netifd/proto scripts.

[BKPepe]

CC: @ecsv

[ecsv]

@ordex I think this was originally from you. @simonwunderlich you are the maintainer

[ordex]

@ordex I think this was originally from you.

I am not sure I worked on this patch in the past (?), in any case the change makes sense to me.

However, please note that I cannot comment on the usage of proto_add_host_dependency as I am not familiar with the proto dep API.

[wififreedom]

@ordex I think this was originally from you.

I am not sure I worked on this patch in the past (?), in any case the change makes sense to me.

However, please note that I cannot comment on the usage of proto_add_host_dependency as I am not familiar with the proto dep API.

Thank you for yout remark. It encouraged me look into the call to proto_add_host_dependency much deeper. That function call, while not harmful, does not seem to ensure that the batadv interface has already been created as I had interpreted. Instead, it ensures a call to interface_ip_add_target_route(), which a batadv VLAN does not seem to need, because it works well without it.

So I think it best to remove the call to proto_add_host_dependency.

[wififreedom]

The build system did not like that I committed a change to my fork of the routing repository with my github no-reply e-mail address and without a required format of git commit message. Also, it wants a "Signed-off-by" e-mail address. So it failed the build.

I have little to no experience with pull requests, the only way I can see to fix this myself is by creating a new pull request.

[ecsv]

@wififreedom

git rebase -i 44f2d58cbf95fe5a18291238984db97bc36237ec
# change the second line to "squash"
# pick 38a0d6d Improve batadv_vlan.sh for LuCI integration
# squash 1eb3536 Remove call to proto_add_host_dependency.

git format-patch -1
# open 0001-Improve-batadv_vlan.sh-for-LuCI-integration.patch in an editor and fix the From line and save it

git reset --hard 44f2d58cbf95fe5a18291238984db97bc36237ec
git am -s 0001-Improve-batadv_vlan.sh-for-LuCI-integration.patch

The modify the Makefile and increase the PKG_RELEASE by 1.

git commit -a --amend -s

# adjust the first line to contain "batman-adv: " as suffix and save

You must definitely modify a lot of more things to make this acceptable. At the end, you can then push everything via git push git@github.com:wififreedom/routing batadv_vlan --force

[wififreedom]

Removed and commit squashed into the rebase.

[wififreedom]

All right, I will try. Thank you for your patience.

[wififreedom]

Redid all the 'Run tested' tests I describe in the commit message, all pass.

[wififreedom]

About $INCLUDE_ONLY: I followed https://openwrt.org/docs/guide-developer/network-scripting?s[]=network#writing_protocol_handlers and I looked at the other protocol handler scripts that followed the guide.

The guide does not say why. It looks like a performance optimization to me, or the protocol handler script could be included by some program that does not have the included files available, for example to only discover which functions the protocol handler script defines.

Either the guide is wrong, or the script is wrong.

Either way, the script works.

[wififreedom]

I've adjusted my github profile e-mail settings and I've performed another rebase in an attempt to make the build system happy.

[wififreedom]

Almost. Rebased again to add the Signed-off-by.

[wififreedom]

@ecsv and @ordex: Your reviews are great and to the point. I appreciate your help very much.

This has been a learning experience for me.

However, all of this sure is a lot of work and a lot of trial and error, and definitely costs more time and effort than it really should.

As a newbie with a newbie view at the non-trivial procedures I need to follow I can make a few suggestions:

1. Document what the requirements are for your commit e-mail for a pull request: you cannot use a github no-reply address for the commit, in the Signed-off-by in the commit message, or even for your github account. Also document why this all is, it feels like bureaucracy but probably is not. And it is not safe to expose your e-mail to harvesting and spamming by botnets, yet you must, for these reasons.

2. Document how to make a pull request: fork the repo, make a branch, commit with certain commit message format, push.

3. Document the commit message format. Prefix "batman-adv: " required in the first line if you commit in section batman-adv, etc. No capital letter after the "batman-adv: " prefix, or the build will fail. No long lines longer than X characters in your commit message. Do not add the Maintainer and what you tested in the commit message.

4. If possible, add a pre-commit hook (I remember that this existed when I used git a lifetime ago, but not what it can and can't do exactly) that checks the format of the commit message beforehand, with meaningful error messages so that this can be fixed before it reaches the build system hours later. Turnaround time will be much shorter if you do not have to wait for a build.

5. Document how to change a pull request after review: how to rebase and change the PKG_RELEASE number.

6. Document when and how to increase the PKG_RELEASE number in which Makefile. Better yet, automate this away.

If the documentation how to successfully complete a pull request already exists, make it so that it stares you in the face if you want to contribute. A big link in the README on the front page of the repo, for example.

Do with this what you wish.

[wififreedom]

The above does not mean that I'm abandoning the effort to get this pull request merged, I'm still here.

[ecsv]

@wififreedom You understand that neither @ordex or myself are the maintainer(s)? Even when you still incorrectly say in the PR that I am. We only tried to help get this merged. We have absolutely nothing to say about the formal requirement. It is there and it is automatically tested by the CI.

For the guidelines/requirements: https://openwrt.org/faq/how_can_i_help_or_contribute - and the automated tests, see the checks at the end of the PR.

About the skills for working with git - I don't think that I should write a tutorial in some kind of readme. This would quite a lot of duplicated material. And you have to keep in mind - we are not the maintainers. Why should I spend my time writing this?

[wififreedom]

My apologies, @ecsv.

In the instructions about how the initial pull request message should be formatted was something about: look at the Makefile to see who is the maintainer. I looked at the Makefile, but in the wrong way: who had committed in the git log, and your name was there for the last almost all commits. I now see in the Makefile that you are not the maintainer.

You are very kind to help me anyway.

[wififreedom]

I have updated the PR.

[simonwunderlich]

@wififreedom thank you for pushing this through, it's now merged! @ecsv and @ordex, thank you for all the inputs!