[libvirt] [PATCH v2 11/20] network: Introduce virNetworkObj{Get|Set}Autostart

[Michal Privoznik]

On 08/15/2017 10:42 PM, John Ferlan wrote:
> 
> 
> On 08/15/2017 11:32 AM, Michal Privoznik wrote:
>> On 07/26/2017 05:05 PM, John Ferlan wrote:
>>> In preparation for privatizing the virNetworkObj structure, create
>>> accessors for the obj->autostart.
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/conf/virnetworkobj.c    | 15 +++++++++++++++
>>>  src/conf/virnetworkobj.h    |  9 ++++++++-
>>>  src/libvirt_private.syms    |  2 ++
>>>  src/network/bridge_driver.c | 20 ++++++++++----------
>>>  src/test/test_driver.c      |  5 +++--
>>>  5 files changed, 38 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
>>> index 631f8cd..36d4bff 100644
>>> --- a/src/conf/virnetworkobj.c
>>> +++ b/src/conf/virnetworkobj.c
>>> @@ -129,6 +129,21 @@ virNetworkObjGetNewDef(virNetworkObjPtr obj)
>>>  }
>>>  
>>>  
>>> +int
>>> +virNetworkObjGetAutostart(virNetworkObjPtr obj)
>>> +{
>>> +    return obj->autostart;
>>> +}
>>> +
>>> +
>>> +void
>>> +virNetworkObjSetAutostart(virNetworkObjPtr obj,
>>> +                          int autostart)
>>> +{
>>> +    obj->autostart = autostart;
>>> +}
>>> +
>>> +
>>>  pid_t
>>>  virNetworkObjGetDnsmasqPid(virNetworkObjPtr obj)
>>>  {
>>> diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h
>>> index 90ce0ca..a526d30 100644
>>> --- a/src/conf/virnetworkobj.h
>>> +++ b/src/conf/virnetworkobj.h
>>> @@ -32,7 +32,7 @@ struct _virNetworkObj {
>>>      pid_t dnsmasqPid;
>>>      pid_t radvdPid;
>>>      unsigned int active : 1;
>>> -    unsigned int autostart : 1;
>>> +    int autostart;
>>
>> Since we are touching this does it make sense to convert this to bool?
>> Interestingly, you're doing that conversion for @active in the next patch.
>>
> 
> I think because it was an external API provided value I left it at
> 'int'. For the other two active and persistent - those are boolean
> states as a result of other internal operations and not outwardly
> facing.  IOW: There's no virNetworkSetPersistent or virNetworkSetActive
> type API's.
> 
> I think also the GetAutostart algorithm which assigns *autostart =
> obj->autostart caused me to pause especially since the code is
> "repeated" in domain, storage, and test. Initially I think I was
> thinking of combining all those places that make the same sequence of
> calls, but that got shot down. I think changing it to a bool would
> probably be a "next step" exercise, but "technically", the Get algorithm
> would be *autostart = obj->autostart ? 1 : 0; as opposed to the current
> *autostart = obj->autostart;

I don't think that different get is a problem. But okay, I don't care
that much to stop this. If you want to change it to bool do, if not I
can live with that too.

ACK if you fix the mem leak issue.

Michal