[libvirt] [PATCH Rust 1/4] connect: cleanup around Connect::open_auth()'s method
In this refactor we avoid to enclose all the code with unsafe tags and just use it when necessary. Also we add annotations to explain why it's safe to use. The integrations tests related are also been reviewed to avoid using panic. Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- src/connect.rs | 91 ++++++++++++++++++++++----------------- tests/integration_qemu.rs | 16 +++---- 2 files changed, 56 insertions(+), 51 deletions(-) diff --git a/src/connect.rs b/src/connect.rs index 0fa8551..108224d 100644 --- a/src/connect.rs +++ b/src/connect.rs @@ -240,6 +240,41 @@ extern "C" { -> *mut libc::c_char; } +extern "C" fn connectCallback(ccreds: sys::virConnectCredentialPtr, + ncred: libc::c_uint, + cbdata: *mut libc::c_void) + -> libc::c_int { + let callback: ConnectAuthCallback = unsafe { + // Safe because connectCallback is private and only used by + // Connect::open_auth(). In open_auth() we transmute the + // callback allocate in *void. + mem::transmute(cbdata) + }; + let mut rcreds: Vec<ConnectCredential> = Vec::new(); + for i in 0..ncred as isize { + unsafe { + // Safe because ccreds is allocated. + let c = ConnectCredential::from_ptr(ccreds.offset(i)); + rcreds.push(c); + } + } + callback(&mut rcreds); + for i in 0..ncred as isize { + if rcreds[i as usize].result.is_some() { + if let Some(ref result) = rcreds[i as usize].result { + unsafe { + // Safe because ccreds is allocated and the result + // is comming from Rust calls. + (*ccreds.offset(i)).resultlen = result.len() as libc::c_uint; + (*ccreds.offset(i)).result = string_to_mut_c_chars!(result.clone()); + } + } + } + } + 0 +} + + pub type ConnectFlags = self::libc::c_uint; pub const VIR_CONNECT_RO: ConnectFlags = 1 << 0; pub const VIR_CONNECT_NO_ALIASES: ConnectFlags = 1 << 1; @@ -412,39 +447,6 @@ impl ConnectAuth { callback: callback, } } - - fn to_cstruct(&mut self) -> sys::virConnectAuth { - unsafe extern "C" fn wrapper(ccreds: sys::virConnectCredentialPtr, - ncred: libc::c_uint, - cbdata: *mut libc::c_void) - -> libc::c_int { - let callback: ConnectAuthCallback = mem::transmute(cbdata); - let mut rcreds: Vec<ConnectCredential> = Vec::new(); - for i in 0..ncred as isize { - let c = ConnectCredential::from_ptr(ccreds.offset(i)); - rcreds.push(c); - } - callback(&mut rcreds); - for i in 0..ncred as isize { - if rcreds[i as usize].result.is_some() { - if let Some(ref result) = rcreds[i as usize].result { - (*ccreds.offset(i)).resultlen = result.len() as libc::c_uint; - (*ccreds.offset(i)).result = string_to_mut_c_chars!(result.clone()); - } - } - } - 0 - } - - unsafe { - sys::virConnectAuth { - credtype: &mut self.creds[0], - ncredtype: self.creds.len() as u32, - cb: wrapper, - cbdata: mem::transmute(self.callback), - } - } - } } /// Provides APIs for the management of hosts. @@ -554,14 +556,23 @@ impl Connect { auth: &mut ConnectAuth, flags: ConnectFlags) -> Result<Connect, Error> { - unsafe { - let mut cauth = auth.to_cstruct(); - let c = virConnectOpenAuth(string_to_c_chars!(uri), &mut cauth, flags as libc::c_uint); - if c.is_null() { - return Err(Error::new()); - } - return Ok(Connect::new(c)); + let mut cauth = unsafe { + // Safe because Rust forces to allocate all attributes of + // the struct ConnectAuth. + sys::virConnectAuth { + credtype: &mut auth.creds[0], + ncredtype: auth.creds.len() as libc::c_uint, + cb: connectCallback, + cbdata: mem::transmute(auth.callback), + } + }; + let c = unsafe { + virConnectOpenAuth(string_to_c_chars!(uri), &mut cauth, flags as libc::c_uint) + }; + if c.is_null() { + return Err(Error::new()); } + return Ok(Connect::new(c)); } diff --git a/tests/integration_qemu.rs b/tests/integration_qemu.rs index 49e07c4..79aa2bd 100644 --- a/tests/integration_qemu.rs +++ b/tests/integration_qemu.rs @@ -108,14 +108,9 @@ fn test_connection_with_auth() { let mut auth = ConnectAuth::new(vec![::virt::connect::VIR_CRED_AUTHNAME, ::virt::connect::VIR_CRED_PASSPHRASE], callback); - match Connect::open_auth("test+tcp://127.0.0.1/default", &mut auth, 0) { - Ok(c) => common::close(c), - Err(e) => { - panic!("open_auth did not work: code {}, message: {}", - e.code, - e.message) - } - } + let c = Connect::open_auth("test+tcp://127.0.0.1/default", &mut auth, 0); + assert_eq!(true, c.is_ok()); + common::close(c.unwrap()); } @@ -141,9 +136,8 @@ fn test_connection_with_auth_wrong() { let mut auth = ConnectAuth::new(vec![::virt::connect::VIR_CRED_AUTHNAME, ::virt::connect::VIR_CRED_PASSPHRASE], callback); - if Connect::open_auth("test+tcp://127.0.0.1/default", &mut auth, 0).is_ok() { - panic!("open_auth did not work: code {}, message:"); - } + let c = Connect::open_auth("test+tcp://127.0.0.1/default", &mut auth, 0); + assert_eq!(false, c.is_ok()); } #[test] -- 2.17.1
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- src/lib.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f9ede21..77bf4a9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -107,10 +107,6 @@ macro_rules! string_to_mut_c_chars { ($x:expr) => (::std::ffi::CString::new($x).unwrap().into_raw()) } -macro_rules! string_to_mut_c_chars { - ($x:expr) => (::std::ffi::CString::new($x).unwrap().into_raw()) -} - macro_rules! impl_from { // Largely inspired by impl_from! in rust core/num/mod.rs ($Small: ty, $Large: ty) => { -- 2.17.1
In this commit we also add todo and warning to avoid using them + remove them in future. Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- src/lib.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 77bf4a9..64d49cd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -99,12 +99,31 @@ macro_rules! c_chars_to_string { } +// Those two macros are not completely safe and we should probably +// stop using them to avoid possibility of pointers dangling. The +// memory may be freed too early. +// +// To avoid that, the right pattern would be: +// +// let cstring = CString::new(rs_string).unwrap(); +// unsafe { +// some_c_function(cstring.as_ptr() as *const libc::c_char); +// } +// +// So we ensure the pointer passed to 'some_c_function()' will live +// until 'cstring' exists. +// +// TODO(sahid): fix code + remove macros. + macro_rules! string_to_c_chars { - ($x:expr) => (::std::ffi::CString::new($x).unwrap().as_ptr()) + ($x:expr) => ( + ::std::ffi::CString::new($x).unwrap().as_ptr() as *const libc::c_char) } macro_rules! string_to_mut_c_chars { - ($x:expr) => (::std::ffi::CString::new($x).unwrap().into_raw()) + ($x:expr) => ( + // Usage of this should ensure deallocation. + ::std::ffi::CString::new($x).unwrap().into_raw() as *mut libc::c_char) } macro_rules! impl_from { -- 2.17.1
On Wed, Jul 17, 2019 at 08:59:32AM +0000, Sahid Orentino Ferdjaoui wrote:
In this commit we also add todo and warning to avoid using them + remove them in future.
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- src/lib.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/lib.rs b/src/lib.rs index 77bf4a9..64d49cd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -99,12 +99,31 @@ macro_rules! c_chars_to_string {
}
+// Those two macros are not completely safe and we should probably +// stop using them to avoid possibility of pointers dangling. The +// memory may be freed too early. +// +// To avoid that, the right pattern would be: +// +// let cstring = CString::new(rs_string).unwrap(); +// unsafe { +// some_c_function(cstring.as_ptr() as *const libc::c_char); +// } +// +// So we ensure the pointer passed to 'some_c_function()' will live +// until 'cstring' exists. +// +// TODO(sahid): fix code + remove macros. +
Just in case you won't get around to do that, could you put this to the BiteSizedTasks[1] wiki? Also while checking the usage numbers of these macros I found out you pushed this already, so sorry for late review. [1] https://wiki.libvirt.org/page/BiteSizedTasks
macro_rules! string_to_c_chars { - ($x:expr) => (::std::ffi::CString::new($x).unwrap().as_ptr()) + ($x:expr) => ( + ::std::ffi::CString::new($x).unwrap().as_ptr() as *const libc::c_char) }
macro_rules! string_to_mut_c_chars { - ($x:expr) => (::std::ffi::CString::new($x).unwrap().into_raw()) + ($x:expr) => ( + // Usage of this should ensure deallocation. + ::std::ffi::CString::new($x).unwrap().into_raw() as *mut libc::c_char) }
macro_rules! impl_from { -- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
This is fixing the invalid email address in Cargo.toml + update the command examples to send patches so they CC the current maintainer. Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- .gitpublish | 3 ++- Cargo.toml | 2 +- README.md | 12 +++++++----- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/.gitpublish b/.gitpublish index f7a938f..b42ee56 100644 --- a/.gitpublish +++ b/.gitpublish @@ -1,4 +1,5 @@ [gitpublishprofile "default"] base = master to = libvir-list@redhat.com -prefix = rust PATCH +cc = sahid.ferdjaoui@canonical.com +prefix = PATCH Rust diff --git a/Cargo.toml b/Cargo.toml index 25538f0..2136aaf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "virt" version = "0.2.10" -authors = ["Sahid Orentino Ferdjaoui <sahid.ferdjaoui@redhat.com>",] +authors = ["Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com>",] license = "LGPL-2.1" readme = "README.md" description = "Rust bindings to the libvirt C library" diff --git a/README.md b/README.md index 6e1ad64..93cd461 100644 --- a/README.md +++ b/README.md @@ -77,18 +77,20 @@ to submit patches to the libvir-list@redhat.com mailing list. eg. to send a single patch ``` - git send-email --to libvir-list@redhat.com --subject-prefix "PATCH rust" \ - --smtp-server=$HOSTNAME -1 + git send-email --to libvir-list@redhat.com --cc sahid.ferdjaoui@canonical.com \ + --subject-prefix "PATCH Rust" --smtp-server=$HOSTNAME -1 ``` Or to send all patches on the current branch, against master ``` - git send-email --to libvir-list@redhat.com --subject-prefix "PATCH rust" \ - --smtp-server=$HOSTNAME --no-chain-reply-to --cover-letter --annotate \ - master.. + git send-email --to libvir-list@redhat.com --cc sahid.ferdjaoui@canonical.com \ + --subject-prefix "PATCH Rust" --smtp-server=$HOSTNAME --no-chain-reply-to \ + --cover-letter --annotate master.. ``` +It is also possible to use git-publish. + Note the master GIT repository is at * https://libvirt.org/git/?p=libvirt-rust.git;a=summary -- 2.17.1
On Wed, Jul 17, 2019 at 08:59:33AM +0000, Sahid Orentino Ferdjaoui wrote:
This is fixing the invalid email address in Cargo.toml + update the command examples to send patches so they CC the current maintainer.
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- .gitpublish | 3 ++- Cargo.toml | 2 +- README.md | 12 +++++++----- 3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/.gitpublish b/.gitpublish index f7a938f..b42ee56 100644 --- a/.gitpublish +++ b/.gitpublish @@ -1,4 +1,5 @@ [gitpublishprofile "default"] base = master to = libvir-list@redhat.com -prefix = rust PATCH +cc = sahid.ferdjaoui@canonical.com +prefix = PATCH Rust
The prefix should stay the same so that it uses the same subject format as other patch submissions.
diff --git a/Cargo.toml b/Cargo.toml index 25538f0..2136aaf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "virt" version = "0.2.10" -authors = ["Sahid Orentino Ferdjaoui <sahid.ferdjaoui@redhat.com>",] +authors = ["Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com>",] license = "LGPL-2.1" readme = "README.md" description = "Rust bindings to the libvirt C library" diff --git a/README.md b/README.md index 6e1ad64..93cd461 100644 --- a/README.md +++ b/README.md @@ -77,18 +77,20 @@ to submit patches to the libvir-list@redhat.com mailing list. eg. to send a single patch
``` - git send-email --to libvir-list@redhat.com --subject-prefix "PATCH rust" \ - --smtp-server=$HOSTNAME -1 + git send-email --to libvir-list@redhat.com --cc sahid.ferdjaoui@canonical.com \ + --subject-prefix "PATCH Rust" --smtp-server=$HOSTNAME -1 ```
dtto (and with other examples as well)
Or to send all patches on the current branch, against master
``` - git send-email --to libvir-list@redhat.com --subject-prefix "PATCH rust" \ - --smtp-server=$HOSTNAME --no-chain-reply-to --cover-letter --annotate \ - master.. + git send-email --to libvir-list@redhat.com --cc sahid.ferdjaoui@canonical.com \ + --subject-prefix "PATCH Rust" --smtp-server=$HOSTNAME --no-chain-reply-to \ + --cover-letter --annotate master.. ```
+It is also possible to use git-publish. + Note the master GIT repository is at
* https://libvirt.org/git/?p=libvirt-rust.git;a=summary -- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 17, 2019 at 08:59:30AM +0000, Sahid Orentino Ferdjaoui wrote:
In this refactor we avoid to enclose all the code with unsafe tags and just use it when necessary. Also we add annotations to explain why it's safe to use.
The integrations tests related are also been reviewed to avoid using panic.
Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com> --- src/connect.rs | 91 ++++++++++++++++++++++----------------- tests/integration_qemu.rs | 16 +++---- 2 files changed, 56 insertions(+), 51 deletions(-)
diff --git a/src/connect.rs b/src/connect.rs index 0fa8551..108224d 100644 --- a/src/connect.rs +++ b/src/connect.rs @@ -240,6 +240,41 @@ extern "C" { -> *mut libc::c_char; }
+extern "C" fn connectCallback(ccreds: sys::virConnectCredentialPtr, + ncred: libc::c_uint, + cbdata: *mut libc::c_void) + -> libc::c_int { + let callback: ConnectAuthCallback = unsafe { + // Safe because connectCallback is private and only used by + // Connect::open_auth(). In open_auth() we transmute the + // callback allocate in *void. + mem::transmute(cbdata) + }; + let mut rcreds: Vec<ConnectCredential> = Vec::new(); + for i in 0..ncred as isize { + unsafe { + // Safe because ccreds is allocated. + let c = ConnectCredential::from_ptr(ccreds.offset(i)); + rcreds.push(c); + } + } + callback(&mut rcreds); + for i in 0..ncred as isize { + if rcreds[i as usize].result.is_some() { + if let Some(ref result) = rcreds[i as usize].result { + unsafe { + // Safe because ccreds is allocated and the result + // is comming from Rust calls. + (*ccreds.offset(i)).resultlen = result.len() as libc::c_uint; + (*ccreds.offset(i)).result = string_to_mut_c_chars!(result.clone()); + } + } + } + } + 0 +} + + pub type ConnectFlags = self::libc::c_uint; pub const VIR_CONNECT_RO: ConnectFlags = 1 << 0; pub const VIR_CONNECT_NO_ALIASES: ConnectFlags = 1 << 1; @@ -412,39 +447,6 @@ impl ConnectAuth { callback: callback, } } - - fn to_cstruct(&mut self) -> sys::virConnectAuth { - unsafe extern "C" fn wrapper(ccreds: sys::virConnectCredentialPtr, - ncred: libc::c_uint, - cbdata: *mut libc::c_void) - -> libc::c_int { - let callback: ConnectAuthCallback = mem::transmute(cbdata); - let mut rcreds: Vec<ConnectCredential> = Vec::new(); - for i in 0..ncred as isize { - let c = ConnectCredential::from_ptr(ccreds.offset(i)); - rcreds.push(c); - } - callback(&mut rcreds); - for i in 0..ncred as isize { - if rcreds[i as usize].result.is_some() { - if let Some(ref result) = rcreds[i as usize].result { - (*ccreds.offset(i)).resultlen = result.len() as libc::c_uint; - (*ccreds.offset(i)).result = string_to_mut_c_chars!(result.clone()); - } - } - } - 0 - } - - unsafe { - sys::virConnectAuth { - credtype: &mut self.creds[0], - ncredtype: self.creds.len() as u32, - cb: wrapper, - cbdata: mem::transmute(self.callback), - } - } - } }
/// Provides APIs for the management of hosts. @@ -554,14 +556,23 @@ impl Connect { auth: &mut ConnectAuth, flags: ConnectFlags) -> Result<Connect, Error> { - unsafe { - let mut cauth = auth.to_cstruct(); - let c = virConnectOpenAuth(string_to_c_chars!(uri), &mut cauth, flags as libc::c_uint); - if c.is_null() { - return Err(Error::new()); - } - return Ok(Connect::new(c)); + let mut cauth = unsafe { + // Safe because Rust forces to allocate all attributes of + // the struct ConnectAuth. + sys::virConnectAuth { + credtype: &mut auth.creds[0], + ncredtype: auth.creds.len() as libc::c_uint, + cb: connectCallback, + cbdata: mem::transmute(auth.callback), + } + }; + let c = unsafe { + virConnectOpenAuth(string_to_c_chars!(uri), &mut cauth, flags as libc::c_uint) + }; + if c.is_null() { + return Err(Error::new()); } + return Ok(Connect::new(c)); }
diff --git a/tests/integration_qemu.rs b/tests/integration_qemu.rs index 49e07c4..79aa2bd 100644 --- a/tests/integration_qemu.rs +++ b/tests/integration_qemu.rs @@ -108,14 +108,9 @@ fn test_connection_with_auth() { let mut auth = ConnectAuth::new(vec![::virt::connect::VIR_CRED_AUTHNAME, ::virt::connect::VIR_CRED_PASSPHRASE], callback); - match Connect::open_auth("test+tcp://127.0.0.1/default", &mut auth, 0) { - Ok(c) => common::close(c), - Err(e) => { - panic!("open_auth did not work: code {}, message: {}", - e.code, - e.message) - } - } + let c = Connect::open_auth("test+tcp://127.0.0.1/default", &mut auth, 0); + assert_eq!(true, c.is_ok());
Wouldn't it make more sense to do: assert!(c.is_ok());
+ common::close(c.unwrap()); }
@@ -141,9 +136,8 @@ fn test_connection_with_auth_wrong() { let mut auth = ConnectAuth::new(vec![::virt::connect::VIR_CRED_AUTHNAME, ::virt::connect::VIR_CRED_PASSPHRASE], callback); - if Connect::open_auth("test+tcp://127.0.0.1/default", &mut auth, 0).is_ok() { - panic!("open_auth did not work: code {}, message:"); - } + let c = Connect::open_auth("test+tcp://127.0.0.1/default", &mut auth, 0); + assert_eq!(false, c.is_ok());
Same here with: assert!(c.is_err()); ???
}
#[test] -- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Martin Kletzander -
Sahid Orentino Ferdjaoui