Merge from trunk

4 files changed, 185 insertions, 183 deletions

diff --git a/nova/virt/disk.py b/nova/virt/disk.py
index 2bded07a4..5d499c42c 100644
--- a/nova/virt/disk.py
+++ b/nova/virt/disk.py
@@ -49,10 +49,10 @@ def extend(image, size):
     file_size = os.path.getsize(image)
     if file_size >= size:
         return
-    utils.execute('truncate -s %s %s' % (size, image))
+    utils.execute('truncate', '-s', size, image)
     # NOTE(vish): attempts to resize filesystem
-    utils.execute('e2fsck -fp %s' % image, check_exit_code=False)
-    utils.execute('resize2fs %s' % image, check_exit_code=False)
+    utils.execute('e2fsck', '-fp', image, check_exit_code=False)
+    utils.execute('resize2fs', image, check_exit_code=False)
 
 
 def inject_data(image, key=None, net=None, partition=None, nbd=False):
@@ -68,7 +68,7 @@ def inject_data(image, key=None, net=None, partition=None, nbd=False):
     try:
         if not partition is None:
             # create partition
-            out, err = utils.execute('sudo kpartx -a %s' % device)
+            out, err = utils.execute('sudo', 'kpartx', '-a', device)
             if err:
                 raise exception.Error(_('Failed to load partition: %s') % err)
             mapped_device = '/dev/mapper/%sp%s' % (device.split('/')[-1],
@@ -84,13 +84,14 @@ def inject_data(image, key=None, net=None, partition=None, nbd=False):
                                   mapped_device)
 
         # Configure ext2fs so that it doesn't auto-check every N boots
-        out, err = utils.execute('sudo tune2fs -c 0 -i 0 %s' % mapped_device)
+        out, err = utils.execute('sudo', 'tune2fs',
+                                 '-c', 0, '-i', 0, mapped_device)
 
         tmpdir = tempfile.mkdtemp()
         try:
             # mount loopback to dir
             out, err = utils.execute(
-                    'sudo mount %s %s' % (mapped_device, tmpdir))
+                    'sudo', 'mount', mapped_device, tmpdir)
             if err:
                 raise exception.Error(_('Failed to mount filesystem: %s')
                                       % err)
@@ -103,13 +104,13 @@ def inject_data(image, key=None, net=None, partition=None, nbd=False):
                     _inject_net_into_fs(net, tmpdir)
             finally:
                 # unmount device
-                utils.execute('sudo umount %s' % mapped_device)
+                utils.execute('sudo', 'umount', mapped_device)
         finally:
             # remove temporary directory
-            utils.execute('rmdir %s' % tmpdir)
+            utils.execute('rmdir', tmpdir)
             if not partition is None:
                 # remove partitions
-                utils.execute('sudo kpartx -d %s' % device)
+                utils.execute('sudo', 'kpartx', '-d', device)
     finally:
         _unlink_device(device, nbd)
 
@@ -118,7 +119,7 @@ def _link_device(image, nbd):
     """Link image to device using loopback or nbd"""
     if nbd:
         device = _allocate_device()
-        utils.execute('sudo qemu-nbd -c %s %s' % (device, image))
+        utils.execute('sudo', 'qemu-nbd', '-c', device, image)
         # NOTE(vish): this forks into another process, so give it a chance
         #             to set up before continuuing
         for i in xrange(FLAGS.timeout_nbd):
@@ -127,7 +128,7 @@ def _link_device(image, nbd):
             time.sleep(1)
         raise exception.Error(_('nbd device %s did not show up') % device)
     else:
-        out, err = utils.execute('sudo losetup --find --show %s' % image)
+        out, err = utils.execute('sudo', 'losetup', '--find', '--show', image)
         if err:
             raise exception.Error(_('Could not attach image to loopback: %s')
                                   % err)
@@ -137,10 +138,10 @@ def _link_device(image, nbd):
 def _unlink_device(device, nbd):
     """Unlink image from device using loopback or nbd"""
     if nbd:
-        utils.execute('sudo qemu-nbd -d %s' % device)
+        utils.execute('sudo', 'qemu-nbd', '-d', device)
         _free_device(device)
     else:
-        utils.execute('sudo losetup --detach %s' % device)
+        utils.execute('sudo', 'losetup', '--detach', device)
 
 
 _DEVICES = ['/dev/nbd%s' % i for i in xrange(FLAGS.max_nbd_devices)]
@@ -170,11 +171,12 @@ def _inject_key_into_fs(key, fs):
     fs is the path to the base of the filesystem into which to inject the key.
     """
     sshdir = os.path.join(fs, 'root', '.ssh')
-    utils.execute('sudo mkdir -p %s' % sshdir)  # existing dir doesn't matter
-    utils.execute('sudo chown root %s' % sshdir)
-    utils.execute('sudo chmod 700 %s' % sshdir)
+    utils.execute('sudo', 'mkdir', '-p', sshdir)  # existing dir doesn't matter
+    utils.execute('sudo', 'chown', 'root', sshdir)
+    utils.execute('sudo', 'chmod', '700', sshdir)
     keyfile = os.path.join(sshdir, 'authorized_keys')
-    utils.execute('sudo tee -a %s' % keyfile, '\n' + key.strip() + '\n')
+    utils.execute('sudo', 'tee', '-a', keyfile,
+            process_input='\n' + key.strip() + '\n')
 
 
 def _inject_net_into_fs(net, fs):
@@ -183,8 +185,8 @@ def _inject_net_into_fs(net, fs):
     net is the contents of /etc/network/interfaces.
     """
     netdir = os.path.join(os.path.join(fs, 'etc'), 'network')
-    utils.execute('sudo mkdir -p %s' % netdir)  # existing dir doesn't matter
-    utils.execute('sudo chown root:root %s' % netdir)
-    utils.execute('sudo chmod 755 %s' % netdir)
+    utils.execute('sudo', 'mkdir', '-p', netdir)  # existing dir doesn't matter
+    utils.execute('sudo', 'chown', 'root:root', netdir)
+    utils.execute('sudo', 'chmod', 755, netdir)
     netfile = os.path.join(netdir, 'interfaces')
-    utils.execute('sudo tee %s' % netfile, net)
+    utils.execute('sudo', 'tee', netfile, net)
diff --git a/nova/virt/images.py b/nova/virt/images.py
index 7a6fef330..2e3f2ee4d 100644
--- a/nova/virt/images.py
+++ b/nova/virt/images.py
@@ -28,29 +28,32 @@ import time
 import urllib2
 import urlparse
 
+from nova import context
 from nova import flags
 from nova import log as logging
 from nova import utils
 from nova.auth import manager
 from nova.auth import signer
-from nova.objectstore import image
 
 
 FLAGS = flags.FLAGS
-flags.DEFINE_bool('use_s3', True,
-                  'whether to get images from s3 or use local copy')
-
 LOG = logging.getLogger('nova.virt.images')
 
 
-def fetch(image, path, user, project):
-    if FLAGS.use_s3:
-        f = _fetch_s3_image
-    else:
-        f = _fetch_local_image
-    return f(image, path, user, project)
+def fetch(image_id, path, _user, _project):
+    # TODO(vish): Improve context handling and add owner and auth data
+    #             when it is added to glance.  Right now there is no
+    #             auth checking in glance, so we assume that access was
+    #             checked before we got here.
+    image_service = utils.import_object(FLAGS.image_service)
+    with open(path, "wb") as image_file:
+        elevated = context.get_admin_context()
+        metadata = image_service.get(elevated, image_id, image_file)
+    return metadata
 
 
+# NOTE(vish): The methods below should be unnecessary, but I'm leaving
+#             them in case the glance client does not work on windows.
 def _fetch_image_no_curl(url, path, headers):
     request = urllib2.Request(url)
     for (k, v) in headers.iteritems():
@@ -94,8 +97,7 @@ def _fetch_s3_image(image, path, user, project):
             cmd += ['-H', '\'%s: %s\'' % (k, v)]
 
         cmd += ['-o', path]
-        cmd_out = ' '.join(cmd)
-        return utils.execute(cmd_out)
+        return utils.execute(*cmd)
 
 
 def _fetch_local_image(image, path, user, project):
@@ -103,13 +105,15 @@ def _fetch_local_image(image, path, user, project):
     if sys.platform.startswith('win'):
         return shutil.copy(source, path)
     else:
-        return utils.execute('cp %s %s' % (source, path))
+        return utils.execute('cp', source, path)
 
 
 def _image_path(path):
     return os.path.join(FLAGS.images_path, path)
 
 
+# TODO(vish): xenapi should use the glance client code directly instead
+#             of retrieving the image using this method.
 def image_url(image):
     if FLAGS.image_service == "nova.image.glance.GlanceImageService":
         return "http://%s:%s/images/%s" % (FLAGS.glance_host,
diff --git a/nova/virt/libvirt_conn.py b/nova/virt/libvirt_conn.py
index 9f7315c17..61ef256f9 100644
--- a/nova/virt/libvirt_conn.py
+++ b/nova/virt/libvirt_conn.py
@@ -44,9 +44,8 @@ import uuid
 from xml.dom import minidom
 
 
-from eventlet import greenthread
-from eventlet import event
 from eventlet import tpool
+from eventlet import semaphore
 
 import IPy
 
@@ -57,7 +56,6 @@ from nova import flags
 from nova import log as logging
 #from nova import test
 from nova import utils
-#from nova.api import context
 from nova.auth import manager
 from nova.compute import instance_types
 from nova.compute import power_state
@@ -439,8 +437,10 @@ class LibvirtConnection(object):
 
         if virsh_output.startswith('/dev/'):
             LOG.info(_("cool, it's a device"))
-            out, err = utils.execute("sudo dd if=%s iflag=nonblock" %
-                                     virsh_output, check_exit_code=False)
+            out, err = utils.execute('sudo', 'dd',
+                                     "if=%s" % virsh_output,
+                                     'iflag=nonblock',
+                                     check_exit_code=False)
             return out
         else:
             return ''
@@ -462,11 +462,11 @@ class LibvirtConnection(object):
         console_log = os.path.join(FLAGS.instances_path, instance['name'],
                                    'console.log')
 
-        utils.execute('sudo chown %d %s' % (os.getuid(), console_log))
+        utils.execute('sudo', 'chown', os.getuid(), console_log)
 
         if FLAGS.libvirt_type == 'xen':
             # Xen is special
-            virsh_output = utils.execute("virsh ttyconsole %s" %
+            virsh_output = utils.execute('virsh', 'ttyconsole',
                                          instance['name'])
             data = self._flush_xen_console(virsh_output)
             fpath = self._append_to_file(data, console_log)
@@ -483,9 +483,10 @@ class LibvirtConnection(object):
                 port = random.randint(int(start_port), int(end_port))
                 # netcat will exit with 0 only if the port is in use,
                 # so a nonzero return value implies it is unused
-                cmd = 'netcat 0.0.0.0 %s -w 1 </dev/null || echo free' % (port)
-                stdout, stderr = utils.execute(cmd)
-                if stdout.strip() == 'free':
+                cmd = 'netcat', '0.0.0.0', port, '-w', '1'
+                try:
+                    stdout, stderr = utils.execute(*cmd, process_input='')
+                except ProcessExecutionError:
                     return port
             raise Exception(_('Unable to find an open port'))
 
@@ -512,7 +513,10 @@ class LibvirtConnection(object):
         subprocess.Popen(cmd, shell=True)
         return {'token': token, 'host': host, 'port': port}
 
-    def _cache_image(self, fn, target, fname, cow=False, *args, **kwargs):
+    _image_sems = {}
+
+    @staticmethod
+    def _cache_image(fn, target, fname, cow=False, *args, **kwargs):
         """Wrapper for a method that creates an image that caches the image.
 
         This wrapper will save the image into a common store and create a
@@ -531,14 +535,21 @@ class LibvirtConnection(object):
             if not os.path.exists(base_dir):
                 os.mkdir(base_dir)
             base = os.path.join(base_dir, fname)
-            if not os.path.exists(base):
-                fn(target=base, *args, **kwargs)
+
+            if fname not in LibvirtConnection._image_sems:
+                LibvirtConnection._image_sems[fname] = semaphore.Semaphore()
+            with LibvirtConnection._image_sems[fname]:
+                if not os.path.exists(base):
+                    fn(target=base, *args, **kwargs)
+            if not LibvirtConnection._image_sems[fname].locked():
+                del LibvirtConnection._image_sems[fname]
+
             if cow:
-                utils.execute('qemu-img create -f qcow2 -o '
-                              'cluster_size=2M,backing_file=%s %s'
-                              % (base, target))
+                utils.execute('qemu-img', 'create', '-f', 'qcow2', '-o',
+                              'cluster_size=2M,backing_file=%s' % base,
+                              target)
             else:
-                utils.execute('cp %s %s' % (base, target))
+                utils.execute('cp', base, target)
 
     def _fetch_image(self, target, image_id, user, project, size=None):
         """Grab image and optionally attempt to resize it"""
@@ -548,7 +559,7 @@ class LibvirtConnection(object):
 
     def _create_local(self, target, local_gb):
         """Create a blank image of specified size"""
-        utils.execute('truncate %s -s %dG' % (target, local_gb))
+        utils.execute('truncate', target, '-s', "%dG" % local_gb)
         # TODO(vish): should we format disk by default?
 
     def _create_image(self, inst, libvirt_xml, suffix='', disk_images=None):
@@ -559,7 +570,7 @@ class LibvirtConnection(object):
                                 fname + suffix)
 
         # ensure directories exist and are writable
-        utils.execute('mkdir -p %s' % basepath(suffix=''))
+        utils.execute('mkdir', '-p', basepath(suffix=''))
 
         LOG.info(_('instance %s: Creating image'), inst['name'])
         f = open(basepath('libvirt.xml'), 'w')
@@ -579,21 +590,23 @@ class LibvirtConnection(object):
                            'ramdisk_id': inst['ramdisk_id']}
 
         if disk_images['kernel_id']:
+            fname = '%08x' % int(disk_images['kernel_id'])
             self._cache_image(fn=self._fetch_image,
                               target=basepath('kernel'),
-                              fname=disk_images['kernel_id'],
+                              fname=fname,
                               image_id=disk_images['kernel_id'],
                               user=user,
                               project=project)
             if disk_images['ramdisk_id']:
+                fname = '%08x' % int(disk_images['ramdisk_id'])
                 self._cache_image(fn=self._fetch_image,
                                   target=basepath('ramdisk'),
-                                  fname=disk_images['ramdisk_id'],
+                                  fname=fname,
                                   image_id=disk_images['ramdisk_id'],
                                   user=user,
                                   project=project)
 
-        root_fname = disk_images['image_id']
+        root_fname = '%08x' % int(disk_images['image_id'])
         size = FLAGS.minimum_root_size
         if inst['instance_type'] == 'm1.tiny' or suffix == '.rescue':
             size = None
@@ -659,7 +672,7 @@ class LibvirtConnection(object):
                         ' data into image %(img_id)s (%(e)s)') % locals())
 
         if FLAGS.libvirt_type == 'uml':
-            utils.execute('sudo chown root %s' % basepath('disk'))
+            utils.execute('sudo', 'chown', 'root', basepath('disk'))
 
     def to_xml(self, instance, rescue=False):
         # TODO(termie): cache?
@@ -1208,10 +1221,14 @@ class NWFilterFirewall(FirewallDriver):
 
 class IptablesFirewallDriver(FirewallDriver):
     def __init__(self, execute=None, **kwargs):
-        self.execute = execute or utils.execute
+        from nova.network import linux_net
+        self.iptables = linux_net.iptables_manager
         self.instances = {}
         self.nwfilter = NWFilterFirewall(kwargs['get_connection'])
 
+        self.iptables.ipv4['filter'].add_chain('sg-fallback')
+        self.iptables.ipv4['filter'].add_rule('sg-fallback', '-j DROP')
+
     def setup_basic_filtering(self, instance):
         """Use NWFilter from libvirt for this."""
         return self.nwfilter.setup_basic_filtering(instance)
@@ -1220,126 +1237,97 @@ class IptablesFirewallDriver(FirewallDriver):
         """No-op. Everything is done in prepare_instance_filter"""
         pass
 
-    def remove_instance(self, instance):
+    def unfilter_instance(self, instance):
         if instance['id'] in self.instances:
             del self.instances[instance['id']]
+            self.remove_filters_for_instance(instance)
+            self.iptables.apply()
         else:
             LOG.info(_('Attempted to unfilter instance %s which is not '
                        'filtered'), instance['id'])
 
-    def add_instance(self, instance):
+    def prepare_instance_filter(self, instance):
         self.instances[instance['id']] = instance
+        self.add_filters_for_instance(instance)
+        self.iptables.apply()
 
-    def unfilter_instance(self, instance):
-        self.remove_instance(instance)
-        self.apply_ruleset()
+    def add_filters_for_instance(self, instance):
+        chain_name = self._instance_chain_name(instance)
 
-    def prepare_instance_filter(self, instance):
-        self.add_instance(instance)
-        self.apply_ruleset()
-
-    def apply_ruleset(self):
-        current_filter, _ = self.execute('sudo iptables-save -t filter')
-        current_lines = current_filter.split('\n')
-        new_filter = self.modify_rules(current_lines, 4)
-        self.execute('sudo iptables-restore',
-                     process_input='\n'.join(new_filter))
-        if(FLAGS.use_ipv6):
-            current_filter, _ = self.execute('sudo ip6tables-save -t filter')
-            current_lines = current_filter.split('\n')
-            new_filter = self.modify_rules(current_lines, 6)
-            self.execute('sudo ip6tables-restore',
-                         process_input='\n'.join(new_filter))
+        self.iptables.ipv4['filter'].add_chain(chain_name)
+        ipv4_address = self._ip_for_instance(instance)
+        self.iptables.ipv4['filter'].add_rule('local',
+                                              '-d %s -j $%s' %
+                                              (ipv4_address, chain_name))
+
+        if FLAGS.use_ipv6:
+            self.iptables.ipv6['filter'].add_chain(chain_name)
+            ipv6_address = self._ip_for_instance_v6(instance)
+            self.iptables.ipv6['filter'].add_rule('local',
+                                                  '-d %s -j $%s' %
+                                                  (ipv6_address,
+                                                   chain_name))
+
+        ipv4_rules, ipv6_rules = self.instance_rules(instance)
+
+        for rule in ipv4_rules:
+            self.iptables.ipv4['filter'].add_rule(chain_name, rule)
+
+        if FLAGS.use_ipv6:
+            for rule in ipv6_rules:
+                self.iptables.ipv6['filter'].add_rule(chain_name, rule)
+
+    def remove_filters_for_instance(self, instance):
+        chain_name = self._instance_chain_name(instance)
+
+        self.iptables.ipv4['filter'].remove_chain(chain_name)
+        if FLAGS.use_ipv6:
+            self.iptables.ipv6['filter'].remove_chain(chain_name)
 
-    def modify_rules(self, current_lines, ip_version=4):
+    def instance_rules(self, instance):
         ctxt = context.get_admin_context()
-        # Remove any trace of nova rules.
-        new_filter = filter(lambda l: 'nova-' not in l, current_lines)
-
-        seen_chains = False
-        for rules_index in range(len(new_filter)):
-            if not seen_chains:
-                if new_filter[rules_index].startswith(':'):
-                    seen_chains = True
-            elif seen_chains == 1:
-                if not new_filter[rules_index].startswith(':'):
-                    break
 
-        our_chains = [':nova-fallback - [0:0]']
-        our_rules = ['-A nova-fallback -j DROP']
-
-        our_chains += [':nova-local - [0:0]']
-        our_rules += ['-A FORWARD -j nova-local']
-        our_rules += ['-A OUTPUT -j nova-local']
-
-        security_groups = {}
-        # Add our chains
-        # First, we add instance chains and rules
-        for instance_id in self.instances:
-            instance = self.instances[instance_id]
-            chain_name = self._instance_chain_name(instance)
-            if(ip_version == 4):
-                ip_address = self._ip_for_instance(instance)
-            elif(ip_version == 6):
-                ip_address = self._ip_for_instance_v6(instance)
-
-            our_chains += [':%s - [0:0]' % chain_name]
-
-            # Jump to the per-instance chain
-            our_rules += ['-A nova-local -d %s -j %s' % (ip_address,
-                                                         chain_name)]
-
-            # Always drop invalid packets
-            our_rules += ['-A %s -m state --state '
-                          'INVALID -j DROP' % (chain_name,)]
-
-            # Allow established connections
-            our_rules += ['-A %s -m state --state '
-                          'ESTABLISHED,RELATED -j ACCEPT' % (chain_name,)]
-
-            # Jump to each security group chain in turn
-            for security_group in \
-                            db.security_group_get_by_instance(ctxt,
-                                                              instance['id']):
-                security_groups[security_group['id']] = security_group
-
-                sg_chain_name = self._security_group_chain_name(
-                                                          security_group['id'])
+        ipv4_rules = []
+        ipv6_rules = []
 
-                our_rules += ['-A %s -j %s' % (chain_name, sg_chain_name)]
-
-            if(ip_version == 4):
-                # Allow DHCP responses
-                dhcp_server = self._dhcp_server_for_instance(instance)
-                our_rules += ['-A %s -s %s -p udp --sport 67 --dport 68 '
-                                    '-j ACCEPT ' % (chain_name, dhcp_server)]
-                #Allow project network traffic
-                if (FLAGS.allow_project_net_traffic):
-                    cidr = self._project_cidr_for_instance(instance)
-                    our_rules += ['-A %s -s %s -j ACCEPT' % (chain_name, cidr)]
-            elif(ip_version == 6):
-                # Allow RA responses
-                ra_server = self._ra_server_for_instance(instance)
-                if ra_server:
-                    our_rules += ['-A %s -s %s -p icmpv6 -j ACCEPT' %
-                                  (chain_name, ra_server + "/128")]
-                #Allow project network traffic
-                if (FLAGS.allow_project_net_traffic):
-                    cidrv6 = self._project_cidrv6_for_instance(instance)
-                    our_rules += ['-A %s -s %s -j ACCEPT' %
-                                        (chain_name, cidrv6)]
-
-            # If nothing matches, jump to the fallback chain
-            our_rules += ['-A %s -j nova-fallback' % (chain_name,)]
+        # Always drop invalid packets
+        ipv4_rules += ['-m state --state ' 'INVALID -j DROP']
+        ipv6_rules += ['-m state --state ' 'INVALID -j DROP']
 
-        # then, security group chains and rules
-        for security_group_id in security_groups:
-            chain_name = self._security_group_chain_name(security_group_id)
-            our_chains += [':%s - [0:0]' % chain_name]
+        # Allow established connections
+        ipv4_rules += ['-m state --state ESTABLISHED,RELATED -j ACCEPT']
+        ipv6_rules += ['-m state --state ESTABLISHED,RELATED -j ACCEPT']
 
-            rules = \
-              db.security_group_rule_get_by_security_group(ctxt,
-                                                          security_group_id)
+        dhcp_server = self._dhcp_server_for_instance(instance)
+        ipv4_rules += ['-s %s -p udp --sport 67 --dport 68 '
+                       '-j ACCEPT' % (dhcp_server,)]
+
+        #Allow project network traffic
+        if FLAGS.allow_project_net_traffic:
+            cidr = self._project_cidr_for_instance(instance)
+            ipv4_rules += ['-s %s -j ACCEPT' % (cidr,)]
+
+        # We wrap these in FLAGS.use_ipv6 because they might cause
+        # a DB lookup. The other ones are just list operations, so
+        # they're not worth the clutter.
+        if FLAGS.use_ipv6:
+            # Allow RA responses
+            ra_server = self._ra_server_for_instance(instance)
+            if ra_server:
+                ipv6_rules += ['-s %s/128 -p icmpv6 -j ACCEPT' % (ra_server,)]
+
+            #Allow project network traffic
+            if FLAGS.allow_project_net_traffic:
+                cidrv6 = self._project_cidrv6_for_instance(instance)
+                ipv6_rules += ['-s %s -j ACCEPT' % (cidrv6,)]
+
+        security_groups = db.security_group_get_by_instance(ctxt,
+                                                            instance['id'])
+
+        # then, security group chains and rules
+        for security_group in security_groups:
+            rules = db.security_group_rule_get_by_security_group(ctxt,
+                                                          security_group['id'])
 
             for rule in rules:
                 logging.info('%r', rule)
@@ -1350,14 +1338,16 @@ class IptablesFirewallDriver(FirewallDriver):
                     continue
 
                 version = _get_ip_version(rule.cidr)
-                if version != ip_version:
-                    continue
+                if version == 4:
+                    rules = ipv4_rules
+                else:
+                    rules = ipv6_rules
 
                 protocol = rule.protocol
                 if version == 6 and rule.protocol == 'icmp':
                     protocol = 'icmpv6'
 
-                args = ['-A', chain_name, '-p', protocol, '-s', rule.cidr]
+                args = ['-p', protocol, '-s', rule.cidr]
 
                 if rule.protocol in ['udp', 'tcp']:
                     if rule.from_port == rule.to_port:
@@ -1378,32 +1368,39 @@ class IptablesFirewallDriver(FirewallDriver):
                             icmp_type_arg += '/%s' % icmp_code
 
                     if icmp_type_arg:
-                        if(ip_version == 4):
+                        if version == 4:
                             args += ['-m', 'icmp', '--icmp-type',
                                      icmp_type_arg]
-                        elif(ip_version == 6):
+                        elif version == 6:
                             args += ['-m', 'icmp6', '--icmpv6-type',
                                      icmp_type_arg]
 
                 args += ['-j ACCEPT']
-                our_rules += [' '.join(args)]
+                rules += [' '.join(args)]
+
+        ipv4_rules += ['-j $sg-fallback']
+        ipv6_rules += ['-j $sg-fallback']
 
-        new_filter[rules_index:rules_index] = our_rules
-        new_filter[rules_index:rules_index] = our_chains
-        logging.info('new_filter: %s', '\n'.join(new_filter))
-        return new_filter
+        return ipv4_rules, ipv6_rules
 
     def refresh_security_group_members(self, security_group):
         pass
 
     def refresh_security_group_rules(self, security_group):
-        self.apply_ruleset()
+        for instance in self.instances.values():
+            # We use the semaphore to make sure noone applies the rule set
+            # after we've yanked the existing rules but before we've put in
+            # the new ones.
+            with self.iptables.semaphore:
+                self.remove_filters_for_instance(instance)
+                self.add_filters_for_instance(instance)
+        self.iptables.apply()
 
     def _security_group_chain_name(self, security_group_id):
         return 'nova-sg-%s' % (security_group_id,)
 
     def _instance_chain_name(self, instance):
-        return 'nova-inst-%s' % (instance['id'],)
+        return 'inst-%s' % (instance['id'],)
 
     def _ip_for_instance(self, instance):
         return db.instance_get_fixed_address(context.get_admin_context(),
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index ce081a2d6..4e6c71446 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -917,14 +917,13 @@ def _write_partition(virtual_size, dev):
     LOG.debug(_('Writing partition table %(primary_first)d %(primary_last)d'
             ' to %(dest)s...') % locals())
 
-    def execute(cmd, process_input=None, check_exit_code=True):
-        return utils.execute(cmd=cmd,
-                             process_input=process_input,
-                             check_exit_code=check_exit_code)
-
-    execute('parted --script %s mklabel msdos' % dest)
-    execute('parted --script %s mkpart primary %ds %ds' %
-            (dest, primary_first, primary_last))
+    def execute(*cmd, **kwargs):
+        return utils.execute(*cmd, **kwargs)
+
+    execute('parted', '--script', dest, 'mklabel', 'msdos')
+    execute('parted', '--script', dest, 'mkpart', 'primary',
+            '%ds' % primary_first,
+            '%ds' % primary_last)
 
     LOG.debug(_('Writing partition table %s done.'), dest)