diff mbox series

[v2,11/11] tests/9pfs: add local Tmkdir test

Message ID 9488e95ff86b6ceea0254dab6a7dff67cbe0bf16.1601639563.git.qemu_oss@crudebyte.com
State Superseded
Headers show
Series 9pfs: add tests using local fs driver | expand

Commit Message

Christian Schoenebeck Oct. 2, 2020, 11:51 a.m. UTC
This test case uses the 9pfs 'local' driver to create a directory
and then checks if the expected directory was actually created
(as real directory) on host side.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

Comments

Daniel P. Berrangé Oct. 2, 2020, 12:56 p.m. UTC | #1
On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> This test case uses the 9pfs 'local' driver to create a directory

> and then checks if the expected directory was actually created

> (as real directory) on host side.

> 

> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

> ---

>  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++

>  1 file changed, 139 insertions(+)

> 

> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c

> index af7e169d3a..93161a4b35 100644

> --- a/tests/qtest/virtio-9p-test.c

> +++ b/tests/qtest/virtio-9p-test.c

> @@ -18,6 +18,62 @@

>  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)

>  static QGuestAllocator *alloc;

>  

> +/*

> + * Used to auto generate new fids. Start with arbitrary high value to avoid

> + * collision with hard coded fids in basic test code.

> + */

> +static uint32_t fid_generator = 1000;

> +

> +static uint32_t genfid(void)

> +{

> +    return fid_generator++;

> +}

> +

> +/**

> + * Splits the @a in string by @a delim into individual (non empty) strings

> + * and outputs them to @a out. The output array @a out is NULL terminated.

> + *

> + * Output array @a out must be freed by calling split_free().

> + *

> + * @returns number of individual elements in output array @a out (without the

> + *          final NULL terminating element)

> + */

> +static int split(const char *in, const char *delim, char ***out)

> +{

> +    int n = 0, i = 0;

> +    char *tmp, *p;

> +

> +    tmp = g_strdup(in);

> +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {

> +        if (strlen(p) > 0) {

> +            ++n;

> +        }

> +    }

> +    g_free(tmp);

> +

> +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL delimiter */


Surely this should be  (n + 1) * sizeof(char *), because the last
element still needs to be large enough to hold a pointer, not a
single extra byte.

> +

> +    tmp = g_strdup(in);

> +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {

> +        if (strlen(p) > 0) {

> +            (*out)[i++] = g_strdup(p);

> +        }

> +    }

> +    g_free(tmp);

> +

> +    return n;

> +}


This seems to largely re-invent g_strsplit 

https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html#g-strsplit

> +

> +static void split_free(char ***out)

> +{

> +    int i;

> +    for (i = 0; (*out)[i]; ++i) {

> +        g_free((*out)[i]);

> +    }

> +    g_free(*out);

> +    *out = NULL;

> +}


And g_strfreev



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Christian Schoenebeck Oct. 2, 2020, 1:41 p.m. UTC | #2
On Freitag, 2. Oktober 2020 14:56:14 CEST Daniel P. Berrangé wrote:
> On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:

> > This test case uses the 9pfs 'local' driver to create a directory

> > and then checks if the expected directory was actually created

> > (as real directory) on host side.

> > 

> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

> > ---

> > 

> >  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++

> >  1 file changed, 139 insertions(+)

> > 

> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c

> > index af7e169d3a..93161a4b35 100644

> > --- a/tests/qtest/virtio-9p-test.c

> > +++ b/tests/qtest/virtio-9p-test.c

> > @@ -18,6 +18,62 @@

> > 

> >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)

> >  static QGuestAllocator *alloc;

> > 

> > +/*

> > + * Used to auto generate new fids. Start with arbitrary high value to

> > avoid + * collision with hard coded fids in basic test code.

> > + */

> > +static uint32_t fid_generator = 1000;

> > +

> > +static uint32_t genfid(void)

> > +{

> > +    return fid_generator++;

> > +}

> > +

> > +/**

> > + * Splits the @a in string by @a delim into individual (non empty)

> > strings

> > + * and outputs them to @a out. The output array @a out is NULL

> > terminated.

> > + *

> > + * Output array @a out must be freed by calling split_free().

> > + *

> > + * @returns number of individual elements in output array @a out (without

> > the + *          final NULL terminating element)

> > + */

> > +static int split(const char *in, const char *delim, char ***out)

> > +{

> > +    int n = 0, i = 0;

> > +    char *tmp, *p;

> > +

> > +    tmp = g_strdup(in);

> > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {

> > +        if (strlen(p) > 0) {

> > +            ++n;

> > +        }

> > +    }

> > +    g_free(tmp);

> > +

> > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL

> > delimiter */

> Surely this should be  (n + 1) * sizeof(char *), because the last

> element still needs to be large enough to hold a pointer, not a

> single extra byte.


Right, good catch!

> > +

> > +    tmp = g_strdup(in);

> > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {

> > +        if (strlen(p) > 0) {

> > +            (*out)[i++] = g_strdup(p);

> > +        }

> > +    }

> > +    g_free(tmp);

> > +

> > +    return n;

> > +}

> 

> This seems to largely re-invent g_strsplit

> 

> https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html#g-s

> trsplit


Yes, except that g_strsplit() outputs empty array elements as well. That's not 
desired/working for this patch.

> > +

> > +static void split_free(char ***out)

> > +{

> > +    int i;

> > +    for (i = 0; (*out)[i]; ++i) {

> > +        g_free((*out)[i]);

> > +    }

> > +    g_free(*out);

> > +    *out = NULL;

> > +}

> 

> And g_strfreev

> 

> 

> 

> Regards,

> Daniel


Best regards,
Christian Schoenebeck
Daniel P. Berrangé Oct. 2, 2020, 1:44 p.m. UTC | #3
On Fri, Oct 02, 2020 at 03:41:07PM +0200, Christian Schoenebeck wrote:
> On Freitag, 2. Oktober 2020 14:56:14 CEST Daniel P. Berrangé wrote:
> > On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> > > This test case uses the 9pfs 'local' driver to create a directory
> > > and then checks if the expected directory was actually created
> > > (as real directory) on host side.
> > > 
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > ---
> > > 
> > >  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 139 insertions(+)
> > > 
> > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > > index af7e169d3a..93161a4b35 100644
> > > --- a/tests/qtest/virtio-9p-test.c
> > > +++ b/tests/qtest/virtio-9p-test.c
> > > @@ -18,6 +18,62 @@
> > > 
> > >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
> > >  static QGuestAllocator *alloc;
> > > 
> > > +/*
> > > + * Used to auto generate new fids. Start with arbitrary high value to
> > > avoid + * collision with hard coded fids in basic test code.
> > > + */
> > > +static uint32_t fid_generator = 1000;
> > > +
> > > +static uint32_t genfid(void)
> > > +{
> > > +    return fid_generator++;
> > > +}
> > > +
> > > +/**
> > > + * Splits the @a in string by @a delim into individual (non empty)
> > > strings
> > > + * and outputs them to @a out. The output array @a out is NULL
> > > terminated.
> > > + *
> > > + * Output array @a out must be freed by calling split_free().
> > > + *
> > > + * @returns number of individual elements in output array @a out (without
> > > the + *          final NULL terminating element)
> > > + */
> > > +static int split(const char *in, const char *delim, char ***out)
> > > +{
> > > +    int n = 0, i = 0;
> > > +    char *tmp, *p;
> > > +
> > > +    tmp = g_strdup(in);
> > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > > +        if (strlen(p) > 0) {
> > > +            ++n;
> > > +        }
> > > +    }
> > > +    g_free(tmp);
> > > +
> > > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL
> > > delimiter */
> > Surely this should be  (n + 1) * sizeof(char *), because the last
> > element still needs to be large enough to hold a pointer, not a
> > single extra byte.
> 
> Right, good catch!
> 
> > > +
> > > +    tmp = g_strdup(in);
> > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > > +        if (strlen(p) > 0) {
> > > +            (*out)[i++] = g_strdup(p);
> > > +        }
> > > +    }
> > > +    g_free(tmp);
> > > +
> > > +    return n;
> > > +}
> > 
> > This seems to largely re-invent g_strsplit
> > 
> > https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html#g-s
> > trsplit
> 
> Yes, except that g_strsplit() outputs empty array elements as well. That's not 
> desired/working for this patch.

Either make the caller ignore/skip over empty elements, or make
this method call g_strsplit and then filter out the empty elements.

Regards,
Daniel
Christian Schoenebeck Oct. 2, 2020, 2:09 p.m. UTC | #4
On Freitag, 2. Oktober 2020 15:44:40 CEST Daniel P. Berrangé wrote:
> On Fri, Oct 02, 2020 at 03:41:07PM +0200, Christian Schoenebeck wrote:

> > On Freitag, 2. Oktober 2020 14:56:14 CEST Daniel P. Berrangé wrote:

> > > On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:

> > > > This test case uses the 9pfs 'local' driver to create a directory

> > > > and then checks if the expected directory was actually created

> > > > (as real directory) on host side.

> > > > 

> > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

> > > > ---

> > > > 

> > > >  tests/qtest/virtio-9p-test.c | 139

> > > >  +++++++++++++++++++++++++++++++++++

> > > >  1 file changed, 139 insertions(+)

> > > > 

> > > > diff --git a/tests/qtest/virtio-9p-test.c

> > > > b/tests/qtest/virtio-9p-test.c

> > > > index af7e169d3a..93161a4b35 100644

> > > > --- a/tests/qtest/virtio-9p-test.c

> > > > +++ b/tests/qtest/virtio-9p-test.c

> > > > @@ -18,6 +18,62 @@

> > > > 

> > > >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)

> > > >  static QGuestAllocator *alloc;

> > > > 

> > > > +/*

> > > > + * Used to auto generate new fids. Start with arbitrary high value to

> > > > avoid + * collision with hard coded fids in basic test code.

> > > > + */

> > > > +static uint32_t fid_generator = 1000;

> > > > +

> > > > +static uint32_t genfid(void)

> > > > +{

> > > > +    return fid_generator++;

> > > > +}

> > > > +

> > > > +/**

> > > > + * Splits the @a in string by @a delim into individual (non empty)

> > > > strings

> > > > + * and outputs them to @a out. The output array @a out is NULL

> > > > terminated.

> > > > + *

> > > > + * Output array @a out must be freed by calling split_free().

> > > > + *

> > > > + * @returns number of individual elements in output array @a out

> > > > (without

> > > > the + *          final NULL terminating element)

> > > > + */

> > > > +static int split(const char *in, const char *delim, char ***out)

> > > > +{

> > > > +    int n = 0, i = 0;

> > > > +    char *tmp, *p;

> > > > +

> > > > +    tmp = g_strdup(in);

> > > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim))

> > > > {

> > > > +        if (strlen(p) > 0) {

> > > > +            ++n;

> > > > +        }

> > > > +    }

> > > > +    g_free(tmp);

> > > > +

> > > > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL

> > > > delimiter */

> > > 

> > > Surely this should be  (n + 1) * sizeof(char *), because the last

> > > element still needs to be large enough to hold a pointer, not a

> > > single extra byte.

> > 

> > Right, good catch!

> > 

> > > > +

> > > > +    tmp = g_strdup(in);

> > > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim))

> > > > {

> > > > +        if (strlen(p) > 0) {

> > > > +            (*out)[i++] = g_strdup(p);

> > > > +        }

> > > > +    }

> > > > +    g_free(tmp);

> > > > +

> > > > +    return n;

> > > > +}

> > > 

> > > This seems to largely re-invent g_strsplit

> > > 

> > > https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html

> > > #g-s trsplit

> > 

> > Yes, except that g_strsplit() outputs empty array elements as well. That's

> > not desired/working for this patch.

> 

> Either make the caller ignore/skip over empty elements, or make


Not an option, since it would create too much test code overhead. I really 
need [const int elements] and [array without empty elements].

> this method call g_strsplit and then filter out the empty elements.


Mmm, so you're suggesting to allocate a new array, copy elements from 
g_strsplit() array to the new array, and eventually pass that manually 
allocated array to g_strfreev()? Wouldn't that be a bit unsafe regarding 
potential future changes in how glib allocates/structures those string arrays?

At least I don't see a glib function that would filter string arrays out of 
the box.

Best regards,
Christian Schoenebeck
Daniel P. Berrangé Oct. 2, 2020, 2:23 p.m. UTC | #5
On Fri, Oct 02, 2020 at 04:09:53PM +0200, Christian Schoenebeck wrote:
> On Freitag, 2. Oktober 2020 15:44:40 CEST Daniel P. Berrangé wrote:

> > On Fri, Oct 02, 2020 at 03:41:07PM +0200, Christian Schoenebeck wrote:

> > > On Freitag, 2. Oktober 2020 14:56:14 CEST Daniel P. Berrangé wrote:

> > > > On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:

> > > > > This test case uses the 9pfs 'local' driver to create a directory

> > > > > and then checks if the expected directory was actually created

> > > > > (as real directory) on host side.

> > > > > 

> > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

> > > > > ---

> > > > > 

> > > > >  tests/qtest/virtio-9p-test.c | 139

> > > > >  +++++++++++++++++++++++++++++++++++

> > > > >  1 file changed, 139 insertions(+)

> > > > > 

> > > > > diff --git a/tests/qtest/virtio-9p-test.c

> > > > > b/tests/qtest/virtio-9p-test.c

> > > > > index af7e169d3a..93161a4b35 100644

> > > > > --- a/tests/qtest/virtio-9p-test.c

> > > > > +++ b/tests/qtest/virtio-9p-test.c

> > > > > @@ -18,6 +18,62 @@

> > > > > 

> > > > >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)

> > > > >  static QGuestAllocator *alloc;

> > > > > 

> > > > > +/*

> > > > > + * Used to auto generate new fids. Start with arbitrary high value to

> > > > > avoid + * collision with hard coded fids in basic test code.

> > > > > + */

> > > > > +static uint32_t fid_generator = 1000;

> > > > > +

> > > > > +static uint32_t genfid(void)

> > > > > +{

> > > > > +    return fid_generator++;

> > > > > +}

> > > > > +

> > > > > +/**

> > > > > + * Splits the @a in string by @a delim into individual (non empty)

> > > > > strings

> > > > > + * and outputs them to @a out. The output array @a out is NULL

> > > > > terminated.

> > > > > + *

> > > > > + * Output array @a out must be freed by calling split_free().

> > > > > + *

> > > > > + * @returns number of individual elements in output array @a out

> > > > > (without

> > > > > the + *          final NULL terminating element)

> > > > > + */

> > > > > +static int split(const char *in, const char *delim, char ***out)

> > > > > +{

> > > > > +    int n = 0, i = 0;

> > > > > +    char *tmp, *p;

> > > > > +

> > > > > +    tmp = g_strdup(in);

> > > > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim))

> > > > > {

> > > > > +        if (strlen(p) > 0) {

> > > > > +            ++n;

> > > > > +        }

> > > > > +    }

> > > > > +    g_free(tmp);

> > > > > +

> > > > > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL

> > > > > delimiter */

> > > > 

> > > > Surely this should be  (n + 1) * sizeof(char *), because the last

> > > > element still needs to be large enough to hold a pointer, not a

> > > > single extra byte.

> > > 

> > > Right, good catch!

> > > 

> > > > > +

> > > > > +    tmp = g_strdup(in);

> > > > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim))

> > > > > {

> > > > > +        if (strlen(p) > 0) {

> > > > > +            (*out)[i++] = g_strdup(p);

> > > > > +        }

> > > > > +    }

> > > > > +    g_free(tmp);

> > > > > +

> > > > > +    return n;

> > > > > +}

> > > > 

> > > > This seems to largely re-invent g_strsplit

> > > > 

> > > > https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html

> > > > #g-s trsplit

> > > 

> > > Yes, except that g_strsplit() outputs empty array elements as well. That's

> > > not desired/working for this patch.

> > 

> > Either make the caller ignore/skip over empty elements, or make

> 

> Not an option, since it would create too much test code overhead. I really 

> need [const int elements] and [array without empty elements].

> 

> > this method call g_strsplit and then filter out the empty elements.

> 

> Mmm, so you're suggesting to allocate a new array, copy elements from 

> g_strsplit() array to the new array, and eventually pass that manually 

> allocated array to g_strfreev()? Wouldn't that be a bit unsafe regarding 

> potential future changes in how glib allocates/structures those string arrays?


No need to realloate a new array - just shuffle down the elements in
the array you get back from g_strsplit to discard the empty ones. Sure
the array will end up with a series of NULLs at the end but that is
harmless.

both g_strsplit/g_strfreev work in terms of a normal allocated "char **"
array, so there's no risk of changes to way memory is allocated.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Christian Schoenebeck Oct. 2, 2020, 2:30 p.m. UTC | #6
On Freitag, 2. Oktober 2020 16:23:31 CEST Daniel P. Berrangé wrote:
> No need to realloate a new array - just shuffle down the elements in

> the array you get back from g_strsplit to discard the empty ones. Sure

> the array will end up with a series of NULLs at the end but that is

> harmless.

> 

> both g_strsplit/g_strfreev work in terms of a normal allocated "char **"

> array, so there's no risk of changes to way memory is allocated.


Ok then, if you say so ... fine with me.

Best regards,
Christian Schoenebeck
Greg Kurz Oct. 2, 2020, 2:32 p.m. UTC | #7
On Fri, 2 Oct 2020 13:56:14 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:
> > This test case uses the 9pfs 'local' driver to create a directory
> > and then checks if the expected directory was actually created
> > (as real directory) on host side.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 139 insertions(+)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index af7e169d3a..93161a4b35 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -18,6 +18,62 @@
> >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
> >  static QGuestAllocator *alloc;
> >  
> > +/*
> > + * Used to auto generate new fids. Start with arbitrary high value to avoid
> > + * collision with hard coded fids in basic test code.
> > + */
> > +static uint32_t fid_generator = 1000;
> > +
> > +static uint32_t genfid(void)
> > +{
> > +    return fid_generator++;
> > +}
> > +
> > +/**
> > + * Splits the @a in string by @a delim into individual (non empty) strings
> > + * and outputs them to @a out. The output array @a out is NULL terminated.
> > + *
> > + * Output array @a out must be freed by calling split_free().
> > + *
> > + * @returns number of individual elements in output array @a out (without the
> > + *          final NULL terminating element)
> > + */
> > +static int split(const char *in, const char *delim, char ***out)
> > +{
> > +    int n = 0, i = 0;
> > +    char *tmp, *p;
> > +
> > +    tmp = g_strdup(in);
> > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > +        if (strlen(p) > 0) {
> > +            ++n;
> > +        }
> > +    }
> > +    g_free(tmp);
> > +
> > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL delimiter */
> 
> Surely this should be  (n + 1) * sizeof(char *), because the last
> element still needs to be large enough to hold a pointer, not a
> single extra byte.
> 

If you decide to keep this split() function, maybe use g_new0(char *, n + 1) ?
This buys you the math and does type checking as an extra.

> > +
> > +    tmp = g_strdup(in);
> > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
> > +        if (strlen(p) > 0) {
> > +            (*out)[i++] = g_strdup(p);
> > +        }
> > +    }
> > +    g_free(tmp);
> > +
> > +    return n;
> > +}
> 
> This seems to largely re-invent g_strsplit 
> 
> https://developer.gnome.org/glib/2.62/glib-String-Utility-Functions.html#g-strsplit
> 
> > +
> > +static void split_free(char ***out)
> > +{
> > +    int i;
> > +    for (i = 0; (*out)[i]; ++i) {
> > +        g_free((*out)[i]);
> > +    }
> > +    g_free(*out);
> > +    *out = NULL;
> > +}
> 
> And g_strfreev
> 
> 
> 
> Regards,
> Daniel
Christian Schoenebeck Oct. 2, 2020, 3 p.m. UTC | #8
On Freitag, 2. Oktober 2020 16:32:55 CEST Greg Kurz wrote:
> On Fri, 2 Oct 2020 13:56:14 +0100

> 

> Daniel P. Berrangé <berrange@redhat.com> wrote:

> > On Fri, Oct 02, 2020 at 01:51:54PM +0200, Christian Schoenebeck wrote:

> > > This test case uses the 9pfs 'local' driver to create a directory

> > > and then checks if the expected directory was actually created

> > > (as real directory) on host side.

> > > 

> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

> > > ---

> > > 

> > >  tests/qtest/virtio-9p-test.c | 139 +++++++++++++++++++++++++++++++++++

> > >  1 file changed, 139 insertions(+)

> > > 

> > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c

> > > index af7e169d3a..93161a4b35 100644

> > > --- a/tests/qtest/virtio-9p-test.c

> > > +++ b/tests/qtest/virtio-9p-test.c

> > > @@ -18,6 +18,62 @@

> > > 

> > >  #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)

> > >  static QGuestAllocator *alloc;

> > > 

> > > +/*

> > > + * Used to auto generate new fids. Start with arbitrary high value to

> > > avoid + * collision with hard coded fids in basic test code.

> > > + */

> > > +static uint32_t fid_generator = 1000;

> > > +

> > > +static uint32_t genfid(void)

> > > +{

> > > +    return fid_generator++;

> > > +}

> > > +

> > > +/**

> > > + * Splits the @a in string by @a delim into individual (non empty)

> > > strings

> > > + * and outputs them to @a out. The output array @a out is NULL

> > > terminated.

> > > + *

> > > + * Output array @a out must be freed by calling split_free().

> > > + *

> > > + * @returns number of individual elements in output array @a out

> > > (without the + *          final NULL terminating element)

> > > + */

> > > +static int split(const char *in, const char *delim, char ***out)

> > > +{

> > > +    int n = 0, i = 0;

> > > +    char *tmp, *p;

> > > +

> > > +    tmp = g_strdup(in);

> > > +    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {

> > > +        if (strlen(p) > 0) {

> > > +            ++n;

> > > +        }

> > > +    }

> > > +    g_free(tmp);

> > > +

> > > +    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL

> > > delimiter */> 

> > Surely this should be  (n + 1) * sizeof(char *), because the last

> > element still needs to be large enough to hold a pointer, not a

> > single extra byte.

> 

> If you decide to keep this split() function, maybe use g_new0(char *, n + 1)

> ? This buys you the math and does type checking as an extra.


Yes, that was my plan B.

But if Daniel sais its long-term safe to pass a re-shuffled array to 
g_strfreev(), well then I go that route.

Best regards,
Christian Schoenebeck
diff mbox series

Patch

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index af7e169d3a..93161a4b35 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -18,6 +18,62 @@ 
 #define QVIRTIO_9P_TIMEOUT_US (10 * 1000 * 1000)
 static QGuestAllocator *alloc;
 
+/*
+ * Used to auto generate new fids. Start with arbitrary high value to avoid
+ * collision with hard coded fids in basic test code.
+ */
+static uint32_t fid_generator = 1000;
+
+static uint32_t genfid(void)
+{
+    return fid_generator++;
+}
+
+/**
+ * Splits the @a in string by @a delim into individual (non empty) strings
+ * and outputs them to @a out. The output array @a out is NULL terminated.
+ *
+ * Output array @a out must be freed by calling split_free().
+ *
+ * @returns number of individual elements in output array @a out (without the
+ *          final NULL terminating element)
+ */
+static int split(const char *in, const char *delim, char ***out)
+{
+    int n = 0, i = 0;
+    char *tmp, *p;
+
+    tmp = g_strdup(in);
+    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+        if (strlen(p) > 0) {
+            ++n;
+        }
+    }
+    g_free(tmp);
+
+    *out = g_malloc0(n * sizeof(char *) + 1); /* last element NULL delimiter */
+
+    tmp = g_strdup(in);
+    for (p = strtok(tmp, delim); p != NULL; p = strtok(NULL, delim)) {
+        if (strlen(p) > 0) {
+            (*out)[i++] = g_strdup(p);
+        }
+    }
+    g_free(tmp);
+
+    return n;
+}
+
+static void split_free(char ***out)
+{
+    int i;
+    for (i = 0; (*out)[i]; ++i) {
+        g_free((*out)[i]);
+    }
+    g_free(*out);
+    *out = NULL;
+}
+
 static void pci_config(void *obj, void *data, QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
@@ -201,6 +257,7 @@  static const char *rmessage_name(uint8_t id)
         id == P9_RWALK ? "RWALK" :
         id == P9_RLOPEN ? "RLOPEN" :
         id == P9_RWRITE ? "RWRITE" :
+        id == P9_RMKDIR ? "RMKDIR" :
         id == P9_RFLUSH ? "RFLUSH" :
         id == P9_RREADDIR ? "READDIR" :
         "<unknown>";
@@ -578,6 +635,39 @@  static bool fs_dirents_contain_name(struct V9fsDirent *e, const char* name)
     return false;
 }
 
+/* size[4] Tmkdir tag[2] dfid[4] name[s] mode[4] gid[4] */
+static P9Req *v9fs_tmkdir(QVirtio9P *v9p, uint32_t dfid, const char *name,
+                          uint32_t mode, uint32_t gid, uint16_t tag)
+{
+    P9Req *req;
+
+    uint32_t body_size = 4 + 4 + 4;
+    uint16_t string_size = v9fs_string_size(name);
+
+    g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
+    body_size += string_size;
+
+    req = v9fs_req_init(v9p, body_size, P9_TMKDIR, tag);
+    v9fs_uint32_write(req, dfid);
+    v9fs_string_write(req, name);
+    v9fs_uint32_write(req, mode);
+    v9fs_uint32_write(req, gid);
+    v9fs_req_send(req);
+    return req;
+}
+
+/* size[4] Rmkdir tag[2] qid[13] */
+static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid)
+{
+    v9fs_req_recv(req, P9_RMKDIR);
+    if (qid) {
+        v9fs_memread(req, qid, 13);
+    } else {
+        v9fs_memskip(req, 13);
+    }
+    v9fs_req_free(req);
+}
+
 /* basic readdir test where reply fits into a single response message */
 static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc)
 {
@@ -877,6 +967,30 @@  static void fs_flush_ignored(void *obj, void *data, QGuestAllocator *t_alloc)
     g_free(wnames[0]);
 }
 
+static void fs_mkdir(void *obj, void *data, QGuestAllocator *t_alloc,
+                     const char *path, const char *cname)
+{
+    QVirtio9P *v9p = obj;
+    alloc = t_alloc;
+    char **wnames;
+    char *const name = g_strdup(cname);
+    P9Req *req;
+    const uint32_t fid = genfid();
+
+    int nwnames = split(path, "/", &wnames);
+
+    req = v9fs_twalk(v9p, 0, fid, nwnames, wnames, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rwalk(req, NULL, NULL);
+
+    req = v9fs_tmkdir(v9p, fid, name, 0750, 0, 0);
+    v9fs_req_wait_for_reply(req, NULL);
+    v9fs_rmkdir(req, NULL);
+
+    g_free(name);
+    split_free(&wnames);
+}
+
 static void fs_readdir_split_128(void *obj, void *data,
                                  QGuestAllocator *t_alloc)
 {
@@ -895,6 +1009,30 @@  static void fs_readdir_split_512(void *obj, void *data,
     fs_readdir_split(obj, data, t_alloc, 512);
 }
 
+
+/* tests using the 9pfs 'local' fs driver */
+
+static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
+{
+    QVirtio9P *v9p = obj;
+    struct stat st;
+    char *root_path = virtio_9p_test_path("");
+    char *new_dir = virtio_9p_test_path("01");
+
+    g_assert(root_path != NULL);
+
+    fs_attach(v9p, NULL, t_alloc);
+    fs_mkdir(v9p, data, t_alloc, "/", "01");
+
+    /* check if created directory really exists now ... */
+    g_assert(stat(new_dir, &st) == 0);
+    /* ... and is actually a directory */
+    g_assert((st.st_mode & S_IFMT) == S_IFDIR);
+
+    g_free(new_dir);
+    g_free(root_path);
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
     virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
@@ -934,6 +1072,7 @@  static void register_virtio_9p_test(void)
     /* 9pfs test cases using the 'local' filesystem driver */
     opts.before = assign_9p_local_driver;
     qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
+    qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
 }
 
 libqos_init(register_virtio_9p_test);