search

epics-base / pva2pva

PV Access gateway/proxy and EPICS Process Database integration
https://epics-base.github.io/pva2pva/
Other
4 stars 13 forks source link

Configuration of boolean "*_AUTO_*" settings does not work #30

Closed ralphlange closed 4 years ago

ralphlange commented 4 years ago

Reported by Bertrand Bauvir (@ITER):

Configuring the EPICS_PVA_AUTO_ADDR_LIST setting through the JSON autoaddrlist flag does not work. The name resolution requests are always broadcast to all network interfaces, i.e. the effective setting is always the default of EPICS_PVA_AUTO_ADDR_LIST=YES.

ralphlange commented 4 years ago

Reason: The code in gwmain.cpp registers the flag as a boolean in the configuration structure. However, the environment variable setting must be a string ("YES"/"NO"). https://github.com/epics-base/pva2pva/blob/29a6f261dcd28e213d273d5b2acb0ae7408dfcb2/p2pApp/gwmain.cpp#L142

The following workaround/fix for EPICS_PVA_AUTO_ADDR_LIST solves the issue.

Instead of

    .add("EPICS_PVA_AUTO_ADDR_LIST", conf->getSubFieldT<pvd::PVBoolean>("autoaddrlist")->get())

do

    bool autoaddr = conf->getSubFieldT<pvd::PVBoolean>("autoaddrlist")->get();
    std::string autoaddr_str;
    if (autoaddr) autoaddr_str = "YES"; else autoaddr_str = "NO";
...
    .add("EPICS_PVA_AUTO_ADDR_LIST", autoaddr_str)

The other option would be patching pvAccess/src/utils/configuration.cpp to always return "YES"/"NO" for reading boolean configuration values as strings. Seems a bit too general (I would rather expect "true"/"false"), but might be ok in this context.

The same applies to EPICS_PVAS_AUTO_BEACON_ADDR_LIST.

ralphlange commented 4 years ago

@mdavidsaver: Any chance that this fix will be included in the soon-to-come EPICS 7 release?

mdavidsaver commented 4 years ago

Could you test?

diff --git a/p2pApp/gwmain.cpp b/p2pApp/gwmain.cpp
index 724ace2..a4190f5 100644
--- a/p2pApp/gwmain.cpp
+++ b/p2pApp/gwmain.cpp
@@ -139,7 +139,7 @@ GWServerChannelProvider::shared_pointer configure_client(ServerConfig& arg, cons

     pva::Configuration::shared_pointer C(pva::ConfigurationBuilder()
                                          .add("EPICS_PVA_ADDR_LIST", conf->getSubFieldT<pvd::PVString>("addrlist")->get())
-                                         .add("EPICS_PVA_AUTO_ADDR_LIST", conf->getSubFieldT<pvd::PVBoolean>("autoaddrlist")->get())
+                                         .add("EPICS_PVA_AUTO_ADDR_LIST", conf->getSubFieldT<pvd::PVScalar>("autoaddrlist")->getAs<std::string>())
                                          .add("EPICS_PVA_SERVER_PORT", conf->getSubFieldT<pvd::PVScalar>("serverport")->getAs<pvd::uint16>())
                                          .add("EPICS_PVA_BROADCAST_PORT", conf->getSubFieldT<pvd::PVScalar>("bcastport")->getAs<pvd::uint16>())
                                          .add("EPICS_PVA_DEBUG", arg.debug>=5 ? 5 : 0)
@@ -163,7 +163,7 @@ pva::ServerContext::shared_pointer configure_server(ServerConfig& arg, const pvd
     pva::Configuration::shared_pointer C(pva::ConfigurationBuilder()
                                          .add("EPICS_PVAS_INTF_ADDR_LIST", conf->getSubFieldT<pvd::PVString>("interface")->get())
                                          .add("EPICS_PVAS_BEACON_ADDR_LIST", conf->getSubFieldT<pvd::PVString>("addrlist")->get())
-                                         .add("EPICS_PVAS_AUTO_BEACON_ADDR_LIST", conf->getSubFieldT<pvd::PVBoolean>("autoaddrlist")->get())
+                                         .add("EPICS_PVAS_AUTO_BEACON_ADDR_LIST", conf->getSubFieldT<pvd::PVScalar>("autoaddrlist")->getAs<std::string>())
                                          .add("EPICS_PVAS_SERVER_PORT", conf->getSubFieldT<pvd::PVScalar>("serverport")->getAs<pvd::uint16>())
                                          .add("EPICS_PVAS_BROADCAST_PORT", conf->getSubFieldT<pvd::PVScalar>("bcastport")->getAs<pvd::uint16>())
                                          .add("EPICS_PVA_DEBUG", arg.debug>=5 ? 5 : 0)

Obligatory reminder that the p2p gateway is deprecated in favor of p4p.gw.

mdavidsaver commented 4 years ago

Patch edited.

anjohnson commented 4 years ago

Obligatory reminder that the p2p gateway is deprecated in favor of p4p.gw

I can't find any announcement to that effect on tech-talk or in the pva2pva release notes. When and where was any notice of that deprecation published to the community, and do you have any plans or timetable to remove the p2p gateway code from pva2pva at all?

mdavidsaver commented 4 years ago

https://epics.anl.gov/tech-talk/2019/msg01896.php

This PVA gateway is intended to replace the prototype gateway from the pva2pva module.

mdavidsaver commented 4 years ago

I don't have any specific timetable. This prototype will continue to work as well as it has. "deprecation" in this context mainly means I'm not going to be spending much time on it. This hasn't been much of an issue since I announced p4p.gw in Dec. as, excepting this issue, no one has asked me to spend time on it.

I'm happy to remove it though.

anjohnson commented 4 years ago

Removing it will cause problems for both ITER and APS, I believe both are using it in close-to-production-level systems, so I think both Ralph and I would appreciate it if you don't do that yet. Announcing a replacement isn't quite the same as deprecating what it's intended to replace, as I demonstrated by not finding any notices to that effect.

ralphlange commented 4 years ago

Bertrand will test that fix. I can't introduce a new product before the next release (Feb 2021), and the current pva2pva - prototype or not, as it was the only available PVA gateway two years ago - is in our "stable" release and will be used in ITER I&C Integration (delivery and acceptance testing) for at least two more years.

mdavidsaver commented 4 years ago

Ok... Shifting to a more productive line. How do you think this deprecation should be communicated?

ralphlange commented 4 years ago

Make users type "I have read and fully understood that this is a prototype and has been replaced by a different, final, improved product." as mandatory part of the build.

mdavidsaver commented 4 years ago

Be careful. I am considering p2p --ack-deprecation ...

anjohnson commented 4 years ago

There's letting users know, and then there's being gratuitous about it. If you insist, how about requiring a setting in either the configure/CONFIG_SITE file or on the command-line for it to build, and have the build display a warning if it's not set:

$ make -s
Makefile:10: The p2p Gateway is deprecated and not being built.
Makefile:10: Set BUILD_P2P=YES to build it anyway, or use p4p.gw
$ make -s runtests
Makefile:10: The p2p Gateway is deprecated and not being built.
Makefile:10: Set BUILD_P2P=YES to build it anyway, or use p4p.gw
testweak.t ......... ok     
testtest.t ......... ok     
testpvif.t ......... ok     
testpdb.t .......... ok     
testpvalink.t ...... ok     
testgroupconfig.t .. ok     
testdbf_copy.t ..... ok     
All tests successful.

Test Summary Report
-------------------
testpvif.t       (Wstat: 0 Tests: 88 Failed: 0)
  TODO passed:   35
Files=7, Tests=342,  2 wallclock secs ( 0.10 usr  0.03 sys +  0.20 cusr  0.09 csys =  0.42 CPU)
Result: PASS
$ make -s BUILD_P2P=YES
...
7 warnings generated.
woz$ git diff
diff --git a/Makefile b/Makefile
index aa56547..c254059 100644
--- a/Makefile
+++ b/Makefile
@@ -7,12 +7,16 @@ DIRS += configure
 DIRS += $(wildcard *App)
 DIRS += $(wildcard iocBoot)

+DIRS := $(if $(BUILD_P2P), $(DIRS), $(filter-out p2pApp, $(DIRS)) \
+       $(warning The p2p Gateway is deprecated and not being built.) \
+       $(warning Set BUILD_P2P=YES to build it anyway, or use p4p.gw))
+
 # pdbApp depends on configure for CONFIG_QSRV_VERSION
 pdbApp_DEPEND_DIRS = configure

 # iocBoot depends on all *App dirs
 iocBoot_DEPEND_DIRS += $(filter %App,$(DIRS))
-testApp_DEPEND_DIRS += p2pApp pdbApp
+testApp_DEPEND_DIRS += pdbApp

 # Add any additional dependency rules here:

It seems that testApp doesn't actually depend on p2pApp after all.

mdavidsaver commented 4 years ago

Bertrand will test that fix.

@ralphlange Any result?

ralphlange commented 4 years ago

I'll ping him Monday.

ralphlange commented 4 years ago

Bertrand tested and recommends the patch:

I have deployed the fix. As far as I can make out, the behaviour is expected. The PVA is configured to broadcast exclusively on one interface ADDR_LIST= and AUTO_ADDR_LIST=no.

I do not see name resolution traffic on other interfaces as I reported we had with the CCSv6.1.2 genuine version.

This is the only configuration I have tested obviously .. AUTO_ADDR_LIST=no .. I presume the default behaviour itself is part of the regular tests Michael runs.

Safe to commit the fix.

mdavidsaver commented 4 years ago

Fixed with 68708ff530579c6b1fff856d000a489f1ab64990

mdavidsaver commented 4 years ago

Released in module version 1.2.3 with Base 7.0.4