On Thu, Aug 29, 2019 at 02:05:12AM -0700, Sage Imel wrote:
> From: Sage Imel <sage@xxxxxxxxxxxx>
>
> Always returns the full list of volumes,
> can't just ask it how many volumes are in the pool
>
> Signed-off-by: Sage Imel <sage@xxxxxxxxxxx>
> ---
That looks to be a nice add, thank you for this contribution, can you
just consider the comments that i have added?
> src/storage_pool.rs | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/src/storage_pool.rs b/src/storage_pool.rs
> index 38676c2..e8ed21c 100644
> --- a/src/storage_pool.rs
> +++ b/src/storage_pool.rs
> @@ -18,7 +18,7 @@
>
> extern crate libc;
>
> -use std::str;
> +use std::{str, ptr};
>
> use connect::sys::virConnectPtr;
> use storage_vol::sys::virStorageVolPtr;
> @@ -57,6 +57,10 @@ extern "C" {
> xml: *const libc::c_char,
> flags: libc::c_uint)
> -> sys::virStoragePoolPtr;
> + fn virStoragePoolListAllVolumes(ptr: sys::virStoragePoolPtr,
> + vols: *mut *mut virStorageVolPtr,
> + flags:libc::c_uint)
> + -> libc::c_int;
> fn virStoragePoolLookupByID(c: virConnectPtr, id: libc::c_int) -> sys::virStoragePoolPtr;
> fn virStoragePoolLookupByName(c: virConnectPtr,
> id: *const libc::c_char)
> @@ -103,6 +107,8 @@ pub const STORAGE_POOL_CREATE_WITH_BUILD: StoragePoolCreateFlags = 1 << 0;
> pub const STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE: StoragePoolCreateFlags = 1 << 1;
> pub const STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE: StoragePoolCreateFlags = 1 << 2;
>
> +pub type VirStoragePoolListAllVolumesFlags = self::libc::c_uint;
> +
You well added the new type but as you can see in the other types we
prefer to remove the "vir" prefix. So that should be:
pub type StoragePoolListAllVolumesFlags = self::libc::c_uint;
Also that, you have to declare the related flags, see:
include/libvirt-storage.h
> pub type StoragePoolState = self::libc::c_uint;
> pub const VIR_STORAGE_POOL_INACTIVE: StoragePoolState = 0;
> pub const VIR_STORAGE_POOL_BUILDING: StoragePoolState = 1;
> @@ -201,6 +207,27 @@ impl StoragePool {
> }
> }
>
> + pub fn list_all_volumes(&self,
> + flags: VirStoragePoolListAllVolumesFlags)
> + -> Result<Vec<StorageVol>, Error> {
Please consider to well align the code.
> + unsafe {
I know that you will find it everywhere but now we prefer to avoid
using a global "unsafe". It was not right. You should just use it when
it's necessary and also you should add a comment to explain why it is
safe. You can find an example in connect::open_auth().
> + let mut volumes: *mut virStorageVolPtr = ptr::null_mut();
> + let size =
> + virStoragePoolListAllVolumes(self.as_ptr(), &mut volumes, flags as libc::c_uint);
> + if size == -1 {
> + return Err(Error::new());
> + }
> +
> + let mut array: Vec<StorageVol> = Vec::new();
> + for x in 0..size as isize {
> + array.push(StorageVol::new(*volumes.offset(x)));
> + }
> + libc::free(volumes as *mut libc::c_void);
> +
> + return Ok(array);
> + }
> + }
> +
> pub fn lookup_by_id(conn: &Connect, id: u32) -> Result<StoragePool, Error> {
> unsafe {
> let ptr = virStoragePoolLookupByID(conn.as_ptr(), id as libc::c_int);
It would be great if you can provide a test case or integration test
with it.
Thanks,
s.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
