Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-150839902
ink_file_lmtime
- Should be named "ink_file_time".
- I don't see why we need to use ink_timezone() here, or anywhere else for
that matter.
- This should return ink_hrtime (using ink_hrtime_from_timespec) rather
than time_t.
If you make these ink_file_mtime changes and update Rollback to use
ink_file_mtime, I'll merge that as a separate patch.
This change crashes traffic_manager every time for me, with the following
backtrace:
(lldb) bt
* thread #2: tid = 0xe4f28, 0x000000010003b7eb
traffic_manager`LocalManager::signalFileChange(this=0x0000000100a26680,
var_name="proxy.config.ssl.server.multicert.filename", incVersion=true) + 27 at
LocalManager.cc:769, stop reason = breakpoint 1.1
* frame #0: 0x000000010003b7eb
traffic_manager`LocalManager::signalFileChange(this=0x0000000100a26680,
var_name="proxy.config.ssl.server.multicert.filename", incVersion=true) + 27 at
LocalManager.cc:769
frame #1: 0x0000000100004ad1
traffic_manager`fileUpdated(fname="ssl_multicert.config", incVersion=false) +
993 at traffic_manager.cc:941
frame #2: 0x000000010003700a
traffic_manager`FileManager::fileChanged(this=0x0000000100a270a0,
baseFileName="ssl_multicert.config", incVersion=false) + 122 at
FileManager.cc:189
frame #3: 0x000000010004109e
traffic_manager`Rollback::checkForUserUpdate(this=0x0000000100a27ce0,
how=ROLLBACK_CHECK_ONLY) + 510 at Rollback.cc:928
frame #4: 0x0000000100038458
traffic_manager`FileManager::isConfigStale(this=0x0000000100a270a0) + 120 at
FileManager.cc:657
frame #5: 0x000000010008a378
traffic_manager`sync_thr(data=0x0000000100a270a0) + 376 at RecLocal.cc:97
frame #6: 0x00007fff915469b1 libsystem_pthread.dylib`_pthread_body
+ 131
frame #7: 0x00007fff9154692e libsystem_pthread.dylib`_pthread_start
+ 168
frame #8: 0x00007fff91544385 libsystem_pthread.dylib`thread_start +
13
It looks like this is a side-effect of always sending events.
listened_var_name is an array literal, so you can always calculate
its size at compile time using countof(). Use this for iteration
and in place of listened_var_name_size.
SSLCertificateConfig::need_reconfigure() should not have the
side-effect of triggerring a reconfiguration.
There are various problems with SSLCertificateConfig::FileInfo and VarInfo:
- set_value() should not be a separate call, do it in the constructor
- check_value() should be called "bool changed() const;"
We should not need the static data in SSLCertificateConfig.
In general, I don't like this approach. The additional config files for SSL
configuration (if they are handled at all), should be handled like other
configuration files by the Rollback mechanics. I certainly could see that
you
don't want to create previous versions of keys, however, which makes me
winder
whether these really should be treated like config files?
It looks like you handle the additional files for a SSL context can
be specified in a ssl_multicert.config entry by collecting all the
file names into the SSLCertificateConfig static data and then always
triggerring a reload event from traffic_manager. I think that this
approach adds complexity to the SSL subsystem, which is already far
to complex.
A better approach to explore is adding the capability for traffic_server
to send a message up to traffic_manager to tell it to watch a set
of files. traffic_manager is already doing this, so much of the
mechanism should be there. This capability could eventually be used
by other subsystems and even by plugins. One tricky aspect of this
approach is finding a clean way to tell traffic_manager to stop
watching a set of files.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
