Re: [PATCH 4/4] Create a new property value that means 'undefined'.

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

On Fri, Oct 22, 2010 at 12:50:58PM -0700, John Bonesio wrote:
> On Thu, 2010-10-21 at 17:14 +1100, David Gibson wrote:
> > On Wed, Oct 20, 2010 at 02:45:22PM -0700, John Bonesio wrote:
[snip]
> > > @@ -188,6 +197,25 @@ void add_property(struct node *node, struct property *prop)
> > >  	*p = prop;
> > >  }
> > >  
> > > +void remove_property(struct node *node, struct property *prop)
> > > +{
> > > +	struct property **p;
> > > +	int found = 0;
> > > +
> > > +	p = &node->proplist;
> > > +	while (*p) {
> > > +		if (*p == prop) {
> > > +			*p = (*p)->next;
> > > +			found = 1;
> > > +			break;
> > > +		}
> > > +		p = &((*p)->next);
> > > +	}
> > > +	/* property not in the node? it's probably an error, so flag it. */
> > > +	assert(found);
> > 
> > assert()s are for finding bugs in code, not detecting errors in user
> > input.  AFAICT this *can* be generated by usre input so it should
> > print an error message and if possible continue, not cause a SIGABRT.
> 
> I agree that assert() shouldn't be used if it can be caused by user
> input, but I don't see how the assert can be triggered by user input. In
> my mind this is a programming error.
> 
> I the only way I see this being triggered by user input is if the user
> specifies that a property be deleted but it doesn't exist in the current
> in-memory merged tree. I believe the merge_nodes() routine takes care
> to avoid problems here by making sure the property already exists in the
> in-memory merged tree before calling remove_property() on it.

Well, it does, but remove_property() is called *from* merge_nodes()
and it's called *before* the new property is added to the old node.
So, unless I'm missing something even more subtle, the user attempting
to remove a property which does not exist will indeed trigger this
assert().  This would make a good testcase...

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson