Re: UMA zinit/zctor function error returning

On Fri, Jul 02, 2004 at 06:43:24PM +0000, Bosko Milekic wrote:
> 
> Brian Fundakowski Feldman wrote:
> > <URL:http://green.homeunix.org/~green/uma_error_checking.patch>
> 
>   Good work, overall.  I've looked at it, here are comments:
> 
>   - in mb_ctor_mbuf(), you need to free the mbuf if mac_init_mbuf()
>     failed, before you return error (SERIOUS).

These are the only two places that can call uz_ctor, aren't they?

uma_core.c:
			if (zone->uz_ctor) {
				if (zone->uz_ctor(item, zone->uz_keg->uk_size,
				    udata, flags) != 0) {
					uma_zfree_internal(zone, item, udata,
					    SKIP_DTOR);
					return (NULL);
				}
			}
			...
	if (zone->uz_ctor != NULL) {
		if (zone->uz_ctor(item, keg->uk_size, udata, flags) != 0) {
			uma_zfree_internal(zone, item, udata, SKIP_DTOR);
			return (NULL);
		}
	}


>   - return (0) should be "return 0" (STYLE).

That's not true according to style(9), even if it looks weird.

>   - in mb_ctor_pack(), you need to free the mbuf if mac_init_mbuf()
>     failed, before you return error (SERIOUS).

Same as above.

>   - you didn't make the change for keginit and you split out the
>     init function types into init (for zones) and keginit (for kegs).
>     However, keg inits should be able to fail too and it should
>     probably be the SAME type as the zone init.  This is a mistake.
>     It is especially a mistake since most zones actually use the
>     keg init, not the zone init (by default).  Only if you add
>     a secondary zone do you get to talk about zone inits.  Let me
>     know if this isn't clear for you.

Well, I only did that because there were no places that called for
a keg init being used in the first place (no calls to the
uma_zone_set_init() function).

>   - You seem to misunderstand where inits are called.  Refer to
>     my netbuf paper at www.unixdaemons.com/~bmilekic/netbuf_bmilekic.pdf
>     (specifically the diagrams) to see where the zone init and keg inits
>     are called; for example, I have no idea why you've now defined
>     zero_keginit and zero_init, this makes no sense.  This part of the
>     patch (including how you now cast to uma_keginit explicitly in
>     zone_ctor) needs to be redone.  I don't think you should split up
>     the init types (i.e., zone init and keg init have same types) and
>     have both of them be able to fail.  I know you might find this
>     tougher since you have to deal with freeing back to the slab from
>     the keg code on allocation failure, but that complexity is part of
>     this change's requirement.

The only reason for the two zero functions is that one has to return (0),
so they have different type signatures.  Where do you see a keginit
that you would want to be able to fail (or a keginit at all), other
than completeness's sake (which isn't really a bad reason).  I don't think
there is technically any problem now except for changing this for
completeness's sake; the only reason there's any cast at all is that
the zone constructor is shared between zones and kegs.

>   - the uma_dbg.c changes call the ctor explicitly from the fini.
>     I forget the exact reason this is, but now that we're expecting
>     the ctor to return something, if you are ignoring the return
>     value, please cast void explicitly here (STYLE).  Alternately,
>     make both keg and zone inits able to fail and propagate the failure,
>     if any, upwards.

I'm not sure what you mean here.  There are no return values I see ignored,
but a couple functions with similar names (trash_ctor()/mtrash_ctor()).

Thanks for taking a look,
-- 
Brian Fundakowski Feldman                           \'[ FreeBSD ]''''''''''\
  <> green_at_FreeBSD.org                               \  The Power to Serve! \
 Opinions expressed are my own.                       \,,,,,,,,,,,,,,,,,,,,,,\