Re: [PATCH v2 17/18] mini-os: use function vectors instead of switch for file operations

[Andrew Cooper <amc96@xxxxxxxx>]

On 12/01/2022 07:34, Juergen Gross wrote:
> On 11.01.22 19:08, Andrew Cooper wrote:
>> On 11/01/2022 14:58, Juergen Gross wrote:
>>> +
>>> +off_t lseek_default(int fd, off_t offset, int whence);
>>> +bool select_yes(int fd);
>>> +bool select_read_flag(int fd);
>>> +
>>>   LIST_HEAD(evtchn_port_list, evtchn_port_info);
>>>     struct evtchn_port_info {
>>> @@ -183,7 +210,7 @@ struct evtchn_port_info {
>>>   };
>>>     struct file {
>>> -    enum fd_type type;
>>> +    unsigned int type;
>>>       bool read;    /* maybe available for read */
>>>       off_t offset;
>>>       union {
>>> @@ -199,7 +226,7 @@ struct file {
>>>   extern struct file files[];
>>>     struct file *get_file_from_fd(int fd);
>>> -int alloc_fd(enum fd_type type);
>>> +int alloc_fd(unsigned int type);
>>>   void close_all_files(void);
>>>   extern struct thread *main_thread;
>>>   void sparse(unsigned long data, size_t size);
>>> diff --git a/lib/sys.c b/lib/sys.c
>>> index 323a7cd..c327247 100644
>>> --- a/lib/sys.c
>>> +++ b/lib/sys.c
>>> @@ -98,6 +98,39 @@ struct file files[NOFILE] = {
>>>       { .type = FTYPE_CONSOLE }, /* stderr */
>>>   };
>>>   +static struct file_ops file_ops_none = {
>>> +    .name = "none",
>>> +};
>>> +
>>> +static struct file_ops *file_ops[FTYPE_N + FTYPE_SPARE] = {
>>> +    [FTYPE_NONE] = &file_ops_none,
>>> +};
>>
>> Both want to be const, because all file_ops ought to live in rodata.
>
> file_ops[] can't be const, it will just point to const data.

Oh, of course.

>>> @@ -709,6 +805,18 @@ static void dump_pollfds(struct pollfd *pfd,
>>> int nfds, int timeout)
>>>   #define dump_pollfds(pfds, nfds, timeout)
>>>   #endif
>>>   +bool select_yes(int fd)
>>> +{
>>> +    return true;
>>> +}
>>> +
>>> +bool select_read_flag(int fd)
>>> +{
>>> +    struct file *file = get_file_from_fd(fd);
>>> +
>>> +    return file->read;
>>> +}
>>
>> I don't see these getting used, even in a fallback case.
>
> They will be used in later patches.

Yeah - I found them in later patches.  It's probably worth a note in the
commit message saying "provide some functions useful for file_ops in
future patches", particularly as it's actually a later series which
takes them (due to the ordering constraints).

~Andrew