Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> > > > @@ -1301,8 +1346,7 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> > > >
> > > >  	/* Check that all device private data has been freed */
> > > >  	for (i = 0; i < nb_drivers; i++) {
> > > [Fiona] Use the sess.nb_drivers rather than the global.
> >
> > Ok, though doesn't really matter here.
> > get_sym_session_private_data() will return NULL for invalid index anyway.
> [Fiona] Except that you removed the NULL check below and are using that
> invalid index to deref the array.

Ah yes, you right - have to use sess.nb_drivers here.

> 
> > > Actually maybe name slightly differently to reduce the chance of that mistake happening, e.g.
> > > rename sess.nb_drivers to sess.num_drivers?
> > >
> > > > -		sess_priv = get_sym_session_private_data(sess, i);
> > > > -		if (sess_priv != NULL)
> > > > +		if (sess->sess_data[i].refcnt != 0)
> > > >  			return -EBUSY;
> > > >  	}
> > > >
> > > > @@ -1313,6 +1357,23 @@ rte_cryptodev_sym_session_free(struct rte_cryptodev_sym_session *sess)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +unsigned int
> > > > +rte_cryptodev_sym_session_max_data_size(void)
> > > [Fiona] Suggest renaming this
> > > rte_cryptodev_sym_session_max_array_size()
> >
> > I usually don't care much about names, but that seems just confusing:
> > totally unclear which array we are talking about.
> > Any better naming ideas?
> [Fiona] Isn't it returning the size of the array of structs in the session hdr?
> Seems a bit more informative than "data".

Yes, but user doesn't have to know it is an array of pointers or
something different.

> Can't think of anything better, if you find array confusing then I
> don't feel that strongly about it, stick with data.
> Or just get rid of the function altogether and put inside
> rte_cryptodev_sym_session_max_size()
> Unless it's called elsewhere.

Sounds like a good option.
Konstantin