diff mbox series

[07/29] tools/xenlogd: add 9pfs attach request support

Message ID 20231101093325.30302-8-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series tools: enable xenstore-stubdom to use 9pfs | expand

Commit Message

Jürgen Groß Nov. 1, 2023, 9:33 a.m. UTC
Add the attach request of the 9pfs protocol. This introduces the "fid"
scheme of the 9pfs protocol.

As this will be needed later, use a dedicated memory allocation
function in alloc_fid().

For filling the qid data take the approach from the qemu 9pfs backend
implementation.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenlogd/io.c      | 128 ++++++++++++++++++++++++++++++++++++++++
 tools/xenlogd/xenlogd.c |   1 +
 tools/xenlogd/xenlogd.h |  11 ++++
 3 files changed, 140 insertions(+)

Comments

Jason Andryuk Nov. 3, 2023, 3:13 p.m. UTC | #1
On Wed, Nov 1, 2023 at 5:54 AM Juergen Gross <jgross@suse.com> wrote:
>
> Add the attach request of the 9pfs protocol. This introduces the "fid"
> scheme of the 9pfs protocol.
>
> As this will be needed later, use a dedicated memory allocation
> function in alloc_fid().
>
> For filling the qid data take the approach from the qemu 9pfs backend
> implementation.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/xenlogd/io.c      | 128 ++++++++++++++++++++++++++++++++++++++++
>  tools/xenlogd/xenlogd.c |   1 +
>  tools/xenlogd/xenlogd.h |  11 ++++
>  3 files changed, 140 insertions(+)
>
> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> index f35520018f..fa825c9f39 100644
> --- a/tools/xenlogd/io.c
> +++ b/tools/xenlogd/io.c

> +static struct p9_fid *alloc_fid_mem(device *device, unsigned int fid,
> +                                    const char *path)
> +{
> +    struct p9_fid *fidp;
> +    size_t pathlen;
> +
> +    pathlen = strlen(device->host_path) + strlen(path) + 1;
> +    fidp = calloc(sizeof(*fidp) + pathlen, 1);
> +    if ( !fidp )
> +        return NULL;
> +
> +    fidp->fid = fid;
> +    snprintf(fidp->path, pathlen, "%s%s", device->host_path, path);

check_host_path() should be enhanced to ensure host_path has a
trailing '/', or switch this to "%s/%s" to ensure it's always present?

> +
> +    return fidp;
> +}
> +


> +static void free_fid(device *device, struct p9_fid *fidp)
> +{
> +    if ( !fidp )
> +        return;
> +
> +    device->n_fids--;
> +    XEN_TAILQ_REMOVE(&device->fids, fidp, list);
> +    free(fidp);
> +}
> +
> +static int fill_qid(const char *path, struct p9_qid *qid, struct stat *stbuf)

Nit: ordering is input, output, optional input, so you might want to re-order?

stbuf can be const?

> +{
> +    struct stat st;
> +
> +    if ( !stbuf )
> +    {
> +        if ( stat(path, &st) )
> +            return errno;
> +
> +        stbuf = &st;
> +    }
> +
> +    qid->type = S_ISDIR(stbuf->st_mode) ? QID_TYPE_DIR : 0;
> +    qid->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
> +    qid->path = stbuf->st_ino;
> +
> +    return 0;
> +}
> +
>  static void p9_error(device *device, uint16_t tag, uint32_t err)
>  {
>      unsigned int erroff;
> @@ -476,6 +565,41 @@ static void p9_version(device *device, struct p9_header *hdr)
>                  version);
>  }
>
> +static void p9_attach(device *device, struct p9_header *hdr)
> +{
> +    uint32_t fid;
> +    uint32_t dummy_u32;
> +    unsigned int dummy_uint;
> +    struct p9_qid qid;
> +    int ret;
> +
> +    ret = fill_data(device, "UUSSU", &fid, &dummy_u32, &dummy_uint, &dummy_uint,
> +                    &dummy_u32);
> +    if ( ret != 5 )
> +    {
> +        p9_error(device, hdr->tag, errno);
> +        return;
> +    }

We might want to check the first dummy_u32 (afid) to ensure it's NOFID?
"""
If the client does not wish to authenticate the connection, or knows
that authentication is not required, the afid field in the attach mes-
sage should be set to NOFID, defined as (u32int)~0 in fcall.h. If the
client does wish to authenticate, it must acquire and validate an afid
using an auth message before doing the attach.
"""

Since auth isn't implemented, it's probably not necessary to check afid?

I've been looking at these as reference:
https://ericvh.github.io/9p-rfc/rfc9p2000.html
https://ericvh.github.io/9p-rfc/rfc9p2000.u.html

> +
> +    device->root_fid = alloc_fid(device, fid, "");
> +    if ( !device->root_fid )
> +    {
> +        p9_error(device, hdr->tag, errno);
> +        return;
> +    }
> +
> +    ret = fill_qid(device->host_path, &qid, NULL);
> +    if ( ret )
> +    {
> +        free_fid(device, device->root_fid);

root_fid is only freed in this error path.  Maybe free_device() should
free all the fids?

Regards,
Jason
Jürgen Groß Nov. 6, 2023, 7:52 a.m. UTC | #2
On 03.11.23 16:13, Jason Andryuk wrote:
> On Wed, Nov 1, 2023 at 5:54 AM Juergen Gross <jgross@suse.com> wrote:
>>
>> Add the attach request of the 9pfs protocol. This introduces the "fid"
>> scheme of the 9pfs protocol.
>>
>> As this will be needed later, use a dedicated memory allocation
>> function in alloc_fid().
>>
>> For filling the qid data take the approach from the qemu 9pfs backend
>> implementation.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/xenlogd/io.c      | 128 ++++++++++++++++++++++++++++++++++++++++
>>   tools/xenlogd/xenlogd.c |   1 +
>>   tools/xenlogd/xenlogd.h |  11 ++++
>>   3 files changed, 140 insertions(+)
>>
>> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
>> index f35520018f..fa825c9f39 100644
>> --- a/tools/xenlogd/io.c
>> +++ b/tools/xenlogd/io.c
> 
>> +static struct p9_fid *alloc_fid_mem(device *device, unsigned int fid,
>> +                                    const char *path)
>> +{
>> +    struct p9_fid *fidp;
>> +    size_t pathlen;
>> +
>> +    pathlen = strlen(device->host_path) + strlen(path) + 1;
>> +    fidp = calloc(sizeof(*fidp) + pathlen, 1);
>> +    if ( !fidp )
>> +        return NULL;
>> +
>> +    fidp->fid = fid;
>> +    snprintf(fidp->path, pathlen, "%s%s", device->host_path, path);
> 
> check_host_path() should be enhanced to ensure host_path has a
> trailing '/', or switch this to "%s/%s" to ensure it's always present?

No, "path" is always starting with a "/" if it is not empty.

> 
>> +
>> +    return fidp;
>> +}
>> +
> 
> 
>> +static void free_fid(device *device, struct p9_fid *fidp)
>> +{
>> +    if ( !fidp )
>> +        return;
>> +
>> +    device->n_fids--;
>> +    XEN_TAILQ_REMOVE(&device->fids, fidp, list);
>> +    free(fidp);
>> +}
>> +
>> +static int fill_qid(const char *path, struct p9_qid *qid, struct stat *stbuf)
> 
> Nit: ordering is input, output, optional input, so you might want to re-order?

Hmm, I did it on purpose, as the last parameter is optional (as you said).

> 
> stbuf can be const?

Yes.

> 
>> +{
>> +    struct stat st;
>> +
>> +    if ( !stbuf )
>> +    {
>> +        if ( stat(path, &st) )
>> +            return errno;
>> +
>> +        stbuf = &st;
>> +    }
>> +
>> +    qid->type = S_ISDIR(stbuf->st_mode) ? QID_TYPE_DIR : 0;
>> +    qid->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
>> +    qid->path = stbuf->st_ino;
>> +
>> +    return 0;
>> +}
>> +
>>   static void p9_error(device *device, uint16_t tag, uint32_t err)
>>   {
>>       unsigned int erroff;
>> @@ -476,6 +565,41 @@ static void p9_version(device *device, struct p9_header *hdr)
>>                   version);
>>   }
>>
>> +static void p9_attach(device *device, struct p9_header *hdr)
>> +{
>> +    uint32_t fid;
>> +    uint32_t dummy_u32;
>> +    unsigned int dummy_uint;
>> +    struct p9_qid qid;
>> +    int ret;
>> +
>> +    ret = fill_data(device, "UUSSU", &fid, &dummy_u32, &dummy_uint, &dummy_uint,
>> +                    &dummy_u32);
>> +    if ( ret != 5 )
>> +    {
>> +        p9_error(device, hdr->tag, errno);
>> +        return;
>> +    }
> 
> We might want to check the first dummy_u32 (afid) to ensure it's NOFID?
> """
> If the client does not wish to authenticate the connection, or knows
> that authentication is not required, the afid field in the attach mes-
> sage should be set to NOFID, defined as (u32int)~0 in fcall.h. If the
> client does wish to authenticate, it must acquire and validate an afid
> using an auth message before doing the attach.
> """
> 
> Since auth isn't implemented, it's probably not necessary to check afid?

That was the idea, yes.

> 
> I've been looking at these as reference:
> https://ericvh.github.io/9p-rfc/rfc9p2000.html
> https://ericvh.github.io/9p-rfc/rfc9p2000.u.html
> 
>> +
>> +    device->root_fid = alloc_fid(device, fid, "");
>> +    if ( !device->root_fid )
>> +    {
>> +        p9_error(device, hdr->tag, errno);
>> +        return;
>> +    }
>> +
>> +    ret = fill_qid(device->host_path, &qid, NULL);
>> +    if ( ret )
>> +    {
>> +        free_fid(device, device->root_fid);
> 
> root_fid is only freed in this error path.  Maybe free_device() should
> free all the fids?

No, but io_thread() should do so at the end.


Juergen
diff mbox series

Patch

diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
index f35520018f..fa825c9f39 100644
--- a/tools/xenlogd/io.c
+++ b/tools/xenlogd/io.c
@@ -16,6 +16,8 @@ 
 #include <stdlib.h>
 #include <string.h>
 #include <syslog.h>
+#include <sys/types.h>
+#include <sys/stat.h>
 #include <xenctrl.h>           /* For cpu barriers. */
 #include <xen-tools/common-macros.h>
 
@@ -23,6 +25,7 @@ 
 
 /* P9 protocol commands (response is either cmd+1 or P9_CMD_ERROR). */
 #define P9_CMD_VERSION    100
+#define P9_CMD_ATTACH     104
 #define P9_CMD_ERROR      107
 
 #define P9_MIN_MSIZE      2048
@@ -434,6 +437,92 @@  static int fill_data(device *device, const char *fmt, ...)
     return pars;
 }
 
+static struct p9_fid *find_fid(device *device, unsigned int fid)
+{
+    struct p9_fid *fidp;
+
+    XEN_TAILQ_FOREACH(fidp, &device->fids, list)
+    {
+        if ( fidp->fid == fid )
+            return fidp;
+    }
+
+    return NULL;
+}
+
+static struct p9_fid *alloc_fid_mem(device *device, unsigned int fid,
+                                    const char *path)
+{
+    struct p9_fid *fidp;
+    size_t pathlen;
+
+    pathlen = strlen(device->host_path) + strlen(path) + 1;
+    fidp = calloc(sizeof(*fidp) + pathlen, 1);
+    if ( !fidp )
+        return NULL;
+
+    fidp->fid = fid;
+    snprintf(fidp->path, pathlen, "%s%s", device->host_path, path);
+
+    return fidp;
+}
+
+static struct p9_fid *alloc_fid(device *device, unsigned int fid,
+                                const char *path)
+{
+    struct p9_fid *fidp;
+
+    if ( find_fid(device, fid) )
+    {
+        errno = EBADFD;
+        return NULL;
+    }
+
+    if ( device->n_fids >= device->max_open_files )
+    {
+        errno = EMFILE;
+        return NULL;
+    }
+
+    fidp = alloc_fid_mem(device, fid, path);
+    if ( !fidp )
+        return NULL;
+
+    XEN_TAILQ_INSERT_HEAD(&device->fids, fidp, list);
+    device->n_fids++;
+
+    return fidp;
+}
+
+static void free_fid(device *device, struct p9_fid *fidp)
+{
+    if ( !fidp )
+        return;
+
+    device->n_fids--;
+    XEN_TAILQ_REMOVE(&device->fids, fidp, list);
+    free(fidp);
+}
+
+static int fill_qid(const char *path, struct p9_qid *qid, struct stat *stbuf)
+{
+    struct stat st;
+
+    if ( !stbuf )
+    {
+        if ( stat(path, &st) )
+            return errno;
+
+        stbuf = &st;
+    }
+
+    qid->type = S_ISDIR(stbuf->st_mode) ? QID_TYPE_DIR : 0;
+    qid->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
+    qid->path = stbuf->st_ino;
+
+    return 0;
+}
+
 static void p9_error(device *device, uint16_t tag, uint32_t err)
 {
     unsigned int erroff;
@@ -476,6 +565,41 @@  static void p9_version(device *device, struct p9_header *hdr)
                 version);
 }
 
+static void p9_attach(device *device, struct p9_header *hdr)
+{
+    uint32_t fid;
+    uint32_t dummy_u32;
+    unsigned int dummy_uint;
+    struct p9_qid qid;
+    int ret;
+
+    ret = fill_data(device, "UUSSU", &fid, &dummy_u32, &dummy_uint, &dummy_uint,
+                    &dummy_u32);
+    if ( ret != 5 )
+    {
+        p9_error(device, hdr->tag, errno);
+        return;
+    }
+
+    device->root_fid = alloc_fid(device, fid, "");
+    if ( !device->root_fid )
+    {
+        p9_error(device, hdr->tag, errno);
+        return;
+    }
+
+    ret = fill_qid(device->host_path, &qid, NULL);
+    if ( ret )
+    {
+        free_fid(device, device->root_fid);
+        device->root_fid = NULL;
+        p9_error(device, hdr->tag, ret);
+        return;
+    }
+
+    fill_buffer(device, hdr->cmd + 1, hdr->tag, "Q", &qid);
+}
+
 void *io_thread(void *arg)
 {
     device *device = arg;
@@ -535,6 +659,10 @@  void *io_thread(void *arg)
                 p9_version(device, &hdr);
                 break;
 
+            case P9_CMD_ATTACH:
+                p9_attach(device, &hdr);
+                break;
+
             default:
                 syslog(LOG_DEBUG, "%u.%u sent unhandled command %u\n",
                        device->domid, device->devid, hdr.cmd);
diff --git a/tools/xenlogd/xenlogd.c b/tools/xenlogd/xenlogd.c
index da0a09a122..d2de7bfbf2 100644
--- a/tools/xenlogd/xenlogd.c
+++ b/tools/xenlogd/xenlogd.c
@@ -274,6 +274,7 @@  static device *new_device(unsigned int domid, unsigned int devid)
 
     pthread_cond_init(&device->cond, NULL);
     pthread_mutex_init(&device->mutex, NULL);
+    XEN_TAILQ_INIT(&device->fids);
 
     val = read_backend_node(device, "security_model");
     if ( !val || strcmp(val, "none") )
diff --git a/tools/xenlogd/xenlogd.h b/tools/xenlogd/xenlogd.h
index c10c6aa9e5..bd2a283ccb 100644
--- a/tools/xenlogd/xenlogd.h
+++ b/tools/xenlogd/xenlogd.h
@@ -19,6 +19,12 @@  struct p9_header {
     uint16_t tag;
 } __attribute__((packed));
 
+struct p9_fid {
+    XEN_TAILQ_ENTRY(struct p9_fid) list;
+    unsigned int fid;
+    char path[];
+};
+
 typedef struct device device;
 struct device {
     /* Admin data. */
@@ -60,6 +66,11 @@  struct device {
     char *str;              /* String work space. */
     unsigned int str_size;  /* Size of *str. */
     unsigned int str_used;  /* Currently used size of *str. */
+
+    /* File system handling. */
+    XEN_TAILQ_HEAD(fidhead, struct p9_fid) fids;
+    struct p9_fid *root_fid;
+    unsigned int n_fids;
 };
 
 extern xenevtchn_handle *xe;