Bug 884874
| Summary: | [PATCH] snapshot API, and fix for create linear API. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | James Antill <james.antill> | ||||||||
| Component: | python-lvm | Assignee: | Andy Grover <agrover> | ||||||||
| Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||||||
| Severity: | unspecified | Docs Contact: | |||||||||
| Priority: | unspecified | ||||||||||
| Version: | 18 | CC: | agk, agrover, tasleson | ||||||||
| Target Milestone: | --- | ||||||||||
| Target Release: | --- | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Whiteboard: | |||||||||||
| Fixed In Version: | Doc Type: | Bug Fix | |||||||||
| Doc Text: | Story Points: | --- | |||||||||
| Clone Of: | Environment: | ||||||||||
| Last Closed: | 2013-01-08 00:59:18 UTC | Type: | Bug | ||||||||
| Regression: | --- | Mount Type: | --- | ||||||||
| Documentation: | --- | CRM: | |||||||||
| Verified Versions: | Category: | --- | |||||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||||
| Embargoed: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
James Antill
2012-12-06 22:05:38 UTC
Created attachment 659068 [details]
Add snapshot C API
Created attachment 659069 [details]
Add python snapshot API
Hi James, My colleague Tony actually posted some very similar patches back about a month ago, but they weren't committed -- I'm not sure why. If someone other than just Tony and I are working on this code we need to be better about this! :-( Sorry. OK about the code: * for the fix, can't we simply cast &size to unsigned long * instead of needing a new variable? Also, last added line uses spaces instead of tab, something to watch out for. * for the C api patch, looking at the two versions I think Tony's version here: https://www.redhat.com/archives/lvm-devel/2012-November/msg00023.html might be a little more complete. Would you mind reviewing his patch and posting any edits you have here, so we can commit the best of both? * For the Python patch, I think your patch is a little more straightforward than Tony's: https://www.redhat.com/archives/lvm-devel/2012-November/msg00022.html but also uses asize, so maybe we can just use a cast here too? (In reply to comment #3) > * For the Python patch, I think your patch is a little more straightforward > than Tony's: I was trying to eliminate dupliate code that is common between creating a linear volume and creating a snapshot. I would be fine with either approach. Good to know I got the solution pretty close to perfect then :).
On the C API side, it seemed more "logical" to me that the API looks similar to lvm_vg_create_lv_linear() (and be at the same "level" etc.) ... but you guys know the code better than me, so I'll defer to you there. Having size == 0 work is a nice bonus, and it's probably worth have lv.snapshot("foo") work using size==0.
Not sure why you need to set segtype and stripes (it worked without, and I'd assume they'd be inherited from the origin) ... but I could easily be missing something there :).
For the python side, I was trying to do the opposite of Tony ... make sure I didn't break the create function while adding/testing the snapshot one :). Having less code is fine by me :).
You can't just cast though as sizeof(long) == 4 and sizeof(uint64_t) == 8 on i386, so AFAIK the only way to workaround that is to have a second variable.
This has been applied to lvm2 -- python-lvm is EOLed so not going to add new features. |