Netkvm: use WinDump for capture and Wireshark for analysis #4097
Conversation
|
Test Result: |
|
Hi @leidwang and @yanglei-rh, Could you guys help me review this patch when you are free? Thanks in advance. |
|
Please re-write the test steps, the whole process is not installed |
|
Hi @yanglei-rh, Thanks for your advice. I will provide the whole test cases which include unattended_install and the case that I want to test. For this patch, I have installed the |
| x86_64: | ||
| wireshark_name = "Wireshark-win64-1.10.1.exe" | ||
| i386, i686: | ||
| wireshark_name = "Wireshark-win32-1.10.1.exe" | ||
| install_wireshark_cmd = "xcopy WIN_UTILS:\${wireshark_name} c:\ /y && c:\${wireshark_name} /S" | ||
| installation_cmd = "xcopy WIN_UTILS:\${WinDump_name} c:\ /y && xcopy WIN_UTILS:\${wireshark_name} c:\ /y && c:\${wireshark_name} /S" |
There was a problem hiding this comment.
Seems this cmd will copy windump and wireshark to C and then install wireshark, is this expected?
There was a problem hiding this comment.
As the title of this PR says: "Netkvm: Replace tshark.exe with WinDump.exe".
Why does your code still install wireshark but not windump?
There was a problem hiding this comment.
At the end of this case, we still need to analyze the packet's result with Wireshark only. However, we have used Windump to capture NetFlow data since Wireshark has a bug.
There was a problem hiding this comment.
For the WinDump part, WinDump.exe is a portable software that does not need to be installed.
Thank @leidwang and let me think about the patch title again : )
But what confuses me is that it always failed with " avocado.core.exceptions.TestError: Failed to parse session log file, status=9009, output='"C:\Program Files\Wireshark\tshark.exe"' is not recognized as an internal or external command," on my side, why you can test pass? Could you please try to test it again with other guest? Such as Win11 or Win2025, or both two. |
|
Thanks, guys. Let me do more tests. |
|
Hi @heywji Please mark this PR as draft status if it is not ready to review,thanks. |
heywji
force-pushed
the
change_to_tshark
branch
from
a64cc14 to
a1850ec
Compare
heywji
force-pushed
the
change_to_tshark
branch
2 times, most recently
from
1901e8d to
857bfcd
Compare
heywji
force-pushed
the
change_to_tshark
branch
from
857bfcd to
9d784a3
Compare
heywji
changed the title
heywji
force-pushed
the
change_to_tshark
branch
from
9d784a3 to
e7fce8b
Compare
|
Hi @yanglei-rh @leidwang, Could you guys help review it again since everything works well now? Test result: |
|
Hi @vivianQizhu, Could you help review this as well? Thanks : ) |
heywji
force-pushed
the
change_to_tshark
branch
from
e7fce8b to
791578d
Compare
heywji
force-pushed
the
change_to_tshark
branch
3 times, most recently
from
5d22a50 to
e7ec1d1
Compare
|
Test result: |
qemu/tests/enable_scatter_windows.py
Outdated
| error_context.context("Check if WinDump is installed", test.log.info) | ||
| check_result = session.cmd_output(check_windump_installed_cmd) | ||
| if windump_name not in check_result: | ||
| error_context.context("Install WinDump", test.log.info) | ||
| windump_install_cmd = utils_misc.set_winutils_letter( | ||
| session, windump_install_cmd | ||
| ) | ||
| status, output = session.cmd_status_output(windump_install_cmd, timeout=timeout) | ||
| if status != 0: | ||
| test.error( | ||
| "Failed to install WinDump, status=%s, output=%s" % (status, output) | ||
| ) | ||
| else: | ||
| test.log.info("WinDump is already installed") | ||
|
|
||
| error_context.context("Check if wireshark is installed", test.log.info) | ||
| check_installed_cmd = params.get("check_installed_cmd") | ||
| check_result = session.cmd_output(check_installed_cmd) | ||
| check_wireshark_installed_cmd = params.get("check_wireshark_installed_cmd") | ||
| check_result = session.cmd_output(check_wireshark_installed_cmd) | ||
| if "tshark" not in check_result: | ||
| error_context.context("Install wireshark", test.log.info) | ||
| install_wireshark_cmd = params.get("install_wireshark_cmd") | ||
| install_wireshark_cmd = utils_misc.set_winutils_letter( | ||
| session, install_wireshark_cmd | ||
| wireshark_installation_cmd = params.get("wireshark_installation_cmd") | ||
| wireshark_installation_cmd = utils_misc.set_winutils_letter( | ||
| session, wireshark_installation_cmd | ||
| ) | ||
| status, output = session.cmd_status_output( | ||
| install_wireshark_cmd, timeout=timeout | ||
| wireshark_installation_cmd, timeout=timeout | ||
| ) | ||
| if status: | ||
| test.error( |
There was a problem hiding this comment.
Could you try to implement them into a for loop? e.g. for tool_name in ("windump", "wireshark") or it could be a dict as you might want to name a different prefix instead of the tool name itself.
heywji
force-pushed
the
change_to_tshark
branch
2 times, most recently
from
399d4d3 to
03af606
Compare
heywji
force-pushed
the
change_to_tshark
branch
2 times, most recently
from
3f7652b to
cb94ac9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@heywji please re-install a image and clone this patch to test again to verify you patch is correct, since there are include install steps. |
heywji
force-pushed
the
change_to_tshark
branch
4 times, most recently
from
7bd62fb to
afc9167
Compare
qemu/tests/enable_scatter_windows.py
Outdated
| @@ -143,7 +147,7 @@ def _get_driver_version(session): | |||
| vm.verify_alive() | |||
| host_ip = utils_net.get_host_ip_address(params) | |||
|
|
|||
| session = vm.wait_for_login(timeout=timeout) | |||
| session = vm.wait_for_serial_login(timeout=timeout) | |||
There was a problem hiding this comment.
Hi all,
I found that the ws2016 of this patch failed consistently. After debugging, I modified this line 150 as well from ssh_login to serial_login cause it is the same as the patch Netkvm: Fix session handling issue during netkvm driver installation #4158.
It looks good for now. I will post the test result as soon as possible.
Thanks,
Wecom
There was a problem hiding this comment.
Cancelled this code change cause the problem has gone.
heywji
force-pushed
the
change_to_tshark
branch
from
afc9167 to
1ebbdcd
Compare
|
Test result: |
heywji
force-pushed
the
change_to_tshark
branch
4 times, most recently
from
b7b532b to
2a834da
Compare
| ), | ||
| 20, | ||
| 5, | ||
| 1, |
There was a problem hiding this comment.
Do we have to change it this way?
There was a problem hiding this comment.
Yes, The pre-commit tells me I must modify the code this way.
qemu/tests/enable_scatter_windows.py
Outdated
| tools = { | ||
| "windump": { | ||
| "check_cmd": params.get("check_windump_installed_cmd"), | ||
| "install_cmd": utils_misc.set_winutils_letter( | ||
| session, params.get("windump_install_cmd") | ||
| ), | ||
| "process_name": params.get("windump_name"), | ||
| }, | ||
| "wireshark": { | ||
| "check_cmd": params.get("check_wireshark_installed_cmd"), | ||
| "install_cmd": utils_misc.set_winutils_letter( | ||
| session, params.get("wireshark_installation_cmd") | ||
| ), | ||
| "process_name": "tshark", | ||
| }, | ||
| } |
There was a problem hiding this comment.
This simply does not make sense.
for tool in ['windump', 'tshark']:
check_cmd = params.get("check_%s_installed_cmd" % tool)
install_cmd = utils_misc.set_winutils_letter(session, params.get("%s_install_cmd" % tool))
And replace params wireshark_* to tshark_* in cfg.
There was a problem hiding this comment.
Also it should be params[""] instead of params.get(), as if check_cmd is idle the following steps would fail.
qemu/tests/enable_scatter_windows.py
Outdated
| timeout, | ||
| 20, | ||
| 3, | ||
| ) |
There was a problem hiding this comment.
I think this could apply to both wireshark and windump.
heywji
force-pushed
the
change_to_tshark
branch
3 times, most recently
from
21cf5b4 to
e690073
Compare
|
@vivianQizhu, I am calling you to review this patch again. It includes all the recommendations you mentioned. Test result on prewhql267: |
qemu/tests/enable_scatter_windows.py
Outdated
| if status: | ||
| test.error( | ||
| "Failed to install wireshark, status=%s, output=%s" % (status, output) | ||
| process_name = tool |
There was a problem hiding this comment.
Why do we need this renaming? You might directly have for process_name in ["windump", "tshark"] if you feel like name it this way.
qemu/tests/enable_scatter_windows.py
Outdated
| is_installed = False | ||
| error_context.context("Check if %s is installed" % process_name, test.log.info) | ||
| check_result = session.cmd_output(check_cmd) | ||
| is_installed = process_name in check_result |
There was a problem hiding this comment.
One line should be sufficient:
| is_installed = False | |
| error_context.context("Check if %s is installed" % process_name, test.log.info) | |
| check_result = session.cmd_output(check_cmd) | |
| is_installed = process_name in check_result | |
| is_installed = process_name in session.cmd_output(check_cmd) |
qemu/tests/enable_scatter_windows.py
Outdated
| check_result = session.cmd_output(check_cmd) | ||
| check_result = check_result.lower() # For Python to be case-sensitive |
There was a problem hiding this comment.
Would the format checker be happy with one line implementation?
| check_result = session.cmd_output(check_cmd) | |
| check_result = check_result.lower() # For Python to be case-sensitive | |
| check_result = session.cmd_output(check_cmd).lower() |
Replace tshark.exe with WinDump.exe for capturing network traffic. This resolves the issue where enabling netkvm driver TxLSO results in no packet length >= 1514 after file transfer, identified as a Wireshark problem. Signed-off-by: wji <[email protected]>
heywji
force-pushed
the
change_to_tshark
branch
from
e690073 to
3e8a964
Compare
|
Test result: All passed. |
Replace tshark.exe with WinDump.exe for capturing network traffic. This resolves the issue where enabling netkvm driver TxLSO results in no packet length >= 1514 after file transfer, identified as a Wireshark problem.
ID: 2395
Signed-off-by: wji [email protected]