Re: [Qemu-devel] [PATCH v6 8/8] linux-user: Add support for FDSETEMSGTRESH, FDSETMAXERRS, and FDGETMAXERRS ioctls

[Laurent Vivier]

Le 10/09/2019 à 21:15, Aleksandar Markovic a écrit :
> 
> 10.09.2019. 20.58, address@hidden
> <mailto:address@hidden> је написао/ла:
>>
>>
>> 06.09.2019. 12.47, "Laurent Vivier" <address@hidden
> <mailto:address@hidden>> је написао/ла:
>> >
>> > Le 04/09/2019 à 14:59, Aleksandar Markovic a écrit :
>> > > From: Aleksandar Markovic <address@hidden
> <mailto:address@hidden>>
>> > >
>> > > FDSETEMSGTRESH, FDSETMAXERRS, and FDGETMAXERRS ioctls are commands
>> > > for controlling error reporting of a floppy drive.
>> > >
>> > > Signed-off-by: Aleksandar Markovic <address@hidden
> <mailto:address@hidden>>
>> > > ---
>> > >  linux-user/ioctls.h        |  2 ++
>> > >  linux-user/syscall_defs.h  | 19 +++++++++++++++++++
>> > >  linux-user/syscall_types.h |  7 +++++++
>> > >  3 files changed, 28 insertions(+)
>> > >
>> > > diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
>> > > index 622874b..0c75d03 100644
>> > > --- a/linux-user/ioctls.h
>> > > +++ b/linux-user/ioctls.h
>> > > @@ -118,6 +118,8 @@
>> > >       IOCTL(FDFMTTRK, IOC_W, MK_PTR(MK_STRUCT(STRUCT_format_descr)))
>> > >       IOCTL(FDFMTEND, 0, TYPE_NULL)
>> > >       IOCTL(FDFLUSH, 0, TYPE_NULL)
>> > > +     IOCTL(FDSETMAXERRS, IOC_W,
> MK_PTR(MK_STRUCT(STRUCT_floppy_max_errors)))
>> > > +     IOCTL(FDGETMAXERRS, IOC_R,
> MK_PTR(MK_STRUCT(STRUCT_floppy_max_errors)))
>> >
>> > where is FDSETEMSGTRESH?
>> >
>> > >       IOCTL(FDRESET, 0, TYPE_NULL)
>> > >       IOCTL(FDRAWCMD, 0, TYPE_NULL)
>> > >       IOCTL(FDTWADDLE, 0, TYPE_NULL)
>> > > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
>> > > index 834a085..7c5b614 100644
>> > > --- a/linux-user/syscall_defs.h
>> > > +++ b/linux-user/syscall_defs.h
>> > > @@ -890,12 +890,31 @@ struct target_pollfd {
>> > > 
>> > >  /* From <linux/fd.h> */
>> > > 
>> > > +struct target_floppy_max_errors {
>> > > +    abi_uint        abort;
>> > > +    abi_uint        read_track;
>> > > +    abi_uint        reset;
>> > > +    abi_uint        recal;
>> > > +    abi_uint        reporting;
>> > > +};
>> >
>> > You don't need this, you can use floppy_max_errors from <linux/fd.h>.
>> >
>> > But you can define it if you want because it is used to know the size of
>> > the target structure (and if alignment or data types differ it can
>> > mismatch. With "int" it's not the case).
>> >
>>
>> Laurent, thanks for the review, I'll correct this in the next version.
>>
>> Just a follow-up question:
>>
>> If the structure of related to a (not-yet-supported in QEMU) ioctl was:
>>
>> struct hd_geometry {
>>       unsigned char heads;
>>       unsigned char sectors;
>>       unsigned short cylinders;
>>       unsigned long start;
>> };
>>
>> ... would "target_hd_geometry" be needed, or not?
>>
> 
> Actually, that ioctl is already implemented in QEMU (HDIO_GETGEO),
> without defining target_hd_geometry. Is this fine?

No, for instance you can check it:

#include <stdio.h>
#include <stddef.h>
#include <linux/hdreg.h>

int main(void)
{
    printf("heads %zd\n", offsetof(struct hd_geometry, heads));
    printf("sectors %zd\n", offsetof(struct hd_geometry, sectors));
    printf("cylinders %zd\n", offsetof(struct hd_geometry, cylinders));
    printf("start %zd\n", offsetof(struct hd_geometry, start));
}

$ cc -o test_align test_align.c

on i386:

$ ./test_align
heads 0
sectors 1
cylinders 2
start 4

on x86_64:

$ ./test_align
heads 0
sectors 1
cylinders 2
start 8

Thanks,
Laurent