[MINI-OS PATCH v2 0/2] 9pfs: add some file operation hooks

[MINI-OS PATCH v2 0/2] 9pfs: add some file operation hooks

[Juergen Gross]

For being able to store the Xenstore state in preparation of a live
update operation, 9pfront is needing support for fstat and lseek.

Changes in V2:
- addressed comment for patch 1

Juergen Gross (2):
  9pfs: add fstat file operation hook
  9pfs: add lseek file operation hook

 9pfront.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

-- 
2.43.0

[MINI-OS PATCH v2 1/2] 9pfs: add fstat file operation hook

[Juergen Gross]

Add a file operations fstat hook to the 9pfs frontend.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
V2:
- or file access mode into st_mode (Jason Andryuk)
---
 9pfront.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/9pfront.c b/9pfront.c
index 1741d600..7257a07e 100644
--- a/9pfront.c
+++ b/9pfront.c
@@ -85,6 +85,8 @@ struct file_9pfs {
 
 #define P9_QID_SIZE    13
 
+#define QID_TYPE_DIR   0x80     /* Applies to qid[0]. */
+
 struct p9_header {
     uint32_t size;
     uint8_t cmd;
@@ -950,6 +952,32 @@ static int write_9pfs(struct file *file, const void *buf, size_t nbytes)
     return ret;
 }
 
+static int fstat_9pfs(struct file *file, struct stat *buf)
+{
+    struct file_9pfs *f9pfs = file->filedata;
+    struct p9_stat stat;
+    int ret;
+
+    ret = p9_stat(f9pfs->dev, f9pfs->fid, &stat);
+    if ( ret )
+    {
+        errno = EIO;
+        return -1;
+    }
+
+    buf->st_mode = (stat.qid[0] == QID_TYPE_DIR) ? S_IFDIR : S_IFREG;
+    buf->st_mode |= stat.mode & 0777;
+    buf->st_atime = stat.atime;
+    buf->st_mtime = stat.mtime;
+    buf->st_size = stat.length;
+    buf->st_uid = stat.n_uid;
+    buf->st_gid = stat.n_gid;
+
+    free_stat(&stat);
+
+    return 0;
+}
+
 static int close_9pfs(struct file *file)
 {
     struct file_9pfs *f9pfs = file->filedata;
@@ -1296,6 +1324,7 @@ static const struct file_ops ops_9pfs = {
     .read = read_9pfs,
     .write = write_9pfs,
     .close = close_9pfs,
+    .fstat = fstat_9pfs,
 };
 
 __attribute__((constructor))
-- 
2.43.0

[MINI-OS PATCH v2 2/2] 9pfs: add lseek file operation hook

[Juergen Gross]

Add a file operations lseek hook to the 9pfs frontend. Just use the
lseek_default() implementation.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
 9pfront.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/9pfront.c b/9pfront.c
index 7257a07e..8bf3a91e 100644
--- a/9pfront.c
+++ b/9pfront.c
@@ -1325,6 +1325,7 @@ static const struct file_ops ops_9pfs = {
     .write = write_9pfs,
     .close = close_9pfs,
     .fstat = fstat_9pfs,
+    .lseek = lseek_default,
 };
 
 __attribute__((constructor))
-- 
2.43.0

Re: [MINI-OS PATCH v2 1/2] 9pfs: add fstat file operation hook

[Samuel Thibault]

Juergen Gross, le ven. 21 mars 2025 10:31:44 +0100, a ecrit:
> Add a file operations fstat hook to the 9pfs frontend.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> V2:
> - or file access mode into st_mode (Jason Andryuk)
> ---
>  9pfront.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/9pfront.c b/9pfront.c
> index 1741d600..7257a07e 100644
> --- a/9pfront.c
> +++ b/9pfront.c
> @@ -85,6 +85,8 @@ struct file_9pfs {
>  
>  #define P9_QID_SIZE    13
>  
> +#define QID_TYPE_DIR   0x80     /* Applies to qid[0]. */
> +
>  struct p9_header {
>      uint32_t size;
>      uint8_t cmd;
> @@ -950,6 +952,32 @@ static int write_9pfs(struct file *file, const void *buf, size_t nbytes)
>      return ret;
>  }
>  
> +static int fstat_9pfs(struct file *file, struct stat *buf)
> +{
> +    struct file_9pfs *f9pfs = file->filedata;
> +    struct p9_stat stat;
> +    int ret;
> +
> +    ret = p9_stat(f9pfs->dev, f9pfs->fid, &stat);
> +    if ( ret )
> +    {
> +        errno = EIO;
> +        return -1;
> +    }
> +
> +    buf->st_mode = (stat.qid[0] == QID_TYPE_DIR) ? S_IFDIR : S_IFREG;
> +    buf->st_mode |= stat.mode & 0777;
> +    buf->st_atime = stat.atime;
> +    buf->st_mtime = stat.mtime;

Should we perhaps also fill st_ctime? Leaving it at 0 could surprise
other software layers.

> +    buf->st_size = stat.length;
> +    buf->st_uid = stat.n_uid;
> +    buf->st_gid = stat.n_gid;
> +
> +    free_stat(&stat);
> +
> +    return 0;
> +}
> +
>  static int close_9pfs(struct file *file)
>  {
>      struct file_9pfs *f9pfs = file->filedata;
> @@ -1296,6 +1324,7 @@ static const struct file_ops ops_9pfs = {
>      .read = read_9pfs,
>      .write = write_9pfs,
>      .close = close_9pfs,
> +    .fstat = fstat_9pfs,
>  };
>  
>  __attribute__((constructor))
> -- 
> 2.43.0
> 

Re: [MINI-OS PATCH v2 2/2] 9pfs: add lseek file operation hook

[Samuel Thibault]

Juergen Gross, le ven. 21 mars 2025 10:31:45 +0100, a ecrit:
> Add a file operations lseek hook to the 9pfs frontend. Just use the
> lseek_default() implementation.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  9pfront.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/9pfront.c b/9pfront.c
> index 7257a07e..8bf3a91e 100644
> --- a/9pfront.c
> +++ b/9pfront.c
> @@ -1325,6 +1325,7 @@ static const struct file_ops ops_9pfs = {
>      .write = write_9pfs,
>      .close = close_9pfs,
>      .fstat = fstat_9pfs,
> +    .lseek = lseek_default,
>  };
>  
>  __attribute__((constructor))
> -- 
> 2.43.0
> 

Re: [MINI-OS PATCH v2 1/2] 9pfs: add fstat file operation hook

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1691 bytes --]

On 23.03.25 01:01, Samuel Thibault wrote:
> Juergen Gross, le ven. 21 mars 2025 10:31:44 +0100, a ecrit:
>> Add a file operations fstat hook to the 9pfs frontend.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>> ---
>> V2:
>> - or file access mode into st_mode (Jason Andryuk)
>> ---
>>   9pfront.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/9pfront.c b/9pfront.c
>> index 1741d600..7257a07e 100644
>> --- a/9pfront.c
>> +++ b/9pfront.c
>> @@ -85,6 +85,8 @@ struct file_9pfs {
>>   
>>   #define P9_QID_SIZE    13
>>   
>> +#define QID_TYPE_DIR   0x80     /* Applies to qid[0]. */
>> +
>>   struct p9_header {
>>       uint32_t size;
>>       uint8_t cmd;
>> @@ -950,6 +952,32 @@ static int write_9pfs(struct file *file, const void *buf, size_t nbytes)
>>       return ret;
>>   }
>>   
>> +static int fstat_9pfs(struct file *file, struct stat *buf)
>> +{
>> +    struct file_9pfs *f9pfs = file->filedata;
>> +    struct p9_stat stat;
>> +    int ret;
>> +
>> +    ret = p9_stat(f9pfs->dev, f9pfs->fid, &stat);
>> +    if ( ret )
>> +    {
>> +        errno = EIO;
>> +        return -1;
>> +    }
>> +
>> +    buf->st_mode = (stat.qid[0] == QID_TYPE_DIR) ? S_IFDIR : S_IFREG;
>> +    buf->st_mode |= stat.mode & 0777;
>> +    buf->st_atime = stat.atime;
>> +    buf->st_mtime = stat.mtime;
> 
> Should we perhaps also fill st_ctime? Leaving it at 0 could surprise
> other software layers.

I can set it to the same value as st_mtime. I don't think there is a way
to get the "real" ctime value via the 9p protocol.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [MINI-OS PATCH v2 1/2] 9pfs: add fstat file operation hook

[Samuel Thibault]

Jürgen Groß, le dim. 23 mars 2025 15:57:16 +0100, a ecrit:
> On 23.03.25 01:01, Samuel Thibault wrote:
> > Juergen Gross, le ven. 21 mars 2025 10:31:44 +0100, a ecrit:
> > > Add a file operations fstat hook to the 9pfs frontend.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > > Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> > > ---
> > > V2:
> > > - or file access mode into st_mode (Jason Andryuk)
> > > ---
> > >   9pfront.c | 29 +++++++++++++++++++++++++++++
> > >   1 file changed, 29 insertions(+)
> > > 
> > > diff --git a/9pfront.c b/9pfront.c
> > > index 1741d600..7257a07e 100644
> > > --- a/9pfront.c
> > > +++ b/9pfront.c
> > > @@ -85,6 +85,8 @@ struct file_9pfs {
> > >   #define P9_QID_SIZE    13
> > > +#define QID_TYPE_DIR   0x80     /* Applies to qid[0]. */
> > > +
> > >   struct p9_header {
> > >       uint32_t size;
> > >       uint8_t cmd;
> > > @@ -950,6 +952,32 @@ static int write_9pfs(struct file *file, const void *buf, size_t nbytes)
> > >       return ret;
> > >   }
> > > +static int fstat_9pfs(struct file *file, struct stat *buf)
> > > +{
> > > +    struct file_9pfs *f9pfs = file->filedata;
> > > +    struct p9_stat stat;
> > > +    int ret;
> > > +
> > > +    ret = p9_stat(f9pfs->dev, f9pfs->fid, &stat);
> > > +    if ( ret )
> > > +    {
> > > +        errno = EIO;
> > > +        return -1;
> > > +    }
> > > +
> > > +    buf->st_mode = (stat.qid[0] == QID_TYPE_DIR) ? S_IFDIR : S_IFREG;
> > > +    buf->st_mode |= stat.mode & 0777;
> > > +    buf->st_atime = stat.atime;
> > > +    buf->st_mtime = stat.mtime;
> > 
> > Should we perhaps also fill st_ctime? Leaving it at 0 could surprise
> > other software layers.
> 
> I can set it to the same value as st_mtime.

That'd be preferrable, yes.

With that,

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Thanks!
Samuel

Re: [MINI-OS PATCH v2 1/2] 9pfs: add fstat file operation hook

[Jan Beulich]

On 23.03.2025 15:57, Jürgen Groß wrote:
> On 23.03.25 01:01, Samuel Thibault wrote:
>> Juergen Gross, le ven. 21 mars 2025 10:31:44 +0100, a ecrit:
>>> Add a file operations fstat hook to the 9pfs frontend.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>>> ---
>>> V2:
>>> - or file access mode into st_mode (Jason Andryuk)
>>> ---
>>>   9pfront.c | 29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/9pfront.c b/9pfront.c
>>> index 1741d600..7257a07e 100644
>>> --- a/9pfront.c
>>> +++ b/9pfront.c
>>> @@ -85,6 +85,8 @@ struct file_9pfs {
>>>   
>>>   #define P9_QID_SIZE    13
>>>   
>>> +#define QID_TYPE_DIR   0x80     /* Applies to qid[0]. */
>>> +
>>>   struct p9_header {
>>>       uint32_t size;
>>>       uint8_t cmd;
>>> @@ -950,6 +952,32 @@ static int write_9pfs(struct file *file, const void *buf, size_t nbytes)
>>>       return ret;
>>>   }
>>>   
>>> +static int fstat_9pfs(struct file *file, struct stat *buf)
>>> +{
>>> +    struct file_9pfs *f9pfs = file->filedata;
>>> +    struct p9_stat stat;
>>> +    int ret;
>>> +
>>> +    ret = p9_stat(f9pfs->dev, f9pfs->fid, &stat);
>>> +    if ( ret )
>>> +    {
>>> +        errno = EIO;
>>> +        return -1;
>>> +    }
>>> +
>>> +    buf->st_mode = (stat.qid[0] == QID_TYPE_DIR) ? S_IFDIR : S_IFREG;
>>> +    buf->st_mode |= stat.mode & 0777;
>>> +    buf->st_atime = stat.atime;
>>> +    buf->st_mtime = stat.mtime;
>>
>> Should we perhaps also fill st_ctime? Leaving it at 0 could surprise
>> other software layers.
> 
> I can set it to the same value as st_mtime.

Maybe the smaller of atime and mtime?

Jan

> I don't think there is a way
> to get the "real" ctime value via the 9p protocol.
> 
> 
> Juergen

Re: [MINI-OS PATCH v2 1/2] 9pfs: add fstat file operation hook

[Samuel Thibault]

Jan Beulich, le lun. 24 mars 2025 11:21:48 +0100, a ecrit:
> On 23.03.2025 15:57, Jürgen Groß wrote:
> > On 23.03.25 01:01, Samuel Thibault wrote:
> >> Juergen Gross, le ven. 21 mars 2025 10:31:44 +0100, a ecrit:
> >>> Add a file operations fstat hook to the 9pfs frontend.
> >>>
> >>> Signed-off-by: Juergen Gross <jgross@suse.com>
> >>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> >>> ---
> >>> V2:
> >>> - or file access mode into st_mode (Jason Andryuk)
> >>> ---
> >>>   9pfront.c | 29 +++++++++++++++++++++++++++++
> >>>   1 file changed, 29 insertions(+)
> >>>
> >>> diff --git a/9pfront.c b/9pfront.c
> >>> index 1741d600..7257a07e 100644
> >>> --- a/9pfront.c
> >>> +++ b/9pfront.c
> >>> @@ -85,6 +85,8 @@ struct file_9pfs {
> >>>   
> >>>   #define P9_QID_SIZE    13
> >>>   
> >>> +#define QID_TYPE_DIR   0x80     /* Applies to qid[0]. */
> >>> +
> >>>   struct p9_header {
> >>>       uint32_t size;
> >>>       uint8_t cmd;
> >>> @@ -950,6 +952,32 @@ static int write_9pfs(struct file *file, const void *buf, size_t nbytes)
> >>>       return ret;
> >>>   }
> >>>   
> >>> +static int fstat_9pfs(struct file *file, struct stat *buf)
> >>> +{
> >>> +    struct file_9pfs *f9pfs = file->filedata;
> >>> +    struct p9_stat stat;
> >>> +    int ret;
> >>> +
> >>> +    ret = p9_stat(f9pfs->dev, f9pfs->fid, &stat);
> >>> +    if ( ret )
> >>> +    {
> >>> +        errno = EIO;
> >>> +        return -1;
> >>> +    }
> >>> +
> >>> +    buf->st_mode = (stat.qid[0] == QID_TYPE_DIR) ? S_IFDIR : S_IFREG;
> >>> +    buf->st_mode |= stat.mode & 0777;
> >>> +    buf->st_atime = stat.atime;
> >>> +    buf->st_mtime = stat.mtime;
> >>
> >> Should we perhaps also fill st_ctime? Leaving it at 0 could surprise
> >> other software layers.
> > 
> > I can set it to the same value as st_mtime.
> 
> Maybe the smaller of atime and mtime?

That'd be better, yes.

Samuel

Re: [MINI-OS PATCH v2 1/2] 9pfs: add fstat file operation hook

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2203 bytes --]

On 24.03.25 11:30, Samuel Thibault wrote:
> Jan Beulich, le lun. 24 mars 2025 11:21:48 +0100, a ecrit:
>> On 23.03.2025 15:57, Jürgen Groß wrote:
>>> On 23.03.25 01:01, Samuel Thibault wrote:
>>>> Juergen Gross, le ven. 21 mars 2025 10:31:44 +0100, a ecrit:
>>>>> Add a file operations fstat hook to the 9pfs frontend.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>>>>> ---
>>>>> V2:
>>>>> - or file access mode into st_mode (Jason Andryuk)
>>>>> ---
>>>>>    9pfront.c | 29 +++++++++++++++++++++++++++++
>>>>>    1 file changed, 29 insertions(+)
>>>>>
>>>>> diff --git a/9pfront.c b/9pfront.c
>>>>> index 1741d600..7257a07e 100644
>>>>> --- a/9pfront.c
>>>>> +++ b/9pfront.c
>>>>> @@ -85,6 +85,8 @@ struct file_9pfs {
>>>>>    
>>>>>    #define P9_QID_SIZE    13
>>>>>    
>>>>> +#define QID_TYPE_DIR   0x80     /* Applies to qid[0]. */
>>>>> +
>>>>>    struct p9_header {
>>>>>        uint32_t size;
>>>>>        uint8_t cmd;
>>>>> @@ -950,6 +952,32 @@ static int write_9pfs(struct file *file, const void *buf, size_t nbytes)
>>>>>        return ret;
>>>>>    }
>>>>>    
>>>>> +static int fstat_9pfs(struct file *file, struct stat *buf)
>>>>> +{
>>>>> +    struct file_9pfs *f9pfs = file->filedata;
>>>>> +    struct p9_stat stat;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = p9_stat(f9pfs->dev, f9pfs->fid, &stat);
>>>>> +    if ( ret )
>>>>> +    {
>>>>> +        errno = EIO;
>>>>> +        return -1;
>>>>> +    }
>>>>> +
>>>>> +    buf->st_mode = (stat.qid[0] == QID_TYPE_DIR) ? S_IFDIR : S_IFREG;
>>>>> +    buf->st_mode |= stat.mode & 0777;
>>>>> +    buf->st_atime = stat.atime;
>>>>> +    buf->st_mtime = stat.mtime;
>>>>
>>>> Should we perhaps also fill st_ctime? Leaving it at 0 could surprise
>>>> other software layers.
>>>
>>> I can set it to the same value as st_mtime.
>>
>> Maybe the smaller of atime and mtime?
> 
> That'd be better, yes.

According to the references I could find ctime is changed whenever
either file contents OR file status (uid, gid, permissions) are
modified. So using the same value as mtime seems appropriate.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [MINI-OS PATCH v2 1/2] 9pfs: add fstat file operation hook

[Jan Beulich]

On 24.03.2025 12:18, Jürgen Groß wrote:
> On 24.03.25 11:30, Samuel Thibault wrote:
>> Jan Beulich, le lun. 24 mars 2025 11:21:48 +0100, a ecrit:
>>> On 23.03.2025 15:57, Jürgen Groß wrote:
>>>> On 23.03.25 01:01, Samuel Thibault wrote:
>>>>> Juergen Gross, le ven. 21 mars 2025 10:31:44 +0100, a ecrit:
>>>>>> Add a file operations fstat hook to the 9pfs frontend.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>>>>>> ---
>>>>>> V2:
>>>>>> - or file access mode into st_mode (Jason Andryuk)
>>>>>> ---
>>>>>>    9pfront.c | 29 +++++++++++++++++++++++++++++
>>>>>>    1 file changed, 29 insertions(+)
>>>>>>
>>>>>> diff --git a/9pfront.c b/9pfront.c
>>>>>> index 1741d600..7257a07e 100644
>>>>>> --- a/9pfront.c
>>>>>> +++ b/9pfront.c
>>>>>> @@ -85,6 +85,8 @@ struct file_9pfs {
>>>>>>    
>>>>>>    #define P9_QID_SIZE    13
>>>>>>    
>>>>>> +#define QID_TYPE_DIR   0x80     /* Applies to qid[0]. */
>>>>>> +
>>>>>>    struct p9_header {
>>>>>>        uint32_t size;
>>>>>>        uint8_t cmd;
>>>>>> @@ -950,6 +952,32 @@ static int write_9pfs(struct file *file, const void *buf, size_t nbytes)
>>>>>>        return ret;
>>>>>>    }
>>>>>>    
>>>>>> +static int fstat_9pfs(struct file *file, struct stat *buf)
>>>>>> +{
>>>>>> +    struct file_9pfs *f9pfs = file->filedata;
>>>>>> +    struct p9_stat stat;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = p9_stat(f9pfs->dev, f9pfs->fid, &stat);
>>>>>> +    if ( ret )
>>>>>> +    {
>>>>>> +        errno = EIO;
>>>>>> +        return -1;
>>>>>> +    }
>>>>>> +
>>>>>> +    buf->st_mode = (stat.qid[0] == QID_TYPE_DIR) ? S_IFDIR : S_IFREG;
>>>>>> +    buf->st_mode |= stat.mode & 0777;
>>>>>> +    buf->st_atime = stat.atime;
>>>>>> +    buf->st_mtime = stat.mtime;
>>>>>
>>>>> Should we perhaps also fill st_ctime? Leaving it at 0 could surprise
>>>>> other software layers.
>>>>
>>>> I can set it to the same value as st_mtime.
>>>
>>> Maybe the smaller of atime and mtime?
>>
>> That'd be better, yes.
> 
> According to the references I could find ctime is changed whenever
> either file contents OR file status (uid, gid, permissions) are
> modified. So using the same value as mtime seems appropriate.

Hmm, yes, one always learns something new. Having come from the DOS/Windows
world originally, 'c' in the name to me only ever could stand for "create".
When really, as you say, it's "change". I'm sorry for introducing confusion
here.

Jan