Add support for FreeBSD to ltfs_ordered_copy #380
Conversation
Values for errno are system-dependent. Now we are using the Python errno module instead of hard-coded values.
FreeBSD has xattr library instead of pyxattr, but both libraries have the same methods by now.
| @@ -34,6 +34,7 @@ | |||
| # OO_Copyright_END | |||
|
|
|||
| import sys | |||
| import errno | |||
There was a problem hiding this comment.
This errno handling looks good to me.
| except Exception as e: | ||
| self.vuuid = '' | ||
| return (self.vuuid, self.part, self.start) | ||
|
|
||
| try: | ||
| self.part = xattr.get(self.src, self.vea_pre + 'ltfs.partition') | ||
| start_str = xattr.get(self.src, self.vea_pre + 'ltfs.startblock') | ||
| self.part = xattr.getxattr(self.src, self.vea_pre + 'ltfs.partition') |
There was a problem hiding this comment.
Are you using the pyxattr module? Now we are expecting the xattr module. And I think xattr.getxattr is not provided in the xattr package.
The reason why we assume the xattr module is it is provided by the pyhon-xattr on the official repository of RHEL7/8. As you see, the first target of this project is RHEL7 and RHEL8.
So I cannot accept this change. Please create another ltfs_ordered_copy for pyxattr.
There was a problem hiding this comment.
In FreeBSD I use xattr, which provides xattr.getxattr and alike.
I don't have access to RHEL7/8 but I've seen some documents from redhat with a list of packages and their licenses, an I see one called pyxattr (in RHEL7) and one called python3-pyxattr (in RHEL8) with LGPLv2+ license. Based on that, I assume that RHEL is really using pyxattr module (because the other xattr is MIT licensed).
The pyxattr module (the one I assume is in RHEL) deprectared xattr.getxattr some time ago in favor of xattr.get. But the deprecated functions are still available (at least this is what I see in the main branch of pyxattr in github).
So, by now the use of xattr.xattrget works on both modules.
I understand you may have concerns with this PR because we don't know what the pyxattr folks will do regarding these deprecated functions. So, maybe the best way to deal with that is to only merge the errno handling code, and then I may ask the FreeBSD port maintainer to do the rest of the patch there.
There was a problem hiding this comment.
Oops, sorry I was completely confused and I need to apologize to you.
The module we are using is pyxattr, so we can accept this. Let me have more time to confirm that this modification doesn't break the behavior on RHEL7/8.
The IBM docs says it is pyxattr.
There was a problem hiding this comment.
But I think it is a good idea to sepalate errno fix and getxattr fix. I can merge the errno fix ASAP as you mentioned.
There was a problem hiding this comment.
Oops, sorry for keeping as is so long time.
I decided to merge this on the master branch. We have a time to test it before releasing from this branch!
|
@micronn , Thank you for your great contribution. We will test the code in the next release phase. |
Summary of changes
Do not hardcore errno numbers in ltfs_ordered_copy because those are system-dependent. Python has an errno module in its standard library which provides the correct numbers on each platform. I've also put a helper function to check for errno because some may not be present (some systems use ENODATA but other systems do not have it defined and instead use ENOATTR).
Use methods like getxattr instead of get. The pyxattr library obsoleted those, but by now these are still available, and some systems like FreeBSD use the xattr library which do not have the get method. If, in the future, pyxattr library removes these methods, we could do something like if not hasattr(xattr, 'getxattr'): xattr.getxattr = xattr.get, but by now I think we're fine.