[virt-tools-list] [virt-manager PATCH 3/3] Rework disk target assignment

[Martin Kletzander]

On Wed, Feb 12, 2014 at 01:41:56PM -0500, Cole Robinson wrote:
> On 02/12/2014 12:18 PM, Martin Kletzander wrote:
> > Using previous patches, this one makes the disk target generation a
> > bit more intelligent, so adding multiple disks with various
> > controllers is not a problem anymore.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1036716
> >
> > Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> > ---
> >  virtManager/addhardware.py | 48 +++++++++++++++++++++++++++++-----------------
> >  virtinst/devicedisk.py     |  3 ++-
> >  2 files changed, 32 insertions(+), 19 deletions(-)
> >
> > diff --git a/virtManager/addhardware.py b/virtManager/addhardware.py
> > index 121c75b..ecb5fc8 100644
> > --- a/virtManager/addhardware.py
> > +++ b/virtManager/addhardware.py
> > @@ -1,5 +1,5 @@
> >  #
> > -# Copyright (C) 2006-2007, 2013, 2014 Red Hat, Inc.
> > +# Copyright (C) 2006-2007, 2012-2014 Red Hat, Inc.
> >  # Copyright (C) 2006 Hugh O. Brock <hbrock at redhat.com>
> >  #
> >  # This program is free software; you can redistribute it and/or modify
> > @@ -20,6 +20,7 @@
> >
> >  import logging
> >  import traceback
> > +import collections
> >
> >  # pylint: disable=E0611
> >  from gi.repository import Gtk
> > @@ -1412,34 +1413,45 @@ class vmmAddHardware(vmmGObjectUI):
> >              self._dev.validate()
> >          return ret
> >
> > -    def _set_disk_controller(self, disk, controller_model):
> > +    def _set_disk_controller(self, disk, controller_model, used_disks):
> >          # Add a SCSI controller with model virtio-scsi if needed
> >          disk.vmm_controller = None
> >          if controller_model != "virtio-scsi":
> > -            return
> > +            return None
> >
> > +        # Get SCSI controllers
> >          controllers = self.vm.get_controller_devices()
> >          ctrls_scsi = [x for x in controllers if
> >                  (x.type == VirtualController.TYPE_SCSI)]
> > -        if len(ctrls_scsi) > 0:
> > -            index_new = max([x.index for x in ctrls_scsi]) + 1
> > -        else:
> > -            index_new = 0
> >
> > +        # Create possible new controller
> >          controller = VirtualController(self.conn.get_backend())
> >          controller.type = "scsi"
> >          controller.model = controller_model
> > -        disk.vmm_controller = controller
> > -        for d in controllers:
> > -            if controller.type == d.type:
> > -                controller.index = index_new
> > -            if controller_model == d.model:
> > -                disk.vmm_controller = None
> > -                controller = d
> > +
> > +        # And set its index
> > +        controller.index = 0
> > +        if ctrls_scsi:
> > +            controller.index = max([x.index for x in ctrls_scsi]) + 1
> > +
> > +        # Take only virtio-scsi ones
> > +        ctrls_scsi = [x for x in ctrls_scsi
> > +                      if x.model == controller_model]
> > +
> > +        # Save occupied places per controller
> > +        occupied = collections.defaultdict(int)
> > +        for d in used_disks:
> > +            if d.bus == disk.bus:
> > +                num = virtinst.VirtualDisk.target_to_num(d.target)
> > +                occupied[num / 7] += 1
> > +        for c in ctrls_scsi:
> > +            if occupied[c.index] < 7:
> > +                controller = c
> >                  break
> > +        else:
> > +            disk.vmm_controller = controller
> >
> > -        disk.address.type = disk.address.ADDRESS_TYPE_DRIVE
> > -        disk.address.controller = controller.index
> > +        return controller.index
> >
> >      def validate_page_storage(self):
> >          bus = self.get_config_disk_bus()
> > @@ -1471,9 +1483,9 @@ class vmmAddHardware(vmmGObjectUI):
> >                  for d in disks:
> >                      used.append(d.target)
> >
> > -                disk.generate_target(used)
> > +            prefer_ctrl = self._set_disk_controller(disk, controller_model, disks)
> > +            disk.generate_target(used, prefer_ctrl)
> >
> > -            self._set_disk_controller(disk, controller_model)
> >          except Exception, e:
> >              return self.err.val_err(_("Storage parameter error."), e)
> >
> > diff --git a/virtinst/devicedisk.py b/virtinst/devicedisk.py
> > index 935add0..28c55ba 100644
> > --- a/virtinst/devicedisk.py
> > +++ b/virtinst/devicedisk.py
> > @@ -480,7 +480,8 @@ class VirtualDisk(VirtualDevice):
> >          Convert disk /dev number (like hda, hdb, hdaa, etc.) to an index
> >          """
> >          num = 0
> > -        if tgt[0] == 'x'
> > +        if tgt[0] == 'x':
> > +            # This case is here for 'xvda'
> >              tgt = tgt[1:]
> >          for i, c in enumerate(reversed(tgt[2:])):
> >              num += (ord(c) - ord('a') + 1) * (26 ** i)
> >
>
> This bit should be squashed into patch #1. ACK series with that change
>
> - Cole

I made a mistake while doing interactive rebase with this as a fixup,
this definitely needs to be in 1/3.

Thanks for the review, pushed now.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20140212/5f8e3326/attachment.sig>