CA-403952: Fix TapCtl._errmsg() bytes/str type issue

[stephenchengCloud]

TapCtl runs in binary mode when handling encryption key, so the stderr is of 'bytes' type,
while 'str.rstrip' in _errmsg expects a 'str' object. This results in a bytes/str type mismatch issue.

Locally tested:
Wrote a simple test where line1 is a bytes and line2 is a str

errinfo = [b"line1 ", "line2 ", " lin3"]
# output = map(str.rstrip, errinfo)
output = (line.rstrip() if isinstance(line, str) else line.decode('utf-8').rstrip() for line in errinfo)
print("; ".join(output))

The output is correct:

(3.11.0) stephenche@39:~/temp$ python3 rstrip.py 
line1; line2;  lin3

[MarkSymsCtx]

I so hate the breaking changes between Python 2 & 3 :(

[andyhhp]

eww, the whole point of subprocess's universal_newlines is to get consistently back to having text.

Does tap-ctl -E really produce binary data?

[stephenchengCloud]

eww, the whole point of subprocess's universal_newlines is to get consistently back to having text.

Does tap-ctl -E really produce binary data?

It's not -E option, it's the text_mode = False that matters
See https://github.com/xapi-project/sm/blob/master/drivers/blktap2.py#L388

            text_mode = False
            args.append('-E')

[andyhhp]

That's my question. What is so special about -E so as to need binary rather than textural pipes ?

[MarkSymsCtx]

That's my question. What is so special about -E so as to need binary rather than textural pipes ?

What's so special is that it is supplied an encryption key, in binary form, as stdin

[andyhhp]

That's my question. What is so special about -E so as to need binary rather than textural pipes ?

What's so special is that it is supplied an encryption key, in binary form, as stdin

Well, that's the root of all of this mess then.

There doesn't appear to be a way to get Popen() to have differing text/binary mode on stdin vs others. It's all or nothing.

What's the likelihood of getting a version of tap-ctl which behaves more like normal posix tools? Low, I'm guessing

[MarkSymsCtx]

That's my question. What is so special about -E so as to need binary rather than textural pipes ?

What's so special is that it is supplied an encryption key, in binary form, as stdin

Well, that's the root of all of this mess then.

There doesn't appear to be a way to get Popen() to have differing text/binary mode on stdin vs others. It's all or nothing.

What's the likelihood of getting a version of tap-ctl which behaves more like normal posix tools? Low, I'm guessing

@andyhhp How would you make it handlle getting a binary feed on the stdin? This was an intentional design choice when support for encryption was added to avoid the key being written to disk or recorded on the commandline for the process. We have control of the tap-ctl process so improvements, if reasonable are doable

[andyhhp]

What's so special is that it is supplied an encryption key, in binary form, as stdin

Well, that's the root of all of this mess then.
There doesn't appear to be a way to get Popen() to have differing text/binary mode on stdin vs others. It's all or nothing.
What's the likelihood of getting a version of tap-ctl which behaves more like normal posix tools? Low, I'm guessing

@andyhhp How would you make it handlle getting a binary feed on the stdin? This was an intentional design choice when support for encryption was added to avoid the key being written to disk or recorded on the commandline for the process. We have control of the tap-ctl process so improvements, if reasonable are doable

Taking the key on stdin is fine, but the far more normal way of doing this would be to use base64 encoded text, not binary.

[MarkSymsCtx]

@andyhhp has valid points here, I'm going to raise a follow on ticket to update the tap-ctl command to permit the use of base64 encoded binary data for the encryption key. There are similar issues around VHD and CBT bitmaps

• https://github.com/xapi-project/sm/blob/master/drivers/cbtutil.py#L85
• https://github.com/xapi-project/sm/blob/master/drivers/vhdutil.py#L302
  So those utils should also be extended to be able to return base64 encoded binary data

[stephenchengCloud]

As discussed above, need to use base64 for the encryption key.
So this PR is not needed. Close this PR.