On 16-07-15 18:28, John Ferlan wrote:
On 07/14/2015 04:13 PM, Josh Durgin wrote:
On 07/14/2015 12:42 PM, John Ferlan wrote:
On 07/14/2015 04:15 AM, Wido den Hollander wrote:
We used to look at the librbd code version and depending on that we would invoke rbd_create3() or rbd_create().
Since librbd version 0.67.9 we can however tell RBD that it should create rbd format 2 images even if we invoke rbd_create().
The>> less options we pass to librbd, the more we can lean on the sane defaults it uses.
For rbd_create3() we had things like the stripe count and unit hardcoded in libvirt and that might cause problems down the road.
Hardcoding the feature bits is even worse. I think this is the right approach.
OK - just trying to be sure... and since no one else spoke up yet, seems the approach is fine.
Yes, I think Josh gave the right comments.
...
virStorageBackendRBDCreateImage(rados_ioctx_t io, char *name, long capacity) { int order = 0; -#if LIBRBD_VERSION_CODE > 260 - uint64_t features = 3; - uint64_t stripe_count = 1; - uint64_t stripe_unit = 4194304; - - if (rbd_create3(io, name, capacity, features, &order, - stripe_unit, stripe_count) < 0) { -#else if (rbd_create(io, name, capacity, &order) < 0) { -#endif
This change fails make syntax-check due to unnecessary curly braces.
Before I push, the question I see a second issue...
The current code returns -1 regardless of error, which is then used in a call to virReportSystemError(-r,...)... That means regardless of the error you're returning EPERM which could throw someone off if something other than PERM was returned from rbd_create
I can fix that as well by :
return rbd_create(io, name, capacity, &order);
Seems like a sane approach to me. Less if-statements and the code still works just fine. You can apply my patch and fix this afterwards? Or do you want a new patch from me? Wido
FWIW: This reporting snafu was introduced by commit id '761491e' which just took the return of virStorageBackendRBDCreateImage and used it as the basis for the message generated.
John