From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@citrix.com>
Subject: Re: [PATCH for 4.5] xl: Return proper error codes for
 block-attach and block-detach
Date: Thu, 13 Nov 2014 08:16:54 +0000
Message-ID: <1415866614.31613.24.camel@citrix.com>
References: <1415813493-25330-1-git-send-email-george.dunlap@eu.citrix.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: <1415813493-25330-1-git-send-email-george.dunlap@eu.citrix.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: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@citrix.com>, Wei Liu <wei.liu2@citrix.com>, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

On Wed, 2014-11-12 at 17:31 +0000, George Dunlap wrote:
> @@ -6444,6 +6445,7 @@ int main_blockdetach(int argc, char **argv)
>      rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
>      if (rc) {
>          fprintf(stderr, "libxl_device_disk_remove failed.\n");
> +        return 1;
>      }
>      libxl_device_disk_dispose(&disk);
>      return rc;

This one seems odd to me, you have inadvertently arranged to skip the
disk dispose and the old code returned non-zero in the failure case
already, since it returns rc.

If it is important to return 0 or 1 then perhaps the last line should
just be:
        return rc ? 1 : 0;
        
Or maybe the ultimate caller (i.e. the xl command dispatcher) ought to
be translating libxl ERROR_FOO into suitable process exit codes (perhaps
as simplistically as suggested above).

Skipping the dispose is a memory leak, albeit in a process which is just
about to die, but we like to try and keep xl "valgrind clean" so far as
possible such that leaks in the underlying libxl stand out.

Ian.