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.

Sign up for GitHub

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

qcontainer: clean up QEMU machine option related code #1922

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

qcontainer: clean up QEMU machine option related code #1922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions selftests/unit/test_qemu_devices.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,18 +566,20 @@ def setUp(self):
def tearDown(self):
self.god.unstub_all()

def create_qdev(self, vm_name='vm1', strict_mode="no",
def create_qdev(self, vm_name='vm1', machine_type='pc', strict_mode="no",
allow_hotplugged_vm="yes"):
""" :return: Initialized qcontainer.DevContainer object """
qemu_cmd = '/usr/bin/qemu_kvm'
qcontainer.process.system_output.expect_call('%s -help' % qemu_cmd,
qcontainer.process.system_output.expect_call('%s -M %s -help' %
(qemu_cmd, machine_type),
timeout=10,
ignore_status=True,
shell=True,
verbose=False
).and_return(QEMU_HELP)
qcontainer.process.system_output.expect_call("%s -device \? 2>&1"
% qemu_cmd, timeout=10,
qcontainer.process.system_output.expect_call("%s -M %s -device \? 2>&1"
% (qemu_cmd, machine_type),
timeout=10,
ignore_status=True,
shell=True,
verbose=False
Expand All @@ -588,7 +590,8 @@ def create_qdev(self, vm_name='vm1', strict_mode="no",
shell=True,
verbose=False
).and_return(QEMU_MACHINE)
cmd = "echo -e 'help\nquit' | %s -monitor stdio -vnc none" % qemu_cmd
cmd = "echo -e 'help\nquit' | %s -M %s -monitor stdio -vnc none" % \
(qemu_cmd, machine_type)
qcontainer.process.system_output.expect_call(cmd, timeout=10,
ignore_status=True,
shell=True,
Expand All @@ -597,8 +600,8 @@ def create_qdev(self, vm_name='vm1', strict_mode="no",
cmd = ('echo -e \'{ "execute": "qmp_capabilities" }\n'
'{ "execute": "query-commands", "id": "RAND91" }\n'
'{ "execute": "quit" }\''
'| %s -qmp stdio -vnc none | grep return |'
' grep RAND91' % qemu_cmd)
' | %s -M %s -qmp stdio -vnc none | grep return | grep RAND91'
% (qemu_cmd, machine_type))
qcontainer.process.system_output.expect_call(cmd, timeout=10,
ignore_status=True,
shell=True,
Expand All @@ -608,15 +611,15 @@ def create_qdev(self, vm_name='vm1', strict_mode="no",
cmd = ('echo -e \'{ "execute": "qmp_capabilities" }\n'
'{ "execute": "query-commands", "id": "RAND91" }\n'
'{ "execute": "quit" }\' | (sleep 1; cat )'
'| %s -qmp stdio -vnc none | grep return |'
' grep RAND91' % qemu_cmd)
' | %s -M %s -qmp stdio -vnc none | grep return | grep RAND91'
% (qemu_cmd, machine_type))
qcontainer.process.system_output.expect_call(cmd, timeout=10,
ignore_status=True,
shell=True,
verbose=False
).and_return(QEMU_QMP)

qdev = qcontainer.DevContainer(qemu_cmd, vm_name, strict_mode, 'no',
qdev = qcontainer.DevContainer(qemu_cmd, vm_name, machine_type, strict_mode, 'no',
allow_hotplugged_vm)

self.god.check_playback()
Expand Down Expand Up @@ -825,7 +828,7 @@ def test_qdev_functional(self):

def test_qdev_hotplug(self):
""" Test the hotplug/unplug functionality """
qdev = self.create_qdev('vm1', False, True)
qdev = self.create_qdev('vm1', 'pc', False, True)
devs = qdev.machine_by_params(ParamsDict({'machine_type': 'pc'}))
for dev in devs:
qdev.insert(dev)
Expand Down
80 changes: 35 additions & 45 deletions virttest/qemu_devices/qcontainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,22 @@ class DevContainer(object):
"""
# General methods

def __init__(self, qemu_binary, vmname, strict_mode="no",
def __init__(self, qemu_binary, vmname, machine_type, strict_mode="no",
Copy link
Contributor

Choose a reason for hiding this comment

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

Always add new arguments to the end of the list to avoid breaking existing calls... What I mean is someone might be using DevContainer(bin, name, "yes") directly (and there is at least one test io-github-autotest-qemu/qemu/tests/slof_device_tree.py) that would be affected by your change. When you introduce the new argument as last providing a safe default, the change will be backward compatible. The question is what is backward compatible as s390 does not define virt and arm won't work without -M...

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we could perhaps use machine_type=none. Not sure whether we use any features that would be affected.

workaround_qemu_qmp_crash="no", allow_hotplugged_vm="yes"):
"""
:param qemu_binary: qemu binary
:param vm: related VM
:param machine_type: VM machine type
:param strict_mode: Use strict mode (set optional params)
"""
def get_hmp_cmds(qemu_binary):
def get_hmp_cmds():
""" :return: list of human monitor commands """
_ = decode_to_text(process.system_output("echo -e 'help\nquit' | %s -monitor "
"stdio -vnc none" % qemu_binary,
timeout=10, ignore_status=True,
shell=True, verbose=False))
c = "echo -e 'help\nquit' | %s -M %s -monitor stdio -vnc none" % \
Copy link
Contributor

Choose a reason for hiding this comment

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

c (nor c1, ...) match the pylint style (minimum 3 chars).

(self.__qemu_binary, self.__machine_type)
_ = decode_to_text(process.system_output(c, timeout=10,
ignore_status=True,
shell=True,
verbose=False))
_ = re.findall(r'^([^()\|\[\sA-Z]+\|?\w+)', _, re.M)
hmp_cmds = []
for cmd in _:
Expand All @@ -65,26 +68,26 @@ def get_hmp_cmds(qemu_binary):
hmp_cmds.extend(cmd.split('|'))
return hmp_cmds

def get_qmp_cmds(qemu_binary, workaround_qemu_qmp_crash=False):
def get_qmp_cmds(workaround_qemu_qmp_crash=False):
""" :return: list of qmp commands """
c1 = ('echo -e \'{ "execute": "qmp_capabilities" }\n'
'{ "execute": "query-commands", "id": "RAND91" }\n'
'{ "execute": "quit" }\'')
c2 = '(sleep 1; cat )'
c3 = '%s -M %s -qmp stdio -vnc none | grep return | grep RAND91' % \
(self.__qemu_binary, self.__machine_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I slightly prefer having it embedded directly (for the sake of memory consumption), but it can be a bit easier to read like that. Anyway the variables would have to be named properly to express what they stand for and more importantly, do not mix style fixes with refactors, nor new features. They have to be in separate commits. (a good indication of a bad commit is when you need to use also or and in the commit message to join 2 different things)


cmds = None
if not workaround_qemu_qmp_crash:
cmds = decode_to_text(process.system_output('echo -e \''
'{ "execute": "qmp_capabilities" }\n'
'{ "execute": "query-commands", "id": "RAND91" }\n'
'{ "execute": "quit" }\''
'| %s -qmp stdio -vnc none | grep return |'
' grep RAND91' % qemu_binary, timeout=10,
ignore_status=True, shell=True,
cmds = decode_to_text(process.system_output("%s | %s" % (c1, c3),
timeout=10,
ignore_status=True,
shell=True,
verbose=False)).splitlines()
if not cmds:
# Some qemu versions crashes when qmp used too early; add sleep
cmds = decode_to_text(process.system_output('echo -e \''
'{ "execute": "qmp_capabilities" }\n'
'{ "execute": "query-commands", "id": "RAND91" }\n'
'{ "execute": "quit" }\' | (sleep 1; cat )'
'| %s -qmp stdio -vnc none | grep return |'
' grep RAND91' % qemu_binary, timeout=10,
cmds = decode_to_text(process.system_output("%s | %s | %s" % (c1, c2, c3),
timeout=10,
ignore_status=True, shell=True,
verbose=False)).splitlines()
if cmds:
Expand All @@ -93,34 +96,18 @@ def get_qmp_cmds(qemu_binary, workaround_qemu_qmp_crash=False):
return cmds

self.__state = -1 # -1 synchronized, 0 synchronized after hotplug
self.__machine_type = machine_type
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use the term __base_machine_type or __early_machine_type as later in the code the machine_type could be different/changed/....

self.__qemu_binary = qemu_binary
self.__execute_qemu_last = None
self.__execute_qemu_out = ""
# Check whether we need to add machine_type
cmd = "%s -device \? 2>&1" % qemu_binary
result = process.run(cmd, timeout=10,
ignore_status=True,
shell=True,
verbose=False)
# Some architectures (arm) require machine type to be always set
if result.exit_status and b"machine specified" in result.stdout:
self.__workaround_machine_type = True
basic_qemu_cmd = "%s -machine virt" % qemu_binary
else:
self.__workaround_machine_type = False
basic_qemu_cmd = qemu_binary

self.__qemu_help = self.execute_qemu("-help", 10)
# escape the '?' otherwise it will fail if we have a single-char
# filename in cwd
self.__device_help = self.execute_qemu("-device \? 2>&1", 10)
self.__machine_types = decode_to_text(process.system_output("%s -M \?" % qemu_binary,
timeout=10,
ignore_status=True,
shell=True,
verbose=False))
self.__hmp_cmds = get_hmp_cmds(basic_qemu_cmd)
self.__qmp_cmds = get_qmp_cmds(basic_qemu_cmd,
workaround_qemu_qmp_crash == 'always')
self.__machine_types = decode_to_text(self.execute_qemu("-M \?", 10))
self.__hmp_cmds = get_hmp_cmds()
self.__qmp_cmds = get_qmp_cmds(workaround_qemu_qmp_crash == 'always')
self.vmname = vmname
self.strict_mode = strict_mode == 'yes'
self.__devices = []
Expand Down Expand Up @@ -430,6 +417,9 @@ def has_qmp_cmd(self, cmd):
def execute_qemu(self, options, timeout=5):
"""
Execute this qemu and return the stdout+stderr output.

The function adds -M option to qemu command line if the options
argument doesn't contain it.
:param options: additional qemu options
:type options: string
:param timeout: execution timeout
Expand All @@ -438,11 +428,11 @@ def execute_qemu(self, options, timeout=5):
:rtype: string
"""
if self.__execute_qemu_last != options:
if self.__workaround_machine_type:
cmd = "%s -machine virt %s 2>&1" % (self.__qemu_binary,
options)
if "-M" in options or "-machine" in options:
cmd = "%s %s" % (self.__qemu_binary, options)
else:
cmd = "%s %s 2>&1" % (self.__qemu_binary, options)
cmd = "%s -M %s %s" % (self.__qemu_binary,
self.__machine_type, options)
result = process.run(cmd, timeout=timeout,
ignore_status=True,
shell=True,
Expand Down
3 changes: 2 additions & 1 deletion virttest/qemu_vm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1493,7 +1493,8 @@ def sort_key(dev):
cmd += "numactl -m %s " % n

# Start constructing devices representation
devices = qcontainer.DevContainer(qemu_binary, self.name,
machine_type = params.get('machine_type').split(':', 1)[-1]
devices = qcontainer.DevContainer(qemu_binary, self.name, machine_type,
Copy link
Contributor

Choose a reason for hiding this comment

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

Well I like the idea of using the machine-type used in testing when probing qemu, but it would affect negative testing with bad machine types. Have you encountered a situation where -M virt was not enough?

params.get('strict_mode'),
params.get(
'workaround_qemu_qmp_crash'),
Expand Down