[virt-tools-list] [virt-manager 2/3] virt-manager: add support for adding panic device

[Chen Hanxiao]

> -----Original Message-----
> From: Cole Robinson [mailto:crobinso at redhat.com]
> Sent: Tuesday, January 07, 2014 4:32 AM
> To: Chen Hanxiao; virt-tools-list at redhat.com
> Subject: Re: [virt-tools-list] [virt-manager 2/3] virt-manager: add
support for
> adding panic device
> 
> On 01/06/2014 03:04 AM, Chen Hanxiao wrote:
> > From: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> >
> > Signed-off-by: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> > ---
> >  ui/addhardware.ui          | 76
> ++++++++++++++++++++++++++++++++++++++++++++--
> >  virtManager/addhardware.py | 42 ++++++++++++++++++++++++-
> >  2 files changed, 115 insertions(+), 3 deletions(-)
> >
> 
> A played with this lightly. A few general comments:
> 
> - UI issues: the labels should be capitalized correctly, left aligned (set
> xalign to 0), and use underline/mnemonics.

v2 will change.

> 
> - Change the UI name of the device from 'PANIC' to 'Panic Notifier'. Might
> need to do the same in the other virt-manager patch
> 
v2 will change.

> - What's the point of iobase? When will a user ever want to change it? If
it's
> only rarely used, we might consider dropping the UI field for it.

If some device port conflict with pvpanic device, this para could help.
Consider that we already set the default value for it, I think keeping it
would not bring
troubles to users.

> 
> Thanks,
> Cole