Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver

[Timur Tabi <timur@freescale.com>]

Arnd Bergmann wrote:
> As mentioned, it would be easier and more readable to just do
> 
> 	switch(cmd) {
> 	case FSL_HV_IOCTL_PARTITION_RESTART:
> 		...
> 
> 	case FSL_HV_IOCTL_PARTITION_GET_STATUS;
> 		...
> 
> There is no need to check the bits individually when you can simply
> compare the command number.

But this will break backwards compatibility with older applications that used
the union as the size parameter.  Although these applications won't compile with
the new header file, older already-compiled applications still work.

I will eventually update the applications to use the new header file, and at
that point I will modify the switch statement as you suggest.  But until then,
I'd like to keep the code as-is.

>> > +enum fsl_hv_ioctl_cmd {
>> > +       FSL_HV_IOCTL_PARTITION_RESTART = _IOWR(0, 1, struct fsl_hv_ioctl_restart),
>> > +       FSL_HV_IOCTL_PARTITION_GET_STATUS = _IOWR(0, 2, struct fsl_hv_ioctl_status),
>> > +       FSL_HV_IOCTL_PARTITION_START = _IOWR(0, 3, struct fsl_hv_ioctl_start),
>> > +       FSL_HV_IOCTL_PARTITION_STOP = _IOWR(0, 4, struct fsl_hv_ioctl_stop),
>> > +       FSL_HV_IOCTL_MEMCPY = _IOWR(0, 5, struct fsl_hv_ioctl_memcpy),
>> > +       FSL_HV_IOCTL_DOORBELL = _IOWR(0, 6, struct fsl_hv_ioctl_doorbell),
>> > +       FSL_HV_IOCTL_GETPROP = _IOWR(0, 7, struct fsl_hv_ioctl_prop),
>> > +       FSL_HV_IOCTL_SETPROP = _IOWR(0, 8, struct fsl_hv_ioctl_prop),
>> > +};

> Using a #define here is usually preferred because then you can use #ifdef in a user
> application to check if a given value has been assigned.

You're right -- I had enum on the brain.

> More importantly, the code you have chose (0) conflicts with existing drivers
> (frame buffer, scsi and wavefront among others). Please chose a free one and
> add it to Documentation/ioctl/ioctl-number.txt in the same patch.

Ok, I was really hoping to avoid doing this.  Like I said, binary compatibility
is important, and changing the type will break my existing apps.  Are you
insisting that I pick a new number?

-- 
Timur Tabi
Linux kernel developer at Freescale