Re: [Minios-devel] [UNIKRAFT PATCH] lib/vfscore: sys_ioctl() - Fix build with nolibc

[Costin Lupu <costin.lup@xxxxxxxxx>]

On 9/12/19 5:32 PM, Sharan Santhanam wrote:
> Hello Costin,
> 
> Also noticed a checkpatch error with this patch.
> 
> Thanks & Regards
> 
> Sharan
> 
> 
> On 9/11/19 6:44 PM, Costin Lupu wrote:
>> On 9/11/19 6:27 PM, Sharan Santhanam wrote:
>>> On 9/11/19 3:38 PM, Costin Lupu wrote:
>>>> Hi Sharan,
>>>>
>>>> On 9/11/19 3:56 PM, Sharan Santhanam wrote:
>>>>> Hello Costin,
>>>>>
>>>>> The fix seems fine. Please find the question inline.
>>>>>
>>>>> Thanks & Regards
>>>>>
>>>>> Sharan
>>>>>
>>>>> On 9/11/19 1:56 PM, Costin Lupu wrote:
>>>>>> Commit 3dcccd04 introduced handling of FIOCLEX and FIONCLEX requests.
>>>>>> However,
>>>>>> these flags are not defined in nolibc.
>>>>>>
>>>>>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>>>>>> ---
>>>>>>     lib/nolibc/include/sys/ioctl.h | 0
>>>>> Why do we introduce a empty file? In newlib we introduces a header
>>>>> imported from musl.
>>>>>
>>>> I didn't get that completely, does it build on your side? Do you get
>>>> any
>>>> errors/warnings?
>>> No, but I guess with a review process you do clarify why you made
>>> certain choices.
>>>>> Why don't we use the same file here?
>>>> This is an open question. The thing is that newlib itself seems to be a
>>>> poor choice if we do copy so much code from musl. Now getting back to
>>>> nolibc, if we do add more and more code from musl then we can simply
>>>> get
>>>> rid of it too and use musl instead. In conclusion, I fail to see why we
>>>> should copy code to nolibc instead using musl directly.
>>> But in this case we are introducing the FIONCLEX and FIOCLEX within the
>>> core Unikraft and it is expected to work with nolibc. Instead of adding
>>> #ifdef it would be better to make it feature complete.
>> Now why would you say it is expected to work with nolibc? The two flags
>> are actually related with close-on-exec logic. Why would any app running
>> on top of Unikraft and which would not use a libc implementation need to
>> call exec() since this is not possible? Besides this, there is no exec()
>> function in nolibc.
> 
> From my perspective,
> 
> fcntl(fd, F_SETFD, FD_CLOEXEC);
> 
> and
> 
> ioctl(fd, FIOCLEX);
> 
> is functionally equivalent. The former is supported in nolibc while the
> latter is not. As far as exec call is concerned, we don't support exec
> family in either of the libc except for glue to compile it. So when exec
> family is available we can decide on the libcs to support the exec.

I see, the precedent was created already. I'll send the v2 with the
header copied from musl. To recap, I copied the header only to newlib
glue code in the previous patch.

A question that we need to answer though is until when shall we continue
copying files from musl to both newlib and nolibc? Because this is
getting ridiculous and non-productive.

Cheers,
Costin

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel