[virt-tools-list] [virt-manager PATCH] details: add UI interface for LXC user namespace

[Chen Hanxiao]

> -----Original Message-----
> From: Cole Robinson [mailto:crobinso at redhat.com]
> Sent: Thursday, February 20, 2014 12:22 AM
> To: Chen Hanxiao; virt-tools-list at redhat.com
> Subject: Re: [virt-tools-list] [virt-manager PATCH] details: add UI
interface for LXC
> user namespace
> 
> On 02/19/2014 06:34 AM, Chen Hanxiao wrote:
> > We could config user namespace for LXC container
> > in details->overview page.
> >
> > Signed-off-by: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> > ---
> >  ui/details.ui          | 239
> +++++++++++++++++++++++++++++++++++++++++++++++--
> >  virtManager/details.py |  58 +++++++++++-
> >  virtManager/domain.py  |  15 ++++
> >  3 files changed, 305 insertions(+), 7 deletions(-)
> >
> 
> Some UI hints to make things more consistent:
> 
> - Expander label should be bold, and rename it to 'User Namespace'
> - Rename 'Config User Namespace' to 'Enable user namespace'
> - The box/grid under the expander should be inside a GtkAlignment, and
> indented. Copy the indentation of the 'Basic Details' section
> - The fields should be spin boxes.
> - The grid should use column and row spacing of 6, which is the standard
we
> use for most other grids.
> - Capitalize the column names (target, count)
> - 'Config User Namespace' needs to enable the 'apply' button when its
changed.
> - If the VM has an <idmap> block, and I uncheck 'Config User Namespace',
it
> should do idmap.clear() and remove the entire block from the XML.
> 
> > diff --git a/virtManager/details.py b/virtManager/details.py
> > index 72e79da..f4b1339 100644
> > --- a/virtManager/details.py
> > +++ b/virtManager/details.py
> > @@ -103,7 +103,9 @@ EDIT_FS,
> >
> >  EDIT_HOSTDEV_ROMBAR,
> >
> > -) = range(1, 42)
> > +EDIT_IDMAP,
> > +
> > +) = range(1, 43)
> >
> 
> It's a minor point, but move this EDIT_IDMAP definition up near EDIT_NAME
and
> EDIT_DESC, we group the values by what page they appear on.
> 
> >
> >  # Columns in hw list model
> > @@ -579,6 +581,13 @@ class vmmDetails(vmmGObjectUI):
> >              "on_overview_name_changed": lambda *x:
> self.enable_apply(x, EDIT_NAME),
> >              "on_overview_title_changed": lambda *x:
self.enable_apply(x,
> EDIT_TITLE),
> >              "on_machine_type_changed": lambda *x: self.enable_apply(x,
> EDIT_MACHTYPE),
> > +            "on_idmap_uid_start_changed": lambda *x:
> self.enable_apply(x, EDIT_IDMAP),
> > +            "on_idmap_uid_target_changed": lambda *x:
> self.enable_apply(x, EDIT_IDMAP),
> > +            "on_idmap_uid_count_changed": lambda *x:
> self.enable_apply(x, EDIT_IDMAP),
> > +            "on_idmap_gid_start_changed": lambda *x:
> self.enable_apply(x, EDIT_IDMAP),
> > +            "on_idmap_gid_target_changed": lambda *x:
> self.enable_apply(x, EDIT_IDMAP),
> > +            "on_idmap_gid_count_changed": lambda *x:
> self.enable_apply(x, EDIT_IDMAP),
> > +            "on_config_idmap_check_toggled": self.toggle_idmap_check,
> >
> >              "on_config_vcpus_changed": self.config_vcpus_changed,
> >              "on_config_maxvcpus_changed":
> self.config_maxvcpus_changed,
> > @@ -1600,6 +1609,24 @@ class vmmDetails(vmmGObjectUI):
> >          if edittype not in self.active_edits:
> >              self.active_edits.append(edittype)
> >
> > +    def toggle_idmap_check(self, src):
> > +        Name = ["uid-start", "uid-target", "uid-count",
> > +                "gid-start", "gid-target", "gid-count"]
> > +        for name in Name:
> > +            self.widget(name).set_sensitive(src.get_active())
> > +        IdMap = self.vm.get_idmap()
> > +        if IdMap.uid_start is not None:
> > +            for name in Name:
> > +                IdMap_proper = getattr(IdMap, name.replace("-", "_"))
> > +                self.widget(name).set_text(str(IdMap_proper))
> > +        elif src.get_active():
> > +            self.widget("uid-start").set_text('0')
> > +            self.widget("uid-target").set_text('1000')
> > +            self.widget("uid-count").set_text('10')
> > +            self.widget("gid-start").set_text('0')
> > +            self.widget("gid-target").set_text('1000')
> > +            self.widget("gid-count").set_text('10')
> > +
> 
> Hmm, I don't like that this is duplicating behavior in the
refresh_overview
> function.
> 
> I recommend changing the check button to hide the entire idmap grid. Then
you
> don't need to worry about setting values here, just set the default values
in
> refresh_overview if the guest doesn't have any idmap block.
> 
> FYI I'm offline until next week, so I'll do follow up reviews after
Monday.
> 
> - Cole

Will do in v2 patch, thanks for your comments.

PS: have a nice weekend.

-Chen