Bug 884874 - [PATCH] snapshot API, and fix for create linear API.
Summary: [PATCH] snapshot API, and fix for create linear API.
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: python-lvm
Version: 18
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Andy Grover
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-12-06 22:05 UTC by James Antill
Modified: 2013-01-08 00:59 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2013-01-08 00:59:18 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Fix create linear. (717 bytes, patch)
2012-12-06 22:05 UTC, James Antill
no flags Details | Diff
Add snapshot C API (2.34 KB, patch)
2012-12-06 22:06 UTC, James Antill
no flags Details | Diff
Add python snapshot API (1.70 KB, patch)
2012-12-06 22:06 UTC, James Antill
no flags Details | Diff

Description James Antill 2012-12-06 22:05:38 UTC
Created attachment 659067 [details]
Fix create linear.

Description of problem:

 We can't create snapshots from the C/python APIs, this adds that functionality ... works for me :).
 Also fix the long vs. uintmax problem on get args. for create_linear.

 This is the fix:
http://james.fedorapeople.org/patches/pylvm2-fix-size-create-linear.patch

 This is to add snapshot creation to the lvm2 C API:
http://james.fedorapeople.org/patches/lvm2-create-snapshot-api.patch

 This is to add lv.snapshot() to the python API:
http://james.fedorapeople.org/patches/pylvm2-create-snapshot-api.patch

 After both you can do:

lv = vg.lvFromName(originname)
lv.snapshot(snapname, snapsize)

Comment 1 James Antill 2012-12-06 22:06:18 UTC
Created attachment 659068 [details]
Add snapshot C API

Comment 2 James Antill 2012-12-06 22:06:46 UTC
Created attachment 659069 [details]
Add python snapshot API

Comment 3 Andy Grover 2012-12-07 19:25:29 UTC
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?

Comment 4 Tony Asleson 2012-12-07 19:36:03 UTC
(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.

Comment 5 James Antill 2012-12-10 15:39:24 UTC
 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.

Comment 6 Andy Grover 2013-01-08 00:59:18 UTC
This has been applied to lvm2 -- python-lvm is EOLed so not going to add new features.


Note You need to log in before you can comment on or make changes to this bug.