Re: [Minios-devel] [UNIKRAFT PATCH v3 13/23] lib/vfscore: introduce fget, fdrop, fdalloc

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Hi,

Sharan Santhanam <sharan.santhanam@xxxxxxxxx> writes:

> Hello Yuri,
>
> Please find the comment inline
>
> Thanks & Regards
> Sharan
>
> On 2/7/19 2:58 PM, Yuri Volchkov wrote:
>> These functions are used in the imported code, which is not enabled
>> yet. This required to expand functionality of vfscore_install_fd and
>> friends with locking and reference counting.
>> 
>> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
>> ---
>>   lib/vfscore/fd.c                   | 67 ++++++++++++++++++++++++++++--
>>   lib/vfscore/file.c                 | 22 +++++++++-
>>   lib/vfscore/include/vfscore/file.h |  9 +++-
>>   3 files changed, 92 insertions(+), 6 deletions(-)
>> diff --git a/lib/vfscore/fd.c b/lib/vfscore/fd.c
>> index 6c220428..9d2afe5a 100644
>> --- a/lib/vfscore/fd.c
>> +++ b/lib/vfscore/fd.c
>> @@ -74,20 +74,41 @@ exit:
>>   
>>   void vfscore_put_fd(int fd)
>>   {
>> +    unsigned long flags;
>> +
>>      UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
>>      /* Currently it is not allowed to free std(in|out|err) */
>>      UK_ASSERT(fd > 2);
>>   
>> -    __uk_clear_bit(fd, &fdtable.bitmap);
>> +    flags = ukplat_lcpu_save_irqf();
>> +    __uk_clear_bit(fd, &fdtable.bitmap);\
>> +    fdtable.files[fd] = NULL;
>> +    ukplat_lcpu_restore_irqf(flags);
> Shouldn't we have a fdrop on the vfscore_file descriptor?
Makes sense

>
>>   }
>>   
>> -void vfscore_install_fd(int fd, struct vfscore_file *file)
>> +int vfscore_install_fd(int fd, struct vfscore_file *file)
>>   {
>> -    UK_ASSERT(fd < (int) FDTABLE_MAX_FILES);
>> -    UK_ASSERT(file);
>> +    unsigned long flags;
>> +    struct vfscore_file *orig;
>> +
>> +    if ((fd >= (int) FDTABLE_MAX_FILES) || (!file))
>> +            return -EBADF;
>> +
>> +    fhold(file);
>>   
>>      file->fd = fd;
>> +
>> +    flags = ukplat_lcpu_save_irqf();
>
> Shouldn't we check if there active reference to the orig before 
> installing the fd?
That was a whole point. If there was something before, it is closed
silently. See 'man dup2'.

>> +    orig = fdtable.files[fd];
>>      fdtable.files[fd] = file;
>> +    ukplat_lcpu_restore_irqf(flags);
>> +
>> +    fdrop(file);
>> +
>> +    if (orig)
> This reference has to be closed as there can be no further reference to it.
Not really. The same fp can have multiple file descriptors. For example
'dup' can create such conditions

>> +            fdrop(orig);
>> +
>> +    return 0;
>>   }
>>   
>>   struct vfscore_file *vfscore_get_file(int fd)
>> @@ -101,12 +122,50 @@ struct vfscore_file *vfscore_get_file(int fd)
>>      if (!(fdtable.bitmap & ((uint64_t) 1 << fd)))
>>              goto exit;
>>      ret = fdtable.files[fd];
>> +    fhold(ret);
>>   
>>   exit:
>>      ukplat_lcpu_restore_irqf(flags);
>>      return ret;
>>   }
>>   
>> +int fget(int fd, struct vfscore_file **out_fp)
>> +{
>> +    int ret = 0;
>> +    struct vfscore_file *fp = vfscore_get_file(fd);
>> +
>> +    if (!fp)
> We are using both -ve and +ve error within this file. Maybe wise to
> stick to a single scheme. Is it because of interaction with the imported 
> OSv code?

You are right. This is broken. Exactly because this was the way it was
implemented in Prex, and subsequently in OSv. Currently it is sort of
works. Because for -ve schemed function return value is check only for
zero.

The idea was to change to negative errors in the followups, if you do
not mind. And I am a bit reluctant to change function using "correct"
-ve scheme  function to "wrong" +ve on in this patch, only to change
them back again in the followups.

This is the first thing on my plate to fix after the series is
upstreamed.
>
>> +            ret = EBADF;
>> +    else
>> +            *out_fp = fp;
>> +
>> +    return ret;
>> +}
>> +
>> +int fdalloc(struct vfscore_file *fp, int *newfd)
>> +{
>> +    int fd, ret = 0;
>> +
>> +    fhold(fp);
>> +
>> +    fd = vfscore_alloc_fd();
>> +    if (fd < 0) {
> Shouldn't we have an fdrop here?
No, fdalloc supposed to allocate a locked fp.

>> @@ -95,3 +95,23 @@ ssize_t read(int fd, void *buf, size_t count)
>>   
>>      return file->fops->read(file, buf, count);
>>   }
>> +
>> +/* TODO: remove stub */
>> +#define vfs_close(fp) (0)
>> +
> For reference counting we could use the <include/uk/refcount.h>. As 
>  
>                                                                mentioned 
> in the previous patch it is not necessary for this patch 
>  
>

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

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