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

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(a)redhat.com&gt; >> --- >> 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;