On Mon, Mar 17, 2025 at 11:51:52PM +0000, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@xxxxxxxxxxxxxxxxxxx> Sent: Wednesday, February 26, 2025 3:08 PM
[...]
> > +static long
> > +mshv_vp_ioctl_get_set_state(struct mshv_vp *vp,
> > + struct mshv_get_set_vp_state __user *user_args,
> > + bool is_set)
> > +{
> > + struct mshv_get_set_vp_state args;
> > + long ret = 0;
> > + union hv_output_get_vp_state vp_state;
> > + u32 data_sz;
> > + struct hv_vp_state_data state_data = {};
> > +
> > + if (copy_from_user(&args, user_args, sizeof(args)))
> > + return -EFAULT;
> > +
> > + if (args.type >= MSHV_VP_STATE_COUNT || mshv_field_nonzero(args, rsvd) ||
> > + !args.buf_sz || !PAGE_ALIGNED(args.buf_sz) ||
> > + !PAGE_ALIGNED(args.buf_ptr))
> > + return -EINVAL;
> > +
> > + if (!access_ok((void __user *)args.buf_ptr, args.buf_sz))
> > + return -EFAULT;
> > +
> > + switch (args.type) {
> > + case MSHV_VP_STATE_LAPIC:
> > + state_data.type = HV_GET_SET_VP_STATE_LAPIC_STATE;
> > + data_sz = HV_HYP_PAGE_SIZE;
> > + break;
> > + case MSHV_VP_STATE_XSAVE:
>
> Just FYI, you can put a semicolon after the colon on the above line, which
> adds a null statement, and then the C compiler will accept the definition
> of local variable data_sz_64 without needing the odd-looking braces.
>
> See https://stackoverflow.com/questions/92396/why-cant-variables-be-declared-in-a-switch-statement/19830820
>
This is a rarely seen pattern in the kernel, so I would prefer to keep
the braces for clarity.
$ git grep -A5 -P 'case\s+\w+:;$'
This shows a few places are using this pattern. But they are not
declaring variables afterwards.
> I learn something new every day! :-)
>
Yep, me too.
Thanks for reviewing the code. Nuno will address the comments. I can fix
them up.
Thanks,
Wei.
