19.09.2016 08:51, Fabian Grünbichler wrote:
I sent you some feedback on patch #1 yesterday.. Since it breaks stuff, it can't be merged (yet). Feel free to send an updated v2 (or if you feel the feedback requires discussion, feel free to respond). Thanks!
Sorry, did not received it. Maybe Google's spam filters munched it.

------------------------------------------------------------------------
general remark, rest of comments inline:

the indentation is all messed up. I know our codebase is not very clean
in this regard anyway, but for new / touched code we try to keep / make
it clean. our indentation for perl code is a mix of tabs and spaces,
with tabs being 8 spaces long.

the first indentation level is indented with 4 spaces, the second with
1 tab, the third with 1 tab and 4 spaces, the fourth with 2 tabs, and
so on. if you use vim, I recommend enabling listmode to easily
differentiate between the two.
Ok. Configured my mcedit. Maybe it would be good idea to all coding style memo to https://pve.proxmox.com/wiki/Developer_Documentation ?

you drop the $mounted_dev assignment in the else branch. after a quick
search of the calling code, this will probably at least break quota
support for such volumes?
Actually it's not. mountpoint_mount() is being called in list context only once (in PVE::API2::LXC) and only $use_loopdev is being used there. In other places it is being called in scalar context, or its returned values are ignored at all. So currently it brakes nothing. But OK, I can keep this assingment for future.

subvols are not only for size == 0 , e.g. ZFS supports subvols with and
without size (limit). ZFS is a bit tricky unfortunately, as for
containers we always want subvols (regular ZFS filesystems), and for
VMs we always want raw volumes ("zvols" in ZFS speak). so changing the
default format does not really work, and we still need a special case
for ZFS (and future storages with the same problem) here?
But why we are limiting zfspool to subvols only? Following this logic, we should forbid raw images for LXC on any other filesystem-based storage and use only subvols there, like it was with OpenVZ. Or we can spread generic behaviour to zfspool, like I did. Maybe as compromise we could add configurable option to plugins to let user decide?

Also, why I can't set volume size to zero on volume creation via WEB UI and so use subvols? Is it bug?

_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Reply via email to