Bug 77271

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.