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@xxxxxxxxxx>
>> ---
>>
>> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
