Re: [PATCH 1/7] parallels: introduce and use string constants for network types and names

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

 



On 17.03.2015 15:25, Maxim Nestratov wrote:
> 17.03.2015 17:15, Michal Privoznik пишет:
>> On 13.03.2015 16:52, Maxim Nestratov wrote:
>>> Signed-off-by: Maxim Nestratov <mnestratov@xxxxxxxxxxxxx>
>>> ---
>>>   src/parallels/parallels_network.c |    6 +++---
>>>   src/parallels/parallels_sdk.c     |    6 +++---
>>>   src/parallels/parallels_utils.h   |    8 +++++++-
>>>   3 files changed, 13 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/parallels/parallels_network.c
>>> b/src/parallels/parallels_network.c
>>> index 1d3b694..bb7ec5e 100644
>>> --- a/src/parallels/parallels_network.c
>>> +++ b/src/parallels/parallels_network.c
>>> @@ -211,12 +211,12 @@ parallelsLoadNetwork(parallelsConnPtr privconn,
>>> virJSONValuePtr jobj)
>>>           goto cleanup;
>>>       }
>>>   -    if (STREQ(tmp, "bridged")) {
>>> +    if (STREQ(tmp, PARALLELS_BRIDGED_NETWORK_TYPE)) {
>>>           def->forward.type = VIR_NETWORK_FORWARD_BRIDGE;
>>>             if (parallelsGetBridgedNetInfo(def, jobj) < 0)
>>>               goto cleanup;
>>> -    } else if (STREQ(tmp, "host-only")) {
>>> +    } else if (STREQ(tmp, PARALLELS_HOSTONLY_NETWORK_TYPE)) {
>>>           def->forward.type = VIR_NETWORK_FORWARD_NONE;
>>>   
>> How about introducing 'int parallelsNetworkTypeFromString(const char *)'
>> using our ENUM_DECL and ENUM_IMPL macros? This code could then be turned
>> into:
>>
>>      if ((def->forward.type = parallelNetworkTypeFromString(tmp)) < 0) {
>>          parallelsParseError();
>>          goto cleanup;
>>      }
>>
>>      switch((virNetworkForwardType) def->forward.type) {
>>      case VIR_NETWORK_FORWARD_BRIDGE:
>>          if (parallelsGetBridgedNetInfo(def, jobj) < 0)
>>              goto cleanup;
>>          break;
>>      case VIR_NETWORK_FORWARD_NONE:
>>          if (parallelsGetHostOnlyNetInfo(def, def->name) < 0)
>>              goto cleanup;
>>          ...
>>      }
>>
>> I find it more future proof then STREQ() spaghetti.
>>
>> Michal
> Ok. Makes sence. I'll do this in the next series version.
> 

D'oh. Now that I'm going through the code again, this patch actually
makes sense and my suggestion doesn't. The problem is, initially I
thought that @tmp holds network type in string, while in fact it holds a
network's name. And it doesn't make much sense to create an enum for
that. Although, other patches in the series need some discussion before
I can push them.

Michal

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