Skip to content

CP-41972: update ACK to python3 and fixed some problems#201

Merged
tianxiahcp merged 1 commit intoxenserver:masterfrom
alexhimmel:private/tianxia/CP-41972
Apr 25, 2023
Merged

CP-41972: update ACK to python3 and fixed some problems#201
tianxiahcp merged 1 commit intoxenserver:masterfrom
alexhimmel:private/tianxia/CP-41972

Conversation

@alexhimmel
Copy link
Copy Markdown
Contributor

All test passed
XenRT Job ID: 3717307, 3717308, 3717309, 3717310, 3717311

@alexhimmel alexhimmel force-pushed the private/tianxia/CP-41972 branch from 765bc61 to b2355dd Compare April 19, 2023 07:45
@alexhimmel
Copy link
Copy Markdown
Contributor Author

Re-tested with xcp changes in: xenserver/python-libs#21
All Passed
Job ID: 3721663, 3721665, 3721666, 3721667, 3721668

Copy link
Copy Markdown

@lindig lindig left a comment

Choose a reason for hiding this comment

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

I'm not a Python expert but this looks good to me.

log.debug("VM %s has these vif IPs %s" % (vm_ref, ifs))

for _, f in ifs.items():
for _, f in list(ifs.items()):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why need to list()

vm_ref, [[f[0], f[1], f[2].split('/')[0]] for _, f in list(ifs.items())])
mif = get_context_vm_mif(vm_ref)
mdevices = [f[0] for _, f in ifs.items() if f[1] == mif[1]]
mdevices = [f[0] for _, f in list(ifs.items()) if f[1] == mif[1]]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same 3 to above.

host_ref = session.xenapi.VM.get_resident_on(vm_ref)
mip = get_context_vm_mip(vm_ref)
for id, vif_info in vifs_info.items():
for id, vif_info in list(vifs_info.items()):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same 3 to above

# get all MAC
all_mac = []
for _, iface in ifs.items():
for _, iface in list(ifs.items()):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same 2 to above

"""Return the xml attributes of a node as a dictionary object"""
attr = {}
for k, v in node._get_attributes().items():
for k, v in list(node._get_attributes().items()):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same 2 to above.

stdout.decode().strip(), "stderr": str(stderr).strip()}
else:
res = {"returncode": process.returncode, "stdout":
str(stdout).strip(), "stderr": str(stderr).strip()}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am not sure if it could return None actually, while the code you should simplify:
{"returncode": process.returncode, "stdout": <Move if else here.>, "stderr": str(stderr).strip()}

@DeliZhangX
Copy link
Copy Markdown

I suggested you to investigate whether subprocess.Popen() (has parameter to keep compatibility with python2) supports text than byte return, so that need not to call encode/decode everywhere.
I checked documents looks the paramter is text=None, so you can try it.

@liulinC
Copy link
Copy Markdown
Contributor

liulinC commented Apr 23, 2023

I suggested you to investigate whether subprocess.Popen() (has parameter to keep compatibility with python2) supports text than byte return, so that need not to call encode/decode everywhere. I checked documents looks the paramter is text=None, so you can try it.

universal_newlines (which is rename to Text) can be used for this:

If the encoding or errors arguments were specified or the text or universal_newlines argument was True, the stream is a text stream, otherwise it is a byte

@alexhimmel alexhimmel force-pushed the private/tianxia/CP-41972 branch from 15c958e to a7e279c Compare April 25, 2023 02:40
@tianxiahcp
Copy link
Copy Markdown
Contributor

For Popen(), if python version > 3.7 can use text=True, in 3.6.8 will use universal_newlines=True
re-run test all passed
Job ID: 3726722 ~ 3726726 and 3726728 ~ 3726732

@tianxiahcp tianxiahcp merged commit 69ac4bc into xenserver:master Apr 25, 2023
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