Re: [PATCH v3 08/11] admin: Add support for URI aliases

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

 



>> -    if (!(conn->uri = virURIParse(uri ? uri : default_uri)))
>> +    if ((!(flags & VIR_CONNECT_NO_ALIASES) &&
>> +         virURIResolveAlias(conf, uri ? uri : default_uri, &alias) < 0))
> 
> this should also be fixed (with what I mentioned in previous review).
> 

Fixed.

>> +        goto error;
>> +
>> +    if (!(conn->uri = virURIParse(alias ? alias : (uri ? uri :
>> default_uri))))
> 
> Also, if you modify virURIResolveAlias() to simply copy the string
> over to @alias if the alias is not found, that would be pretty nice
> syntax-sugar and spare our souls from this ugly thing =)
> 
> ACK with that fixed.

Hmm, I think that in ideal world 1 API should do exactly 1 thing and 1
thing only, but we're not living in an ideal world, do we?! However, in
this specific case I don't think it's worth changing alias resolving in
a way that would copy the searched alias into URI string if the alias
wasn't matched. I could do a wrapper encompassing the alias resolving
and this proposed syntactic sugar, but it still wouldn't make it worth I
guess, since the resolver isn't executed every time unless appropriate
flags have been set. Moreover, since I fixed the ternary operator you
suggested above, the other one also becomes more readable:
virURIParse(alias ? alias : uri)....what do you think?

PS: thanks for the ACK though

Erik

--
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]