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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
