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@xxxxxxxxxx>
>> ---
>> 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;
>> unsigned int persistent : 1;
>>
>> virNetworkDefPtr def; /* The current definition */
>> @@ -60,6 +60,13 @@ virNetworkObjSetDef(virNetworkObjPtr obj,
>> virNetworkDefPtr
>> virNetworkObjGetNewDef(virNetworkObjPtr obj);
>>
>> +int
>> +virNetworkObjGetAutostart(virNetworkObjPtr obj);
>> +
>> +void
>> +virNetworkObjSetAutostart(virNetworkObjPtr obj,
>> + int autostart);
>> +
>> virMacMapPtr
>> virNetworkObjGetMacMap(virNetworkObjPtr obj);
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 84e3eb7..8a3284f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -944,6 +944,7 @@ virNetworkObjFindByName;
>> virNetworkObjFindByNameLocked;
>> virNetworkObjFindByUUID;
>> virNetworkObjFindByUUIDLocked;
>> +virNetworkObjGetAutostart;
>> virNetworkObjGetClassIdMap;
>> virNetworkObjGetDef;
>> virNetworkObjGetDnsmasqPid;
>> @@ -966,6 +967,7 @@ virNetworkObjNew;
>> virNetworkObjRemoveInactive;
>> virNetworkObjReplacePersistentDef;
>> virNetworkObjSaveStatus;
>> +virNetworkObjSetAutostart;
>> virNetworkObjSetDef;
>> virNetworkObjSetDefTransient;
>> virNetworkObjSetDnsmasqPid;
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index b4fbfc5..eb814d3 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -525,7 +525,7 @@ networkAutostartConfig(virNetworkObjPtr obj,
>> int ret = -1;
>>
>> virObjectLock(obj);
>> - if (obj->autostart &&
>> + if (virNetworkObjGetAutostart(obj) &&
>> !virNetworkObjIsActive(obj) &&
>> networkStartNetwork(driver, obj) < 0)
>> goto cleanup;
>> @@ -3963,7 +3963,7 @@ networkGetAutostart(virNetworkPtr net,
>> if (virNetworkGetAutostartEnsureACL(net->conn, virNetworkObjGetDef(obj)) < 0)
>> goto cleanup;
>>
>> - *autostart = obj->autostart;
>> + *autostart = virNetworkObjGetAutostart(obj);
>> ret = 0;
>>
>> cleanup:
>> @@ -3974,12 +3974,13 @@ networkGetAutostart(virNetworkPtr net,
>>
>> static int
>> networkSetAutostart(virNetworkPtr net,
>> - int autostart)
>> + int new_autostart)
>> {
>> virNetworkDriverStatePtr driver = networkGetDriver();
>> virNetworkObjPtr obj;
>> virNetworkDefPtr def;
>> char *configFile = NULL, *autostartLink = NULL;
>> + int cur_autostart;
>> int ret = -1;
>>
>> if (!(obj = networkObjFromNetwork(net)))
>> @@ -3995,9 +3996,9 @@ networkSetAutostart(virNetworkPtr net,
>> goto cleanup;
>> }
>>
>> - autostart = (autostart != 0);
>> -
>> - if (obj->autostart != autostart) {
>> + new_autostart = (new_autostart != 0);
>> + cur_autostart = virNetworkObjGetAutostart(obj);
>> + if (cur_autostart != new_autostart) {
>> if ((configFile = virNetworkConfigFile(driver->networkConfigDir,
>> def->name)) == NULL)
>> goto cleanup;
>> @@ -4005,7 +4006,7 @@ networkSetAutostart(virNetworkPtr net,
>> def->name)) == NULL)
>> goto cleanup;
>>
>> - if (autostart) {
>> + if (new_autostart) {
>> if (virFileMakePath(driver->networkAutostartDir) < 0) {
>> virReportSystemError(errno,
>> _("cannot create autostart directory '%s'"),
>> @@ -4028,13 +4029,12 @@ networkSetAutostart(virNetworkPtr net,
>> }
>> }
>>
>> - obj->autostart = autostart;
>> + virNetworkObjSetAutostart(obj, new_autostart);
>> }
>> +
>> ret = 0;
>>
>> cleanup:
>> - VIR_FREE(configFile);
>> - VIR_FREE(autostartLink);
>
> This doesn't feel right. @configFile and @autostartLink are leaked.
>
oh right - I dragged back the bulk of the algorithm, but not the
VIR_FREE's... I'll restore them. Thanks -
John
>> virNetworkObjEndAPI(&obj);
>> return ret;
>> }
>
> Michal
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
