Re: [PATCH] detect and report qemu-dm failure

["Daniel P. Berrange" <berrange@redhat.com>]

It seems rather complicated, but I can't think of a much better way to deal
with it, so just a handful of minor nit-picks inline...

On Thu, Jun 12, 2008 at 12:23:06PM +0100, Ian Jackson wrote:
> diff --exclude=.hg --exclude=.config --exclude='*.old' --exclude='*.orig' --exclude='*~' -ruN ../xen-unstable-3.hg/tools/python/xen/xend/XendDomain.py tools/python/xen/xend/XendDomain.py
> --- ../xen-unstable-3.hg/tools/python/xen/xend/XendDomain.py	2008-06-12 12:13:04.000000000 +0100
> +++ tools/python/xen/xend/XendDomain.py	2008-06-11 16:42:07.000000000 +0100
> @@ -34,7 +34,7 @@
>  
>  from xen.xend import XendOptions, XendCheckpoint, XendDomainInfo
>  from xen.xend.PrettyPrint import prettyprint
> -from xen.xend import XendConfig
> +from xen.xend import XendConfig, image
>  from xen.xend.XendError import XendError, XendInvalidDomain, VmError
>  from xen.xend.XendError import VMBadState
>  from xen.xend.XendLogging import log
> @@ -179,6 +179,8 @@
>                          log.exception("Failed to create reference to running "
>                                        "domain id: %d" % dom['domid'])
>  
> +	    image.cleanup_stale_sentinel_fifos()
> +
>              # add all managed domains as dormant domains.
>              for dom in managed:


This indentation looks wrong to me.


> diff --exclude=.hg --exclude=.config --exclude='*.old' --exclude='*.orig' --exclude='*~' -ruN ../xen-unstable-3.hg/tools/python/xen/xend/XendDomainInfo.py tools/python/xen/xend/XendDomainInfo.py
> --- ../xen-unstable-3.hg/tools/python/xen/xend/XendDomainInfo.py	2008-06-12 12:13:04.000000000 +0100
> +++ tools/python/xen/xend/XendDomainInfo.py	2008-06-11 15:02:43.000000000 +0100
> @@ -30,6 +30,7 @@
>  import re
>  import copy
>  import os
> +import fcntl
>  import traceback
>  from types import StringTypes
>  


This this change intended ?  You're not adding any calls to fcntl in this patch.

> diff --exclude=.hg --exclude=.config --exclude='*.old' --exclude='*.orig' --exclude='*~' -ruN ../xen-unstable-3.hg/tools/python/xen/xend/image.py tools/python/xen/xend/image.py
> --- ../xen-unstable-3.hg/tools/python/xen/xend/image.py	2008-06-12 12:13:04.000000000 +0100
> +++ tools/python/xen/xend/image.py	2008-06-12 12:16:14.000000000 +0100
>  
>  
>          null = os.open("/dev/null", os.O_RDONLY)
> -        logfd = os.open(logfile, os.O_WRONLY|os.O_CREAT|os.O_TRUNC)
> -        
> +        logfd = os.open(self.logfile, os.O_WRONLY|os.O_CREAT|os.O_TRUNC|os.O_APPEND)
> +

Don't need to add O_APPEND, since we're truncating the file.

> @@ -356,18 +389,26 @@
>                  os.dup2(logfd, 2)
>                  os.close(null)
>                  os.close(logfd)
> +                self.sentinel_fifo.close()
>                  try:
>                      os.execve(self.device_model, args, env)
> -                except:
> -                    os._exit(127)
> +                except Exception, e:
> +                    print >>sys.stderr, (
> +                        'failed to set up fds or execute dm %s: %s' %
> +                        (self.device_model, utils.exception_string(e)))
> +                    os._exit(126)
>              except:
>                  os._exit(127)
>          else:
>              self.pid = pid
>              os.close(null)
>              os.close(logfd)
> +	sentinel_write.close()
>          self.vm.storeDom("image/device-model-pid", self.pid)
>          log.info("device model pid: %d", self.pid)

Broken indentation here. 

>      def destroyDeviceModel(self):
>          if self.device_model is None:
>              return
> +        self

Typo, I guess ?

Regards,
Daniel.
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|