netdev - RE: [PATCH v2 RESEND] xen-netback: prefer xenbus_scanf() over xenbus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 25 Oct 2016 02:22:31 -0600
From:   "Jan Beulich" <JBeulich@...e.com>
To:     "Paul Durrant" <Paul.Durrant@...rix.com>
Cc:     "David Vrabel" <david.vrabel@...rix.com>,
        "Wei Liu" <wei.liu2@...rix.com>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        "boris.ostrovsky@...cle.com" <boris.ostrovsky@...cle.com>,
        "Juergen Gross" <JGross@...e.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH v2 RESEND] xen-netback: prefer xenbus_scanf() over
 xenbus_gather()

>>> On 25.10.16 at 09:52, <Paul.Durrant@...rix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@...e.com]
>> Sent: 24 October 2016 16:08
>> --- 4.9-rc2/drivers/net/xen-netback/xenbus.c
>> +++ 4.9-rc2-xen-netback-prefer-xenbus_scanf/drivers/net/xen-netback/xenbus.c
>> @@ -889,16 +889,16 @@ static int connect_ctrl_ring(struct back
>>  	unsigned int evtchn;
>>  	int err;
>> 
>> -	err = xenbus_gather(XBT_NIL, dev->otherend,
>> -			    "ctrl-ring-ref", "%u", &val, NULL);
>> -	if (err)
>> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
>> +			   "ctrl-ring-ref", "%u", &val);
>> +	if (err <= 0)
> 
> Looking at other uses of xenbus_scanf() in the same code I think the check 
> here should be if (err < 0). It's a nit, since xenbus_scanf() cannot return 0, 
> but it would be better for consistency I think.

Hmm, this goes back to the discussion following from
https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00678.html
which in fact you had given your R-b back then. I continue to be
of the opinion that callers should not leverage the fact that
xenbus_scanf() can't return zero. They instead should check for
an explicit success indicator (which only positive values are). But
you're the maintainer of the code, so if you now think the same
way David does, I guess I'll have to make the adjustment.

>>  		goto done; /* The frontend does not have a control ring */
>> 
>>  	ring_ref = val;
>> 
>> -	err = xenbus_gather(XBT_NIL, dev->otherend,
>> -			    "event-channel-ctrl", "%u", &val, NULL);
>> -	if (err) {
>> +	err = xenbus_scanf(XBT_NIL, dev->otherend,
>> +			   "event-channel-ctrl", "%u", &val);
>> +	if (err <= 0) {
>>  		xenbus_dev_fatal(dev, err,
>>  				 "reading %s/event-channel-ctrl",
>>  				 dev->otherend);
>> @@ -919,7 +919,7 @@ done:
>>  	return 0;
>> 
>>  fail:
>> -	return err;
>> +	return err ?: -ENODATA;
> 
> I don't think you need this.

If the other change gets made, then indeed this isn't needed.

Jan