From f2ba2b58b09661bbe1b6f2c94ac87da420c46b49 Mon Sep 17 00:00:00 2001
From: catborise <catborise@yahoo.com>
Date: Wed, 8 Jan 2020 11:28:46 +0300
Subject: [PATCH] Add editing disk capability to instance. Add instance disk
 options like io, discard, detect zeroes. rewrite detach disk

---
 instances/templates/edit_instance_volume.html | 209 ++++++++----------
 instances/templates/instance.html             |  20 +-
 instances/views.py                            |  47 +++-
 vrtManager/instance.py                        | 125 ++++++++---
 4 files changed, 244 insertions(+), 157 deletions(-)

diff --git a/instances/templates/edit_instance_volume.html b/instances/templates/edit_instance_volume.html
index 8409a44..e0b5fd9 100644
--- a/instances/templates/edit_instance_volume.html
+++ b/instances/templates/edit_instance_volume.html
@@ -1,151 +1,126 @@
 {% load i18n %}
 {% if request.user.is_superuser %}
-    <a href="#addvol" type="button" class="btn btn-success pull-right" data-toggle="modal" title="Add Volume">
-        <span class="glyphicon glyphicon-plus" aria-hidden="true"></span>
-    </a>
+    <button href="#editvol{{ id }}" type="button" class="btn btn-sm btn-default" data-toggle="modal" title="Edit Volume">
+        <i class="glyphicon glyphicon-edit" aria-hidden="true"></i>
+    </button>
 
     <!-- Modal pool -->
-    <div class="modal fade" id="addvol" tabindex="-1" role="dialog" aria-labelledby="addInstanceVolumeLabel" aria-hidden="true">
+    <div class="modal fade" id="editvol{{ id }}" tabindex="-1" role="dialog" aria-labelledby="editInstanceVolumeLabel{{ id }}" aria-hidden="true">
         <div class="modal-dialog">
             <div class="modal-content">
                 <div class="modal-header">
                     <button type="button" class="close" data-dismiss="modal" aria-hidden="true">&times;</button>
-                    <h4 class="modal-title">{% trans "Add Instance Volume" %}</h4>
+                    <h4 class="modal-title">{% trans "Edit Instance Volume" %}</h4>
                 </div>
+
                 <div class="modal-body">
-                    <div class="row">
+
                         <ul class="nav nav-tabs">
-                            <li role="presentation" class="active"><a href="#NewDisk" data-toggle="tab">{% trans 'New Disk' %}</a></li>
-                            <li role="presentation"><a href="#ExistingDisk" data-toggle="tab">{% trans 'Existing Disk' %}</a></li>
+                            <li role="presentation" class="active"><a href="#VirtualDisk{{ id }}" data-toggle="tab">{% trans 'Virtual Disk' %}</a></li>
+                            <li role="presentation"><a href="#PerformanceVolume{{ id }}" data-toggle="tab">{% trans 'Performance' %}</a></li>
+                            <li role="presentation"><a href="#AdvancedVolume{{ id }}" data-toggle="tab">{% trans 'Advanced' %}</a></li>
                         </ul>
 
                         <div class="tab-content">
-                            <div class="tab-pane active" id="NewDisk">
-                                <form class="form-horizontal" method="post" role="form">{% csrf_token %}
 
-                                <p style="font-weight:bold;">{% trans "Volume parameters" %}</p>
+                            <div class="tab-pane active" id="VirtualDisk{{ id }}">
                                 <div class="form-group">
-                                    <label class="col-sm-3 control-label">{% trans "Storage" %}</label>
-                                    <div class="col-sm-4">
-                                        <select name="storage" class="form-control image-format">
-                                            {% for storage in storages_host %}
-                                                <option value="{{ storage }}">{{ storage }}</option>
-                                            {% endfor %}
-                                        </select>
+                                    <label class="col-sm-4 control-label">{% trans 'Volume Path' %}</label>
+                                    <div class="col-sm-8">
+                                        <input class="form-control" name="vol_path" value="{{ disk.path }}"/>
                                     </div>
                                 </div>
                                 <div class="form-group">
-                                    <label class="col-sm-3 control-label">{% trans "Name" %}</label>
-                                    <div class="col-sm-4">
-                                        <input type="text" class="form-control" name="name" placeholder="{% trans "Name" %}" required pattern="[a-zA-Z0-9\.\-_]+">
+                                    <label class="col-sm-4 control-label">{% trans 'Readonly' %}</label>
+                                    <div class="col-sm-8">
+                                        <input class="form-control-static" type="checkbox" name="vol_readonly" value="True" {% if disk.readonly %}checked{% endif %}/>
                                     </div>
                                 </div>
                                 <div class="form-group">
-                                    <label class="col-sm-3 control-label">{% trans "Format" %}</label>
-                                    <div class="col-sm-4">
-                                        <select name="format" class="form-control image-format">
-                                            {% for format in formats %}
-                                                <option value="{{ format }}" {% if format == default_format %}selected{% endif %}>{% trans format %}</option>
-                                            {% endfor %}
-                                        </select>
+                                    <label class="col-sm-4 control-label">{% trans 'Shareable' %}</label>
+                                    <div class="col-sm-8">
+                                        <input class="form-control-static" type="checkbox" name="vol_shareable" value="True" {% if disk.shareable %}checked{% endif %}/>
                                     </div>
                                 </div>
-                                <div class="form-group">
-                                    <label class="col-sm-3 control-label">{% trans "Size" %}</label>
-                                    <div class="col-sm-4">
-                                        <input type="text" class="form-control" name="size" value="10" maxlength="5" required pattern="[0-9]+">
-                                    </div>
-                                    <label class="col-sm-1 control-label">{% trans "GB" %}</label>
-                                </div>
-                                <div class="form-group">
-                                    <label class="col-sm-3 control-label">{% trans "Bus" %}</label>
-                                    <div class="col-sm-4">
-                                        <select name="bus" class="form-control image-format">
-                                            {% for bus in bus_host %}
-                                                <option value="{{ bus }}" {% if bus == default_bus %}selected{% endif %}>{% trans bus %}</option>
-                                            {% endfor %}
-                                        </select>
-                                    </div>
-                                </div>
-                                <div class="form-group">
-                                    <label class="col-sm-3 control-label">{% trans "Cache" %}</label>
-                                    <div class="col-sm-4">
-                                        <select name="cache" class="form-control image-format">
-                                            {% for mode, name in cache_modes %}
-                                                <option value="{{ mode }}" {% if mode == default_cache %}selected{% endif %}>{% trans name %}</option>
-                                            {% endfor %}
-                                        </select>
-                                    </div>
-                                </div>
-                                <div class="form-group meta-prealloc">
-                                    <label class="col-sm-3 control-label">{% trans "Metadata" %}</label>
-                                    <div class="col-sm-4">
-                                        <input type="checkbox" name="meta_prealloc" value="true">
-                                    </div>
-                                </div>
-                                <div class="modal-footer">
-                                    <button type="submit" class="btn btn-lg btn-success pull-right" name="add_new_vol">{% trans "Add Volume" %}</button>
-                                </div>
-                            </form>
                             </div>
-                            <div class="tab-pane" id="ExistingDisk">
-                                <form class="form-horizontal" method="post" role="form">{% csrf_token %}
-                                <div class="modal-body">
-                                    <p style="font-weight:bold;">{% trans "Volume parameters" %}</p>
-                                    <div class="form-group">
-                                        <label class="col-sm-3 control-label">{% trans "Storage" %}</label>
-                                        <div class="col-sm-4">
-                                            <div class="dropdown">
-                                              <button id="select_storage" class="btn btn-default dropdown-toggle form-control" type="button" data-toggle="dropdown">{% trans 'Select Pool...' %}
-                                              <span class="caret"></span></button>
-                                              <ul class="dropdown-menu">
-                                                  {% for storage in storages_host %}
-                                                      <li><a href="#" onclick="get_volumes({{ compute_id }}, '{{ storage }}')">{{ storage }}</a></li>
-                                                  {% endfor %}
-                                              </ul>
-                                              <input id="selected_storage" name="selected_storage" hidden/>
-                                            </div>
-                                        </div>
-                                    </div>
-                                    <div class="form-group">
-                                        <label class="col-sm-3 control-label" >{% trans "Volume" %}</label>
-                                        <div class="col-sm-4">
-                                            <select id="vols" name="vols" class="form-control" disabled>
-                                                <option value="" selected>{% trans 'None' %}</option>
-                                                <!-- populate with javascript -->
-                                            </select>
-                                        </div>
-                                    </div>
-                                    <div class="form-group">
-                                        <label class="col-sm-3 control-label">{% trans "Bus" %}</label>
-                                        <div class="col-sm-4">
-                                            <select name="bus" class="form-control image-format">
-                                                {% for bus in bus_host %}
-                                                    <option value="{{ bus }}" {% if bus == default_bus %}selected{% endif %}>{% trans bus %}</option>
-                                                {% endfor %}
-                                            </select>
-                                        </div>
-                                    </div>
-                                    <div class="form-group">
-                                        <label class="col-sm-3 control-label">{% trans "Cache" %}</label>
-                                        <div class="col-sm-4">
-                                            <select name="cache" class="form-control image-format">
-                                                {% for mode, name in cache_modes %}
-                                                    <option value="{{ mode }}" {% if mode == default_cache %}selected{% endif %}>{% trans name %}</option>
-                                                {% endfor %}
-                                            </select>
-                                        </div>
+
+                            <div class="tab-pane" id="AdvancedVolume{{ id }}">
+                                <div class="form-group">
+                                    <label class="col-sm-4 control-label">{% trans 'Bus' %}</label>
+                                    <div class="col-sm-8">
+                                        <select class="form-control" name="vol_bus">
+                                        {% for bus in bus_host %}
+                                            <option value="{{ bus }}" {% if bus == disk.bus %}selected{% endif %}>{{ bus }}</option>
+                                        {% endfor %}
+                                        </select>
                                     </div>
                                 </div>
-                                <div class="modal-footer">
-                                     <button type="submit" class="btn btn-lg btn-success pull-right" name="add_existing_vol">{% trans "Add Volume" %}</button>
+                                <div class="form-group">
+                                    <label class="col-sm-4 control-label">{% trans 'Serial Number' %}</label>
+                                    <div class="col-sm-8">
+                                        <input class="form-control" type="text" name="vol_serial" value="{{ disk.serial }}"/>
+                                    </div>
+                                </div>
+                                <div class="form-group">
+                                    <label class="col-sm-4 control-label">{% trans 'Storage Format' %}</label>
+                                    <div class="col-sm-8">
+                                        <input class="form-control" type="text" name="vol_format" value="{{ disk.format }}"/>
+                                    </div>
                                 </div>
-                            </form>
                             </div>
-                        </div>
-                    </div> <!-- row -->
+
+                            <div class="tab-pane" id="PerformanceVolume{{ id }}">
+                                <div class="form-group">
+                                    <label class="col-sm-4 control-label">{% trans 'Cache mode' %}</label>
+                                    <div class="col-sm-8">
+                                        <select class="form-control" name="vol_cache">
+                                        {% for key, val in cache_modes %}
+                                            <option value="{{ key }}" {% if key == disk.cache %}selected{% endif %}>{{ val }}</option>
+                                        {% endfor %}
+                                        </select>
+                                    </div>
+                                </div>
+                                <div class="form-group">
+                                    <label class="col-sm-4 control-label">{% trans 'IO mode' %}</label>
+                                    <div class="col-sm-8">
+                                        <select class="form-control" name="vol_io_mode">
+                                        {% for key, val in io_modes %}
+                                            <option value="{{ key }}" {% if key == disk.io %}selected{% endif %}>{{ val }}</option>
+                                        {% endfor %}
+                                        </select>
+                                    </div>
+                                </div>
+                                <div class="form-group">
+                                    <label class="col-sm-4 control-label">{% trans 'Discard mode' %}</label>
+                                    <div class="col-sm-8">
+                                        <select class="form-control" name="vol_discard_mode">
+                                        {% for key, val in discard_modes %}
+                                            <option value="{{ key }}" {% if key == disk.discard %}selected{% endif %}>{{ val }}</option>
+                                        {% endfor %}
+                                        </select>
+                                    </div>
+                                </div>
+                                <div class="form-group">
+                                    <label class="col-sm-4 control-label">{% trans 'Detect zeroes' %}</label>
+                                    <div class="col-sm-8">
+                                        <select class="form-control" name="vol_detect_zeroes">
+                                        {% for key, val in detect_zeroes_modes %}
+                                            <option value="{{ key }}" {% if key == disk.detect_zeroes %}selected{% endif %}>{{ val }}</option>
+                                        {% endfor %}
+                                        </select>
+                                    </div>
+                                </div>
+                            </div><!-- /.tabpane-content -->
+                        </div><!-- /.tab-content -->
                 </div> <!-- /.modal-body -->
+                <div class="clearfix"></div>
+                <div class="modal-footer">
+                    <button type="button" class="btn btn-default" data-dismiss="modal">{% trans "Close" %}</button>
+                    <button type="submit" class="btn btn-success" name="edit_volume">{% trans "Save" %}</button>
+                </div><!-- /.modal-footer -->
+
             </div> <!-- /.modal-content -->
         </div> <!-- /.modal-dialog -->
+
     </div> <!-- /.modal -->
 {% endif %}
diff --git a/instances/templates/instance.html b/instances/templates/instance.html
index 261ab76..ae35f55 100644
--- a/instances/templates/instance.html
+++ b/instances/templates/instance.html
@@ -823,14 +823,14 @@
                                                 <th>{% trans "Capacity" %}</th>
                                                 <th>{% trans "Storage" %}</th>
                                                 <th>{% trans "Source" %}</th>
-                                                <th style="width:100px;">{% trans "Action" %}</th>
+                                                <th>{% trans "Action" %}</th>
                                             </thead>
                                             <tbody>
                                             {% for disk in disks %}
                                                 <tr>
                                                     <td>
                                                         <button type="submit" class="btn btn-sm btn-default"
-                                                                name="details"
+                                                                name="details{{ forloop.counter0 }}"
                                                                 title="{% trans "Details" %}"
                                                                 tabindex="0"
                                                                 data-trigger="focus"
@@ -838,7 +838,13 @@
                                                                 data-html="true"
                                                                 data-content="<strong>Bus:</strong> {{ disk.bus }} <br/>
                                                                               <strong>Format:</strong> {{ disk.format }} <br/>
-                                                                              <strong>Cache:</strong> {{ disk.cache }}">
+                                                                              <strong>Cache:</strong> {{ disk.cache }} <br/>
+                                                                              <strong>Serial:</strong> {{ disk.serial }} <br/>
+                                                                              <strong>Readonly:</strong> {{ disk.readonly }} <br/>
+                                                                              <strong>Shareable:</strong> {{ disk.shareable }}</br>
+                                                                              <strong>IO Mode:</strong> {{ disk.io }} <br/>
+                                                                              <strong>Discard:</strong> {{ disk.discard }} <br/>
+                                                                              <strong>Detect Zeroes:</strong> {{ disk.detect_zeroes }}">
                                                             <i class="fa fa-info"></i>
                                                         </button>
                                                         {{ disk.dev }}
@@ -850,11 +856,12 @@
                                                     <td>{{ disk.storage }}</td>
                                                     <td>{{ disk.path }}</td>
                                                     <td>
-                                                        <form action="" method="post" style="height:10px" role="form">{% csrf_token %}
+                                                        <form action="" method="post" role="form">{% csrf_token %}
                                                             <input type="hidden" name="path" value="{{ disk.path }}">
                                                             <input type="hidden" name="dev" value="{{ disk.dev }}">
                                                             <input type="hidden" name="storage" value="{{ disk.storage }}">
                                                             <input type="hidden" name="name" value="{{ disk.image }}">
+                                                            {% include 'edit_instance_volume.html' with id=forloop.counter0 %}
                                                             {% ifequal status 5 %}
                                                                 <button type="submit" class="btn btn-sm btn-default" name="detach_vol" value="{{ disk.dev }}" title="{% trans "Detach" %}" onclick="return confirm('{% trans "Are you sure to detach volume?" %}')">
                                                                     <i class="fa fa-eject"></i>
@@ -863,14 +870,15 @@
                                                                     <i class="fa fa-trash"></i>
                                                                 </button>
                                                             {% else %}
-                                                                <button class="btn btn-sm btn-default disabled" name="detach_vol" value="{{ disk.dev }}" title="{% trans "Detach" %}" onclick="return confirm('{% trans "Are you sure to detach volume after shutdown?" %}')">
+                                                                <button class="btn btn-sm btn-default disabled" name="detach_vol" value="{{ disk.dev }}" title="{% trans "Detach" %}" onclick="return confirm('{% trans "Are you sure? This may lead data corruption!" %}')">
                                                                     <i class="fa fa-eject"></i>
                                                                 </button>
-                                                                <button class="btn btn-sm btn-default disabled" name="delete_vol" title="{% trans "Delete" %}" onclick="return confirm('{% trans "Are you sure to delete after shutdown?" %}')">
+                                                                <button class="btn btn-sm btn-default disabled" name="delete_vol" title="{% trans "Delete" %}" onclick="return confirm('{% trans "Are you sure? This may lead data corruption!" %}')">
                                                                     <i class="fa fa-trash"></i>
                                                                 </button>
                                                             {% endifequal %}
                                                         </form>
+
                                                     </td>
                                                 </tr>
                                             {% endfor %}
diff --git a/instances/views.py b/instances/views.py
index 78982d4..195dd7d 100644
--- a/instances/views.py
+++ b/instances/views.py
@@ -313,6 +313,12 @@ def instance(request, compute_id, vname):
         clone_free_names = get_clone_free_names()
         user_quota_msg = check_user_quota(0, 0, 0, 0)
         cache_modes = sorted(conn.get_cache_modes().items())
+        io_modes = sorted(conn.get_io_modes().items())
+        discard_modes = sorted(conn.get_discard_modes().items())
+        detect_zeroes_modes = sorted(conn.get_detect_zeroes_modes().items())
+        default_io = settings.INSTANCE_VOLUME_DEFAULT_IO
+        default_discard = settings.INSTANCE_VOLUME_DEFAULT_DISCARD
+        default_zeroes = settings.INSTANCE_VOLUME_DEFAULT_DETECT_ZEROES
         default_cache = settings.INSTANCE_VOLUME_DEFAULT_CACHE
         default_format = settings.INSTANCE_VOLUME_DEFAULT_FORMAT
         default_owner = settings.INSTANCE_VOLUME_DEFAULT_OWNER
@@ -563,10 +569,10 @@ def instance(request, compute_id, vname):
                 meta_prealloc = True if request.POST.get('meta_prealloc', False) else False
                 bus = request.POST.get('bus', default_bus)
                 cache = request.POST.get('cache', default_cache)
-                target = get_new_disk_dev(media, disks, bus)
+                target_dev = get_new_disk_dev(media, disks, bus)
 
-                path = conn_create.create_volume(storage, name, size, format, meta_prealloc, default_owner)
-                conn.attach_disk(path, target, subdriver=format, cache=cache, targetbus=bus)
+                source = conn_create.create_volume(storage, name, size, format, meta_prealloc, default_owner)
+                conn.attach_disk(source, target_dev, target_bus=bus, driver_type=format, cache_mode=cache)
                 msg = _('Attach new disk {} ({})'.format(name, format))
                 addlogmsg(request.user.username, instance.name, msg)
                 return HttpResponseRedirect(request.get_full_path() + '#disks')
@@ -583,16 +589,43 @@ def instance(request, compute_id, vname):
                                          compute.type,
                                          storage)
 
-                format = conn_create.get_volume_type(name)
+                driver_type = conn_create.get_volume_type(name)
                 path = conn_create.get_target_path()
-                target = get_new_disk_dev(media, disks, bus)
+                target_dev = get_new_disk_dev(media, disks, bus)
                 source = path + "/" + name
 
-                conn.attach_disk(source, target, subdriver=format, cache=cache, targetbus=bus)
-                msg = _('Attach Existing disk: ' + target)
+                conn.attach_disk(source, target_dev, target_bus=bus, driver_type=driver_type, cache_mode=cache)
+                msg = _('Attach Existing disk: ' + target_dev)
                 addlogmsg(request.user.username, instance.name, msg)
                 return HttpResponseRedirect(request.get_full_path() + '#disks')
 
+            if 'edit_volume' in request.POST and allow_admin_or_not_template:
+                target_dev = request.POST.get('dev', '')
+
+                new_path = request.POST.get('vol_path', '')
+                shareable = bool(request.POST.get('vol_shareable', False))
+                readonly = bool(request.POST.get('vol_readonly', False))
+                bus = request.POST.get('vol_bus', '')
+                serial = request.POST.get('vol_serial', '')
+                format = request.POST.get('vol_format', '')
+                cache = request.POST.get('vol_cache', default_cache)
+                io = request.POST.get('vol_io_mode', default_io)
+                discard = request.POST.get('vol_discard_mode', default_discard)
+                zeroes = request.POST.get('vol_detect_zeroes', default_zeroes)
+
+                conn.edit_disk(target_dev, new_path, readonly, shareable, bus, serial, format,
+                               cache, io, discard, zeroes)
+
+                if not conn.get_status() == 5:
+                    messages.success(request, _("Disk changes changes are applied. " +
+                                                "But it will be activated after shutdown"))
+                else:
+                    messages.success(request, _("Disk is changed successfully."))
+                msg = _('Edit disk: ' + target_dev)
+                addlogmsg(request.user.username, instance.name, msg)
+
+                return HttpResponseRedirect(request.get_full_path() + '#disks')
+
             if 'delete_vol' in request.POST and allow_admin_or_not_template:
                 storage = request.POST.get('storage', '')
                 conn_delete = wvmStorage(compute.hostname,
diff --git a/vrtManager/instance.py b/vrtManager/instance.py
index 27354db..75eaa5f 100644
--- a/vrtManager/instance.py
+++ b/vrtManager/instance.py
@@ -388,10 +388,13 @@ class wvmInstance(wvmConnect):
     def get_disk_devices(self):
         def disks(doc):
             result = []
-            dev = volume = storage = src_file = bus = None
-            disk_format = used_size = disk_size = disk_cache = None
             
             for disk in doc.xpath('/domain/devices/disk'):
+                dev = volume = storage = src_file = bus = None
+                disk_format = used_size = disk_size = None
+                disk_cache = disk_io = disk_discard = disk_zeroes = 'default'
+                readonly = shareable = serial = None
+
                 device = disk.xpath('@device')[0]
                 if device == 'disk':
                     try:
@@ -406,6 +409,23 @@ class wvmInstance(wvmConnect):
                             disk_cache = disk.xpath('driver/@cache')[0]
                         except:
                             pass
+                        try:
+                            disk_io = disk.xpath('driver/@io')[0]
+                        except:
+                            pass
+                        try:
+                            disk_discard = disk.xpath('driver/@discard')[0]
+                        except:
+                            pass
+                        try:
+                            disk_zeroes = disk.xpath('driver/@detect_zeroes')[0]
+                        except:
+                            pass
+
+                        readonly = True if disk.xpath('readonly') else False
+                        shareable = True if disk.xpath('shareable') else False
+                        serial = disk.xpath('serial')[0].text if disk.xpath('serial') else None
+
                         try:
                             vol = self.get_volume_by_path(src_file)
                             volume = vol.name()
@@ -421,7 +441,9 @@ class wvmInstance(wvmConnect):
                     finally:
                         result.append(
                             {'dev': dev, 'bus': bus, 'image': volume, 'storage': storage, 'path': src_file,
-                             'format': disk_format, 'size': disk_size, 'used': used_size, 'cache': disk_cache})
+                             'format': disk_format, 'size': disk_size, 'used': used_size,
+                             'cache': disk_cache, 'io': disk_io, 'discard': disk_discard, 'detect_zeroes': disk_zeroes,
+                             'readonly': readonly, 'shareable': shareable, 'serial': serial})
             return result
 
         return util.get_xml_path(self._XMLDesc(0), func=disks)
@@ -620,39 +642,88 @@ class wvmInstance(wvmConnect):
             xmldom = ElementTree.tostring(tree)
         self._defineXML(xmldom)
 
-    def attach_disk(self, source, target, sourcetype='file', device='disk', driver='qemu', subdriver='raw', cache='none', targetbus='ide'):
-        xml_disk = "<disk type='%s' device='%s'>" % (sourcetype, device)
-        if device == 'cdrom':
-            xml_disk += "<driver name='%s' type='%s'/>" % (driver, subdriver)
-        elif device == 'disk':
-            xml_disk += "<driver name='%s' type='%s' cache='%s'/>" % (driver, subdriver, cache)
+    def attach_disk(self, source, target_dev, target_bus='ide', disk_type='file',
+                    disk_device='disk', driver_name='qemu', driver_type='raw',
+                    readonly=False, shareable=False, serial=None,
+                    cache_mode=None, io_mode=None, discard_mode=None, detect_zeroes_mode=None):
+
+        additionals = ''
+        if cache_mode is not None and cache_mode != 'default':
+            additionals += "cache='%s' " % cache_mode
+        if io_mode is not None and io_mode != 'default':
+            additionals += "io='%s' " % io_mode
+        if discard_mode is not None and discard_mode != 'default':
+            additionals += "discard='%s' " % discard_mode
+        if detect_zeroes_mode is not None and detect_zeroes_mode != 'default':
+            additionals += "detect_zeroes='%s' " % detect_zeroes_mode
+
+        xml_disk = "<disk type='%s' device='%s'>" % (disk_type, disk_device)
+        if disk_device == 'cdrom':
+            xml_disk += "<driver name='%s' type='%s'/>" % (driver_name, driver_type)
+        elif disk_device == 'disk':
+            xml_disk += "<driver name='%s' type='%s' %s/>" % (driver_name, driver_type, additionals)
         xml_disk += """<source file='%s'/>
-          <target dev='%s' bus='%s'/>
-        </disk>
-        """ % (source, target, targetbus)
+          <target dev='%s' bus='%s'/>""" % (source, target_dev, target_bus)
+        if readonly:
+            xml_disk += """<readonly/>"""
+        if shareable:
+            xml_disk += """<shareable/>"""
+        if serial is not None and serial != 'None' and serial != '':
+            xml_disk += """<serial>%s</serial>""" % serial
+        xml_disk += """</disk>"""
         if self.get_status() == 1:
             self.instance.attachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_LIVE)
             self.instance.attachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_CONFIG)
         if self.get_status() == 5:
             self.instance.attachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_CONFIG)
 
-    def detach_disk(self, dev):
-        tree = ElementTree.fromstring(self._XMLDesc(0))
+    def detach_disk(self, target_dev):
+        tree = etree.fromstring(self._XMLDesc(0))
 
-        for disk in tree.findall("./devices/disk"):
-            target = disk.find("target")
-            if target.get("dev") == dev:
-                devices = tree.find('devices')
-                devices.remove(disk)
+        disk_el = tree.xpath("./devices/disk/target[@dev='{}']".format(target_dev))[0].getparent()
+        xml_disk = etree.tostring(disk_el)
+        devices = tree.find('devices')
+        devices.remove(disk_el)
 
-                if self.get_status() == 1:
-                    xml_disk = ElementTree.tostring(disk)
-                    ret = self.instance.detachDevice(xml_disk)
-                    xmldom = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
-                if self.get_status() == 5:
-                    xmldom = ElementTree.tostring(tree)
-                break
-        self._defineXML(xmldom)
+        if self.get_status() == 1:
+            self.instance.detachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_LIVE)
+            self.instance.detachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_CONFIG)
+        if self.get_status() == 5:
+            self.instance.detachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_CONFIG)
+
+    def edit_disk(self, target_dev, source, readonly, shareable, target_bus, serial, format, cache_mode, io_mode, discard_mode, detect_zeroes_mode):
+        tree = etree.fromstring(self._XMLDesc(0))
+        disk_el = tree.xpath("./devices/disk/target[@dev='{}']".format(target_dev))[0].getparent()
+        old_disk_type = disk_el.get('type')
+        old_disk_device = disk_el.get('device')
+        old_driver_name = disk_el.xpath('driver/@name')[0]
+
+        additionals = ''
+        if cache_mode is not None and cache_mode != 'default':
+            additionals += "cache='%s' " % cache_mode
+        if io_mode is not None and io_mode != 'default':
+            additionals += "io='%s' " % io_mode
+        if discard_mode is not None and discard_mode != 'default':
+            additionals += "discard='%s' " % discard_mode
+        if detect_zeroes_mode is not None and detect_zeroes_mode != 'default':
+            additionals += "detect_zeroes='%s' " % detect_zeroes_mode
+
+        xml_disk = "<disk type='%s' device='%s'>" % (old_disk_type, old_disk_device)
+        if old_disk_device == 'cdrom':
+            xml_disk += "<driver name='%s' type='%s'/>" % (old_driver_name, format)
+        elif old_disk_device == 'disk':
+            xml_disk += "<driver name='%s' type='%s' %s/>" % (old_driver_name, format, additionals)
+        xml_disk += """<source file='%s'/>
+          <target dev='%s' bus='%s'/>""" % (source, target_dev, target_bus)
+        if readonly:
+            xml_disk += """<readonly/>"""
+        if shareable:
+            xml_disk += """<shareable/>"""
+        if serial is not None and serial != 'None' and serial != '':
+            xml_disk += """<serial>%s</serial>""" % serial
+        xml_disk += """</disk>"""
+
+        self.instance.updateDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_CONFIG)
 
     def cpu_usage(self):
         cpu_usage = {}