Re: [pve-devel] [PATCH librados2-perl 2/6] mon_command: optionally ignore errors and return hashmap

[Aaron Lauterer <a.lauterer@proxmox.com>]

On 2/21/22 16:44, Thomas Lamprecht wrote:
> On 18.02.22 12:38, Aaron Lauterer wrote:
>> This patch requires patch 3 of the series to not break OSD removal!
>> Therefore releasing a new version of librados2-perl and pve-manager
>> needs to be coordinated.
> 
> I don't like that and think it can be avoided.

I'll look into it again.

>> [...]
>>   sub mon_command {
>> -    my ($self, $cmd) = @_;
>> +    my ($self, $cmd, $noerr) = @_;
>> +
>> +    $noerr = 0 if !$noerr;
>>   
>>       $cmd->{format} = 'json' if !$cmd->{format};
>>   
>>       my $json = encode_json($cmd);
>>   
>> -    my $raw = eval { $sendcmd->($self, 'M', $json) };
>> +    my $ret = eval { $sendcmd->($self, 'M', $json, undef, $noerr) };
> 
> I'd rather like to avoid chaining through that $noerr every where, rather pass all the
> info via the die (errors can be references to structured data too, like PVE::Exception is),
> or just avoid the die at the lower level completely and map the error inside the struct
> too, it can then be thrown here, depending on $noerr parameters or what not.

Okay, definitely sounds like the cleaner approach.

>> [...]
>> -    if (ret < 0) {
>> +    if (ret < 0 && noerr == false) {
>>           char msg[4096];
>>           if (outslen > sizeof(msg)) {
>>               outslen = sizeof(msg);
>> @@ -142,9 +143,22 @@ CODE:
>>           die(msg);
>>       }
>>   
>> -    RETVAL = newSVpv(outbuf, outbuflen);
>> +    char status[(int)outslen + 1];
> 
> this is on the stack and could be to large to guarantee it always fits, but...
> 
>> +    if (outslen > sizeof(status)) {
>> +	outslen = sizeof(status);
>> +    }
>> +    snprintf(status, sizeof(status), "%.*s\n", (int)outslen, outs);
> 
> ...why not just chain outs through instead of re-allocating it and writing it out in a
> relatively expensive way?
> 

Yeah, will do that.