Re: [PATCH v3 09/11] tests/9pfs: wipe local 9pfs test directory

[Christian Schoenebeck]

On Donnerstag, 8. Oktober 2020 14:26:28 CEST Christian Schoenebeck wrote:
> On Freitag, 2. Oktober 2020 18:15:05 CEST Christian Schoenebeck wrote:
> > Before running the first 9pfs test case, make sure the test directory
> > for running the 9pfs 'local' tests on is entirely empty. For that
> > reason simply delete the test directory (if any) before (re)creating
> > it on test suite startup.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  tests/qtest/libqos/virtio-9p.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/tests/qtest/libqos/virtio-9p.c
> > b/tests/qtest/libqos/virtio-9p.c index 1bada47af1..41a7c2baaa 100644
> > --- a/tests/qtest/libqos/virtio-9p.c
> > +++ b/tests/qtest/libqos/virtio-9p.c
> > @@ -53,6 +53,18 @@ static void create_local_test_dir(void)
> > 
> >      g_assert((st.st_mode & S_IFMT) == S_IFDIR);
> >  
> >  }
> > 
> > +/* Deletes directory previously created by create_local_test_dir(). */
> > +static void remove_local_test_dir(void)
> > +{
> > +    g_assert(local_test_path != NULL);
> > +    char *cmd = g_strdup_printf("rm -r '%s'\n", local_test_path);
> > +    int res = system(cmd);
> > +    if (res < 0) {
> > +        /* ignore error, dummy check to prevent compiler error */
> > +    }
> > +    g_free(cmd);
> > +}
> > +
> > 
> >  static void virtio_9p_cleanup(QVirtio9P *interface)
> >  {
> >  
> >      qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
> > 
> > @@ -220,6 +232,7 @@ static void virtio_9p_register_nodes(void)
> > 
> >      /* make sure test dir for the 'local' tests exists and is clean */
> >      init_local_test_path();
> > 
> > +    remove_local_test_dir();
> > 
> >      create_local_test_dir();
> >      
> >      QPCIAddress addr = {
> >      
> >          .devfn = QPCI_DEVFN(4, 0),
> >      
> >      };
> >      
> >      QOSGraphEdgeOptions opts = {
> >      
> >          .before_cmd_line = "-fsdev synth,id=fsdev0",
> >      
> >      };
> >      
> >      /* virtio-9p-device */
> >      opts.extra_device_opts = str_simple,
> >      qos_node_create_driver("virtio-9p-device", virtio_9p_device_create);
> >      qos_node_consumes("virtio-9p-device", "virtio-bus", &opts);
> >      qos_node_produces("virtio-9p-device", "virtio");
> >      qos_node_produces("virtio-9p-device", "virtio-9p");
> >      
> >      /* virtio-9p-pci */
> >      opts.extra_device_opts = str_addr;
> >      add_qpci_address(&opts, &addr);
> >      qos_node_create_driver("virtio-9p-pci", virtio_9p_pci_create);
> >      qos_node_consumes("virtio-9p-pci", "pci-bus", &opts);
> >      qos_node_produces("virtio-9p-pci", "pci-device");
> >      qos_node_produces("virtio-9p-pci", "virtio");
> >      qos_node_produces("virtio-9p-pci", "virtio-9p");
> > 
> > }
> > 
> > libqos_init(virtio_9p_register_nodes);
> 
> I wonder why libqos is calling virtio_9p_register_nodes() again after all
> qtests ended.
> 
> That's somewhat suboptimal here, as it causes remove_local_test_dir() to be
> called again after all qtests completed. My intention was actually only to
> wipe the "qtest-9p-local" test directory at the *start* of the test suite
> run. Not at the end of the test suite. Because it would allow developers to
> look at the actual dirs/files created after the tests completed.
> 
> I could of course misuse and add a dedicated "wipedir" test as workaround,
> but that OTOH would break the option of running individual tests with the
> -p CL switch.

I probably leave that issue unfixed for now.

There seems to be no easy way to fix this, as libqos does not having something 
like "run this function once per test suite run", and due to the multi process 
approach a static variable hack would not be viable either.

Fortunately though, if a test case fails virtio_9p_register_nodes() would not 
be called again, so at least in case of failures the test dir is note wiped.

So I don't think it's worth to drill another hole into libqos just for this.

Best regards,
Christian Schoenebeck