Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI

[Sheng Yang <sheng@linux.intel.com>]

On Sat, Dec 27, 2008 at 05:27:25PM -0200, Marcelo Tosatti wrote:
> > > > > +static int kvm_vm_ioctl_request_gsi_msg(struct kvm *kvm,
> > > > > +					struct kvm_assigned_gsi_msg *agsi_msg)
> > > > > +{
> > > > > +	struct kvm_gsi_msg gsi_msg;
> > > > > +	int r;
> > > > > +
> > > > > +	gsi_msg.gsi = agsi_msg->gsi;
> > > > > +	gsi_msg.msg.address_lo = agsi_msg->msg.addr_lo;
> > > > > +	gsi_msg.msg.address_hi = agsi_msg->msg.addr_hi;
> > > > > +	gsi_msg.msg.data = agsi_msg->msg.data;
> > > > > +
> > > > > +	r = kvm_update_gsi_msg(kvm, &gsi_msg);
> > > > > +	if (r == 0)
> > > > > +		agsi_msg->gsi = gsi_msg.gsi;
> > > > > +	return r;
> > > > > +}
> > > >
> > > > Can't see the purpose of this function. Why preserve the user-passed GSI
> > > > value in case of failure? It will return an error anyway...
> > >
> > Oh, we preserved the user-passed GSI in case of userspace want to update one 
> > entry. :)
> 
> The structure is not copied back to userspace in case of failure anyway:
> 
> +           r = kvm_vm_ioctl_request_gsi_msg(kvm, &agsi_msg);
> +               if (r)
> +                       goto out;
> +               r = -EFAULT;
> +           if (copy_to_user(argp, &agsi_msg, sizeof agsi_msg))
> +                   goto out;
> 
> So I don't see the point of doing this?
> 

Oh, Marcelo, finally I got your meaning. I misunderstand you at first time.
And now I think you just means: Why "if (r == 0)" is there? It's unnecessary!

Yes your are right. It can be called a little habit, and preseved value is
useless at all, and maybe I just want to indicate what's the legal return
value here. And from the other side, "if (r != 0)", the value should be
meaningless and assigning it to another variable may not proper.

--
regards
Yang, Sheng