Re: [Minios-devel] [UNIKRAFT PATCH] lib/9pfs: Add getattr filesystem operation

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Hi Roxana,

Thanks for the review.

On 11/4/19 3:43 PM, Roxana Nicolescu wrote:
> Hi Costin,
> 
> Thanks for the patch.
> 
> See inline.
> 
> Roxana
> 
> On 31.10.2019 18:44, Costin Lupu wrote:
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>>   lib/9pfs/9pfs_vnops.c | 31 ++++++++++++++++++++++++++++++-
>>   1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/9pfs/9pfs_vnops.c b/lib/9pfs/9pfs_vnops.c
>> index 53d00b8e..87fde712 100644
>> --- a/lib/9pfs/9pfs_vnops.c
>> +++ b/lib/9pfs/9pfs_vnops.c
>> @@ -549,10 +549,39 @@ out:
>>       return -rc;
>>   }
>>   +static int uk_9pfs_getattr(struct vnode *vp, struct vattr *attr)
>> +{
>> +    struct uk_9pdev *dev = UK_9PFS_MD(vp->v_mount)->dev;
>> +    struct uk_9pfid *fid = UK_9PFS_VFID(vp);
>> +    struct uk_9p_stat stat;
>> +    struct uk_9preq *stat_req;
>> +    int rc;
> `rc` should be initialized with 0. In no error case, the return value
> will be `-rc`, which can be any trashy value on the stack.

Right.

>> +
>> +    stat_req = uk_9p_stat(dev, fid, &stat);
>> +    if (PTRISERR(stat_req)) {
>> +        rc = PTR2ERR(stat_req);
>> +        goto out;
>> +    }
>> +
>> +    /* No stat string fields are used below. */
>> +    uk_9pdev_req_remove(dev, stat_req);
>> +
>> +    attr->va_type = uk_9pfs_vtype_from_mode(stat.mode);
>> +    attr->va_mode = uk_9pfs_posix_mode_from_mode(stat.mode);
>> +    attr->va_nodeid = vp->v_ino;
>> +    attr->va_size = stat.length;
>> +
>> +    memcpy(&(attr->va_atime), &(stat.atime), sizeof(struct timespec));
>> +    memcpy(&(attr->va_mtime), &(stat.mtime), sizeof(struct timespec));
> 
> I am not sure if this is the right way to do the copy.
> 
> `sizeof(struct timespec)` is longer than `sizeof(stat.atime)` and in the
> end, in the destination
> 
> we'll have some unexpected values.
> 

Right, good catch. I just copied it from ramfs without thinking much.
I'll send a v2.

>> +    memset(&(attr->va_ctime), 0, sizeof(struct timespec));
>> +
>> +out:
>> +    return -rc;
>> +}
>> +
>>   #define uk_9pfs_seek        ((vnop_seek_t)vfscore_vop_nullop)
>>   #define uk_9pfs_ioctl        ((vnop_ioctl_t)vfscore_vop_einval)
>>   #define uk_9pfs_fsync        ((vnop_fsync_t)vfscore_vop_nullop)
>> -#define uk_9pfs_getattr        ((vnop_getattr_t)vfscore_vop_nullop)
>>   #define uk_9pfs_setattr        ((vnop_setattr_t)vfscore_vop_nullop)
>>   #define uk_9pfs_truncate    ((vnop_truncate_t)vfscore_vop_nullop)
>>   #define uk_9pfs_link        ((vnop_link_t)vfscore_vop_eperm)
> 
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel

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