On Tue, Apr 23, 2013 at 01:48:51PM +0930, Rusty Russell wrote:
> Asias He <asias@xxxxxxxxxx> 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.
I do not really care. I am OK with either way.
> >> > > > > > > > +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?
Sure. Thanks Rusty!
> Cheers,
> Rusty.
--
Asias
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
