From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@citrix.com>
Subject: Re: [PATCH] libxl: new xlu_disk_parse function
Date: Sat, 2 Apr 2011 09:51:24 +0100
Message-ID: <1301734284.3516.16.camel@localhost.localdomain>
References: <1300988989-18305-1-git-send-email-ian.jackson@eu.citrix.com>
	<1300988989-18305-2-git-send-email-ian.jackson@eu.citrix.com>
	<1301333477.1691.7.camel@qabil.uk.xensource.com>
	<19860.47659.19132.816771@mariner.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <19860.47659.19132.816771@mariner.uk.xensource.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Gianni Tedesco <gianni.tedesco@citrix.com>
List-Id: xen-devel@lists.xenproject.org

On Thu, 2011-03-31 at 18:30 +0100, Ian Jackson wrote:
> Gianni Tedesco writes ("Re: [Xen-devel] [PATCH] libxl: new xlu_disk_parse function"):
> > On Thu, 2011-03-24 at 17:49 +0000, Ian Jackson wrote:
> > > +       return;
> > 
> >        case 1: ??
> 
> Deliberately omitted.
> 
> > > +    case 2:
> > > +    case 3:
> > > +    case 4:
> > > +       break;
> > 
> > Or does it belong here? In which case aborting on a parse error is bad
> > juju.
> >        case 1:
> > > +    default:
> > > +       abort();
> 
> I could add it there for clarity.  The regexp will always match
> capturing with 2, 3 or 4 parens.  None of the other errors from
> dfa_exec are applicable.  So anything other than 2,3,4 or "did not
> match" is due to a bug in the code, not merely bogus input.  Perhaps
> this should be mentioned in a comment.

I think that's a good idea, you'll only up nacking an endless stream of
fixup patches otherwise (inevitably adding the case in the wrong
place...).

IMHO a switch statement in this context obfuscates the error handling
and a couple of error handling if statements would be more obvious e.g.

	if (rc == PCRE_ERROR_NOMATCH) {
		xlu__disk_err(dpc, "bad syntax for target, or missing vdev");
		return;
	}

	/* The regexp will always match capturing with 2, 3 or 4 parens */
	if (rc < 2 || rc > 4)
		abort();

	.. carry on...