Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virttest/utils_test: fixes to virtio serial file transfer #1903

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wainersm
Copy link
Contributor

@wainersm wainersm commented Jan 9, 2019

The run_virtio_serial_file_transfer() function runs python scripts
on (usually) both guest and host to test serial communication. It
assumes Python 2 is installed on both side, which is not always
true. So this change ensure it uses the python binary (python3 preferred)
available. It was added the find_host_python() function which is
used to find the python binary on host.

This change also contain some misc fixes:
a) Ensure proper comparisons of byte type with Python 3.
b) Ensure the working directory is created on the guest.
c) Fix the calculation of received bytes by the guest script.

Signed-off-by: Wainer dos Santos Moschetta [email protected]

The run_virtio_serial_file_transfer() function runs python scripts
on (usually) both guest and host to test serial communication. It
assumes Python 2 is installed on both side, which is not always
true. So this change ensure it uses the python binary (python3 preferred)
available. It was added the find_host_python() function which is
used to find the python binary on host.

This change also contain some misc fixes:
a) Ensure proper comparisons of byte type with Python 3.
b) Ensure the working directory is created on the guest.
c) Fix the calculation of received bytes by the guest script.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
@wainersm wainersm requested review from ldoktor and luckyh January 9, 2019 20:28
@wainersm
Copy link
Contributor Author

wainersm commented Jan 9, 2019

The type_specific.io-github-autotest-qemu.virtio_serial_file_transfer is completely broken if executed on an environment where both host and guest have Python 3 only.

With the fixes I am sending on this PR I was able to execute successfully that test case with file_sender parameter set to either guest or both. However it still fails if file_sender is host, and I suspect that happens even on Python 2 environment.

@@ -180,7 +179,7 @@ def receive(device, filename, p_size=1024):
txt = vio.receive(p_size)
md5_value.update(txt)
file_no.write(txt)
recv_size += p_size
recv_size += len(txt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw you should split the patches. Good hint to splitting patches is when you need to use "also" in either description or comment or, as in your case, you need to itemize the changes. They look fairly simple to split (let me suggest my favorited "git cola" ;-) for that)

host_cmd = "python %s -s %s -f %s -a %s" % (host_script, host_device,
host_data_file, action)

host_python = find_host_python()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative approach is using an inline shell statement $(command -v python3 python | head -1), but this assumes we are running in bash.

guest_script = params.get("guest_script",
"VirtIoChannel_guest_send_receive.py")
guest_script = os.path.join(guest_path, guest_script)

guest_cmd = "python %s -d %s -f %s -a %s" % (guest_script, port_name,
guest_data_file, guest_action)
guest_python = find_python(session)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have windows guests, so that would be a problem..

@luckyh
Copy link
Contributor

luckyh commented Jan 10, 2019

@vivianQizhu please help review this patch, thanks.

out = process.run("which %s" % python, shell=True)
if out.exit_status == 0:
return python

Copy link
Contributor

@yanan-fu yanan-fu Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even use this method, we can improve find_python to make it work with both host and guest, according to the value of session .

if session:
    s = session.cmd_status(***)
else:
    s = process.run(***).exist_status

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about simply using session.cmd_status("%s --version" % python), that should work for linux as well as windows 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, it is available for both linux and windows :)

@@ -433,6 +433,12 @@ def find_python(session, try_binaries=('python3', 'python2', 'python')):
if session.cmd_status("which %s" % python) == 0:
return python

def find_host_python(try_binaries=('python3', 'python2', 'python')):
for python in try_binaries:
out = process.run("which %s" % python, shell=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need ignore_status=True here, otherwise it will raise error on the host without python3.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or simply use process.system

@@ -742,6 +748,7 @@ def transfer_data(session, host_cmd, guest_cmd, n_time, timeout,

host_data_file = os.path.join(dir_name,
"tmp-%s" % utils_misc.generate_random_string(8))
session.cmd("mkdir -p %s" % tmp_dir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this resolves the failure for file_sender = guest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants