[et-mgmt-tools] [PATCH] virt-manager: Make virt-manager remember last image directory

Thu Jun 18 09:15:32 UTC 2009

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 ?
> 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?
> 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) ?
>    
>> 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 ?
>> @@ -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?
>>           self.emit("storage-browse-finish", path)
>>           self.close()
>>      
>
> Thanks,
> Cole
>    
Thanks,
Michal
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/et-mgmt-tools/attachments/20090618/e45f2d25/attachment.htm>