[libvirt] [PATCH Rust 1/4] connect: cleanup around Connect::open_auth()'s method
Martin Kletzander
mkletzan at redhat.com
Wed Jul 24 10:39:58 UTC 2019
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 at 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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190724/c05507b7/attachment-0001.sig>
More information about the libvir-list
mailing list