Re: [PATCH v3 11/18] conf: Rework storage_conf to use adapter specific typedefs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/10/2017 04:10 PM, John Ferlan wrote:
> Rework the helpers/APIs to use the FCHost and SCSIHost adapter types.
> Continue to realign the code for shorter lines.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
>  src/conf/storage_conf.c | 112 ++++++++++++++++++++++++++----------------------
>  1 file changed, 61 insertions(+), 51 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 8709101..45dc860 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2085,16 +2085,16 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools,
>  
>  
>  static int
> -getSCSIHostNumber(virStoragePoolSourceAdapter adapter,

Eww! This function passed a full copy of an object on the stack rather than a pointer to the object??

> +getSCSIHostNumber(virStorageAdapterSCSIHostPtr scsi_host,
>                    unsigned int *hostnum)
>  {
>      int ret = -1;
>      unsigned int num;
>      char *name = NULL;
>  
> -    if (adapter.data.scsi_host.has_parent) {
> -        virPCIDeviceAddress addr = adapter.data.scsi_host.parentaddr;
> -        unsigned int unique_id = adapter.data.scsi_host.unique_id;
> +    if (scsi_host->has_parent) {
> +        virPCIDeviceAddress addr = scsi_host->parentaddr;

And again it's copying an object rather than referencing it. Why? As long as you're eliminating the unnecessary copy of the larger object when calling this function, you may as well make addr a virPCIDeviceAddressPtr too (although I'm sure someone will insist that you do it in a separate patch :-P)


> +        unsigned int unique_id = scsi_host->unique_id;
>  
>          if (!(name = virSCSIHostGetNameByParentaddr(addr.domain,
>                                                      addr.bus,
> @@ -2105,7 +2105,7 @@ getSCSIHostNumber(virStoragePoolSourceAdapter adapter,
>          if (virSCSIHostGetNumber(name, &num) < 0)
>              goto cleanup;
>      } else {
> -        if (virSCSIHostGetNumber(adapter.data.scsi_host.name, &num) < 0)
> +        if (virSCSIHostGetNumber(scsi_host->name, &num) < 0)
>              goto cleanup;
>      }
>  
> @@ -2136,7 +2136,7 @@ virStorageIsSameHostnum(const char *name,
>   * matchFCHostToSCSIHost:
>   *
>   * @conn: Connection pointer
> - * @fc_adapter: fc_host adapter (either def or pool->def)
> + * @fchost: fc_host adapter ptr (either def or pool->def)
>   * @scsi_hostnum: Already determined "scsi_pool" hostnum
>   *
>   * Returns true/false whether there is a match between the incoming
> @@ -2144,7 +2144,7 @@ virStorageIsSameHostnum(const char *name,
>   */
>  static bool
>  matchFCHostToSCSIHost(virConnectPtr conn,
> -                      virStoragePoolSourceAdapter fc_adapter,
> +                      virStorageAdapterFCHostPtr fchost,

Thank God! Where did all this pass-by-value come from anyway???


Meanwhile - ACK.


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux