From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D0B9862449 for ; Mon, 21 Feb 2022 16:44:50 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C09791B764 for ; Mon, 21 Feb 2022 16:44:20 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 0309C1B759 for ; Mon, 21 Feb 2022 16:44:20 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C96E1462F2 for ; Mon, 21 Feb 2022 16:44:19 +0100 (CET) Message-ID: Date: Mon, 21 Feb 2022 16:44:18 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:98.0) Gecko/20100101 Thunderbird/98.0 Content-Language: en-US To: Proxmox VE development discussion , Aaron Lauterer References: <20220218113827.1415641-1-a.lauterer@proxmox.com> <20220218113827.1415641-3-a.lauterer@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20220218113827.1415641-3-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.058 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH librados2-perl 2/6] mon_command: optionally ignore errors and return hashmap X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Feb 2022 15:44:50 -0000 On 18.02.22 12:38, Aaron Lauterer wrote: > In some situations, we do not want to abort if the Ceph API returns an > error (ret != 0). We also want to be able to access all the retured > values from the Ceph API (return code, status, data). > > One such situation can be the 'osd ok-to-stop' call where Ceph will > return a non zero code if it is not okay to stop the OSD, but will also > have useful information in the status buffer that we can use to show the > user. > > For this, let's always return a hashmap with the return code, the status > message and the returned data from RADOS.xs::mon_command. Then decide on > the Perl side (RADOS.pm::mon_command) if we return the scalar data as we > used to, or the contents of the hashmap in an array. > > The new parameter 'noerr' is used to indicate that we want to proceed on > non-zero return values. Omitting it gives us the old behavior to die > with the status message. > > The new parameter needs to be passed to the child process, which causes > some changes there and the resulting hashmaps gets JSON encoded to be > passed back up to the parent process. should be avoidable, the child can always pass through the new info and the actual perl code can do the filtering. > > Signed-off-by: Aaron Lauterer > --- > 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. > > Please have a closer look at the C code that I wrote in RADOS.xs as I > have never written much C and am not sure if I introduced some nasty bug > / security issue. > > PVE/RADOS.pm | 37 ++++++++++++++++++++++--------------- > RADOS.xs | 26 ++++++++++++++++++++------ > 2 files changed, 42 insertions(+), 21 deletions(-) > > @@ -259,19 +259,26 @@ sub cluster_stat { > # example1: { prefix => 'get_command_descriptions'}) > # example2: { prefix => 'mon dump', format => 'json' } > 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. > die "error with '$cmd->{prefix}': $@" if $@; > > + my $raw = decode_json($ret); > + > + my $data = ''; > if ($cmd->{format} && $cmd->{format} eq 'json') { > - return length($raw) ? decode_json($raw) : undef; > + $data = length($raw->{data}) ? decode_json($raw->{data}) : undef; > + } else { > + $data = $raw->{data}; > } > - return $raw; > + return wantarray ? ($raw->{code}, $raw->{status}, $data) : $data; > } > > > diff --git a/RADOS.xs b/RADOS.xs > index 1eb0b5a..3d828e1 100644 > --- a/RADOS.xs > +++ b/RADOS.xs > @@ -98,11 +98,12 @@ CODE: > rados_shutdown(cluster); > } > > -SV * > -pve_rados_mon_command(cluster, cmds) > +HV * > +pve_rados_mon_command(cluster, cmds, noerr=false) > rados_t cluster > AV *cmds > -PROTOTYPE: $$ > +bool noerr > +PROTOTYPE: $$;$ > CODE: > { > const char *cmd[64]; > @@ -129,7 +130,7 @@ CODE: > &outbuf, &outbuflen, > &outs, &outslen); > > - 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? > + > + HV * rh = (HV *)sv_2mortal((SV *)newHV()); > + > + (void)hv_store(rh, "code", 4, newSViv(ret), 0); > + (void)hv_store(rh, "data", 4, newSVpv(outbuf, outbuflen), 0); > + (void)hv_store(rh, "status", 6, newSVpv(status, sizeof(status) - 1), 0); > + RETVAL = rh; > > - rados_buffer_free(outbuf); > + if (outbuf != NULL) { > + rados_buffer_free(outbuf); > + } > > if (outs != NULL) { > rados_buffer_free(outs);