Re: [PATCH v7 3/3] tcm_vhost: Add hotplug/hotunplug support

[Rusty Russell <rusty@rustcorp.com.au>]

Asias He <asias@redhat.com> writes:
> On Mon, Apr 22, 2013 at 04:17:04PM +0300, Michael S. Tsirkin wrote:
>> > > > > > > > +	evt = kzalloc(sizeof(*evt), GFP_KERNEL);
>> > > > > > > 
>> > > > > > > I think kzalloc not needed here, you init all fields.
>> > > > > > 
>> > > > > > Not really! evt->event.lun[4-7] is not initialized. It needs to be 0.
>> > > > > 
>> > > > > So that is 4 bytes just init them when you set rest of lun.
>> > > > 
>> > > > It is not in the fast path. You can do it this way but not a must.
>> > > 
>> > > I think that's a bit cleaner than relying on kzalloc to zero-initialize.
>> > 
>> > I think it is a bit shorter, 4 lines saved. 
>> 
>> OTOH you don't have to think "what about the rest of the bytes?" then
>> hunt through the code and go "ah, it's kzalloc".
>> Looks it's a style issue but I prefer explicit initialization.
>
> It is just make none sense to argue about this. If you really want you
> can make patches to remove all the lines where use kzalloc because it is
> cleaner.

I prefer explicit initialization too, FWIW.

>> > > > > > > > +static void tcm_vhost_do_evt_work(struct vhost_scsi *vs,
>> > > > > > > > +	struct tcm_vhost_evt *evt)
>> > > > > > > > +{
>> > > > > > > > +	struct vhost_virtqueue *vq = &vs->vqs[VHOST_SCSI_VQ_EVT];
>> > > > > > > > +	struct virtio_scsi_event *event = &evt->event;
>> > > > > > > > +	struct virtio_scsi_event __user *eventp;
>> > > > > > > > +	unsigned out, in;
>> > > > > > > > +	int head, ret;
>> > > > > > > > +
>> > > > > > > > +	if (!tcm_vhost_check_endpoint(vq))
>> > > > > > > > +		return;
>> > > > > > > > +
>> > > > > > > > +	mutex_lock(&vs->vs_events_lock);
>> > > > > > > > +	mutex_lock(&vq->mutex);
>> > > > > > > > +again:
>> > > > > > > > +	vhost_disable_notify(&vs->dev, vq);
>> > > > > > > > +	head = vhost_get_vq_desc(&vs->dev, vq, vq->iov,
>> > > > > > > > +			ARRAY_SIZE(vq->iov), &out, &in,
>> > > > > > > > +			NULL, NULL);
>> > > > > > > > +	if (head < 0) {
>> > > > > > > > +		vs->vs_events_dropped = true;
>> > > > > > > > +		goto out;
>> > > > > > > > +	}
>> > > > > > > > +	if (head == vq->num) {
>> > > > > > > > +		if (vhost_enable_notify(&vs->dev, vq))
>> > > > > > > > +			goto again;
>> > > > > > > 
>> > > > > > > Please write loops with while() or for().
>> > > > > > > Not with goto. goto is for error handling.
>> > > > > > 
>> > > > > > This makes extra indention which is more ugly.
>> > > > > 
>> > > > > I don't care. No loops with goto and that's a hard rule.
>> > > > 
>> > > > It is not a loop.
>> > > 
>> > > If same code repeats, it's a loop.
>> > 
>> > We really need here is to call vhost_get_vq_desc once. It's not a loop
>> > like other places to use while() or for() with vhost_get_vq_desc.
>> 
>> Exactly. Like a mutex for example? We only need to
>> lock it once. See __mutex_lock_common - uses a for loop,
>> doesn't it?
>
> So what?
>
>> So fix this please, use while or for or even until.
>
> I do not think it is necessary.

I tend to use again: in such corner cases, where I don't expect it to
be invoked very often.  A do loop looks too normal.

I do exactly the same thing in virtio-net.c, for the same case.

Would you like me to review the entire thing?

Cheers,
Rusty.