rados: add MgrCommand & MgrCommandWithInputBuffer functions #221
Conversation
phlogistonjohn
force-pushed
the
jjm-rados-command-funcs
branch
from
797f277
to
2cdcfe2
Compare
phlogistonjohn
force-pushed
the
jjm-rados-command-funcs
branch
from
b1f5976
to
7d5e58d
Compare
ansiwen
changed the title
| } | ||
|
|
||
| // MonCommandWithInputBuffer sends a command to one of the monitors, with an input buffer | ||
| func (c *Conn) MonCommandWithInputBuffer(args, inputBuffer []byte) ([]byte, string, error) { |
There was a problem hiding this comment.
What's the reason that you wrap a function with identical signature? (I don't have an issue with it actually, just curious.)
There was a problem hiding this comment.
That's the way it was previously done in the codebase before we got here. I've actually considered changing it so that Command calls CommandWithInputBuffer but I didn't want to make those kinds of refactorings here.
| } | ||
|
|
||
| func (c *Conn) monCommand(args, inputBuffer []byte) ([]byte, string, error) { | ||
| argv := C.CString(string(args)) |
There was a problem hiding this comment.
I find argv confusing here, because argv is normally a pointer to a string array. Also, why is the parameter called args, suggesting it's an array of arguments, but in reality it's the string of the first argument. Why is it a byte array, and not a string? Later the PGCommand uses [][]byte as type for args, so its also not consistent. (There is also a CBytes() converter, but I see, we need a char* for the rados API.)
There was a problem hiding this comment.
Again, this predates my involvement in the project.
The naming and the type conistency stuff is something I do want to tackle, but in a somewhat backwards compatible way.
| ) | ||
| inbuf := C.CString(string(inputBuffer)) | ||
| inbufLen := len(inputBuffer) | ||
| defer C.free(unsafe.Pointer(inbuf)) |
|
|
||
| if outslen > 0 { | ||
| info = C.GoStringN(outs, C.int(outslen)) | ||
| C.free(unsafe.Pointer(outs)) |
There was a problem hiding this comment.
The docs say: the caller is expected to release that memory with rados_buffer_free() but you are using the standard free. Also wouldn't it be a safer pattern to defer that in the main scope, since free(NULL) is a noop?
| } | ||
| if outbuflen > 0 { | ||
| buffer = C.GoBytes(unsafe.Pointer(outbuf), C.int(outbuflen)) | ||
| C.free(unsafe.Pointer(outbuf)) |
|
|
||
| if outslen > 0 { | ||
| info = C.GoStringN(outs, C.int(outslen)) | ||
| C.free(unsafe.Pointer(outs)) |
There was a problem hiding this comment.
Didn’t check the docs, but probably you also should use the rados_buffer_free() here?
| } | ||
| if outbuflen > 0 { | ||
| buffer = C.GoBytes(unsafe.Pointer(outbuf), C.int(outbuflen)) | ||
| C.free(unsafe.Pointer(outbuf)) |
| argc := len(args) | ||
| argv := make([]*C.char, argc) | ||
|
|
||
| for i, arg := range args { |
|
|
||
| if outslen > 0 { | ||
| info = C.GoStringN(outs, C.int(outslen)) | ||
| C.free(unsafe.Pointer(outs)) |
| } | ||
| if outbuflen > 0 { | ||
| buffer = C.GoBytes(unsafe.Pointer(outbuf), C.int(outbuflen)) | ||
| C.free(unsafe.Pointer(outbuf)) |
|
@ansiwen please take a look at my WIP/RFC pr #226. It addresses some of your points about consistency and repeated code. |
|
@phlogistonjohn Yeah, I should have reviewed the individual commits, so that I see what is copied and what is from you. But at least the missing |
|
The code in that function in ceph is just a wrapper around free ATM. I agree that we may want to use the proper cleanup function in case ceph chooses to do something more in the future. But I'd still like to tackle that task in #226 or later. |
Organize the rados package a bit more by moving functions that wrap rados_xyz_command functions into a dedicated file. Move related test to a separate file as well. Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Add functions for issuing commands to ceph mgrs with MgrCommand and MgrCommandWithInputBuffer, implemented simliarly to the other *Command style functions. Signed-off-by: John Mulligan <jmulligan@redhat.com>
phlogistonjohn
force-pushed
the
jjm-rados-command-funcs
branch
from
7e6eaf6
to
c7ae138
Compare
First, the *Command functions and tests are moved into separate files to reduce the amount of stuff in conn.go & friends. The function signatures are simplified a bit.
Then new functions MgrCommand & MgrCommandWithInputBuffer are added that wrap rados_mgr_command are added along with tests.
Fixes #216
Checklist