Hi Cole, I'm reordering the paragraphs of your answer to ease mine. On Monday, 7 December 2020 20:39:29 CET Cole Robinson wrote: > Can you explain your motivation a bit: Has this caused you issues > before? My main motivation was to fix the fact that the pool creation dialog offers all the known types to virt-manager, even those that are not supported by the underlying connection. So I thought that, rather than letting the users select any of those unavailable ones to only get a "not supported" error afterwards, it seems better to offer only what can actually be created/setup. Another thing came to my mind by reading this old Debian bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=699701 IIRC the "new volume" dialog used to offer a lot more formats in the past than only raw and qcow2. Still, that dialog has an editable field with the format, so you can change it to anything uncommon or even non-existing. So a possible idea was to validate that, both in the "new volume" dialog and when creating a new volume as result of virt-install --disk parameters, and warn on invalid formats. > Or is there something more useful in the storage XML that you > want to add support for afterwards? One addition I wanted to do is supporting iscsi-direct pools, including their creation. Then I run into "hmm, that will make the combo in the new pool dialog even longer", so that got me into this series. > The code looks fine but I am conflicted about this. I'm not sure it's > worth adding code to read and process storage capabilities XML at all. > I'd rather see the storage dialogs become smaller, not more > featureful/smarter at the cost of increased maintenance and potential > for future feature creep. Considering what I wrote, I can see your POV, however on my defense I don't consider my suggestion that expensive (in terms of complexity & maintenance). > For example sheepdog and mpath can be dropped > entirely IMO. rbd pool creation should probably be dropped because the > UI is never going to be comprehensive enough to handle the common case > which requires specifying an auth secret. Same may apply to gluster but > I'd need to double check. While I can agree with less common pool types like sheepdog and mpath, I'd consider ceph a nice thing to have, especially since it is not trivial. TBH virt-manager really lacks the management of libvirt secrects, which would be immensely useful even for other kind of secrets. Worth creating an RFE ticket to at least not forget about it? > As implemented I'm a little iffy on the UI too. Just hiding options > without giving the user a hint can cause confusion, like why is FOO > available as a pool option for one connection but not the other. There's > ways to fix it but at the cost of more code with goes back to my point > above. Hmm, I guess a possible way would be to make the available pool types as unselectable with a text like "zfs: Unavailable in this connection"? Similar to the URI combo in the "migrate vm" dialog. -- Pino Toscano
Attachment:
signature.asc
Description: This is a digitally signed message part.
