Created attachment 97166 [details] avoid detecting xen dom0 as virtual machine The current vm detection for xen was introduced in Bug 61491. This implementation lacks the distinction between Xen dom0 and Xen domU. Both, dom0 and domU are running inside the hypervisor. Therefore systemd-detect-virt and the ConditionVirtualization directive detect dom0 as a virtual machine. However, dom0 is not using virtual devices but is accessing the real hardware. Therefore dom0 should be considered the virtualisation host and not a virtual machine. Problems from this arise for example when using ConditionVirtualization in a services file for smartd. It won't check the harddisks on dom0 because of the vm detection. Attached patch solves this.
Andy, you proposed the original change. Would this one work fine for your configuration?
On my Xen 3.4.1 PV DomU /proc/xen/capabilities doesn't exist. /proc/xen exists but it's empty. On another Xen 3.4.2 DomU box (which was HVM one day but now it's either PV or PVHVM and I cannot tell which) /proc/xen/capabilities exists but it's empty.
Hm, I've found that using /proc/xen/capabilities is widely used to identifiy dom0 versus domU. If this is not general enough, there are other possibilities listet at: http://wiki.xenproject.org/wiki/Xen_FAQ_Design_and_in_Depth below: "How can I tell if my OS is running on a physical machine or a virtual machine ?" which one would work for you?
That page seems to be wrong/outdated/incomplete: dmidecode won't work on Xen PV systems as there's no DMI emulation there, /proc/xen/capabilities doesn't always exist, and kernel suffix is unreliable as one can put anything there. I think we should leave old Xen detection code using /sys/hypervisor/type and only add a distinction between xen dom0 and domU using the method proposed in the new patch. In pseudocode: if (/sys/hypervisor/type == 'xen') { # we run Xen, and only need to distinguish between dom0 and domU if (exists /proc/xen/capabilities && /proc/xen/capabilities contains 'control_d') { return 'xen-dom0' } else { return 'xen-domU' } }
Ok, agreed. Will you provide the patch?
I don't have build environment set up. I'm an end user, not a C developer. It should be much easier for you to amend your patch.
Created attachment 97822 [details] [review] avoid detecting xen dom0 as virtual machine if possible Updated patch
Ok, attached the new patch, tested as good as I could. In domU: # cat /proc/xen/capabilities # systemd-detect-virt xen-domU # echo $? 0 # mkdir /tmp/void # mount -o bind /tmp/void/ /proc/xen/ # cat /proc/xen/capabilities cat: /proc/xen/capabilities: No such file or directory # systemd-detect-virt xen # echo $? 0 in dom0: # cat /proc/xen/capabilities control_d # systemd-detect-virt xen-dom0 # echo $? 1 # mkdir /tmp/void # mount -o bind /tmp/void /proc/xen/ # cat /proc/xen/capabilities cat: /proc/xen/capabilities: No such file or directory # systemd-detect-virt xen # echo $? 0
My case where /proc/xen/capabilities doesn't exist is xen-domU too. Unless someone has dom0 with missing /proc/xen/capabilities I think it's better not to create confusion and name both cases "xen-domU" instead of "xen-domU" and "xen" as proposed by the updated patch.
Hm, should we really return xen-domU when we don't know wheter it is domU or dom0? I did the distinction by purpose to signal that it cannot be determined wheter it is domU or dom0. However, it is only a string and if it fits better for cases where /proc/xen/capabilities doesn't exit, we could surely return xen-domU. We just need to be aware that this output will be wrong in a dom0.
I agree that if we are not sure we shouldn't output "xen-dom0". My hypothesis was that /proc/xen/capabilities always exists in dom0. If the hypothesis holds then we do know that it's "xen-domU" and there's no need for separate "xen". If there's a chance for /proc/xen/capabilities not to exist in dom0 then we can leave your patch as is.
In SUSE kernels /proc/xen/capabilities is created in privcmd.c (see https://gitorious.org/opensuse/kernel/source/3f3ae894fe6c750ed2185cfdcc30a20d4becca88:drivers/xen/privcmd/privcmd.c). But it seems that other kernel flavours don't have this. (See for example: http://lists.xen.org/archives/html/xen-devel/2012-09/txtV0mkHOtzj_.txt) In older kernels it was not created automatically, but had to be mounted via xenfs. (See: http://old-list-archives.xenproject.org/xen-users/2009-07/msg00490.html). Not sure wheter some flavours still use this. So, I guess we cannot really rely on the fact that /proc/xen/capabilities is always present in dom0.
ok, then the patch looks good to me.
Please provide a git-format-patch formatted patch. Also, unless > 0 is returned from the call you shouldn't set *id. Please keep variable declarations local to the block you need them in. Checking for the feature with a simple strstr() sounds wrong, you need to check for separators before/behind it too. I see no reason to distuingish "xen" from "xen-domU". We probably shouldn't consider xen-dom0 virtualization at all, and everything else should just be "xen".
Created attachment 100447 [details] updated patch Here is the updated patch. Is it ok like that?
(In reply to comment #15) > Created attachment 100447 [details] > updated patch > > Here is the updated patch. > Is it ok like that? Please follow the usual coding style: no redundant {} in single-line if blocks. Only spaces, no tabs. The strsep loop looks needlessly complex. Why not just use "while ((cap = strsep(...)) { .."? Otherwise looks OK.
Created attachment 100513 [details] [review] new updated patch Thanks for the hints. Here is the new updated patch.
Comment on attachment 100513 [details] [review] new updated patch Review of attachment 100513 [details] [review]: ----------------------------------------------------------------- ::: src/shared/virt.c @@ +170,2 @@ > if (r >= 0) { > + while (cap = strsep(&domcap, ",")) { Shouldn't char * cap = NULL be moved here inside if? @@ +170,3 @@ > if (r >= 0) { > + while (cap = strsep(&domcap, ",")) { > + if (streq(cap, "control_d")) Can this be moved above into while condition? while (cap = strsep(&domcap, ",") && !streq(cap, "control_d")) {
(In reply to comment #17) > Created attachment 100513 [details] [review] [review] > new updated patch > > Thanks for the hints. > Here is the new updated patch. The loop actually skips over every second word now... You want to invoke strsep() only once in each iteration...
Created attachment 100789 [details] [review] patch update 3 Thanks for the hints. Here is the updated patch.
To cut this short I merged this now, fixed the memory leak and whitespace problems, simplified things a bit. Can't test this though, as I don't have any Xen machines around. Please check whether I didn't break antyhing. Thanks.
Created attachment 102974 [details] [review] fix 0004-systemd-detect-virt-only-discover-Xen-domU.patch The conditional for detection xen virtualization contained a little mistake. It is checking for i to be empty: 'if (!i) {', but it must check for cap instead, because: 'cap = strsep(&i, ",")' will set cap to the discovered value and i to the next value after the separator. Hence, i would be empty, if there is only control_d in domcap, leading to a wrong domU detection.
Applied in http://cgit.freedesktop.org/systemd/systemd/commit/?id=a71516dfd1.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.