[libvirt] [PATCH v2 0/2] Initialize host boot time global var upfront

This is a v2 for: https://www.redhat.com/archives/libvir-list/2019-December/msg01045.html While technically v1 was fixed I agree with Cole's suggestion, so I'm discarding v1 and sending another version which implements his suggestion. Michal Prívozník (2): virhostuptime: Introduce virHostBootTimeInit() remote_daemon: Initialize host boot time global variable src/libvirt_private.syms | 1 + src/remote/remote_daemon.c | 9 +++++++++ src/util/virhostuptime.c | 13 ++++++++++++- src/util/virhostuptime.h | 3 +++ 4 files changed, 25 insertions(+), 1 deletion(-) -- 2.24.1

The idea is to offer callers an init function that they can call independently to ensure that the global variables get initialized. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virhostuptime.c | 13 ++++++++++++- src/util/virhostuptime.h | 3 +++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7b1ef23bc..257ce00615 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2194,6 +2194,7 @@ virHostMemSetParameters; # util/virhostuptime.h +virHostBootTimeInit; virHostGetBootTime; diff --git a/src/util/virhostuptime.c b/src/util/virhostuptime.c index 8c49c3d40e..6c251c0d2d 100644 --- a/src/util/virhostuptime.c +++ b/src/util/virhostuptime.c @@ -126,7 +126,8 @@ virHostGetBootTimeOnceInit(void) int virHostGetBootTime(unsigned long long *when) { - if (virOnce(&virHostGetBootTimeOnce, virHostGetBootTimeOnceInit) < 0) + if (bootTime == 0 && + virHostBootTimeInit() < 0) return -1; if (bootTimeErrno) { @@ -137,3 +138,13 @@ virHostGetBootTime(unsigned long long *when) *when = bootTime; return 0; } + + +int +virHostBootTimeInit(void) +{ + if (virOnce(&virHostGetBootTimeOnce, virHostGetBootTimeOnceInit) < 0) + return -1; + + return 0; +} diff --git a/src/util/virhostuptime.h b/src/util/virhostuptime.h index 7e2c4c0c81..1ac638fd6e 100644 --- a/src/util/virhostuptime.h +++ b/src/util/virhostuptime.h @@ -25,3 +25,6 @@ int virHostGetBootTime(unsigned long long *when) G_GNUC_NO_INLINE; + +int +virHostBootTimeInit(void); -- 2.24.1

On Thu, Dec 19, 2019 at 12:25:00PM +0100, Michal Privoznik wrote:
The idea is to offer callers an init function that they can call independently to ensure that the global variables get initialized.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virhostuptime.c | 13 ++++++++++++- src/util/virhostuptime.h | 3 +++ 3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7b1ef23bc..257ce00615 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2194,6 +2194,7 @@ virHostMemSetParameters;
# util/virhostuptime.h +virHostBootTimeInit; virHostGetBootTime;
diff --git a/src/util/virhostuptime.c b/src/util/virhostuptime.c index 8c49c3d40e..6c251c0d2d 100644 --- a/src/util/virhostuptime.c +++ b/src/util/virhostuptime.c @@ -126,7 +126,8 @@ virHostGetBootTimeOnceInit(void) int virHostGetBootTime(unsigned long long *when) { - if (virOnce(&virHostGetBootTimeOnce, virHostGetBootTimeOnceInit) < 0) + if (bootTime == 0 && + virHostBootTimeInit() < 0)
The bootTime==0 check is not required & technical a data race. virHostBootTimeInit should be a no-op if it has already been invoked
return -1;
if (bootTimeErrno) { @@ -137,3 +138,13 @@ virHostGetBootTime(unsigned long long *when) *when = bootTime; return 0; } + + +int +virHostBootTimeInit(void) +{ + if (virOnce(&virHostGetBootTimeOnce, virHostGetBootTimeOnceInit) < 0) + return -1; + + return 0; +} diff --git a/src/util/virhostuptime.h b/src/util/virhostuptime.h index 7e2c4c0c81..1ac638fd6e 100644 --- a/src/util/virhostuptime.h +++ b/src/util/virhostuptime.h @@ -25,3 +25,6 @@ int virHostGetBootTime(unsigned long long *when) G_GNUC_NO_INLINE; + +int +virHostBootTimeInit(void); -- 2.24.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

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 virHostBootTimeInit() function which sets the global variable. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index b400b1dd10..27b538b292 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" @@ -1151,6 +1152,14 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } + /* Let's try to initialize global variable that holds the host's boot time. */ + if (virHostBootTimeInit() < 0) { + /* This is acceptable failure. Maybe we won't need the boot time + * anyway, and if we do, then virHostGetBootTime() returns an + * appropriate error. */ + VIR_DEBUG("Ignoring failed boot time init"); + } + daemonSetupNetDevOpenvswitch(config); if (daemonSetupAccessManager(config) < 0) { -- 2.24.1

On 12/19/19 6:24 AM, Michal Privoznik wrote:
This is a v2 for:
https://www.redhat.com/archives/libvir-list/2019-December/msg01045.html
While technically v1 was fixed I agree with Cole's suggestion, so I'm discarding v1 and sending another version which implements his suggestion.
Michal Prívozník (2): virhostuptime: Introduce virHostBootTimeInit() remote_daemon: Initialize host boot time global variable
src/libvirt_private.syms | 1 + src/remote/remote_daemon.c | 9 +++++++++ src/util/virhostuptime.c | 13 ++++++++++++- src/util/virhostuptime.h | 3 +++ 4 files changed, 25 insertions(+), 1 deletion(-)
With the fix Dan pointed out: Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
participants (3)
-
Cole Robinson
-
Daniel P. Berrangé
-
Michal Privoznik