Re: [Minios-devel] [UNIKRAFT PATCH 5/6] lib/devfs: Add /dev/null support

[Costin Lupu <costin.lupu@xxxxxxxxx>]

On 12/9/19 11:29 AM, Simon Kuenzer wrote:
> On 06.12.19 14:41, Costin Lupu wrote:
>> This is shamelessly copied and adapted from our implementation
>> for /dev/random device support.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>>   lib/devfs/Config.uk   |   5 ++
>>   lib/devfs/Makefile.uk |   2 +
>>   lib/devfs/null.c      | 115 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 122 insertions(+)
>>   create mode 100644 lib/devfs/null.c
>>
>> diff --git a/lib/devfs/Config.uk b/lib/devfs/Config.uk
>> index 6f21c01c..3adc171e 100644
>> --- a/lib/devfs/Config.uk
>> +++ b/lib/devfs/Config.uk
>> @@ -8,4 +8,9 @@ if LIBDEVFS
>>           bool "Mount /dev during boot"
>>       depends on LIBVFSCORE_AUTOMOUNT_ROOTFS
>>           default n
>> +
>> +    config LIBDEVS_DEV_NULL
>> +        bool "/dev/null"
>> +        depends on LIBDEVFS_AUTOMOUNT
>> +        default y
> 
> Hum, the `null` device should be actually independent of the automount
> option. You could always provide it because devfs can be in principle
> mounted somewhere else and still provide `null`.
> 
> Instead of adding a hard-dependency, I would set the default to yes as
> soon as libdevfs automount is enabled:
> 
>     config LIBDEVS_DEV_NULL
>         bool "null device"
>         default y if LIBDEVFS_AUTOMOUNT
>         default n
> 
> Of course, the default setting should be `off` in order to follow the
> "default is minimal" principle.
> 

Alright, will do in v2. I would add an observation, "default is minimal"
principle may also refer to how much configuring should a user do before
being able to run a vanilla application (e.g. helloworld).

>>   endif
>> diff --git a/lib/devfs/Makefile.uk b/lib/devfs/Makefile.uk
>> index c496fd56..366db677 100644
>> --- a/lib/devfs/Makefile.uk
>> +++ b/lib/devfs/Makefile.uk
>> @@ -3,6 +3,8 @@ $(eval $(call addlib_s,libdevfs,$(CONFIG_LIBDEVFS)))
>>   CINCLUDES-y += -I$(LIBDEVFS_BASE)/include
>>     LIBDEVFS_CFLAGS-$(call gcc_version_ge,8,0) += -Wno-cast-function-type
>> +LIBDEVFS_CFLAGS-y += -Wno-unused-parameter
>>     LIBDEVFS_SRCS-y += $(LIBDEVFS_BASE)/device.c
>>   LIBDEVFS_SRCS-y += $(LIBDEVFS_BASE)/devfs_vnops.c
>> +LIBDEVFS_SRCS-$(CONFIG_LIBDEVS_DEV_NULL) += $(LIBDEVFS_BASE)/null.c
>> diff --git a/lib/devfs/null.c b/lib/devfs/null.c
>> new file mode 100644
>> index 00000000..38d687d4
>> --- /dev/null
>> +++ b/lib/devfs/null.c
>> @@ -0,0 +1,115 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause */
>> +/*
>> + * Authors: Costin Lupu <costin.lupu@xxxxxxxxx>
>> + *
>> + * Copyright (c) 2019, University Politehnica of Bucharest. All
>> rights reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + *    notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + *    notice, this list of conditions and the following disclaimer in
>> the
>> + *    documentation and/or other materials provided with the
>> distribution.
>> + * 3. Neither the name of the copyright holder nor the names of its
>> + *    contributors may be used to endorse or promote products derived
>> from
>> + *    this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
>> CONTRIBUTORS "AS IS"
>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>> TO, THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>> PURPOSE
>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
>> CONTRIBUTORS BE
>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> BUSINESS
>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
>> WHETHER IN
>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
>> OTHERWISE)
>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
>> ADVISED OF THE
>> + * POSSIBILITY OF SUCH DAMAGE.
>> + *
>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>> + */
>> +
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <uk/ctors.h>
>> +#include <uk/print.h>
>> +#include <vfscore/uio.h>
>> +#include <devfs/device.h>
>> +
>> +#define DEV_NULL_NAME "null"
>> +
>> +/* TODO This can be integrated with random's fill buffer function */
>> +static ssize_t uk_null_fill_buffer(void *buf, size_t buflen)
>> +{
>> +    size_t step, resid, i;
>> +
>> +    step = sizeof(unsigned long);
>> +    resid = buflen % step;
>> +
>> +    for (i = 0; i < buflen - resid; i += step)
>> +        *(unsigned long *)((char *) buf + i) = 0;
>> +
>> +    /* fill the remaining bytes of the buffer */
>> +    for (i = 0; i < resid; i++)
>> +        *((char *) buf + i) = 0;
> 
> memset() would work, too, right? It makes the implementation probably
> easier and we would not need to integrate it with /dev/randmom.
> However, your implementation looks like /dev/zero. /dev/null should only
> return "end-of-file" and no bytes.
> 

Ack.

>> +
>> +    return buflen;
>> +}
>> +
>> +int dev_null_read(struct device *dev, struct uio *uio, int flags)
>> +{
>> +    size_t count;
>> +    char *buf;
>> +
>> +    buf = uio->uio_iov->iov_base;
>> +    count = uio->uio_iov->iov_len;
>> +
>> +    uk_null_fill_buffer(buf, count);
>> +
>> +    uio->uio_resid = 0;
>> +    return 0;
>> +}
>> +
>> +int dev_null_open(struct device *device, int mode)
>> +{
>> +    return 0;
>> +}
>> +
>> +
>> +int dev_null_close(struct device *device)
>> +{
>> +    return 0;
>> +}
>> +
>> +static struct devops null_devops = {
>> +    .read = dev_null_read,
>> +    .open = dev_null_open,
>> +    .close = dev_null_close,
>> +};
> 
> You should also provide the write function. The input is just dropped.
> This should be even the same for /dev/zero.
> 

Ack.

>> +
>> +static struct driver drv_null = {
>> +    .devops = &null_devops,
>> +    .devsz = 0,
>> +    .name = DEV_NULL_NAME
>> +};
>> +
>> +static int devfs_register_null(void)
>> +{
>> +    struct device *dev;
>> +
>> +    uk_pr_info("Register '%s' to devfs\n", DEV_NULL_NAME);
>> +
>> +    /* register /dev/null */
>> +    dev = device_create(&drv_null, DEV_NULL_NAME, D_CHR);
>> +    if (dev == NULL) {
>> +        uk_pr_err("Failed to register '%s' to devfs\n", DEV_NULL_NAME);
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +devfs_initcall(devfs_register_null);
>>
> 
> I think it is worth to provide /dev/zero, too. You have it actually
> already implemented ;-). I think it would make sense to keep both:
> /dev/zero and /dev/null.

Ack.

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