Skip to content

Commit

Permalink
Update Zabbix Rust library
Browse files Browse the repository at this point in the history
  • Loading branch information
@dnaeon
dnaeon committed Jul 22, 2015
1 parent 5871f58 commit faa0e34
Showing 1 changed file with 55 additions and 64 deletions.
119 changes: 55 additions & 64 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
extern crate libc;

use std::{ffi, ptr};
use std::ffi;
use libc::{c_char, c_int, c_uint, uint64_t, c_double, malloc, strncpy};

// Return codes used by module during (un)initialization
Expand Down Expand Up @@ -35,70 +35,63 @@ pub const AR_MESSAGE: c_int = 32;
pub const SYSINFO_RET_OK: c_int = 0;
pub const SYSINFO_RET_FAIL: c_int = 1;

#[repr(C)]
#[derive(Copy)]
pub struct ZabbixMetric {
key: *const c_char,
flags: c_uint,
function: extern "C" fn(*mut ZabbixRequest, *mut ZabbixResult) -> c_int,
test_param: *const c_char,
}

impl Clone for ZabbixMetric {
fn clone(&self) -> Self { *self }
// Type used for creating new Zabbix item keys
pub struct Metric {
pub key: ffi::CString,
pub flags: usize,
pub function: extern "C" fn(*mut AGENT_REQUEST, *mut AGENT_RESULT) -> c_int,
pub test_param: ffi::CString,
}

impl ZabbixMetric {
pub fn new(key: Option<&str>,
flags: Option<u32>,
function: Option<extern "C" fn(*mut ZabbixRequest, *mut ZabbixResult) -> i32>,
test_param: Option<&str>) -> ZabbixMetric {

let c_key: *const c_char = match key {
Some(ref k) => ffi::CString::new(*k).unwrap().as_ptr(),
None => ptr::null(),
};

let flags: c_uint = match flags {
Some(f) => f,
None => CF_NOPARAMS,
};

let function: extern "C" fn(*mut ZabbixRequest, *mut ZabbixResult) -> c_int = match function {
Some(callback) => callback,
None => dummy_callback,
};

let c_test_param: *const c_char = match test_param {
Some(ref t) => ffi::CString::new(*t).unwrap().as_ptr(),
None => ptr::null(),
};

ZabbixMetric {
key: c_key,
impl Metric {
pub fn new(key: &str, flags: usize, function: extern "C" fn(*mut AGENT_REQUEST, *mut AGENT_RESULT) -> c_int, test_param: &str) -> Metric {
Metric {
key: ffi::CString::new(key).unwrap(),
flags: flags,
function: function,
test_param: c_test_param,
test_param: ffi::CString::new(test_param).unwrap(),
}
}

pub fn to_zabbix_item(&self) -> ZBX_METRIC {
ZBX_METRIC {
key: self.key.as_ptr(),
flags: self.flags as c_uint,
function: self.function,
test_param: self.test_param.as_ptr(),
}
}
}

#[repr(C)]
#[derive(Copy)]
pub struct ZBX_METRIC {
pub key: *const c_char,
pub flags: c_uint,
pub function: extern "C" fn(*mut AGENT_REQUEST, *mut AGENT_RESULT) -> c_int,
pub test_param: *const c_char,
}

impl Clone for ZBX_METRIC {
fn clone(&self) -> Self { *self }
}

#[repr(C)]
#[derive(Copy)]
pub struct ZabbixRequest {
pub struct AGENT_REQUEST {
key: *const c_char,
nparam: c_int,
params: *const *const c_char,
lastlogsize: uint64_t,
mtime: c_int,
}

impl Clone for ZabbixRequest {
impl Clone for AGENT_REQUEST {
fn clone(&self) -> Self { *self }
}

impl ZabbixRequest {
pub fn get_params<'a>(request: *mut ZabbixRequest) -> Vec<&'a[u8]> {
impl AGENT_REQUEST {
pub fn get_params<'a>(request: *mut AGENT_REQUEST) -> Vec<&'a[u8]> {
unsafe {
let len = (*request).nparam;
let mut v = Vec::new();
Expand Down Expand Up @@ -132,7 +125,7 @@ impl Clone for zbx_log_t {

#[repr(C)]
#[derive(Copy)]
pub struct ZabbixResult {
pub struct AGENT_RESULT {
_type: c_int,
ui64: uint64_t,
dbl: c_double,
Expand All @@ -142,57 +135,47 @@ pub struct ZabbixResult {
logs: *const *const zbx_log_t,
}

impl Clone for ZabbixResult {
impl Clone for AGENT_RESULT {
fn clone(&self) -> Self { *self }
}

impl ZabbixResult {
pub fn set_uint64_result(result: *mut ZabbixResult, value: u64) {
impl AGENT_RESULT {
pub fn set_uint64_result(result: *mut AGENT_RESULT, value: u64) {
unsafe {
(*result)._type |= AR_UINT64;
(*result).ui64 = value as uint64_t;
}
}

pub fn set_f64_result(result: *mut ZabbixResult, value: f64) {
pub fn set_f64_result(result: *mut AGENT_RESULT, value: f64) {
unsafe {
(*result)._type |= AR_DOUBLE;
(*result).dbl = value as c_double;
}
}

pub fn set_str_result(result: *mut ZabbixResult, value: &str) {
pub fn set_str_result(result: *mut AGENT_RESULT, value: &str) {
unsafe {
(*result)._type |= AR_STRING;
(*result)._str = string_to_malloc_ptr(value);
}
}

pub fn set_text_result(result: *mut ZabbixResult, value: &str) {
pub fn set_text_result(result: *mut AGENT_RESULT, value: &str) {
unsafe {
(*result)._type |= AR_TEXT;
(*result).text = string_to_malloc_ptr(value);
}
}

// TODO: Implement set_log_result(...)

pub fn set_msg_result(result: *mut ZabbixResult, value: &str) {
pub fn set_msg_result(result: *mut AGENT_RESULT, value: &str) {
unsafe {
(*result)._type |= AR_MESSAGE;
(*result).msg = string_to_malloc_ptr(value);
}
}
}

// Dummy Zabbix item callback function.
// Callback is used by NULL-key Zabbix items to specify the
// end of the items list.
// Do not use this callback directly in your crates.
#[no_mangle]
#[allow(unused_variables)]
pub extern fn dummy_callback(request: *mut ZabbixRequest, result: *mut ZabbixResult) -> c_int {
SYSINFO_RET_OK
// TODO: Implement set_log_result(...)
}

// When the result of a Zabbix item is text (string, text and message)
Expand All @@ -208,3 +191,11 @@ unsafe fn string_to_malloc_ptr(src: &str) -> *mut c_char {
dst
}

pub fn create_items(metrics: &Vec<Metric>) -> *const ZBX_METRIC {
let items = metrics
.iter()
.map(|metric| metric.to_zabbix_item())
.collect::<Vec<_>>();

items.as_ptr()

This comment has been minimized.

Sign in to view

Copy link
@gkoz

gkoz Jul 27, 2015

This returns a dangling pointer, as items' lifetime is the body of this function.

A hacky solution could be to leak the vector, making it live forever.

let ptr = items.as_ptr();
mem::forget(items);
ptr

A clean solution is probably to store the vector somewhere and clean up in zbx_module_uninit.

This comment has been minimized.

Sign in to view

Copy link
@dnaeon

dnaeon Jul 28, 2015

Author Owner

Nice catch, thanks! I will work on getting this fixed.

Right now I'm thinking of using atexit if that works or implement something like this here - rust-lang/rust#11695

If you have some thoughts and ideas to share that would be great, thanks!

This comment has been minimized.

Sign in to view

Copy link
@gkoz

gkoz Jul 28, 2015

Well, between atexit and zbx_module_uninit I'd sure choose the latter :)

BTW a commenter on reddit got this 'righter' than I: this actually goes deeper, the ZBX_METRIC structure doesn't own the key so forgetting the vector is not enough.
}

5 comments on commit faa0e34

@dnaeon
Copy link
Owner Author

@dnaeon dnaeon commented on faa0e34 Jul 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, just noticed today that the key needs to be owned as well by the structure. So far the only way I see this as possible is to leak the key and test_param as well.

A clean solution would be indeed to use zbx_module_uninit to de-allocate everything, but so far I can't find a solution that would allow me to register all allocations I make, which could be then de-allocated in zbx_module_uninit, e.g. use something like a global vector to store all pointers, which can then be de-allocated.

Thanks for your input so far!

@gkoz
Copy link

@gkoz gkoz commented on faa0e34 Jul 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If (likely) zbx_module_item_list only gets called once, leaking seems perfectly fine.

In the most general (I have no idea if zabbix guarantees to invoke the callbacks on a single thread) case I'd consider making something like

lazy_static! { // https://crates.io/crates/lazy_static
    static ref STUFF: Mutex<RefCell<Vec<Box<Any + Send>>>> =
        Mutex::new(RefCell::new(Vec::new()));
}

fn stash<T: Any + Send>(x: T) {
    STUFF.lock().borrow_mut().push(Box::new(x));
}

extern fn zbx_module_uninit() {
    STUFF.lock().borrow_mut().clear();
}

This would require ZBX_METRIC and whatever else you want to stash to impl Send but that's okay as long as it owns its data.

@dnaeon
Copy link
Owner Author

@dnaeon dnaeon commented on faa0e34 Jul 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkoz Thanks a lot for your help!

You are correct about zbx_module_item_list being called once only, and I've managed to get it "right" by leaking the key and test_param fields (valgrind doesn't seem happy about it, but at least I'm not passing a dangling pointer anymore).

I'm still in the process of learning how things are done right in Rust, so your feedback is much appreciated!

Considering that leaking the key and test_param seems a bit hackish, I'm considering also the possibility of writing a small C wrapper library around the Zabbix C API that would provide functions for creating & destroying ZBX_METRIC structs, which I could later wrap via the Rust FFI - that might be a cleaner solution, although not sure about it :)

What I'm also surprised to see, though is that the current implementation works and runs just fine, while I would expect to see a segfault since these structs were supposed to be destroyed when going out of scope and the pointers I pass to Zabbix should be invalid.

@gkoz
Copy link

@gkoz gkoz commented on faa0e34 Jul 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience maintaining C glue is too much of a pain in the ass unless it does something truly impossible in Rust (e.g. accessing bit fields of a struct).

Speaking of how things are done in Rust, it would be more Rusty to isolate the client from all the ffi details and raw pointers. Imagine a trait

pub trait Metric {
    fn has_params(&self) -> bool; // not sure how best to handle this
    fn process(&mut self, request: AgentRequest) -> Result<AgentResult, ()>;
}

(AgentRequest and AgentResponse encapsulating any unsafe operations) and the rust-zbx library expecting dummy to export a single function

extern "rust" fn init() -> Vec<(String, Box<Metric>)> {
    vec![("ping".to_string(), Box::new(Ping::new()))]
}

struct Ping { ... }

impl Metric for Ping { ... }

init would create the metrics and return pairs of (key, metric) that would from that point on be managed by rust-zbx (e.g. a HashMap indexed by key).

@dnaeon
Copy link
Owner Author

@dnaeon dnaeon commented on faa0e34 Jul 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkoz That sounds like a nice idea, thanks!

I'll work on getting this implemented soon, thanks again for your help on this! :)

Please sign in to comment.