Re: [1/1] connector/CBUS: new messaging subsystem. Revision number next.

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On 4/26/05, Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> On Tue, 26 Apr 2005 10:57:55 -0500
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > Hi Evgeniy,
> >
> > On 4/11/05, Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> > > /*****************************************/
> > > Kernel Connector.
> > > /*****************************************/
> > ...
> > > +static int cn_call_callback(struct cn_msg *msg, void (*destruct_data) (void *), void *data)
> > > +{
> > > +       struct cn_callback_entry *__cbq;
> > > +       struct cn_dev *dev = &cdev;
> > > +       int found = 0;
> > > +
> > > +       spin_lock_bh(&dev->cbdev->queue_lock);
> > > +       list_for_each_entry(__cbq, &dev->cbdev->queue_list, callback_entry) {
> > > +               if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
> > > +                       __cbq->cb->priv = msg;
> > > +
> > > +                       __cbq->ddata = data;
> > > +                       __cbq->destruct_data = destruct_data;
> > > +
> > > +                       queue_work(dev->cbdev->cn_queue, &__cbq->work);
> >
> > It looks like there is a problem with the code. As far as I can see
> > there is only one cn_callback_entry associated with each callback. So,
> > if someone sends netlink messages with the same id at a high enough
> > rate (so cbdev's work queue does not get a chance to get scheduled and
> > process pending requests) ddata and the destructor will be overwritten
> > which can lead to memory leaks and non-delivery of some messages.
> >
> > Am I missing something?
> 
> Connector needs to check return value here - zero means
> that work was already queued and we must free shared skb.
> 
> There may not be the same work with different data.
> 

Ugh, that really blows. Now every user of a particular message type
has to coordinate efforts with other users of the same message type...

Imability to "fire and forget" undermines usefulness of whole
connector. How will you for example implement hotplug notification
over connector? Have kobject_hotplug wait and block other instances?
But wait on what?

-- 
Dmitry