| Message ID | 20231213200001.1994492-1-gerickson@nuovations.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | connection: Rename 'connection.c' to 'gateway.c' | expand |
Hi Grant, > While historically, "connection.c" might have been a contextually-apt > name, today it might be better named "gateway.c" since its primary > focus is gateway routes and gateway route management. > > Reflective of that, this renames "connection.c" to "gateway.c" and > updates public and private symbol names accordingly. > > Grant Erickson (3): > connection: Rename 'connection.c' to 'gateway.c'. > gateway: Updated @file comment to reflect recent rename. > gateway: Rename public and private symbols after file rename. > > Makefile.am | 2 +- > src/connman.h | 14 +++---- > src/dnsproxy.c | 2 +- > src/{connection.c => gateway.c} | 65 ++++++++++++++++----------------- > src/inet.c | 2 +- > src/ipconfig.c | 8 ++-- > src/main.c | 4 +- > src/network.c | 2 +- > src/provider.c | 4 +- > src/service.c | 22 +++++------ > 10 files changed, 62 insertions(+), 63 deletions(-) > rename src/{connection.c => gateway.c} (98%) all 3 patches have been applied. Regards Marcel
Hello Grant and Marcel, I'm not really understanding what is the real benefit here in renaming connection.c to gateway.c. Granted (pun), it is more clear name for it but it still has some vpn-related functionality to get the physical device index etc. so it is still misleading. My point here is that this just brings a lot of drawbacks: - It is rather complex now to apply patches from older versions, forks or heavily patched forks to upstream that contain changes to old connection.c - It is really not straightforward just to pick an upstream patch made with changes to connection.c into these older versions or forks that from one reason or another, or like million reasons, like with our fork, cannot just be updated directly to latest upstream. - And the name was there for... 15 years? It really hasn't gone through so much changes that it would need to be renamed from my opinion. From my perspective the simple SWOT analysis here would result a negative outcome to renaming and positive towards keeping the name. Also, choose vs. yield, well, for non-native English speakers the first would be more obvious in definition and one glance of the name would tell what it is supposed to do. Personally I'd prefer concise language throughout the project, the function does not really give up anything but selects the most suitable one, right? The commit: https://git.kernel.org/pub/scm/network/connman/connman.git/commit/src/connection.c?id=64a986109880828a4aa727ecb7a82cb97b004623 It is good that stuff gets documented here but reformatting should not be done just because reformatting needs. I'd say that compatibility should weight there a lot when deciding whether to rename or not. If the process of upstreaming because of this kind of changes becomes rather complex (even more than it already is because of historic reasons) and time required to do them increases a lot it just might be that those patches do not get done/sent. But this is just my view on the matter. - Jussi On 12/13/23 21:59, Grant Erickson wrote: > While historically, "connection.c" might have been a contextually-apt > name, today it might be better named "gateway.c" since its primary > focus is gateway routes and gateway route management. > > Reflective of that, this renames "connection.c" to "gateway.c" and > updates public and private symbol names accordingly. > > Grant Erickson (3): > connection: Rename 'connection.c' to 'gateway.c'. > gateway: Updated @file comment to reflect recent rename. > gateway: Rename public and private symbols after file rename. > > Makefile.am | 2 +- > src/connman.h | 14 +++---- > src/dnsproxy.c | 2 +- > src/{connection.c => gateway.c} | 65 ++++++++++++++++----------------- > src/inet.c | 2 +- > src/ipconfig.c | 8 ++-- > src/main.c | 4 +- > src/network.c | 2 +- > src/provider.c | 4 +- > src/service.c | 22 +++++------ > 10 files changed, 62 insertions(+), 63 deletions(-) > rename src/{connection.c => gateway.c} (98%) >
On Dec 19, 2023, at 5:32 AM, Jussi Laakkonen <jussi.laakkonen@jolla.com> wrote: > I'm not really understanding what is the real benefit here in renaming connection.c to gateway.c. Granted (pun), it is more clear name for it but it still has some vpn-related functionality to get the physical device index etc. so it is still misleading. > > My point here is that this just brings a lot of drawbacks: > - It is rather complex now to apply patches from older versions, forks or heavily patched forks to upstream that contain changes to old connection.c > - It is really not straightforward just to pick an upstream patch made with changes to connection.c into these older versions or forks that from one reason or another, or like million reasons, like with our fork, cannot just be updated directly to latest upstream. > - And the name was there for... 15 years? It really hasn't gone through so much changes that it would need to be renamed from my opinion. > > From my perspective the simple SWOT analysis here would result a negative outcome to renaming and positive towards keeping the name. > > Also, choose vs. yield, well, for non-native English speakers the first would be more obvious in definition and one glance of the name would tell what it is supposed to do. Personally I'd prefer concise language throughout the project, the function does not really give up anything but selects the most suitable one, right? The commit: > https://git.kernel.org/pub/scm/network/connman/connman.git/commit/src/connection.c?id=64a986109880828a4aa727ecb7a82cb97b004623 > > It is good that stuff gets documented here but reformatting should not be done just because reformatting needs. I'd say that compatibility should weight there a lot when deciding whether to rename or not. > > If the process of upstreaming because of this kind of changes becomes rather complex (even more than it already is because of historic reasons) and time required to do them increases a lot it just might be that those patches do not get done/sent. But this is just my view on the matter. Jussi, In terms of the file and function rename(s), I approached both as someone who was somewhat-initiated in having worked on connman some 13 years ago but was coming back to it with fresh eyes. I reasoned that if file and function names (those in connection.c/gateway.c, in particular) were confusing to me, then they would be more so to someone even less initiated than I. To the extent Marcel and the community were supportive, I submitted the patches. Having substantially refactored a large portion of connection.c/gateway.c, I believe it is more understandable for the uninitiated today than it was before. For my own insight, are there specific down-revision features of connman on which Sailfish (I assume Sailfish is the fork in question) would not or could not work with top-of-tree connman? Best, Grant
On 12/20/23 07:53, Grant Erickson wrote: > On Dec 19, 2023, at 5:32 AM, Jussi Laakkonen <jussi.laakkonen@jolla.com> wrote: >> I'm not really understanding what is the real benefit here in renaming connection.c to gateway.c. Granted (pun), it is more clear name for it but it still has some vpn-related functionality to get the physical device index etc. so it is still misleading. >> >> My point here is that this just brings a lot of drawbacks: >> - It is rather complex now to apply patches from older versions, forks or heavily patched forks to upstream that contain changes to old connection.c >> - It is really not straightforward just to pick an upstream patch made with changes to connection.c into these older versions or forks that from one reason or another, or like million reasons, like with our fork, cannot just be updated directly to latest upstream. >> - And the name was there for... 15 years? It really hasn't gone through so much changes that it would need to be renamed from my opinion. >> >> From my perspective the simple SWOT analysis here would result a negative outcome to renaming and positive towards keeping the name. >> >> Also, choose vs. yield, well, for non-native English speakers the first would be more obvious in definition and one glance of the name would tell what it is supposed to do. Personally I'd prefer concise language throughout the project, the function does not really give up anything but selects the most suitable one, right? The commit: >> https://git.kernel.org/pub/scm/network/connman/connman.git/commit/src/connection.c?id=64a986109880828a4aa727ecb7a82cb97b004623 >> >> It is good that stuff gets documented here but reformatting should not be done just because reformatting needs. I'd say that compatibility should weight there a lot when deciding whether to rename or not. >> >> If the process of upstreaming because of this kind of changes becomes rather complex (even more than it already is because of historic reasons) and time required to do them increases a lot it just might be that those patches do not get done/sent. But this is just my view on the matter. > > Jussi, > > In terms of the file and function rename(s), I approached both as someone who was somewhat-initiated in having worked on connman some 13 years ago but was coming back to it with fresh eyes. I reasoned that if file and function names (those in connection.c/gateway.c, in particular) were confusing to me, then they would be more so to someone even less initiated than I. To the extent Marcel and the community were supportive, I submitted the patches. Having substantially refactored a large portion of connection.c/gateway.c, I believe it is more understandable for the uninitiated today than it was before. > > For my own insight, are there specific down-revision features of connman on which Sailfish (I assume Sailfish is the fork in question) would not or could not work with top-of-tree connman? > > Best, > > Grant Hi Grant, I was just concerned that the contributions in form of new features and fixing existing we and many others have been doing becomes even more difficult with renaming of the content hence the differences of the forks. Personally I try to avoid renaming much in order to not to break any workflows people already have. I'd just reason it in this way that better to have a bit of odd naming than break anything ;). But this "yield" word just really sounded odd, especially since the definition would be, from my perspective, ok in thread/fork-related cases, or in what https://www.merriam-webster.com/dictionary/yield defines but in selecting the default gw it just seems a bit off? Sorry about the nag on this. Yes I'm talking here about Sailfish OS fork of ConnMan. We have had a bit different direction, I think since 2014, in some service struct reference counting use. I think that is one of them that makes it quite difficult to make some changes to work with upstream of make upstream work directly with ours. There are other differences as well, which haven't been upstreamed throughout the years. But the plan is to gradually try to get closer to upstream by each version upgrade, which should be started next year. We do have some interesting features I'd like to someday push as an RFC (when time allows), such as firewall via configuration files, multiuser support with systemd and a recent CLAT support feature that enables dual-index support for the core to name but the biggest changes. These just drag with them so much of the old content that is done in a different way as the upstream has so it'll take quite a lot of time to usually make the patches. The reason I'd push them as an RFC is that there is no guarantee of how they'd really work with upstream. We've had those either in production or testing already for months or up to years. Added Marcel here too if he'd wish to participate/comment. Cheers, Jussi
On Dec 20, 2023, at 9:10 AM, Jussi Laakkonen <jussi.laakkonen@jolla.com> wrote: > On 12/20/23 07:53, Grant Erickson wrote: >> On Dec 19, 2023, at 5:32 AM, Jussi Laakkonen <jussi.laakkonen@jolla.com> wrote: >>> I'm not really understanding what is the real benefit here in renaming connection.c to gateway.c. Granted (pun), it is more clear name for it but it still has some vpn-related functionality to get the physical device index etc. so it is still misleading. >>> >>> My point here is that this just brings a lot of drawbacks: >>> - It is rather complex now to apply patches from older versions, forks or heavily patched forks to upstream that contain changes to old connection.c >>> - It is really not straightforward just to pick an upstream patch made with changes to connection.c into these older versions or forks that from one reason or another, or like million reasons, like with our fork, cannot just be updated directly to latest upstream. >>> - And the name was there for... 15 years? It really hasn't gone through so much changes that it would need to be renamed from my opinion. >>> >>> From my perspective the simple SWOT analysis here would result a negative outcome to renaming and positive towards keeping the name. >>> >>> Also, choose vs. yield, well, for non-native English speakers the first would be more obvious in definition and one glance of the name would tell what it is supposed to do. Personally I'd prefer concise language throughout the project, the function does not really give up anything but selects the most suitable one, right? The commit: >>> https://git.kernel.org/pub/scm/network/connman/connman.git/commit/src/connection.c?id=64a986109880828a4aa727ecb7a82cb97b004623 >>> >>> It is good that stuff gets documented here but reformatting should not be done just because reformatting needs. I'd say that compatibility should weight there a lot when deciding whether to rename or not. >>> >>> If the process of upstreaming because of this kind of changes becomes rather complex (even more than it already is because of historic reasons) and time required to do them increases a lot it just might be that those patches do not get done/sent. But this is just my view on the matter. >> Jussi, >> In terms of the file and function rename(s), I approached both as someone who was somewhat-initiated in having worked on connman some 13 years ago but was coming back to it with fresh eyes. I reasoned that if file and function names (those in connection.c/gateway.c, in particular) were confusing to me, then they would be more so to someone even less initiated than I. To the extent Marcel and the community were supportive, I submitted the patches. Having substantially refactored a large portion of connection.c/gateway.c, I believe it is more understandable for the uninitiated today than it was before. >> For my own insight, are there specific down-revision features of connman on which Sailfish (I assume Sailfish is the fork in question) would not or could not work with top-of-tree connman? >> Best, >> Grant > > Hi Grant, > > I was just concerned that the contributions in form of new features and fixing existing we and many others have been doing becomes even more difficult with renaming of the content hence the differences of the forks. Personally I try to avoid renaming much in order to not to break any workflows people already have. I'd just reason it in this way that better to have a bit of odd naming than break anything ;). > > But this "yield" word just really sounded odd, especially since the definition would be, from my perspective, ok in thread/fork-related cases, or in what https://www.merriam-webster.com/dictionary/yield defines but in selecting the default gw it just seems a bit off? Sorry about the nag on this. In breaking down the syntactic and semantic choice of “choose” versus “yield”, to me, “choose” implied “you have two gateways to consider that are, at the outset, equal. That is, choose one of the two specified gateways and then give it the (what is now) high-priority (that is, metric 0) default route. However, the reality (somewhat complicated more now by the existence of BOTH high- and low-priority default routes) is not only that the action of the function is not simply a side-effect-free choice but also that it is a demotion / unset / give up / yielding of the high-priority default route for one of the two gateways. Regardless of its function name, on entry to the function one of the two gateways holds the high-priority (that is, metric 0) default route, so there is not such much a choice between equal gateways. On exit from the function, one of those two gateways will have unset it / given it up / have “yielded” the high-priority default route and will have assumed a low-priority (that is, metric > 0) default route. It is not until after ‘yield_default_gateway’ is called that the activated, if it was not the gateway to unset / give up / yield the high-priority route, is promoted from its low-priority route to the high-priority route. Given that this demotion / unset / give up / yielding of the high-priority default route was a central and key side-effect of this function being called, I felt that “yield” was a much more apt action verb than “choose”. Comparing it to, say, “pthread_yield”, in that case it is a thread giving up or yielding the processor / scheduler. In this case, it is a gateway giving up or yielding the high-priority default route. Does that help color and explain the rationale I had?
On 12/22/23 18:24, Grant Erickson wrote: > On Dec 20, 2023, at 9:10 AM, Jussi Laakkonen <jussi.laakkonen@jolla.com> wrote: >> On 12/20/23 07:53, Grant Erickson wrote: >>> On Dec 19, 2023, at 5:32 AM, Jussi Laakkonen <jussi.laakkonen@jolla.com> wrote: >>>> I'm not really understanding what is the real benefit here in renaming connection.c to gateway.c. Granted (pun), it is more clear name for it but it still has some vpn-related functionality to get the physical device index etc. so it is still misleading. >>>> >>>> My point here is that this just brings a lot of drawbacks: >>>> - It is rather complex now to apply patches from older versions, forks or heavily patched forks to upstream that contain changes to old connection.c >>>> - It is really not straightforward just to pick an upstream patch made with changes to connection.c into these older versions or forks that from one reason or another, or like million reasons, like with our fork, cannot just be updated directly to latest upstream. >>>> - And the name was there for... 15 years? It really hasn't gone through so much changes that it would need to be renamed from my opinion. >>>> >>>> From my perspective the simple SWOT analysis here would result a negative outcome to renaming and positive towards keeping the name. >>>> >>>> Also, choose vs. yield, well, for non-native English speakers the first would be more obvious in definition and one glance of the name would tell what it is supposed to do. Personally I'd prefer concise language throughout the project, the function does not really give up anything but selects the most suitable one, right? The commit: >>>> https://git.kernel.org/pub/scm/network/connman/connman.git/commit/src/connection.c?id=64a986109880828a4aa727ecb7a82cb97b004623 >>>> >>>> It is good that stuff gets documented here but reformatting should not be done just because reformatting needs. I'd say that compatibility should weight there a lot when deciding whether to rename or not. >>>> >>>> If the process of upstreaming because of this kind of changes becomes rather complex (even more than it already is because of historic reasons) and time required to do them increases a lot it just might be that those patches do not get done/sent. But this is just my view on the matter. >>> Jussi, >>> In terms of the file and function rename(s), I approached both as someone who was somewhat-initiated in having worked on connman some 13 years ago but was coming back to it with fresh eyes. I reasoned that if file and function names (those in connection.c/gateway.c, in particular) were confusing to me, then they would be more so to someone even less initiated than I. To the extent Marcel and the community were supportive, I submitted the patches. Having substantially refactored a large portion of connection.c/gateway.c, I believe it is more understandable for the uninitiated today than it was before. >>> For my own insight, are there specific down-revision features of connman on which Sailfish (I assume Sailfish is the fork in question) would not or could not work with top-of-tree connman? >>> Best, >>> Grant >> >> Hi Grant, >> >> I was just concerned that the contributions in form of new features and fixing existing we and many others have been doing becomes even more difficult with renaming of the content hence the differences of the forks. Personally I try to avoid renaming much in order to not to break any workflows people already have. I'd just reason it in this way that better to have a bit of odd naming than break anything ;). >> >> But this "yield" word just really sounded odd, especially since the definition would be, from my perspective, ok in thread/fork-related cases, or in what https://www.merriam-webster.com/dictionary/yield defines but in selecting the default gw it just seems a bit off? Sorry about the nag on this. > > In breaking down the syntactic and semantic choice of “choose” versus “yield”, to me, “choose” implied “you have two gateways to consider that are, at the outset, equal. That is, choose one of the two specified gateways and then give it the (what is now) high-priority (that is, metric 0) default route. However, the reality (somewhat complicated more now by the existence of BOTH high- and low-priority default routes) is not only that the action of the function is not simply a side-effect-free choice but also that it is a demotion / unset / give up / yielding of the high-priority default route for one of the two gateways. > > Regardless of its function name, on entry to the function one of the two gateways holds the high-priority (that is, metric 0) default route, so there is not such much a choice between equal gateways. On exit from the function, one of those two gateways will have unset it / given it up / have “yielded” the high-priority default route and will have assumed a low-priority (that is, metric > 0) default route. It is not until after ‘yield_default_gateway’ is called that the activated, if it was not the gateway to unset / give up / yield the high-priority route, is promoted from its low-priority route to the high-priority route. > > Given that this demotion / unset / give up / yielding of the high-priority default route was a central and key side-effect of this function being called, I felt that “yield” was a much more apt action verb than “choose”. > > Comparing it to, say, “pthread_yield”, in that case it is a thread giving up or yielding the processor / scheduler. In this case, it is a gateway giving up or yielding the high-priority default route. > > Does that help color and explain the rationale I had? > Hi Grant, Thanks for the explanation, with the two priorities of routes it makes a bit more sense. Still, it just occurred to me that "provide" might also work now that I thought this again after this detailed explanation of yours. However, would that confuse as "provider" is already used as a term for the VPNs inside ConnMan. Lots of changes happening here and getting hard to keep up with all of those. Oh well, it just takes time. Sorry for the late reply, I was bit of indisposed during the holidays. Cheers! - Jussi
While historically, "connection.c" might have been a contextually-apt name, today it might be better named "gateway.c" since its primary focus is gateway routes and gateway route management. Reflective of that, this renames "connection.c" to "gateway.c" and updates public and private symbol names accordingly. Grant Erickson (3): connection: Rename 'connection.c' to 'gateway.c'. gateway: Updated @file comment to reflect recent rename. gateway: Rename public and private symbols after file rename. Makefile.am | 2 +- src/connman.h | 14 +++---- src/dnsproxy.c | 2 +- src/{connection.c => gateway.c} | 65 ++++++++++++++++----------------- src/inet.c | 2 +- src/ipconfig.c | 8 ++-- src/main.c | 4 +- src/network.c | 2 +- src/provider.c | 4 +- src/service.c | 22 +++++------ 10 files changed, 62 insertions(+), 63 deletions(-) rename src/{connection.c => gateway.c} (98%)