-
Notifications
You must be signed in to change notification settings - Fork 238
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
qemu_guest_agent: Update shutdown() #1391
qemu_guest_agent: Update shutdown() #1391
Conversation
0b11c66
to
2ef134d
Compare
(1/2) smp_8.8192m.repeat1.run_test.Host_RHEL.m7.u3.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.4.x86_64.myprovider.qemu_guest_agent.virtio_serial.check_reset_reboot_shutdown_fsfreeze: PASS (98.68 s) |
tp-qemu patch |
virttest/guest_agent.py
Outdated
self.cmd(cmd=cmd, args=args, success_resp=False) | ||
try: | ||
self.cmd(cmd=cmd, args=args, success_resp=True) | ||
except VAgentCmdError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better return the detailed error msg for debugging usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lijinlijin It will return "The command xxx has been disabled for this instance" error msg when executing shutdown. This msg is from VAgentCmdError, VAgentCmdError depend on qemu monitor. Do you wish like this "except 'The command xxx has been disabled for this instance'": ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lijinlijin In order to tp-qemu can get this error Msg, raise this error Msg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need to check the content of the error message in your test context, here should let self.cmd()
raise the VAgentCmdError
directly instead of catching it and returning False
.
So finally the code would be:
- self.cmd(cmd=cmd, args=args, success_resp=False)
- return True
+ self.cmd(cmd=cmd, args=args)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luckyh You are right, cmd() can raise this Error. I have updated it and re-tested it.
(1/2) Host_RHEL.m7.u6.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.6.x86_64.io-github-autotest-qemu.qemu_guest_agent.virtio_serial.check_reset_reboot_shutdown_fsfreeze: PASS (124.79 s)
(2/2) Host_RHEL.m7.u6.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.6.x86_64.io-github-autotest-qemu.qemu_guest_agent.isa_serial.check_reset_reboot_shutdown_fsfreeze: PASS (143.99 s)
2ef134d
to
a299214
Compare
virttest/guest_agent.py
Outdated
try: | ||
self.cmd(cmd=cmd, args=args, success_resp=True) | ||
except VAgentCmdError: | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above approach has the same meaning as self.cmd(cmd=cmd, args=args, success_resp=True)
, the problem is there are several types of error but only some of them would not be allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luckyh I need to cautch this VAgentCmdError's output. when excepting VAgentCmdError, In order not to interrupt this function. I only replace raise with str(e).
4417434
to
bb8a351
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luckyh return False as below
-
except VAgentCmdError as e:
-
str(e)
-
return False
(1/2) smp_4.1024m.repeat1.run_test.Host_RHEL.m7.u5.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.4.x86_64.io-github-autotest-qemu.qemu_guest_agent.virtio_serial.check_reset_reboot_shutdown_fsfreeze: PASS (81.91 s)
(2/2) smp_4.1024m.repeat1.run_test.Host_RHEL.m7.u5.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.4.x86_64.io-github-autotest-qemu.qemu_guest_agent.isa_serial.check_reset_reboot_shutdown_fsfreeze: PASS (81.57 s)
virttest/guest_agent.py
Outdated
try: | ||
self.cmd(cmd=cmd, args=args, success_resp=True) | ||
except VAgentCmdError: | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luckyh I need to cautch this VAgentCmdError's output. when excepting VAgentCmdError, In order not to interrupt this function. I only replace raise with str(e).
virttest/guest_agent.py
Outdated
self.cmd(cmd=cmd, args=args, success_resp=True) | ||
except VAgentCmdError as e: | ||
str(e) | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on autotest/tp-qemu#1032
except VAgentCmdError:
return False
would be enough.
Can you explain what str(e)
is for on line 515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hereischen You are right, Don't need "str(e)". (original idea was to use str(e) to call str(self) method in Class VAgentCmdError).
8db4d55
to
f8c4ce4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
f8c4ce4
to
df465d6
Compare
@xiangchunfu the code looks good to me now, but I still request a sensible commit message, thanks. |
df465d6
to
7b768f1
Compare
b6caf4d
to
30c1bf0
Compare
f9cad87
to
fee5f09
Compare
7d7df45
to
cf95c17
Compare
@luckyh Would you please check if the commit message proper and help merge it if so? Thanks. |
virttest/guest_agent.py
Outdated
@@ -509,8 +509,7 @@ def shutdown(self, mode=SHUTDOWN_MODE_POWERDOWN): | |||
if mode in [self.SHUTDOWN_MODE_POWERDOWN, self.SHUTDOWN_MODE_REBOOT, | |||
self.SHUTDOWN_MODE_HALT]: | |||
args = {"mode": mode} | |||
self.cmd(cmd=cmd, args=args, success_resp=False) | |||
return True | |||
self.cmd(cmd=cmd, args=args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized that it should ignore the error that there is no response after sending the command, so you should catch VAgentProtocolError
and ignore it at here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luckyh You are right, I have updated.
cf95c17
to
5e05837
Compare
Currently, shutdown() returns "{}" or no response after sending the shutdown command, remove success_resp=False option to raise error message,and remove useless "return True" and ignore VAgentProtocolErro at the same time. Signed-off-by: Xiangchun Fu <xfu@redhat.com>
5e05837
to
24accd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
guest_agent: Update shutdown()
Currently, shutdown() returns "{}" or no response after sending the shutdown command,
remove success_resp=False option to raise error message,and remove useless
"return True" and ignore VAgentProtocolErro at the same time.
ID: 1468530, 1476648
Signed-off-by: Xiangchun Fu xfu@redhat.com