From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matan Azrad Subject: Re: [PATCH] net/failsafe: fix exec parameter parsing error flow Date: Wed, 30 Aug 2017 06:11:47 +0000 Message-ID: References: <1504018748-4766-1-git-send-email-matan@mellanox.com> <20170829163339.GP8124@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , Raslan Darawsheh , "stable@dpdk.org" To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= Return-path: In-Reply-To: <20170829163339.GP8124@bidouze.vm.6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Gaetan > -----Original Message----- > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > Sent: Tuesday, August 29, 2017 7:34 PM > To: Matan Azrad > Cc: dev@dpdk.org; Raslan Darawsheh ; > stable@dpdk.org > Subject: Re: [PATCH] net/failsafe: fix exec parameter parsing error flow >=20 > Hi Matan, >=20 > On Tue, Aug 29, 2017 at 05:59:08PM +0300, Matan Azrad wrote: > > The corrupted code returns success value in case of the execution > > process output stream is empty(EOF). > > It causes to segmentation fault while failsafe polls this command line > > again, than gets success and tries to do hotplug add to the sub device > > by uninitialized pointer dereferencing. > > >=20 > This is a bug and should be fixed, thanks. >=20 > > Morever, when the output is not empty but uncorrect, failsafe returns > > error for its probe function while the expected behavior is to do > > polling until the output is correct. > > >=20 > The expected behavior is for the fail-safe to return an error if the exec= ution > of the given command returns an error. >=20 > The intention is that users writing such script would be able to output a= blank > lines in case there is nothing to probe, but still remain aware of issues= during > the execution of the command. >=20 > The fail-safe ignores errors pertaining to absent devices due to its natu= re. > This does not mean that it should ignore all errors and try to keep on go= ing > while everything else is on fire. >=20 > The contract with the user is that "blank line" without other errors mean= s > "absent device". Garbled output or return code !=3D 0 means runtime error > and should be thrown to the user / application. >=20 OK, good, I would have signed this contract :) What's about if the parsing is not empty and out with error in the polling = process? I think in current code failsafe just continues normally and tries again on= next polling time. Because of this code I thought that if error occurs we should poll it again= ... Can you please add it (the contract) in failsafe documentation for exec par= ameter? > > The fix changes the return value to be -ENODEV for this sub device in > > the two cases. > > By this way, failsafe tries to parse this sub device parameter by exec > > method until the output is correct. > > >=20 > The issue is that this portion of the code will be heavily modified anywa= y. The > errno handling is erroneous and must be fixed, which is in conflict with = your > patch. >=20 > I will send the intended fix shortly, referencing this patch and the issu= e your > highlighted, but both patch won't be compatible. >=20 Good, no problems. > > Fixes: a0194d828100 ("net/failsafe: add flexible device definition") > > Fixes: 35ffe4208140 ("net/failsafe: fix missing pclose after popen") > > Cc: stable@dpdk.org > > > > Signed-off-by: Matan Azrad > > --- > > drivers/net/failsafe/failsafe_args.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/failsafe/failsafe_args.c > > b/drivers/net/failsafe/failsafe_args.c > > index 645c885..61c55df 100644 > > --- a/drivers/net/failsafe/failsafe_args.c > > +++ b/drivers/net/failsafe/failsafe_args.c > > @@ -157,12 +157,16 @@ fs_execute_cmd(struct sub_device *sdev, char > *cmdline) > > ret =3D fs_parse_device(sdev, output); > > if (ret) { > > ERROR("Parsing device '%s' failed", output); > > + ret =3D -ENODEV; Remove the above line for probe function error report. > > goto ret_pclose; > > } > > ret_pclose: > > pclose_ret =3D pclose(fp); > > if (pclose_ret) { > > - pclose_ret =3D errno; > > + if (errno =3D=3D 0) > > + errno =3D -(pclose_ret =3D ret); > > + else > > + pclose_ret =3D errno; > > ERROR("pclose: %s", strerror(errno)); > > errno =3D old_err; > > return pclose_ret; > > -- > > 2.7.4 > > >=20 > Best regards, > -- > Ga=EBtan Rivet > 6WIND Thanks, Matan Azrad