[et-mgmt-tools] [PATCH] virt-manager: Make virt-manager remember last image directory
Cole Robinson
crobinso at redhat.com
Thu Jun 18 12:39:13 UTC 2009
Michal Novotny wrote:
> On 06/18/2009 04:19 AM, Cole Robinson wrote:
>> On 06/16/2009 10:26 AM, Michal Novotny wrote:
>>
>>> # HG changeset patch
>>> # User Michal Novotny<minovotn at redhat.com>
>>> # Date 1245162093 -7200
>>> # Node ID 73427fcfc222006d7f74ce79277f8432d74d7c52
>>> # Parent 74f1f8309b139af9d84edfb1f0186c2306c0f332
>>> Make virt-manager remember last chosen image directory
>>>
>>> Virt-manager never remembers last image directory after choosing file an image
>>> file to be appended to any domain. Since I am having all the domain image files
>>> in the same directory that is not default, this patch saves the last image path
>>> to gconf configuration file and offers it as startfolder next time.
>>>
>>>
>> I like the idea in principle, but I think it needs to be a bit smarter.
>>
>> I have a fix pending that will enable the storage browser for install media on
>> a local connection. We won't want the storage browser squashing the
>> default_image_path in that case, since there's a good chance that their media
>> directory and image directory are not the same.
>>
> Ok, basically I agree. Is there a way to get to know whether we're
> choosing media or an image in StorageBrowser (SB, for short) class ?
Nope, your patch will need to deal with that somehow (or move the gconf
setting out of the browser and into the caller, but then you'll want to
differentiate between a local directory and a storage directory, so it's
kind of hairy).
>> We should have two gconf entries: /paths/last_image_path and
>> /paths/last_media_path, and give the storage_browser a flag or something to
>> allow the caller to opt in on which gconf entry we want to store. There's
>> probably a smarter way to do it but I can't think right now.
>>
> Yeah, that's exactly why I ask. So, if there is no flag, there is a need
> to add some for SB. The SB itself stores the data in my code... Anyway
> you said you have one patch pending, right? What exactly about?
My patch doesn't alter the storage browser at all, it just turns it on
for local install media and cdrom changing. I'll push it today, it will
conflict with your patch a bit though, so you'll have to rebase.
>> A general comment: the gconf entries should change the virt-manager.schema.in.
>> You can probably just use a default value of "", that way gconf won't throw an
>> exception the first time you try to access the key.
>>
> Ok, I didn't know that. This needs further study but now I need to solve
> things about Xen that I am primarily working on.
>> Some comments in line
>>
> Ok, I'll comment it too.
>>> diff -r 74f1f8309b13 -r 73427fcfc222 src/virtManager/addhardware.py
>>> --- a/src/virtManager/addhardware.py Mon Jun 15 15:54:56 2009 +0200
>>> +++ b/src/virtManager/addhardware.py Tue Jun 16 16:21:33 2009 +0200
>>> @@ -674,7 +674,7 @@
>>>
>>> def browse_storage_file_address(self, src, ignore=None):
>>> textent = self.window.get_widget("storage-file-address")
>>> - folder = self.config.get_default_image_dir(self.vm.get_connection())
>>> + folder = self.config.get_user_default_image_dir(self.vm.get_connection())
>>> filename = self._browse_file(_("Locate or Create New Storage File"),
>>> textent, folder=folder,
>>> confirm_overwrite=True)
>>> diff -r 74f1f8309b13 -r 73427fcfc222 src/virtManager/config.py
>>> --- a/src/virtManager/config.py Mon Jun 15 15:54:56 2009 +0200
>>> +++ b/src/virtManager/config.py Tue Jun 16 16:21:33 2009 +0200
>>> @@ -237,7 +237,15 @@
>>> def is_vmlist_network_traffic_visible(self):
>>> return self.conf.get_bool(self.conf_dir + "/vmlist-fields/network_traffic")
>>>
>>> + def get_user_default_image_dir(self, conn):
>>> + try:
>>> + return self.conf.get_value(self.conf_dir + "/paths/default_image_path")
>>> + except ValueError:
>>> + return self.get_default_image_dir(conn)
>>>
>>> + def set_user_default_image_dir(self, dir):
>>> + self.conf.set_value(self.conf_dir + "/paths/default_image_path", dir)
>>> + logging.debug("Setting path to %s", dir)
>>>
>>> def set_vmlist_domain_id_visible(self, state):
>>> self.conf.set_bool(self.conf_dir + "/vmlist-fields/domain_id", state)
>>> @@ -556,6 +564,14 @@
>>>
>>>
>>> def get_default_image_dir(self, connection):
>>> + # Exception for case connection argument is str (happens when
>>> + # creating new domains because we don't have connection object)
>>> + if str(connection) == connection:
>>> + if connection == "Xen":
>>> + return DEFAULT_XEN_IMAGE_DIR
>>> + else:
>>> + return DEFAULT_VIRT_IMAGE_DIR
>>> +
>>> if connection.get_type() == "Xen":
>>> return DEFAULT_XEN_IMAGE_DIR
>>>
>>>
>> The hack shouldn't be necessary (see below).
>>
> I see, so there's a valid connection object in self.conn that provides
> "vmmConnection" (or some) object even when we are creating the domain
> (ie. it doesn't exist yet) ?
Yes, domain installation requires an active libvirt connection.
>>
>>> diff -r 74f1f8309b13 -r 73427fcfc222 src/virtManager/create.py
>>> --- a/src/virtManager/create.py Mon Jun 15 15:54:56 2009 +0200
>>> +++ b/src/virtManager/create.py Tue Jun 16 16:21:33 2009 +0200
>>> @@ -1004,7 +1004,8 @@
>>> self.window.get_widget("config-storage-box").set_sensitive(src.get_active())
>>>
>>> def browse_storage(self, ignore1):
>>> - f = self._browse_file(_("Locate existing storage"))
>>> + folder = self.config.get_user_default_image_dir( self.capsdomain.hypervisor_type )
>>> + f = self._browse_file(_("Locate existing storage"), folder=folder)
>>> if f != None:
>>> self.window.get_widget("config-storage-entry").set_text(f)
>>>
>>>
>> 'self.conn' contains the active connection, so you should use that rather than
>> self.capsdomain...
>>
>>
> Ok, even in domain creation we have this object ?
Yes, you can't even launch the VM wizard without an active connection.
>>> @@ -1653,7 +1654,6 @@
>>> self.detectedDistro = (None, None)
>>>
>>> def _browse_file(self, dialog_name, folder=None, is_media=False):
>>> -
>>> if self.conn.is_remote() or not is_media:
>>> if self.storage_browser == None:
>>> self.storage_browser = vmmStorageBrowser(self.config,
>>> diff -r 74f1f8309b13 -r 73427fcfc222 src/virtManager/storagebrowse.py
>>> --- a/src/virtManager/storagebrowse.py Mon Jun 15 15:54:56 2009 +0200
>>> +++ b/src/virtManager/storagebrowse.py Tue Jun 16 16:21:33 2009 +0200
>>> @@ -20,6 +20,7 @@
>>>
>>> import gobject
>>> import gtk.glade
>>> +import os.path
>>>
>>> import traceback
>>> import logging
>>> @@ -242,6 +243,11 @@
>>> def _do_finish(self, path=None):
>>> if not path:
>>> path = self.current_vol().get_path()
>>> +
>>> + # Save path to config if we use file as storage
>>> + if "/dev" not in path:
>>> + self.config.set_user_default_image_dir( os.path.dirname(path) )
>>> +
>>>
>> A couple things here: This will match any path which contains "/dev", you
>> probably want path.startswith(). Also, this will store the directory of a
>> chosen storage volume, which isn't what we want. If you make this chunk the
>> first thing that happens in the function, it should be fine.
>>
>>
> Oh, right. This is right. I didn't realized that function would be
> better, thanks for your input. This isn't what we won't ? Why not? I
> thought we want to strip eg. /disk2/vms/vm1/image.img to get
> /disk2/vms/vm1/ and this is what we want, right ? Or could you give me
> an example why not this approach?
We really only want to store directories returned from the regular GTK
file chooser, not from the libvirt storage browser. So if the path comes
from current_vol(), we don't want to store it. The gconf entries should
only be for when they manually select an unmanaged path.
Thanks,
Cole
More information about the et-mgmt-tools
mailing list