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).
Sorry, did not received it. Maybe Google's spam filters munched it.
Ok. Configured my mcedit. Maybe it would be good idea to all coding
style memo to https://pve.proxmox.com/wiki/Developer_Documentation ?
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.
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.
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?
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
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?
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