Re: [PATCH][Take 2] VNC authentification

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

On Fri, Sep 29, 2006 at 05:47:34PM +0900, Masami Watanabe wrote:
> Hi,
> 
> This is take 2 on VNC authentification.
> 
> The specification is as mentioned at
> http://lists.xensource.com/archives/html/xen-devel/2006-09/msg00666.html
> The difference is follows.
> - correction that passes information through xenstore.
> - after information is read, qemu deletes information on xenstore.

This patch doesn't compile because it is calling functions before
they are even defined & the implicit definition this results in does
not match the actual definition later

xen-3.0.3-testing-11633/tools/ioemu/vnc.c: In function 'protocol_version':
xen-3.0.3-testing-11633/tools/ioemu/vnc.c:1225: warning: implicit declaration of function 'vnc_auth'
xen-3.0.3-testing-11633/tools/ioemu/vnc.c: In function 'protocol_response':
xen-3.0.3-testing-11633/tools/ioemu/vnc.c:1250: warning: implicit declaration of function 'base64decode'
xen-3.0.3-testing-11633/tools/ioemu/vnc.c: At top level:
xen-3.0.3-testing-11633/tools/ioemu/vnc.c:1399: error: static declaration of 'vnc_auth' follows non-static declaration
xen-3.0.3-testing-11633/tools/ioemu/vnc.c:1225: error: previous implicit declaration of 'vnc_auth' was here
xen-3.0.3-testing-11633/tools/ioemu/vnc.c: In function 'vnc_auth':
xen-3.0.3-testing-11633/tools/ioemu/vnc.c:1417: warning: implicit declaration of function 'make_challenge'
xen-3.0.3-testing-11633/tools/ioemu/vnc.c: At top level:
xen-3.0.3-testing-11633/tools/ioemu/vnc.c:1429: error: static declaration of 'make_challenge' follows non-static declaration
xen-3.0.3-testing-11633/tools/ioemu/vnc.c:1417: error: previous implicit declaration of 'make_challenge' was here
xen-3.0.3-testing-11633/tools/ioemu/vnc.c:1461: error: static declaration of 'base64decode' follows non-static declaration
xen-3.0.3-testing-11633/tools/ioemu/vnc.c:1250: error: previous implicit declaration of 'base64decode' was here


> diff -r 1d0e75523636 tools/python/xen/xend/XendRoot.py
> --- a/tools/python/xen/xend/XendRoot.py Wed Sep 27 17:49:22 2006 +0100
> +++ b/tools/python/xen/xend/XendRoot.py Fri Sep 29 15:46:03 2006 +0900
> @@ -95,6 +95,8 @@ class XendRoot:
>      dom0_min_mem_default = '0'
>  
>      dom0_vcpus_default = '0'
> +
> +    vncpasswd_default = '#None#'

Why not just use the proper Python None value ?



> diff -r 1d0e75523636 tools/python/xen/xend/XendDomainInfo.py
> --- a/tools/python/xen/xend/XendDomainInfo.py   Wed Sep 27 17:49:22 2006 +0100
> +++ b/tools/python/xen/xend/XendDomainInfo.py   Fri Sep 29 15:46:03 2006 +0900
> @@ -391,6 +391,9 @@ def parseConfig(config):
>          else:
>              log.warn("Ignoring malformed and deprecated config option "
>                       "restart = %s", restart)
> +
> +    result['image'].append(
> +        ['vncpasswd_default', xroot.get_vncpasswd_default()])

Why put the system default password into the guest's config record here,
only to remove it again later on. The XendRoot object is a singleton, so
better to just call 'get_vncpasswd_default' when you need it later.... 


> diff -r 1d0e75523636 tools/python/xen/xend/image.py
> --- a/tools/python/xen/xend/image.py    Wed Sep 27 17:49:22 2006 +0100
> +++ b/tools/python/xen/xend/image.py    Fri Sep 29 15:46:03 2006 +0900
> @@ -348,8 +348,20 @@ class HVMImageHandler(ImageHandler):
>          sdl = sxp.child_value(config, 'sdl')
>          ret = []
>          nographic = sxp.child_value(config, 'nographic')
> +
> +        # get password from xend-config(if password omitted, '#None#')
> +        vncpasswd_default = sxp.child_value(config,
> +                                            'vncpasswd_default')

Just call

    vncpassword_default = xen.xend.XendRoot.instance().get_vncpasswd_default()

Then there is no need to keep 'vncpasswd_default' in the guest's sxp at all.

> +        # get password from VM config(if password omitted, None)
> +        vncpasswd_vmconfig = sxp.child_value(config, 'vncpasswd')
> +        vncpasswd = vncpasswd_vmconfig
> +
>          if nographic:
>              ret.append('-nographic')
> +            # remove password
> +            if vncpasswd_vmconfig:
> +                config.remove(['vncpasswd', vncpasswd_vmconfig])
> +            del config[config.index(['vncpasswd_default', vncpasswd_default])]
>              return ret
>          if vnc:
>              vncdisplay = sxp.child_value(config, 'vncdisplay',
> @@ -358,6 +370,19 @@ class HVMImageHandler(ImageHandler):
>              vncunused = sxp.child_value(config, 'vncunused')
>              if vncunused:
>                  ret += ['-vncunused']
> +            # password check
> +            if vncpasswd is None:
> +                if vncpasswd_default=='#None#':
> +                    raise VmError('vncpasswd is not setuped in VMconfig and xend-config.')
> +                else:
> +                    vncpasswd = vncpasswd_default

This should just do  'if vncpasswd_default is None', rather than use some
magic string value '#None#'

> +            if vncpasswd!='':
> +                self.vm.storeVm("vncpasswd", vncpasswd)
> +
> +        # remove password
> +        config.remove(['vncpasswd', vncpasswd_vmconfig])
> +        del config[config.index(['vncpasswd_default', vncpasswd_default])]
> +
>          return ret
>  
>      def createDeviceModel(self):

See comment above about not storing 'vncpasswd_default' in this object 
at all.


Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|