[libvirt] [PATCH] remote_daemon: Log host boot time

[Michal Prívozník]

On 12/18/19 11:07 PM, Cole Robinson wrote:
> On 12/17/19 8:25 AM, Michal Privoznik wrote:
>> This is not strictly needed, but it makes sure we initialize the
>> @bootTime global variable. Thing is, in order to validate XATTRs
>> and prune those set in some previous runs of the host, a
>> timestamp is recorded in XATTRs. The host boot time was unique
>> enough so it was chosen as the timestamp value. And to avoid
>> querying and parsing /proc/uptime every time, the query function
>> does that only once and stores the boot time in a global
>> variable. However, the only time the query function is called is
>> in a child process that does lock files and changes seclabels. So
>> effectively, we are doing exactly what we wanted to prevent from
>> happening.
>>
>> The fix is simple, call the query function whilst the daemon is
>> initializing.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>
>> Since this will be used by security driver only, I was tempted to put
>> this somewhere under src/security/, but especially because of split
>> daemon I didn't. Placing this into remote_daemon.c makes sure all
>> sub-daemons will have the variable initialized and won't suffer the
>> problem.
>>
>>  src/remote/remote_daemon.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
>> index b400b1dd10..5debb6ce97 100644
>> --- a/src/remote/remote_daemon.c
>> +++ b/src/remote/remote_daemon.c
>> @@ -57,6 +57,7 @@
>>  #include "virgettext.h"
>>  #include "util/virnetdevopenvswitch.h"
>>  #include "virsystemd.h"
>> +#include "virhostuptime.h"
>>  
>>  #include "driver.h"
>>  
>> @@ -1020,6 +1021,7 @@ int main(int argc, char **argv) {
>>      bool implicit_conf = false;
>>      char *run_dir = NULL;
>>      mode_t old_umask;
>> +    unsigned long long bootTime;
>>  
>>      struct option opts[] = {
>>          { "verbose", no_argument, &verbose, 'v'},
>> @@ -1151,6 +1153,12 @@ int main(int argc, char **argv) {
>>          exit(EXIT_FAILURE);
>>      }
>>  
>> +    if (virHostGetBootTime(&bootTime) < 0) {
>> +        VIR_DEBUG("Unable to get host boot time");
>> +    } else {
>> +        VIR_DEBUG("host boot time: %lld", bootTime);
>> +    }
>> +
> 
> Please add a comment that this is initializing some global state that we
> need elsewhere, because otherwise it won't be obvious why this is here.
> With that:
> 
> Reviewed-by: Cole Robinson <crobinso at redhat.com>
> 
> Better IMO would be have an explicit virHostBootTimeInit function, and
> any usage of GetBootTime would fail if the init hasn't been called yet.
> It would make this case more self descriptive, and make sure any new
> users aren't doing it wrong

Agreed, patches proposed here:

https://www.redhat.com/archives/libvir-list/2019-December/msg01244.html

Michal