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@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);
+    }
+
     daemonSetupNetDevOpenvswitch(config);
 
     if (daemonSetupAccessManager(config) < 0) {
-- 
2.24.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@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

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list