[PATCH] Clean up dom_get() usage

[PATCH] Clean up dom_get() usage

[Dan Smith]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

This patch adds sanity checks everywhere dom_get() is used.

In the case of XendDomainInfo.update(), if we get None back from
dom_get(), we destroy ourselves.  I believe that this should be the
desired behavior, but arguments to the contrary are welcome.

Signed-off-by: Dan Smith <danms@us.ibm.com>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xend-domget.patch --]
[-- Type: text/x-patch, Size: 1499 bytes --]

diff -r c27431cf81f9 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Thu Sep 15 13:17:24 2005
+++ b/tools/python/xen/xend/XendDomainInfo.py	Thu Sep 15 10:08:16 2005
@@ -117,6 +117,7 @@
     except Exception, err:
         # ignore missing domain
         log.exception("domain_getinfo(%d) failed, ignoring", dom)
+
     return None
 
 class XendDomainInfo:
@@ -356,9 +357,15 @@
         if info:
             self.info = info
         else:
-            di = dom_get(self.domid)
-            if not di:
-                return 
+            self.info = dom_get(self.domid)
+            # If we try to update and fail, we must have been
+            # deleted from the hypervisor
+            if not self.info:
+                log.debug("failed to get info for myself(%s); destroying..."%
+                          self.name)
+                self.destroy()
+                return
+            
         self.memory = self.info['mem_kb'] / 1024
         self.ssidref = self.info['ssidref']
 
@@ -1116,7 +1123,11 @@
                 else:
                     raise
             # get run-time value of vcpus and update store
-            self.configure_vcpus(dom_get(self.domid)['vcpus'])
+            dom0 = dom_get(self.domid)
+            if not dom0:
+                log.error("Failed to get info for dom0 while trying to configure VCPUs!")
+            else:
+                self.configure_vcpus(dom0['vcpus'])
 
 
 def vm_field_ignore(_, _1, _2, _3):

[-- Attachment #3: Type: text/plain, Size: 88 bytes --]


-- 
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms@us.ibm.com

[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [Xen-tools] [PATCH] Clean up dom_get() usage

[Christian Limpach]

On 9/15/05, Dan Smith <danms@us.ibm.com> wrote:

The 1st hunk adds a random empty line, please avoid hunks like that in
the future.

> This patch adds sanity checks everywhere dom_get() is used.

I don't think the 3rd hunk is needed.  It would make more sense to
have a patch which replaces the magic dom0 domid by a global variable
indicating the domid of the domain where xend is running.

> In the case of XendDomainInfo.update(), if we get None back from
> dom_get(), we destroy ourselves.  I believe that this should be the
> desired behavior, but arguments to the contrary are welcome.

It's possibly a sensible behaviour, but I'd like to move away from
code which does stuff as a side effect.  We don't want to end up with
countless places doing random actions just because they happen to
notice a state change first.

     christian

Re: [Xen-tools] [PATCH] Clean up dom_get() usage

[Dan Smith]

CL> I don't think the 3rd hunk is needed.

The temporary variable is a added so that we can verify that dom_get()
returned non-None.  A failure to get the current domain's info from xc
(which could happen) would result in an "unsubscriptable object"
exception in a less-than-obvious place while xend is starting up.

Am I missing something?

CL> It would make more sense to have a patch which replaces the magic
CL> dom0 domid by a global variable indicating the domid of the domain
CL> where xend is running.

The domid in this case isn't magic though, is it?  We just use the
domid that is passed to us, which I think we assume is correctly set
to the privileged domain's ID.

Although the temporary variable is definitely not indicative of this
fact :)

CL> It's possibly a sensible behaviour, but I'd like to move away from
CL> code which does stuff as a side effect.  We don't want to end up
CL> with countless places doing random actions just because they
CL> happen to notice a state change first.

I see your point.  However, I do feel that we should be informative
and defensive where possible.  I think that no matter where it is
detected, the hypervisor reporting a domain is no longer present
should not be ignored.  Very recently, there have been very strange
problems occurring due to stale state that gets left around.

Would it be appropriate to take an action based on what state we think
we're in?  For example, if we were running, mark ourselves as
"crashed"; if we were shutting down, remove ourselves from the list.

-- 
Dan Smith
IBM Linux Technology Center
Open Hypervisor Team
email: danms@us.ibm.com

Re: [Xen-tools] [PATCH] Clean up dom_get() usage

[Christian Limpach]

On 9/16/05, Dan Smith <danms@us.ibm.com> wrote:
> 
> CL> I don't think the 3rd hunk is needed.
> 
> The temporary variable is a added so that we can verify that dom_get()
> returned non-None.  A failure to get the current domain's info from xc
> (which could happen) would result in an "unsubscriptable object"
> exception in a less-than-obvious place while xend is starting up.
> 
> Am I missing something?

How could we not get the current domain's info?

> CL> It would make more sense to have a patch which replaces the magic
> CL> dom0 domid by a global variable indicating the domid of the domain
> CL> where xend is running.
> 
> The domid in this case isn't magic though, is it?  We just use the
> domid that is passed to us, which I think we assume is correctly set
> to the privileged domain's ID.

Actually, the code has also changed a bit with the patch I applied
earlier.  We quite exlicitly now operate on domain with ID 0.

> CL> It's possibly a sensible behaviour, but I'd like to move away from
> CL> code which does stuff as a side effect.  We don't want to end up
> CL> with countless places doing random actions just because they
> CL> happen to notice a state change first.
> 
> I see your point.  However, I do feel that we should be informative
> and defensive where possible.  I think that no matter where it is
> detected, the hypervisor reporting a domain is no longer present
> should not be ignored.  Very recently, there have been very strange
> problems occurring due to stale state that gets left around.

Indeed, I've found that most of the strangeness is caused by random
code mucking about with random information all over the place -- I
don't think we should add any more of that...

    christian