3:42 p.m.
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>
> ---
> 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