[pve-devel] [PATCH pve-storage 0/5] v5 fix regression and indentation

[datanom.net]

> 
> ah, now I get it. please correct me if I misunderstood something
> (again):
> 
> - get all targettoextent mappings and loop over them
> - only look at those with the desired target and with a lunid that is 
> in
>   the "special range" reserved for snapshots and state volumes
> - if the volume has an extent, check if it matches the one of the
>   targettoextent mapping
> - else, proceed to the next potentially still free slot and repeat
> 

This is correct.

> I see two remaining issues:
> - you rely on an implict sorting of t2extents by lunid
> - you always iterate over the whole thing (except for the "exist" case)
> 
> I'd suggest dropping the id++, and instead marking which of the
> potential slots you have already seen (or how many, since LUNs should
> not be duplicated in the result anyway I guess?), and moving the check
> and abort into the foreach.
> 

You might have a point here. I will reconsider this.

> 
> yes, but why do you GET "storage/snapshot?limit=$limit" in
> freenas_list_zvol, but GET "storage/snapshot" without a limit here? 
> it's
> the same API endpoint, but inconsistenly used.
> 

You are right but this is fixed in the new freenas_request construction 
for GET (All gets will be looped until no more rows returned)

> 
> no, you would just require manual setup just like for other storage
> types as well:
> - LVM, LVMThin, local ZFS all require local setup before adding the
>   storage
> - Ceph requires at least putting the keyring into place
> - probably others I missed?
> 

So have the user create a file in /etc/pve/private and read the values 
from this file instead of from storage.cfg?

> 
> you have use regular expressions like /^(base|vm)-(\d+)-disk-\d+$/ in
> your code, where you just want to extract the VMID or similar. IMHO
> those should use parse_volname, so that you only have one place where
> you need to make sure that the RE is correct, and where you need to
> change it if it needs changing.
> 
> I see:
> - /^(base|vm)-(\d+)-disk-\d+$/ in freenas_list_zvols, only $2 is used
> - /^(vm|base)-(\d+)-/ in freenas_get_target_name, only $2 is used
> - /(vm|base)-$vmid-disk-(\d+)/ in freenas_find_free_diskname, only $2 
> is
>   used (but this might be left as is, it is a special case and not
>   returned by parse_volname anyway)
> - /^(vm|base)-\d+-disk-(\d+)$ in freenas_get_lun_number, only $2 is 
> used
>   (not returned by parse_volname)
> 
> at least the former two should be updated, the latter two can stay as 
> is
> I guess.
> 

OK.

> 
> so the first child is a meta-child that represents the sum of all
> children? the more I know about this API the less I like it ;)
> 

It is not the API, this is how FreeNAS does it ;-(
Each time you create a new pool a dataset by the same name is created 
under rpool (Not the why this is done in any other ZFS based 
implementations I know of)

> 
> just to make sure there is no misunderstanding here - the JSON module
> provides ::true and ::false, so you don't need to bless anything
> yourself:
> 
> $ perl -e '
> use strict;
> use warnings;
> use JSON;
> my $one = { force => bless( do{\(my $o = 0)}, "JSON::XS::Boolean") };
> my $two = { force => JSON::false };
> print JSON::encode_json($one), "\n";
> print JSON::encode_json($two), "\n";
> '
> {"force":false}
> {"force":false}
> 

Ok.

> 
> see alloc_image in ZFSPoolPlugin.pm

This is also the way I have done it in v6 (missed if-exists construct)

-- 
Hilsen/Regards
Michael Rasmussen

Get my public GnuPG keys:
michael <at> rasmussen <dot> cc
http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xD3C9A00E
mir <at> datanom <dot> net
http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xE501F51C
mir <at> miras <dot> org
http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xE3E80917
--------------------------------------------------------------

----

This mail was virus scanned and spam checked before delivery.
This mail is also DKIM signed. See header dkim-signature.