qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 13/15] tests/functional: rewrite console handling to be bytew


From: Paolo Bonzini
Subject: Re: [PATCH 13/15] tests/functional: rewrite console handling to be bytewise
Date: Tue, 19 Nov 2024 20:26:20 +0100
User-agent: Mozilla Thunderbird

On 11/19/24 19:54, Daniel P. Berrangé wrote:
+        if success is None or success in msg:

As an optimization, you could use msg.endswith(success) and
msg.endswith(failure), which would avoid the most blatant cases of O(n^2)
behavior.

More important, I think "if success is None" should not be here, because it
will exit after one char.  Instead...

+            done = True
+            break
+        if failure and failure in msg:
+            done = True
+            vm.console_socket.close()
+            test.fail(
+                f"'{failure}' found in console, expected '{success}'")
+
+        if c == b'\n':

Here you can put

                done = success is None

Hmmm, this can only be a problem if "success" is None, and
"failure" is not None, and although the old code would
technically work in that case, I think it is actually an
unknown/invalid usage scenario.

If BOTH "success" and "failure" are None, this method won't
be called at all. It is valid for "failure" to be none, but
I don't think it makes semantic sense for "success" to also
be None, while have "failure" be non-None.

"Read a line and check that it doesn't contain a substring" seemed like a plausible thing to test for, but you're right it doesn't make much sense in this context. It would make more sense if _console_readline returned the full line, at which point it would be a completely different function and probably not underscore-prefixed.

So yeah, this is okay:

  assert send_string is not None or success_message is not None

whether done in the caller or in _console_readline itself, as you prefer.

Paolo




reply via email to

[Prev in Thread] Current Thread [Next in Thread]