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

ceph-disk: convert none str to str before printing it #12760

Merged
1 commit merged into from Jan 5, 2017

Conversation

Projects
None yet
2 participants
@tchaikov
Contributor

tchaikov commented Jan 3, 2017

Error('somethings goes wrong', e) is thrown if exception e is caught
in ceph-disk, where e is not a string. so we can not just concat it in
Error's str(). so cast it to str before doing so.

introduced by d0e29c7

Fixes: http://tracker.ceph.com/issues/18371
Signed-off-by: Kefu Chai kchai@redhat.com

@tchaikov tchaikov requested a review from Jan 3, 2017

@alfredodeza

This comment has been minimized.

Contributor

alfredodeza commented Jan 3, 2017

I think the patch would work, but the function _bytes2str is implicitly suggesting an input of bytes, not a None or anything else.

Looking at the uses for this function, doing a str(string) would mean that a None input would return "None" which would look very strange when converting the string in the line that broke initially:

return ': '.join([doc] + [_bytes2str(a) for a in self.args])

If the exception class is assuming it will always get a string, it should maybe check when that is not the case (vs. changing the helper function that converts bytes)

ceph-disk: convert none str to str before printing it
Error('somethings goes wrong', e) is thrown if exception `e` is caught
in ceph-disk, where e is not a string. so we can not just concat it in
Error's __str__(). so cast it to str before doing so.

introduced by d0e29c7

Fixes: http://tracker.ceph.com/issues/18371
Signed-off-by: Kefu Chai <kchai@redhat.com>

@tchaikov tchaikov requested a review from alfredodeza Jan 4, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 4, 2017

@alfredodeza comments addressed and repushed.

@tchaikov tchaikov added the needs-qa label Jan 4, 2017

@tchaikov tchaikov self-assigned this Jan 4, 2017

str_type = basestring
except NameError:
str_type = str
args = [a if isinstance(a, str_type) else str(a) for a in self.args]

This comment has been minimized.

@alfredodeza

alfredodeza Jan 4, 2017

Contributor

shouldn't this use str_type to convert? e.g.:

args = [a if isinstance(a, str_type) else str_type(a) for a in self.args]

This comment has been minimized.

@tchaikov

tchaikov Jan 5, 2017

Contributor

@alfredodeza in the case of python2, a bytes instance is a basestring, so does str and unicode. in other words, b'hello', 'hello', and u'hello' are all "basestring". the reason i want to check if a is an instance of str_type is to see if we need to convert it to a str (non-unicode) on python2 or a str (unicode) on python3 by calling the magic method of __str__() or __repr__() magic method. here, str is the class we can use it to convert an printable object to its string representation. on python3, there is no more non-unicode str and unicode str, and str is unified. the reason i check if isinstance(a, basestring) is to see if we need to convert it to a str. and basestring type cannot be instantiated. instead it is just used to tell if an object is a "string" in general sense. please note, normally __str__() does not return a bytes instance. so we can't use str_type on python2.

@ghost ghost merged commit 23e6de2 into ceph:master Jan 5, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@tchaikov tchaikov deleted the tchaikov:wip-18371 branch Jan 5, 2017

asheplyakov pushed a commit to asheplyakov/pkg-ceph that referenced this pull request Jan 31, 2017

asheplyakov pushed a commit to asheplyakov/pkg-ceph that referenced this pull request Feb 3, 2017

@ghost

This comment has been minimized.

ghost commented Jul 5, 2017

It seems that this is missing in packages for 11.2.0-0.el7. I've applied the patch successfully.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 11, 2017

@yves-vogl yes, it was merged into kraken after the v11.2.0 was tagged. see #13501 and https://github.com/ceph/ceph/tree/v11.2.0

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment