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.

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

inserted some code to export AC name to enviroment, so that AC name c… #90

Closed
wants to merge 1 commit into from

Conversation

bearmi
Copy link

@bearmi bearmi commented Nov 29, 2017

I inserted some code to export AC name to enviroment, so that AC name can be collected and shown to end user

We want to show the access concentrator to the end user. So I added a variable PeerName in struct PPPoEConnectionStruct, and log the AC name to the variable when parsing PADO tags.

And the value would be exported to env in function PPPOEConnectDevice. So that we can collect it and show it to user.

@fhost
Copy link
Contributor

fhost commented Aug 21, 2018

I'm not sure using a PPPoETag structure to store the value is useful as it is not used as so later. Just using a string looks like a better way IMO.

I find the commit message a bit strange too, doesn't export AC name to script environment look enough?

@Neustradamus Neustradamus mentioned this pull request Jan 4, 2021
@paulusmack
Copy link
Collaborator

This seems like a useful feature, but this commit will need some more work before I can take it. I am not planning to put this in the 2.4.9 release, but it can go in after the release. This change intersects with the changes that @pali has submitted (#209) and should be reworked on top of those changes, I think.

I agree that using a string would be clearer and simpler than a PPPoETag structure.

The commit message needs a sign-off and should start with a headline like "plugins/pppoe: Export access concentrator name to environment", then a blank line, then a paragraph explaining why this is a useful thing to do.

@Neustradamus
Copy link
Member

@bearmi: Can you look the @paulusmack comment?

@enaess
Copy link
Contributor

enaess commented May 31, 2022

I believe @pali has done work in this area. Could he please review this change?

@enaess
Copy link
Contributor

enaess commented May 31, 2022

Change does look good to me though ...

@pali
Copy link
Contributor

pali commented May 31, 2022

This commit does not apply on top of the master branch... it needs to be rebased/reworked first.

@paulusmack
Copy link
Collaborator

Superseded by #492.

@paulusmack paulusmack closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants