From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [Patch v3 3/4] tools/libxl: Fix
	libxl__device_nic_from_xs_be()
Date: Tue, 26 Nov 2013 11:42:35 +0000
Message-ID: <5294892B.5030003@citrix.com>
References: <52939C67.7020108@citrix.com>
	<1385412567-7717-1-git-send-email-andrew.cooper3@citrix.com>
	<21140.34516.685325.920576@mariner.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <21140.34516.685325.920576@mariner.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>, Ian Campbell <Ian.Campbell@citrix.com>, Xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

On 26/11/13 11:32, Ian Jackson wrote:
> Andrew Cooper writes ("[Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> -    tmp = xs_read(ctx->xsh, XBT_NULL,
>> -                  libxl__sprintf(gc, "%s/handle", be_path), &len);
>> -    if ( tmp )
>> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
>> +                                libxl__sprintf(gc, "%s/handle", be_path),
>> +                                &tmp);
>> +
>> +    if ((rc == 0) && strlen(tmp))
> Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> (for the benefit of Ian C.)
>
> This is not correct.  See the doc comment for libxl__xs_read_checked:
>
>     /* On success, *result_out came from the gc.
>      * On error, *result_out is undefined.
>      * ENOENT counts as success but sets *result_out=0
>      */
>     int libxl__xs_read_checked(libxl__gc *gc, xs_transaction_t t,
>                                const char *path, const char **result_out);
>
> So the correct pattern is:
>
>     rc = libxl__xs_read_checked(gc, XBT_NULL, blah blah blah, &tmp);
>     if (rc) goto out;
>
>     if (tmp) {
>         use tmp;
>     } else {
>         the path doesn't exist, do the other thing;
>     }
>
> I don't think there should be any need to check for empty strings
> written to xenstore here ?  The old code doesn't.  Please someone tell
> me there isn't.
>
> Thanks,
> Ian.

Ah - I think I have gotten the wrong indirection on tmp when attempting
to apply the documented ENOENT behaviour.

As this function cant fail, I was trying to force all error paths to
apply safe defaults to the libxl_device_nic structure.

I believe substituting the strlen(tmp) check for NULL checks will
produce the intended behaviour?

~Andrew