On Fri, Aug 05, 2011 at 05:44:38PM +0100, Guido Trotter wrote:
> On Fri, Aug 5, 2011 at 5:37 PM, Iustin Pop <[email protected]> wrote:
>
> >> + elif netutils.IP6Address.IsValid(spice_bind):
> >> + if spice_ip_version != constants.IP6_VERSION:
> >> + raise errors.HypervisorError("spice: got an IPv6 address (%s),
> >> but"
> >> + " the specified IP version is %d."
> >> + % (spice_bind, spice_ip_version))
> >> +
> >> + elif netutils.IsValidInterface(spice_bind):
> >> + addresses = netutils.GetInterfaceIpAddresses(spice_bind,
> >> + spice_ip_version)
> >> + if not addresses:
> >> + raise errors.HypervisorError("spice: unable to get an IPv%d
> >> address"
> >> + " for %s" % (spice_ip_version,
> >> + spice_bind))
> >
> > And this.
>
> No this check needs to stay here (as well) as otherwise if it fails
> later there's no good error.
Here as well, yes, but we also need to check it before, so that cluster
verify checks this.
> Also if it's checked also before let's make sure it's checked on the
> node and not on the master.
I'm not sure we want (inside a node group) to allow non-homogeneous
nodes. We could, but this needs to be thought in a bigger context, not
just for Spice, I think.
> >> + spice_bind = addresses[0]
> >> +
> >> + else:
> >> + raise errors.HypervisorError("spice: the %s parameter must be
> >> either a"
> >> + " valid IP address or interface
> >> name" %
> >> + constants.HV_KVM_SPICE_BIND)
> >> +
> >> +
> >> + spice_arg = "addr=%s,ipv%d,port=%d" % (spice_bind,
> >> + spice_ip_version,
> >> + instance.network_port)
> >> +
> >> + spice_pwd_file = hvp[constants.HV_KVM_SPICE_PASSWORD_FILE]
> >> + if spice_pwd_file:
> >> + try:
> >> + spice_pwd = utils.ReadOneLineFile(spice_pwd_file, strict=True)
> >> + spice_arg = "%s,password=%s" % (spice_arg, spice_pwd)
> >> + except EnvironmentError, err:
> >> + raise errors.HypervisorError("spice: failed to open the
> >> password"
> >> + " file %s: %s" % (spice_pwd_file,
> >> err))
> >
> > I think this too.
> >
>
> Same here. What if the file has disappeared in the meantime?
Here *too*, if you want—but this stuff should be checked at cluster
verify too, hence it needs to be checked in ValidateParameters.
One could simply call ValidateParameters at the start of this function,
so that there's no code duplication.
thanks!
iustin
