[pve-devel] [PATCH storage 1/5] Disks API: allow also unused disks for db/wal

[Thomas Lamprecht]

On 6/4/19 10:20 AM, Dominik Csapak wrote:
> On 6/4/19 9:54 AM, Thomas Lamprecht wrote:
>> On 6/4/19 9:21 AM, Dominik Csapak wrote:
>>> since we will create a pv/vg/lv on it with nautilus
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>>> ---
>>>   PVE/API2/Disks.pm | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/API2/Disks.pm b/PVE/API2/Disks.pm
>>> index ce4acee..7433066 100644
>>> --- a/PVE/API2/Disks.pm
>>> +++ b/PVE/API2/Disks.pm
>>> @@ -137,7 +137,7 @@ __PACKAGE__->register_method ({
>>>           my $entry = $disks->{$disk};
>>>           if ($type eq 'journal_disks') {
>>>           next if $entry->{osdid} >= 0;
>>> -        next if !$entry->{gpt} && $entry->{used} ne 'LVM';
>>> +        next if !$entry->{gpt} && $entry->{used} ne 'LVM' && $entry->{used};
>>
>> is the sub-expression ($entry->{used} ne 'LVM' && $entry->{used})
>> not somewhat redundant? Or should at least be switched, no point in
>> checking if it's a LWN if it's not even defined?
> 
> i guess you are right about the switching but no they are not redundant
> (i admit the code is not the cleanes one) because
> 
> it skips a disk if it is not gpt and it is used and used is not lvm
> 
> i will try to find a better condition that is easier to read

switching should already help, else if it's independent of gpt (prob not or?)
then a own condition just for used and ! LVM could be great too.

> 
>>
>> as get_disk from Diskmanage does:
>>
>> $disklist->{$dev}->{used} = $used if $used;
>>
>> it is either truthy or not defined at all, it cannot be defined but
>> falsy, so switching this would avoid a potential "undefined in ..
>> comparison" warning, or not?
>>
>>>           } elsif ($type eq 'unused') {
>>>           next if $entry->{used};
>>>           } elsif ($type ne '') {
>>>
>>