[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




On 9/12/19 4:40 PM, Costin Lupu wrote:
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.

I agree we need to better define the difference between external more complete libc and nolib.


Cheers,
Costin


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.