| Message ID | cover.1602182956.git.qemu_oss@crudebyte.com |
|---|---|
| Headers | show |
| Series | 9pfs: add tests using local fs driver | expand |
On Mittwoch, 14. Oktober 2020 21:38:16 CEST Greg Kurz wrote: > On Wed, 14 Oct 2020 17:25:35 +0200 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Donnerstag, 8. Oktober 2020 20:34:56 CEST Christian Schoenebeck wrote: > > > All existing 9pfs test cases are using the 'synth' fs driver so far, > > > which > > > means they are not accessing real files, but a purely simulated (in RAM > > > only) file system. > > > > > > Let's make this clear by changing the prefix of the individual qtest > > > case > > > names from 'fs/' to 'synth/'. That way they'll be easily distinguishable > > > from upcoming new 9pfs test cases supposed to be using a different fs > > > driver. > > > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > --- > > > > > > tests/qtest/virtio-9p-test.c | 28 ++++++++++++++-------------- > > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > Queued patches 8 .. 12 on 9p.next: > > > > https://github.com/cschoenebeck/qemu/commits/9p.next > > Hi Chistian, > > I could only have a quick glimpse at the patches and LGTM. > > Thanks for taking care. > > Cheers, > > -- > Greg > Thanks, I appreciate it. Best regards, Christian Schoenebeck
On 08/10/2020 20.34, Christian Schoenebeck wrote: > If qtests are run in verbose mode (i.e. if --verbose CL argument > was provided) then print all environment variables to stdout > before running the individual tests. Why? ... you should provide some rationale in the patch description here, at least to me this is not obvious why it is needed / desired. Thomas
On 08/10/2020 20.34, Christian Schoenebeck wrote: > This new function is purely for debugging purposes. It prints the > current qos graph to stdout and allows to identify problems in the > created qos graph e.g. when writing new qos tests. > > Coloured output is used to mark available nodes in green colour, > whereas unavailable nodes are marked in red colour. > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- > tests/qtest/libqos/qgraph.c | 56 +++++++++++++++++++++++++++++++++++++ > tests/qtest/libqos/qgraph.h | 20 +++++++++++++ > 2 files changed, 76 insertions(+) > > diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c > index 61faf6b27d..af93e38dcb 100644 > --- a/tests/qtest/libqos/qgraph.c > +++ b/tests/qtest/libqos/qgraph.c > @@ -805,3 +805,59 @@ void qos_delete_cmd_line(const char *name) > node->command_line = NULL; > } > } > + > +#define RED(txt) ( \ > + "\033[0;91m" txt \ > + "\033[0m" \ > +) > + > +#define GREEN(txt) ( \ > + "\033[0;92m" txt \ > + "\033[0m" \ > +) I don't think this is very portable - and it will only make logs ugly to read in text editors. Could you please simply drop these macros? Thomas
On Samstag, 24. Oktober 2020 07:56:10 CEST Thomas Huth wrote: > On 08/10/2020 20.34, Christian Schoenebeck wrote: > > If qtests are run in verbose mode (i.e. if --verbose CL argument > > was provided) then print all environment variables to stdout > > before running the individual tests. > > Why? ... you should provide some rationale in the patch description here, at > least to me this is not obvious why it is needed / desired. > > Thomas In my particular case I wanted to know whether there is already some config vector for 'please use this test directory for file tests'. As I didn't find one in any API, I also looked for environment variables to be sure. Especially as there are a bunch of qtest related environment variables already. In general though it is common nowadays, at least being able to output all config vectors in a build chain, especially if it is required to investigate build- and test-issues on foreign/remote machines, which includes environment variables. Staying in the context of writing test cases: there are a bunch of other use cases that would come to my mind from the PoV of a test case author: "Is there an option for short vs. long tests?", "Is there a desired size limitation for large file tests?", "Is there a deadline for the runtime of tests?", ... Best regards, Christian Schoenebeck
On Samstag, 24. Oktober 2020 08:04:20 CEST Thomas Huth wrote: > On 08/10/2020 20.34, Christian Schoenebeck wrote: > > This new function is purely for debugging purposes. It prints the > > current qos graph to stdout and allows to identify problems in the > > created qos graph e.g. when writing new qos tests. > > > > Coloured output is used to mark available nodes in green colour, > > whereas unavailable nodes are marked in red colour. > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > --- > > > > tests/qtest/libqos/qgraph.c | 56 +++++++++++++++++++++++++++++++++++++ > > tests/qtest/libqos/qgraph.h | 20 +++++++++++++ > > 2 files changed, 76 insertions(+) > > > > diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c > > index 61faf6b27d..af93e38dcb 100644 > > --- a/tests/qtest/libqos/qgraph.c > > +++ b/tests/qtest/libqos/qgraph.c > > @@ -805,3 +805,59 @@ void qos_delete_cmd_line(const char *name) > > > > node->command_line = NULL; > > > > } > > > > } > > > > + > > +#define RED(txt) ( \ > > + "\033[0;91m" txt \ > > + "\033[0m" \ > > +) > > + > > +#define GREEN(txt) ( \ > > + "\033[0;92m" txt \ > > + "\033[0m" \ > > +) > > I don't think this is very portable - and it will only make logs ugly to > read in text editors. Could you please simply drop these macros? > > Thomas The precise way I did it here is definitely unclean. And the use case is trivial, so on doubt I could just drop it of course. But allow me one attempt to promote coloured terminal output in general: These are ANSI color escape sequences, a standard with its youngest revision dating back to 1991. It is a well supported standard on all major platforms nowadays: https://en.wikipedia.org/wiki/ANSI_escape_code It works on macOS's standard terminal for at least 20 years, with cmd.exe on Windows 10, on essentially all Linux and BSD distros, and even on many web based CI platforms. So what about introducing some globally shared macros for coloured output instead? Then there would be one central place for changing coloured output support for the entire code base; and I would change the macros to fallback to plain text output if there is any doubt the terminal would not support it. Besides, QEMU just switched to meson which uses coloured output as well, as do clang, GCC, git and many other tools in your build chain. Best regards, Christian Schoenebeck
On 24/10/2020 13.24, Christian Schoenebeck wrote: > On Samstag, 24. Oktober 2020 08:04:20 CEST Thomas Huth wrote: >> On 08/10/2020 20.34, Christian Schoenebeck wrote: >>> This new function is purely for debugging purposes. It prints the >>> current qos graph to stdout and allows to identify problems in the >>> created qos graph e.g. when writing new qos tests. >>> >>> Coloured output is used to mark available nodes in green colour, >>> whereas unavailable nodes are marked in red colour. >>> >>> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> >>> --- >>> >>> tests/qtest/libqos/qgraph.c | 56 +++++++++++++++++++++++++++++++++++++ >>> tests/qtest/libqos/qgraph.h | 20 +++++++++++++ >>> 2 files changed, 76 insertions(+) >>> >>> diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c >>> index 61faf6b27d..af93e38dcb 100644 >>> --- a/tests/qtest/libqos/qgraph.c >>> +++ b/tests/qtest/libqos/qgraph.c >>> @@ -805,3 +805,59 @@ void qos_delete_cmd_line(const char *name) >>> >>> node->command_line = NULL; >>> >>> } >>> >>> } >>> >>> + >>> +#define RED(txt) ( \ >>> + "\033[0;91m" txt \ >>> + "\033[0m" \ >>> +) >>> + >>> +#define GREEN(txt) ( \ >>> + "\033[0;92m" txt \ >>> + "\033[0m" \ >>> +) >> >> I don't think this is very portable - and it will only make logs ugly to >> read in text editors. Could you please simply drop these macros? >> >> Thomas > > The precise way I did it here is definitely unclean. And the use case is > trivial, so on doubt I could just drop it of course. > > But allow me one attempt to promote coloured terminal output in general: These > are ANSI color escape sequences, a standard with its youngest revision dating > back to 1991. It is a well supported standard on all major platforms nowadays: > > https://en.wikipedia.org/wiki/ANSI_escape_code > > It works on macOS's standard terminal for at least 20 years, with cmd.exe on > Windows 10, on essentially all Linux and BSD distros, and even on many web > based CI platforms. > > So what about introducing some globally shared macros for coloured output > instead? Then there would be one central place for changing coloured output > support for the entire code base; and I would change the macros to fallback to > plain text output if there is any doubt the terminal would not support it. > > Besides, QEMU just switched to meson which uses coloured output as well, as do > clang, GCC, git and many other tools in your build chain. Sure, colored output is nice, but we certainly also need a way to disable it, e.g. if you want to collect the log in a file and then have a look at it in a text editor. Thomas
On 10/28/20 12:51 AM, Thomas Huth wrote: >>>> +#define GREEN(txt) ( \ >>>> + "\033[0;92m" txt \ >>>> + "\033[0m" \ >>>> +) >>> >>> I don't think this is very portable - and it will only make logs ugly to >>> read in text editors. Could you please simply drop these macros? >>> > Sure, colored output is nice, but we certainly also need a way to disable > it, e.g. if you want to collect the log in a file and then have a look at it > in a text editor. Agreed. GNU libtextstyle (https://www.gnu.org/software/gettext/libtextstyle/manual/libtextstyle.html) is a much more portable way to do colored output where it becomes easy to enable/disable or even adjust the colors to user preferences. Sadly, it is GPLv3+, and thus unusable for qemu. But the bare minimum that you must have when making colored output gated on whether stdout is a terminal (that is, any program that does color should have a --color=off|auto|on command-line option, and that in turn implies function calls rather than macros to properly encapsulate the decision logic.
On Mittwoch, 28. Oktober 2020 14:00:21 CET Eric Blake wrote: > On 10/28/20 12:51 AM, Thomas Huth wrote: > >>>> +#define GREEN(txt) ( \ > >>>> + "\033[0;92m" txt \ > >>>> + "\033[0m" \ > >>>> +) > >>> > >>> I don't think this is very portable - and it will only make logs ugly to > >>> read in text editors. Could you please simply drop these macros? > > > > Sure, colored output is nice, but we certainly also need a way to disable > > it, e.g. if you want to collect the log in a file and then have a look at > > it in a text editor. > > Agreed. GNU libtextstyle > (https://www.gnu.org/software/gettext/libtextstyle/manual/libtextstyle.html) > is a much more portable way to do colored output where it becomes easy to > enable/disable or even adjust the colors to user preferences. Sadly, it is > GPLv3+, and thus unusable for qemu. But the bare minimum that you must > have when making colored output gated on whether stdout is a > terminal (that is, any program that does color should have a > --color=off|auto|on command-line option, and that in turn implies > function calls rather than macros to properly encapsulate the decision > logic. Not sure if it would make sense adding another dependency just for colour support in QEMU anyway, because rendering the right output sequence is not the big issue, nor auto detecting tty colour support, nor handling user configs. A large number of apps already do that in-tree / inline. The challenge in QEMU though (in contrast to stand-alone apps) is integrating this meaningful for all the (quite different) output channels in QEMU, e.g. host logs, test case output, different modes, etc., while catching misusage and retaining a simple API. I postpone the colour issue for that reason and drop colour from these patches for now. I'll probably rather come up with a dedicated series attempt just for colour at some later point. Best regards, Christian Schoenebeck
The currently existing 9pfs test cases are all solely using the 9pfs 'synth' fileystem driver, which is a very simple and purely simulated (in RAM only) filesystem. There are issues though where the 'synth' fs driver is not sufficient. For example the following two bugs need test cases running the 9pfs 'local' fs driver: https://bugs.launchpad.net/qemu/+bug/1336794 https://bugs.launchpad.net/qemu/+bug/1877384 This patch set for that reason introduces 9pfs test cases using the 9pfs 'local' filesystem driver along to the already existing tests on 'synth'. It consists of 3 parts: 1. libqos patches 1 and 2 remove a limitation of the qtest/libqos subsystem: support for more than one device using the same (official) QEMU device name. 2. Patches 3 to 7 enhance debugging issues with the qtest framework. 3. Patches 8 to 12 actually introduce 9pfs 'local' test cases using the qtest framework. These 'local' tests are adding a test directory 'qtest-9p-local' inside the current working directory (using get_current_dir()), which is typically the build directory, before running the tests. That test directory is automatically recreated next time the test suite is run again, to ensure the 9pfs 'local' tests always run consistently on a clean test directory. The test directory is used by the 'local' driver as root of its export path. So it will add/read/write/delete real files and directories inside that test directory. v3->v4: * Added qos_printf() and qos_printf_literal() (NEW patch 3). * Use qos_printf() and qos_printf_literal() instead of printf() for not breaking TAP output (patch 4, 6, 7). * Added private function regex_replace() to simplify and deduplicate code (patch 9). * RegEx pattern hardening in virtio_9p_assign_local_driver() to avoid potential issues with future changes (patch 9). * Updated commit log comments. v2->v3: * concat_path() is now just a wrapper for g_build_filename() (patch 8). * Dropped function strpr(), using g_strdup_printf() instead (patch 8, 9). * Fixed incorrect size for array allocation in split() function (patch 11). v1->v2: * The libqos debugging features are now turned on by command line argument '--verbose' instead of using environment variables (patches 4, 5, 6). * The new 9pfs 'local' tests no longer depend on patches 1 and 2 by no longer using a libqos multi-device approach, but rather modifying the final QEMU command line for each test instead; see discussion of v1 for reason (patches 7 to 11). * Use GCC_FMT_ATTR on helper function strpr() (patch 8). * Updated commit log comments. Christian Schoenebeck (12): libqos/qgraph: add qemu_name to QOSGraphNode libqos/qgraph: add qos_node_create_driver_named() libqos/qgraph_internal: add qos_printf() and qos_printf_literal() libqos/qgraph: add qos_dump_graph() tests/qtest/qos-test: dump qos graph if verbose tests/qtest/qos-test: dump environment variables if verbose tests/qtest/qos-test: dump QEMU command if verbose tests/9pfs: change qtest name prefix to synth tests/9pfs: introduce local tests tests/9pfs: wipe local 9pfs test directory tests/9pfs: add virtio_9p_test_path() tests/9pfs: add local Tmkdir test tests/qtest/libqos/qgraph.c | 110 ++++++++++++++- tests/qtest/libqos/qgraph.h | 36 +++++ tests/qtest/libqos/qgraph_internal.h | 12 ++ tests/qtest/libqos/virtio-9p.c | 100 ++++++++++++++ tests/qtest/libqos/virtio-9p.h | 10 ++ tests/qtest/qos-test.c | 15 +- tests/qtest/virtio-9p-test.c | 197 ++++++++++++++++++++++++--- 7 files changed, 455 insertions(+), 25 deletions(-)