8:27 a.m.
A couple I forgot from earlier series, but a few things found since then
along with some clarifications to make the ObjAdd processing a bit more
like I've found for nwfilter, nodedev, and interface as well as making
the ObjListRemove processing a bit clearer and more consistent.
Along with the other series and the virObject v2 adjustments - this brings
everything close enough to start sending the patches that will tie all this
stuff together to create/use a common object model.
John Ferlan (8):
secret: Whitespace modification for secret_driver
secret: Alter FindByUUID to expect the formatted uuidstr
secret: Rename variable in virSecretObjListAdd
secret: Clean up virSecretObjListAdd processing
secret: Remove need for local configFile and base64File in ObjectAdd
secret: Have virSecretObjNew consume newdef
secret: Properly handle @def after virSecretObjAdd in driver
secret: Handle object list removal and deletion properly
src/conf/virsecretobj.c | 125 +++++++++++++++++++++------------------------
src/conf/virsecretobj.h | 2 +-
src/secret/secret_driver.c | 50 ++++++++++++------
3 files changed, 94 insertions(+), 83 deletions(-)
--
2.9.4
8:27 a.m.
New subject: [libvirt] [PATCH 1/8] secret: Whitespace modification for secret_driver
Ensure two empty lines between functions.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/secret/secret_driver.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index fc01e6d..a096fdb 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -72,6 +72,7 @@ secretDriverLock(void)
virMutexLock(&driver->lock);
}
+
static void
secretDriverUnlock(void)
{
@@ -79,7 +80,6 @@ secretDriverUnlock(void)
}
-
static virSecretObjPtr
secretObjFromSecret(virSecretPtr secret)
{
@@ -109,6 +109,7 @@ secretConnectNumOfSecrets(virConnectPtr conn)
conn);
}
+
static int
secretConnectListSecrets(virConnectPtr conn,
char **uuids,
@@ -279,6 +280,7 @@ secretDefineXML(virConnectPtr conn,
return ret;
}
+
static char *
secretGetXMLDesc(virSecretPtr secret,
unsigned int flags)
@@ -304,6 +306,7 @@ secretGetXMLDesc(virSecretPtr secret,
return ret;
}
+
static int
secretSetValue(virSecretPtr secret,
const unsigned char *value,
@@ -340,6 +343,7 @@ secretSetValue(virSecretPtr secret,
return ret;
}
+
static unsigned char *
secretGetValue(virSecretPtr secret,
size_t *value_size,
@@ -377,6 +381,7 @@ secretGetValue(virSecretPtr secret,
return ret;
}
+
static int
secretUndefine(virSecretPtr secret)
{
@@ -415,6 +420,7 @@ secretUndefine(virSecretPtr secret)
return ret;
}
+
static int
secretStateCleanup(void)
{
@@ -435,6 +441,7 @@ secretStateCleanup(void)
return 0;
}
+
static int
secretStateInitialize(bool privileged,
virStateInhibitCallback callback ATTRIBUTE_UNUSED,
@@ -486,6 +493,7 @@ secretStateInitialize(bool privileged,
return -1;
}
+
static int
secretStateReload(void)
{
@@ -500,6 +508,7 @@ secretStateReload(void)
return 0;
}
+
static int
secretConnectSecretEventRegisterAny(virConnectPtr conn,
virSecretPtr secret,
@@ -521,6 +530,7 @@ secretConnectSecretEventRegisterAny(virConnectPtr conn,
return callbackID;
}
+
static int
secretConnectSecretEventDeregisterAny(virConnectPtr conn,
int callbackID)
@@ -565,6 +575,7 @@ static virStateDriver stateDriver = {
.stateReload = secretStateReload,
};
+
int
secretRegister(void)
{
--
2.9.4
10:52 a.m.
New subject: [libvirt] [PATCH 1/8] secret: Whitespace modification for secret_driver
On 06/03/2017 03:27 PM, John Ferlan wrote:
Ensure two empty lines between functions.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/secret/secret_driver.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
ACK
Michal
8:27 a.m.
New subject: [libvirt] [PATCH 2/8] secret: Alter FindByUUID to expect the formatted uuidstr
Since we're storing a virUUIDFormat'd string in our Hash Table, let's
modify the Lookup API to receive a formatted string as well.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 18 +++++++-----------
src/conf/virsecretobj.h | 2 +-
src/secret/secret_driver.c | 10 +++++-----
3 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 0311335..62b86b7 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -171,12 +171,8 @@ virSecretObjListDispose(void *obj)
*/
static virSecretObjPtr
virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets,
- const unsigned char *uuid)
+ const char *uuidstr)
{
- char uuidstr[VIR_UUID_STRING_BUFLEN];
-
- virUUIDFormat(uuid, uuidstr);
-
return virObjectRef(virHashLookup(secrets->objs, uuidstr));
}
@@ -184,7 +180,7 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets,
/**
* virSecretObjFindByUUID:
* @secrets: list of secret objects
- * @uuid: secret uuid to find
+ * @uuidstr: secret uuid to find
*
* This function locks @secrets and finds the secret object which
* corresponds to @uuid.
@@ -193,12 +189,12 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets,
*/
virSecretObjPtr
virSecretObjListFindByUUID(virSecretObjListPtr secrets,
- const unsigned char *uuid)
+ const char *uuidstr)
{
virSecretObjPtr obj;
virObjectLock(secrets);
- obj = virSecretObjListFindByUUIDLocked(secrets, uuid);
+ obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr);
virObjectUnlock(secrets);
if (obj)
virObjectLock(obj);
@@ -346,13 +342,14 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
if (oldDef)
*oldDef = NULL;
+ virUUIDFormat(newdef->uuid, uuidstr);
+
/* Is there a secret already matching this UUID */
- if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) {
+ if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) {
virObjectLock(obj);
def = obj->def;
if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) {
- virUUIDFormat(def->uuid, uuidstr);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("a secret with UUID %s is already defined for "
"use with %s"),
@@ -390,7 +387,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
/* Generate the possible configFile and base64File strings
* using the configDir, uuidstr, and appropriate suffix
*/
- virUUIDFormat(newdef->uuid, uuidstr);
if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
!(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
goto cleanup;
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
index a355da7..0196813 100644
--- a/src/conf/virsecretobj.h
+++ b/src/conf/virsecretobj.h
@@ -40,7 +40,7 @@ virSecretObjListNew(void);
virSecretObjPtr
virSecretObjListFindByUUID(virSecretObjListPtr secrets,
- const unsigned char *uuid);
+ const char *uuidstr);
virSecretObjPtr
virSecretObjListFindByUsage(virSecretObjListPtr secrets,
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index a096fdb..8ddae57 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -86,8 +86,8 @@ secretObjFromSecret(virSecretPtr secret)
virSecretObjPtr obj;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- if (!(obj = virSecretObjListFindByUUID(driver->secrets, secret->uuid))) {
- virUUIDFormat(secret->uuid, uuidstr);
+ virUUIDFormat(secret->uuid, uuidstr);
+ if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) {
virReportError(VIR_ERR_NO_SECRET,
_("no secret with matching uuid '%s'"),
uuidstr);
return NULL;
@@ -148,10 +148,10 @@ secretLookupByUUID(virConnectPtr conn,
virSecretPtr ret = NULL;
virSecretObjPtr obj;
virSecretDefPtr def;
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
- if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuid))) {
- char uuidstr[VIR_UUID_STRING_BUFLEN];
- virUUIDFormat(uuid, uuidstr);
+ virUUIDFormat(uuid, uuidstr);
+ if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) {
virReportError(VIR_ERR_NO_SECRET,
_("no secret with matching uuid '%s'"),
uuidstr);
goto cleanup;
--
2.9.4
10:52 a.m.
New subject: [libvirt] [PATCH 2/8] secret: Alter FindByUUID to expect the formatted uuidstr
On 06/03/2017 03:27 PM, John Ferlan wrote:
Since we're storing a virUUIDFormat'd string in our Hash
Table, let's
modify the Lookup API to receive a formatted string as well.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 18 +++++++-----------
src/conf/virsecretobj.h | 2 +-
src/secret/secret_driver.c | 10 +++++-----
3 files changed, 13 insertions(+), 17 deletions(-)
ACK
Michal
8:27 a.m.
New subject: [libvirt] [PATCH 3/8] secret: Rename variable in virSecretObjListAdd
Rename @def to @objdef - it'll make future patches easier.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 62b86b7..e3bcbe5 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -332,7 +332,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
virSecretDefPtr *oldDef)
{
virSecretObjPtr obj;
- virSecretDefPtr def;
+ virSecretDefPtr objdef;
virSecretObjPtr ret = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
char *configFile = NULL, *base64File = NULL;
@@ -347,26 +347,26 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
/* Is there a secret already matching this UUID */
if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) {
virObjectLock(obj);
- def = obj->def;
+ objdef = obj->def;
- if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) {
+ if (STRNEQ_NULLABLE(objdef->usage_id, newdef->usage_id)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("a secret with UUID %s is already defined for "
"use with %s"),
- uuidstr, def->usage_id);
+ uuidstr, objdef->usage_id);
goto cleanup;
}
- if (def->isprivate && !newdef->isprivate) {
+ if (objdef->isprivate && !newdef->isprivate) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot change private flag on existing
secret"));
goto cleanup;
}
if (oldDef)
- *oldDef = def;
+ *oldDef = objdef;
else
- virSecretDefFree(def);
+ virSecretDefFree(objdef);
obj->def = newdef;
} else {
/* No existing secret with same UUID,
@@ -375,8 +375,8 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
newdef->usage_type,
newdef->usage_id))) {
virObjectLock(obj);
- def = obj->def;
- virUUIDFormat(def->uuid, uuidstr);
+ objdef = obj->def;
+ virUUIDFormat(objdef->uuid, uuidstr);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("a secret with UUID %s already defined for "
"use with %s"),
--
2.9.4
10:52 a.m.
New subject: [libvirt] [PATCH 3/8] secret: Rename variable in virSecretObjListAdd
On 06/03/2017 03:27 PM, John Ferlan wrote:
Rename @def to @objdef - it'll make future patches easier.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
ACK
Michal
8:27 a.m.
New subject: [libvirt] [PATCH 4/8] secret: Clean up virSecretObjListAdd processing
Make use of an error: label to handle the failure and need to call
virSecretObjEndAPI for the object to set it to NULL for return.
Also rather than an if/else processing - have the initial "if" which
is just replacing the @newdef into obj->def goto cleanup, thus allowing
the remaining code to be extracted from the else and appear to more inline.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 74 ++++++++++++++++++++++++-------------------------
1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index e3bcbe5..1bafd0b 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
{
virSecretObjPtr obj;
virSecretDefPtr objdef;
- virSecretObjPtr ret = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
char *configFile = NULL, *base64File = NULL;
@@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
_("a secret with UUID %s is already defined for "
"use with %s"),
uuidstr, objdef->usage_id);
- goto cleanup;
+ goto error;
}
if (objdef->isprivate && !newdef->isprivate) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot change private flag on existing
secret"));
- goto cleanup;
+ goto error;
}
if (oldDef)
@@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
else
virSecretDefFree(objdef);
obj->def = newdef;
- } else {
- /* No existing secret with same UUID,
- * try look for matching usage instead */
- if ((obj = virSecretObjListFindByUsageLocked(secrets,
- newdef->usage_type,
- newdef->usage_id))) {
- virObjectLock(obj);
- objdef = obj->def;
- virUUIDFormat(objdef->uuid, uuidstr);
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("a secret with UUID %s already defined for "
- "use with %s"),
- uuidstr, newdef->usage_id);
- goto cleanup;
- }
+ goto cleanup;
+ }
- /* Generate the possible configFile and base64File strings
- * using the configDir, uuidstr, and appropriate suffix
- */
- if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
- !(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
- goto cleanup;
+ /* No existing secret with same UUID,
+ * try to look for matching usage instead */
+ if ((obj = virSecretObjListFindByUsageLocked(secrets,
+ newdef->usage_type,
+ newdef->usage_id))) {
+ virObjectLock(obj);
+ objdef = obj->def;
+ virUUIDFormat(objdef->uuid, uuidstr);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("a secret with UUID %s already defined for "
+ "use with %s"),
+ uuidstr, newdef->usage_id);
+ goto error;
+ }
- if (!(obj = virSecretObjNew()))
- goto cleanup;
+ /* Generate the possible configFile and base64File strings
+ * using the configDir, uuidstr, and appropriate suffix
+ */
+ if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
+ !(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
+ goto cleanup;
- if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
- goto cleanup;
+ if (!(obj = virSecretObjNew()))
+ goto cleanup;
- obj->def = newdef;
- VIR_STEAL_PTR(obj->configFile, configFile);
- VIR_STEAL_PTR(obj->base64File, base64File);
- virObjectRef(obj);
- }
+ if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
+ goto error;
- ret = obj;
- obj = NULL;
+ obj->def = newdef;
+ VIR_STEAL_PTR(obj->configFile, configFile);
+ VIR_STEAL_PTR(obj->base64File, base64File);
+ virObjectRef(obj);
cleanup:
- virSecretObjEndAPI(&obj);
VIR_FREE(configFile);
VIR_FREE(base64File);
virObjectUnlock(secrets);
- return ret;
+ return obj;
+
+ error:
+ virSecretObjEndAPI(&obj);
+ goto cleanup;
}
--
2.9.4
10:52 a.m.
New subject: [libvirt] [PATCH 4/8] secret: Clean up virSecretObjListAdd processing
On 06/03/2017 03:27 PM, John Ferlan wrote:
Make use of an error: label to handle the failure and need to call
virSecretObjEndAPI for the object to set it to NULL for return.
Also rather than an if/else processing - have the initial "if" which
is just replacing the @newdef into obj->def goto cleanup, thus allowing
the remaining code to be extracted from the else and appear to more inline.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 74 ++++++++++++++++++++++++-------------------------
1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index e3bcbe5..1bafd0b 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
{
virSecretObjPtr obj;
virSecretDefPtr objdef;
- virSecretObjPtr ret = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
char *configFile = NULL, *base64File = NULL;
@@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
_("a secret with UUID %s is already defined for "
"use with %s"),
uuidstr, objdef->usage_id);
- goto cleanup;
+ goto error;
}
if (objdef->isprivate && !newdef->isprivate) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot change private flag on existing
secret"));
- goto cleanup;
+ goto error;
}
if (oldDef)
@@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
else
virSecretDefFree(objdef);
obj->def = newdef;
- } else {
- /* No existing secret with same UUID,
- * try look for matching usage instead */
- if ((obj = virSecretObjListFindByUsageLocked(secrets,
- newdef->usage_type,
- newdef->usage_id))) {
- virObjectLock(obj);
- objdef = obj->def;
- virUUIDFormat(objdef->uuid, uuidstr);
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("a secret with UUID %s already defined for "
- "use with %s"),
- uuidstr, newdef->usage_id);
- goto cleanup;
- }
+ goto cleanup;
+ }
- /* Generate the possible configFile and base64File strings
- * using the configDir, uuidstr, and appropriate suffix
- */
- if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
- !(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
- goto cleanup;
+ /* No existing secret with same UUID,
+ * try to look for matching usage instead */
+ if ((obj = virSecretObjListFindByUsageLocked(secrets,
+ newdef->usage_type,
+ newdef->usage_id))) {
+ virObjectLock(obj);
+ objdef = obj->def;
+ virUUIDFormat(objdef->uuid, uuidstr);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("a secret with UUID %s already defined for "
+ "use with %s"),
+ uuidstr, newdef->usage_id);
+ goto error;
+ }
- if (!(obj = virSecretObjNew()))
- goto cleanup;
+ /* Generate the possible configFile and base64File strings
+ * using the configDir, uuidstr, and appropriate suffix
+ */
+ if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
+ !(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
+ goto cleanup;
- if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
- goto cleanup;
+ if (!(obj = virSecretObjNew()))
+ goto cleanup;
- obj->def = newdef;
- VIR_STEAL_PTR(obj->configFile, configFile);
- VIR_STEAL_PTR(obj->base64File, base64File);
- virObjectRef(obj);
- }
+ if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
+ goto error;
Frankly, I find this very confusing. While reading the commit message, I
understand why you sometimes jump to cleanup and other times to error
label. But if I were to just look at the code alone, it's completely
random to me. I think I'd much rather see the pattern that's here.
However, if you really dislike if-else branching (dislike also as in
prepare for upcoming patches), I suggest changing just that.
Michal
11:36 a.m.
New subject: [libvirt] [PATCH 4/8] secret: Clean up virSecretObjListAdd processing
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
On 06/03/2017 03:27 PM, John Ferlan wrote:
> Make use of an error: label to handle the failure and need to call
> virSecretObjEndAPI for the object to set it to NULL for return.
>
> Also rather than an if/else processing - have the initial "if" which
> is just replacing the @newdef into obj->def goto cleanup, thus allowing
> the remaining code to be extracted from the else and appear to more inline.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/virsecretobj.c | 74 ++++++++++++++++++++++++-------------------------
> 1 file changed, 37 insertions(+), 37 deletions(-)
>
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index e3bcbe5..1bafd0b 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> {
> virSecretObjPtr obj;
> virSecretDefPtr objdef;
> - virSecretObjPtr ret = NULL;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> char *configFile = NULL, *base64File = NULL;
>
> @@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> _("a secret with UUID %s is already defined for
"
> "use with %s"),
> uuidstr, objdef->usage_id);
> - goto cleanup;
> + goto error;
> }
>
> if (objdef->isprivate && !newdef->isprivate) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("cannot change private flag on existing
secret"));
> - goto cleanup;
> + goto error;
> }
>
> if (oldDef)
> @@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> else
> virSecretDefFree(objdef);
> obj->def = newdef;
> - } else {
> - /* No existing secret with same UUID,
> - * try look for matching usage instead */
> - if ((obj = virSecretObjListFindByUsageLocked(secrets,
> - newdef->usage_type,
> - newdef->usage_id))) {
> - virObjectLock(obj);
> - objdef = obj->def;
> - virUUIDFormat(objdef->uuid, uuidstr);
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("a secret with UUID %s already defined for "
> - "use with %s"),
> - uuidstr, newdef->usage_id);
> - goto cleanup;
> - }
> + goto cleanup;
> + }
>
> - /* Generate the possible configFile and base64File strings
> - * using the configDir, uuidstr, and appropriate suffix
> - */
> - if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml"))
||
> - !(base64File = virFileBuildPath(configDir, uuidstr,
".base64")))
> - goto cleanup;
> + /* No existing secret with same UUID,
> + * try to look for matching usage instead */
> + if ((obj = virSecretObjListFindByUsageLocked(secrets,
> + newdef->usage_type,
> + newdef->usage_id))) {
> + virObjectLock(obj);
> + objdef = obj->def;
> + virUUIDFormat(objdef->uuid, uuidstr);
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("a secret with UUID %s already defined for "
> + "use with %s"),
> + uuidstr, newdef->usage_id);
> + goto error;
> + }
>
> - if (!(obj = virSecretObjNew()))
> - goto cleanup;
> + /* Generate the possible configFile and base64File strings
> + * using the configDir, uuidstr, and appropriate suffix
> + */
> + if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
> + !(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
> + goto cleanup;
>
> - if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
> - goto cleanup;
> + if (!(obj = virSecretObjNew()))
> + goto cleanup;
>
> - obj->def = newdef;
> - VIR_STEAL_PTR(obj->configFile, configFile);
> - VIR_STEAL_PTR(obj->base64File, base64File);
> - virObjectRef(obj);
> - }
> + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
> + goto error;
Frankly, I find this very confusing. While reading the commit message, I
understand why you sometimes jump to cleanup and other times to error
label. But if I were to just look at the code alone, it's completely
random to me. I think I'd much rather see the pattern that's here.
However, if you really dislike if-else branching (dislike also as in
prepare for upcoming patches), I suggest changing just that.
Michal
I'll return the if - else - logic... and update the commit message.
Would you like to see a version of that? It does affect the next couple
of patches. For patch 5 it's just a simple indention adjustment. Since
there's questions in 6 I'll address those there.
John
3:15 a.m.
New subject: [libvirt] [PATCH 4/8] secret: Clean up virSecretObjListAdd processing
On 07/13/2017 06:36 PM, John Ferlan wrote:
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Make use of an error: label to handle the failure and need to call
>> virSecretObjEndAPI for the object to set it to NULL for return.
>>
>> Also rather than an if/else processing - have the initial "if" which
>> is just replacing the @newdef into obj->def goto cleanup, thus allowing
>> the remaining code to be extracted from the else and appear to more inline.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/virsecretobj.c | 74 ++++++++++++++++++++++++-------------------------
>> 1 file changed, 37 insertions(+), 37 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index e3bcbe5..1bafd0b 100644
>> --- a/src/conf/virsecretobj.c
>> +++ b/src/conf/virsecretobj.c
>> @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> {
>> virSecretObjPtr obj;
>> virSecretDefPtr objdef;
>> - virSecretObjPtr ret = NULL;
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> char *configFile = NULL, *base64File = NULL;
>>
>> @@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> _("a secret with UUID %s is already defined for
"
>> "use with %s"),
>> uuidstr, objdef->usage_id);
>> - goto cleanup;
>> + goto error;
>> }
>>
>> if (objdef->isprivate && !newdef->isprivate) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("cannot change private flag on existing
secret"));
>> - goto cleanup;
>> + goto error;
>> }
>>
>> if (oldDef)
>> @@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> else
>> virSecretDefFree(objdef);
>> obj->def = newdef;
>> - } else {
>> - /* No existing secret with same UUID,
>> - * try look for matching usage instead */
>> - if ((obj = virSecretObjListFindByUsageLocked(secrets,
>> - newdef->usage_type,
>> - newdef->usage_id))) {
>> - virObjectLock(obj);
>> - objdef = obj->def;
>> - virUUIDFormat(objdef->uuid, uuidstr);
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("a secret with UUID %s already defined for
"
>> - "use with %s"),
>> - uuidstr, newdef->usage_id);
>> - goto cleanup;
>> - }
>> + goto cleanup;
>> + }
>>
>> - /* Generate the possible configFile and base64File strings
>> - * using the configDir, uuidstr, and appropriate suffix
>> - */
>> - if (!(configFile = virFileBuildPath(configDir, uuidstr,
".xml")) ||
>> - !(base64File = virFileBuildPath(configDir, uuidstr,
".base64")))
>> - goto cleanup;
>> + /* No existing secret with same UUID,
>> + * try to look for matching usage instead */
>> + if ((obj = virSecretObjListFindByUsageLocked(secrets,
>> + newdef->usage_type,
>> + newdef->usage_id))) {
>> + virObjectLock(obj);
>> + objdef = obj->def;
>> + virUUIDFormat(objdef->uuid, uuidstr);
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("a secret with UUID %s already defined for "
>> + "use with %s"),
>> + uuidstr, newdef->usage_id);
>> + goto error;
>> + }
>>
>> - if (!(obj = virSecretObjNew()))
>> - goto cleanup;
>> + /* Generate the possible configFile and base64File strings
>> + * using the configDir, uuidstr, and appropriate suffix
>> + */
>> + if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml"))
||
>> + !(base64File = virFileBuildPath(configDir, uuidstr,
".base64")))
>> + goto cleanup;
>>
>> - if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
>> - goto cleanup;
>> + if (!(obj = virSecretObjNew()))
>> + goto cleanup;
>>
>> - obj->def = newdef;
>> - VIR_STEAL_PTR(obj->configFile, configFile);
>> - VIR_STEAL_PTR(obj->base64File, base64File);
>> - virObjectRef(obj);
>> - }
>> + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
>> + goto error;
>
> Frankly, I find this very confusing. While reading the commit message, I
> understand why you sometimes jump to cleanup and other times to error
> label. But if I were to just look at the code alone, it's completely
> random to me. I think I'd much rather see the pattern that's here.
> However, if you really dislike if-else branching (dislike also as in
> prepare for upcoming patches), I suggest changing just that.
>
> Michal
>
I'll return the if - else - logic... and update the commit message.
Would you like to see a version of that?
Yes please.
It does affect the next couple
of patches. For patch 5 it's just a simple indention adjustment. Since
there's questions in 6 I'll address those there.
That's okay, you can just send another version (and state that some
patches were ACKed already).
Michal
8:27 a.m.
New subject: [libvirt] [PATCH 5/8] secret: Remove need for local configFile and base64File in ObjectAdd
Rather than assign to a local variable, let's just assign directly to the
object using the error path for cleanup.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 1bafd0b..c0bcfab 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -334,7 +334,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
virSecretObjPtr obj;
virSecretDefPtr objdef;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- char *configFile = NULL, *base64File = NULL;
virObjectLock(secrets);
@@ -385,27 +384,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
goto error;
}
+ if (!(obj = virSecretObjNew()))
+ goto cleanup;
+
/* Generate the possible configFile and base64File strings
* using the configDir, uuidstr, and appropriate suffix
*/
- if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
- !(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
- goto cleanup;
-
- if (!(obj = virSecretObjNew()))
- goto cleanup;
+ if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml"))
||
+ !(obj->base64File = virFileBuildPath(configDir, uuidstr,
".base64")))
+ goto error;
if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
goto error;
obj->def = newdef;
- VIR_STEAL_PTR(obj->configFile, configFile);
- VIR_STEAL_PTR(obj->base64File, base64File);
virObjectRef(obj);
cleanup:
- VIR_FREE(configFile);
- VIR_FREE(base64File);
virObjectUnlock(secrets);
return obj;
--
2.9.4
10:52 a.m.
New subject: [libvirt] [PATCH 5/8] secret: Remove need for local configFile and base64File in ObjectAdd
On 06/03/2017 03:27 PM, John Ferlan wrote:
Rather than assign to a local variable, let's just assign
directly to the
object using the error path for cleanup.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
ACK
Michal
8:27 a.m.
New subject: [libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef
Move the consumption of @newdef into virSecretObjNew and then handle that
in the calling path. Because on error the calling code expects to free
@newdef, unset obj->def for the creation failure error paths.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index c0bcfab..ca13cf8 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virSecretObj)
static virSecretObjPtr
-virSecretObjNew(void)
+virSecretObjNew(virSecretDefPtr def)
{
virSecretObjPtr obj;
@@ -98,6 +98,7 @@ virSecretObjNew(void)
return NULL;
virObjectLock(obj);
+ obj->def = def;
return obj;
}
@@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
goto error;
}
- if (!(obj = virSecretObjNew()))
+ if (!(obj = virSecretObjNew(newdef)))
goto cleanup;
/* Generate the possible configFile and base64File strings
* using the configDir, uuidstr, and appropriate suffix
*/
if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml"))
||
- !(obj->base64File = virFileBuildPath(configDir, uuidstr,
".base64")))
+ !(obj->base64File = virFileBuildPath(configDir, uuidstr,
".base64"))) {
+ obj->def = NULL;
goto error;
+ }
- if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
+ if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) {
+ obj->def = NULL;
goto error;
+ }
- obj->def = newdef;
virObjectRef(obj);
cleanup:
--
2.9.4
10:52 a.m.
New subject: [libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef
On 06/03/2017 03:27 PM, John Ferlan wrote:
Move the consumption of @newdef into virSecretObjNew and then handle
that
in the calling path. Because on error the calling code expects to free
@newdef, unset obj->def for the creation failure error paths.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index c0bcfab..ca13cf8 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virSecretObj)
static virSecretObjPtr
-virSecretObjNew(void)
+virSecretObjNew(virSecretDefPtr def)
{
virSecretObjPtr obj;
@@ -98,6 +98,7 @@ virSecretObjNew(void)
return NULL;
virObjectLock(obj);
+ obj->def = def;
return obj;
}
@@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
goto error;
}
- if (!(obj = virSecretObjNew()))
+ if (!(obj = virSecretObjNew(newdef)))
goto cleanup;
/* Generate the possible configFile and base64File strings
* using the configDir, uuidstr, and appropriate suffix
*/
if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml"))
||
- !(obj->base64File = virFileBuildPath(configDir, uuidstr,
".base64")))
+ !(obj->base64File = virFileBuildPath(configDir, uuidstr,
".base64"))) {
+ obj->def = NULL;
goto error;
+ }
I don't quite see the value of this patch, esp. because you have to
manually unset the ->def in each error path.
Michal
11:43 a.m.
New subject: [libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
On 06/03/2017 03:27 PM, John Ferlan wrote:
> Move the consumption of @newdef into virSecretObjNew and then handle that
> in the calling path. Because on error the calling code expects to free
> @newdef, unset obj->def for the creation failure error paths.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/virsecretobj.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index c0bcfab..ca13cf8 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
> VIR_ONCE_GLOBAL_INIT(virSecretObj)
>
> static virSecretObjPtr
> -virSecretObjNew(void)
> +virSecretObjNew(virSecretDefPtr def)
> {
> virSecretObjPtr obj;
>
> @@ -98,6 +98,7 @@ virSecretObjNew(void)
> return NULL;
>
> virObjectLock(obj);
> + obj->def = def;
>
> return obj;
> }
> @@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> goto error;
> }
>
> - if (!(obj = virSecretObjNew()))
> + if (!(obj = virSecretObjNew(newdef)))
> goto cleanup;
>
> /* Generate the possible configFile and base64File strings
> * using the configDir, uuidstr, and appropriate suffix
> */
> if (!(obj->configFile = virFileBuildPath(configDir, uuidstr,
".xml")) ||
> - !(obj->base64File = virFileBuildPath(configDir, uuidstr,
".base64")))
> + !(obj->base64File = virFileBuildPath(configDir, uuidstr,
".base64"))) {
> + obj->def = NULL;
> goto error;
> + }
I don't quite see the value of this patch, esp. because you have to
manually unset the ->def in each error path.
Michal
Well that's part of that "longer term" vision thing where I was having
the @def be consumed in a new object. I've had to scale that back a bit
due to comments related to the object, but this code was already was all
being done in parallel - so that's why it's like that.
I could drop this one, although having @def consumed by vir*ObjNew() is
something that I have been doing throughout the various changes. So
far, virInterfaceObjNew already has this, but patches for nwfilter and
nodedev also follow the same pattern.
I'm 50/50 right now on it and can drop it if you'd prefer. Yes, the
drawback is "obvious" that on failure, clearing obj->def needs to be
done to avoid the potential double free problem.
John
3:15 a.m.
New subject: [libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef
On 07/13/2017 06:43 PM, John Ferlan wrote:
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Move the consumption of @newdef into virSecretObjNew and then handle that
>> in the calling path. Because on error the calling code expects to free
>> @newdef, unset obj->def for the creation failure error paths.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/virsecretobj.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index c0bcfab..ca13cf8 100644
>> --- a/src/conf/virsecretobj.c
>> +++ b/src/conf/virsecretobj.c
>> @@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
>> VIR_ONCE_GLOBAL_INIT(virSecretObj)
>>
>> static virSecretObjPtr
>> -virSecretObjNew(void)
>> +virSecretObjNew(virSecretDefPtr def)
>> {
>> virSecretObjPtr obj;
>>
>> @@ -98,6 +98,7 @@ virSecretObjNew(void)
>> return NULL;
>>
>> virObjectLock(obj);
>> + obj->def = def;
>>
>> return obj;
>> }
>> @@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> goto error;
>> }
>>
>> - if (!(obj = virSecretObjNew()))
>> + if (!(obj = virSecretObjNew(newdef)))
>> goto cleanup;
>>
>> /* Generate the possible configFile and base64File strings
>> * using the configDir, uuidstr, and appropriate suffix
>> */
>> if (!(obj->configFile = virFileBuildPath(configDir, uuidstr,
".xml")) ||
>> - !(obj->base64File = virFileBuildPath(configDir, uuidstr,
".base64")))
>> + !(obj->base64File = virFileBuildPath(configDir, uuidstr,
".base64"))) {
>> + obj->def = NULL;
>> goto error;
>> + }
>
> I don't quite see the value of this patch, esp. because you have to
> manually unset the ->def in each error path.
>
> Michal
>
Well that's part of that "longer term" vision thing where I was having
the @def be consumed in a new object. I've had to scale that back a bit
due to comments related to the object, but this code was already was all
being done in parallel - so that's why it's like that.
I could drop this one, although having @def consumed by vir*ObjNew() is
something that I have been doing throughout the various changes. So
far, virInterfaceObjNew already has this, but patches for nwfilter and
nodedev also follow the same pattern.
I know that you're doing it in other patches, but I don't think we need
to do that. It's not like we will make obj->def private. But maybe I'm
missing big picture here. What is your reasoning why should vir*ObjNew
take def? Moreover, other object members are initialized "old way" too
(e.g. obj->base64File). So mixing approaches might be confusing IMO.
I'm 50/50 right now on it and can drop it if you'd prefer. Yes, the
drawback is "obvious" that on failure, clearing obj->def needs to be
done to avoid the potential double free problem.
Yeah, I'd prefer it to be dropped, but then again - maybe I'm missing
big picture. So I'm not gonna tell you to do that.
Michal
8:27 a.m.
New subject: [libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver
Since the virSecretObjListAdd technically consumes @def on success,
the secretDefineXML should fetch the @def from the object afterwards
and manage as an @objdef and set @def = NULL immediately.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/secret/secret_driver.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 8ddae57..32cd8bb 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn,
{
virSecretPtr ret = NULL;
virSecretObjPtr obj = NULL;
+ virSecretDefPtr objdef;
virSecretDefPtr backup = NULL;
virSecretDefPtr def;
virObjectEventPtr event = NULL;
@@ -225,8 +226,10 @@ secretDefineXML(virConnectPtr conn,
if (!(obj = virSecretObjListAdd(driver->secrets, def,
driver->configDir, &backup)))
goto cleanup;
+ def = NULL;
+ objdef = virSecretObjGetDef(obj);
- if (!def->isephemeral) {
+ if (!objdef->isephemeral) {
if (backup && backup->isephemeral) {
if (virSecretObjSaveData(obj) < 0)
goto restore_backup;
@@ -248,17 +251,16 @@ secretDefineXML(virConnectPtr conn,
/* Saved successfully - drop old values */
virSecretDefFree(backup);
- event = virSecretEventLifecycleNew(def->uuid,
- def->usage_type,
- def->usage_id,
+ event = virSecretEventLifecycleNew(objdef->uuid,
+ objdef->usage_type,
+ objdef->usage_id,
VIR_SECRET_EVENT_DEFINED,
0);
ret = virGetSecret(conn,
- def->uuid,
- def->usage_type,
- def->usage_id);
- def = NULL;
+ objdef->uuid,
+ objdef->usage_type,
+ objdef->usage_id);
goto cleanup;
restore_backup:
--
2.9.4
10:52 a.m.
New subject: [libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver
On 06/03/2017 03:27 PM, John Ferlan wrote:
Since the virSecretObjListAdd technically consumes @def on success,
the secretDefineXML should fetch the @def from the object afterwards
and manage as an @objdef and set @def = NULL immediately.
Really? virSecretObjListAdd sets ->def pointer in the object, but it
doesn't touch the definition otherwise. So I think a caller is safe to
continue using the pointer.
Michal
11:54 a.m.
New subject: [libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
On 06/03/2017 03:27 PM, John Ferlan wrote:
> Since the virSecretObjListAdd technically consumes @def on success,
> the secretDefineXML should fetch the @def from the object afterwards
> and manage as an @objdef and set @def = NULL immediately.
Really? virSecretObjListAdd sets ->def pointer in the object, but it
doesn't touch the definition otherwise. So I think a caller is safe to
continue using the pointer.
Michal
Let's consider the code before my change...
@def is added to the @obj
"Something" causes us to jump to the "restore_backup:" label and
@backup
== NULL.
That means virSecretObjListRemove runs and because @obj has @def it ends
up calling virSecretDefFree
The very next thing we do is call virSecretDefFree on the same @def address.
While it is the same thing and I could do a VIR_STEAL_PTR(objdef, def);
instead, the patch just follows the pattern I've adopted in other
patches where @def = NULL and objdef = vir*ObjGetDef()
IDC either way, but to avoid a path where @def could be free'd twice,
something needs to be done.
John
3:26 a.m.
New subject: [libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver
On 07/13/2017 06:54 PM, John Ferlan wrote:
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Since the virSecretObjListAdd technically consumes @def on success,
>> the secretDefineXML should fetch the @def from the object afterwards
>> and manage as an @objdef and set @def = NULL immediately.
>
> Really? virSecretObjListAdd sets ->def pointer in the object, but it
> doesn't touch the definition otherwise. So I think a caller is safe to
> continue using the pointer.
>
> Michal
>
Let's consider the code before my change...
@def is added to the @obj
"Something" causes us to jump to the "restore_backup:" label and
@backup
== NULL.
That means virSecretObjListRemove runs and because @obj has @def it ends
up calling virSecretDefFree
The only way this can happen is when @obj has refcnt == 1. Because then
unref() calls dispose() which calls virSecretDefFree(). However, @obj
will have at least 2 references when entering restore_backup label. In
virSecretObjListAdd() when creating the new object via virSecretObjNew()
obj has refcnt = 1, and then we ref the object again. But wait a second:
if the object is already in both of the hash tables we return that
reference and don't increase the refcnt! So in the end,
virSecretObjListAdd() can return an object with refcnt == 1 and refcnt
== 2. This is obviously wrong and root cause of the problem you are
seeing. As I describe in the other e-mail, this breaks refcounting and
needs to be fixed.
Michal
7:01 a.m.
New subject: [libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver
On 07/14/2017 04:26 AM, Michal Privoznik wrote:
On 07/13/2017 06:54 PM, John Ferlan wrote:
>
>
> On 07/11/2017 11:52 AM, Michal Privoznik wrote:
>> On 06/03/2017 03:27 PM, John Ferlan wrote:
>>> Since the virSecretObjListAdd technically consumes @def on success,
>>> the secretDefineXML should fetch the @def from the object afterwards
>>> and manage as an @objdef and set @def = NULL immediately.
>>
>> Really? virSecretObjListAdd sets ->def pointer in the object, but it
>> doesn't touch the definition otherwise. So I think a caller is safe to
>> continue using the pointer.
>>
>> Michal
>>
>
> Let's consider the code before my change...
>
> @def is added to the @obj
>
> "Something" causes us to jump to the "restore_backup:" label and
@backup
> == NULL.
>
> That means virSecretObjListRemove runs and because @obj has @def it ends
> up calling virSecretDefFree
The only way this can happen is when @obj has refcnt == 1. Because then
unref() calls dispose() which calls virSecretDefFree(). However, @obj
will have at least 2 references when entering restore_backup label. In
virSecretObjListAdd() when creating the new object via virSecretObjNew()
obj has refcnt = 1, and then we ref the object again. But wait a second:
if the object is already in both of the hash tables we return that
reference and don't increase the refcnt! So in the end,
virSecretObjListAdd() can return an object with refcnt == 1 and refcnt
== 2. This is obviously wrong and root cause of the problem you are
seeing. As I describe in the other e-mail, this breaks refcounting and
needs to be fixed.
Michal
Ah - I see what's happened - in my mind I'm already at the next patch
where the else has a virObjectUnref(obj) after the ListRemove call and
thus falling into cleanup and doing a virSecretDefFree would have been
bad if @def was not NULL.
I don't understand the "But wait a second: if the object is already in
both of the hash tables we return that reference and don't increase the
refcnt! "
When we leave ObjListAdd - the refcnt should include 1 for New and 1 for
the HashTable, so it should be 2. This is where I contend domainobj's
have it wonky (or wrong) because if the Remove is called each HashRemove
will decrement the refcnt by 1. But all the callers there "know" this
and thus "choose" to use just Unlock at times rather than EndAPI. When
they use EndAPI, they always will Ref the object prior which IMO causes
too much thinking/knowledge of the consumer.
I'll go read/respond to your 8/8 reply in a moment - the caffeine is
starting to work through the morning haze...
I understand you object to the virSecretObjGetDef call as unnecessary;
however, what if I use VIR_STEAL_PTR. In the long run it's protection
against needing to appropriately place the def = NULL much later in this
code because we know the object owns it, but we wanted to use it and not
create another temporary. It protects against some future adjustment
that doesn't account for @def isn't owned by us and jumps to cleanup
free'ing @def when we don't own it.
John
7:26 a.m.
New subject: [libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver
On 07/14/2017 02:01 PM, John Ferlan wrote:
On 07/14/2017 04:26 AM, Michal Privoznik wrote:
> On 07/13/2017 06:54 PM, John Ferlan wrote:
>>
>>
>> On 07/11/2017 11:52 AM, Michal Privoznik wrote:
>>> On 06/03/2017 03:27 PM, John Ferlan wrote:
>>>> Since the virSecretObjListAdd technically consumes @def on success,
>>>> the secretDefineXML should fetch the @def from the object afterwards
>>>> and manage as an @objdef and set @def = NULL immediately.
>>>
>>> Really? virSecretObjListAdd sets ->def pointer in the object, but it
>>> doesn't touch the definition otherwise. So I think a caller is safe to
>>> continue using the pointer.
>>>
>>> Michal
>>>
>>
>> Let's consider the code before my change...
>>
>> @def is added to the @obj
>>
>> "Something" causes us to jump to the "restore_backup:" label
and @backup
>> == NULL.
>>
>> That means virSecretObjListRemove runs and because @obj has @def it ends
>> up calling virSecretDefFree
>
> The only way this can happen is when @obj has refcnt == 1. Because then
> unref() calls dispose() which calls virSecretDefFree(). However, @obj
> will have at least 2 references when entering restore_backup label. In
> virSecretObjListAdd() when creating the new object via virSecretObjNew()
> obj has refcnt = 1, and then we ref the object again. But wait a second:
> if the object is already in both of the hash tables we return that
> reference and don't increase the refcnt! So in the end,
> virSecretObjListAdd() can return an object with refcnt == 1 and refcnt
> == 2. This is obviously wrong and root cause of the problem you are
> seeing. As I describe in the other e-mail, this breaks refcounting and
> needs to be fixed.
>
> Michal
>
Ah - I see what's happened - in my mind I'm already at the next patch
where the else has a virObjectUnref(obj) after the ListRemove call and
thus falling into cleanup and doing a virSecretDefFree would have been
bad if @def was not NULL.
I don't understand the "But wait a second: if the object is already in
both of the hash tables we return that reference and don't increase the
refcnt! "
When we leave ObjListAdd - the refcnt should include 1 for New and 1 for
the HashTable, so it should be 2. This is where I contend domainobj's
have it wonky (or wrong) because if the Remove is called each HashRemove
will decrement the refcnt by 1. But all the callers there "know" this
and thus "choose" to use just Unlock at times rather than EndAPI. When
they use EndAPI, they always will Ref the object prior which IMO causes
too much thinking/knowledge of the consumer.
Oh, you're right. I misread the code. So the virSecretObjListAdd()
should return an object with 3 references. Two are for the two hash
tables object is in, third is for the caller to use and later free by
calling EndAPI.
I'll go read/respond to your 8/8 reply in a moment - the caffeine is
starting to work through the morning haze...
I understand you object to the virSecretObjGetDef call as unnecessary;
I don't care that much. I just find it surprising that introducing new
variable (which I have to remember anyway when reading the code) is
considered as more readable than dereferencing directly. Moreover, the
Levensthein distance between the two is just 2 ;-)
however, what if I use VIR_STEAL_PTR.
How?
In the long run it's protection
against needing to appropriately place the def = NULL much later in this
code because we know the object owns it, but we wanted to use it and not
create another temporary. It protects against some future adjustment
that doesn't account for @def isn't owned by us and jumps to cleanup
free'ing @def when we don't own it.
John
Michal
8:14 a.m.
New subject: [libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver
On 07/14/2017 08:26 AM, Michal Privoznik wrote:
On 07/14/2017 02:01 PM, John Ferlan wrote:
>
>
> On 07/14/2017 04:26 AM, Michal Privoznik wrote:
>> On 07/13/2017 06:54 PM, John Ferlan wrote:
>>>
>>>
>>> On 07/11/2017 11:52 AM, Michal Privoznik wrote:
>>>> On 06/03/2017 03:27 PM, John Ferlan wrote:
>>>>> Since the virSecretObjListAdd technically consumes @def on success,
>>>>> the secretDefineXML should fetch the @def from the object afterwards
>>>>> and manage as an @objdef and set @def = NULL immediately.
>>>>
>>>> Really? virSecretObjListAdd sets ->def pointer in the object, but it
>>>> doesn't touch the definition otherwise. So I think a caller is safe
to
>>>> continue using the pointer.
>>>>
>>>> Michal
>>>>
>>>
>>> Let's consider the code before my change...
>>>
>>> @def is added to the @obj
>>>
>>> "Something" causes us to jump to the "restore_backup:"
label and @backup
>>> == NULL.
>>>
>>> That means virSecretObjListRemove runs and because @obj has @def it ends
>>> up calling virSecretDefFree
>>
>> The only way this can happen is when @obj has refcnt == 1. Because then
>> unref() calls dispose() which calls virSecretDefFree(). However, @obj
>> will have at least 2 references when entering restore_backup label. In
>> virSecretObjListAdd() when creating the new object via virSecretObjNew()
>> obj has refcnt = 1, and then we ref the object again. But wait a second:
>> if the object is already in both of the hash tables we return that
>> reference and don't increase the refcnt! So in the end,
>> virSecretObjListAdd() can return an object with refcnt == 1 and refcnt
>> == 2. This is obviously wrong and root cause of the problem you are
>> seeing. As I describe in the other e-mail, this breaks refcounting and
>> needs to be fixed.
Coffee is strong this morning or light has dawned on marblehead.
Let me go back to the "existing" code for a moment where @def isn't set
to NULL when we get to restore_backup and call ObjListRemove
After Add, we have R=2,L=1
After Remove, we have R=1
fall into cleanup:
Call virSecretDefFree(def) where @def != NULL, so it free's it
Call virSecretObjEndAPI(&obj) which calls Unref, setting R=0 and calling
virSecretDefFree(obj->def)... But wait, we already did that because we
allowed the DefFree(def) to free something it didn't own.
>>
>> Michal
>>
>
> Ah - I see what's happened - in my mind I'm already at the next patch
> where the else has a virObjectUnref(obj) after the ListRemove call and
> thus falling into cleanup and doing a virSecretDefFree would have been
> bad if @def was not NULL.
>
> I don't understand the "But wait a second: if the object is already in
> both of the hash tables we return that reference and don't increase the
> refcnt! "
>
> When we leave ObjListAdd - the refcnt should include 1 for New and 1 for
> the HashTable, so it should be 2. This is where I contend domainobj's
> have it wonky (or wrong) because if the Remove is called each HashRemove
> will decrement the refcnt by 1. But all the callers there "know" this
> and thus "choose" to use just Unlock at times rather than EndAPI. When
> they use EndAPI, they always will Ref the object prior which IMO causes
> too much thinking/knowledge of the consumer.
Oh, you're right. I misread the code. So the virSecretObjListAdd()
should return an object with 3 references. Two are for the two hash
tables object is in, third is for the caller to use and later free by
calling EndAPI.
Object is only in one hash table at this point in time, so the return is
ref = 2, lock = 1. One ref is for the object allocation and one ref for
the table.
IMO, when EndAPI is called, it's an indication that the caller is done
with the obj; however, because the obj is in a hash table, the other ref
remains. I've always considered EndAPI an indication that we're done
with our reference to the object whether that was from an Add or a
Lookup, both of which should return with an incremented refcnt and a
locked object. As an aside, Add should also increment for each hash
table we're in because Remove will decrement for each. That's where I
think the domainobj code is wrong.
>
> I'll go read/respond to your 8/8 reply in a moment - the caffeine is
> starting to work through the morning haze...
>
> I understand you object to the virSecretObjGetDef call as unnecessary;
I don't care that much. I just find it surprising that introducing new
variable (which I have to remember anyway when reading the code) is
considered as more readable than dereferencing directly. Moreover, the
Levensthein distance between the two is just 2 ;-)
As long as someone remain vigilant that @def is set to NULL at some
point in the code after Add but before cleanup:, then we're good. But on
the off chance that it doesn't (which I've shown above), then we're not
in a happy state.
> however, what if I use VIR_STEAL_PTR.
How?
Instead of:
if (!(obj = virSecretObjListAdd(driver->secrets, def,
driver->configDir, &backup)))
goto cleanup;
def = NULL;
objdef = virSecretObjGetDef(obj);
it'd be
if (!(obj = virSecretObjListAdd(driver->secrets, def,
driver->configDir, &backup)))
goto cleanup;
VIR_STEAL_PTR(objdef, def);
John
> In the long run it's protection
> against needing to appropriately place the def = NULL much later in this
> code because we know the object owns it, but we wanted to use it and not
> create another temporary. It protects against some future adjustment
> that doesn't account for @def isn't owned by us and jumps to cleanup
> free'ing @def when we don't own it.
>
> John
>
Michal
8:27 a.m.
New subject: [libvirt] [PATCH 8/8] secret: Handle object list removal and deletion properly
Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
after the call virSecretObjListRemove, be more explicit by calling
virObjectUnref and setting @obj to NULL for secretUndefine and in
the error path of secretDefineXML.
This also fixes a leak during virSecretLoad if the virSecretLoadValue
fails the code jumps to cleanup without setting @ret = obj, thus calling
virSecretObjListRemove which only accounts for the object reference
related to adding the object to the list during virSecretObjListAdd,
but does not account for the reference to the object itself as the
return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
on the object recently added thus reducing the refcnt to zero. Thus
cleaning up the virSecretLoadValue error path to make it clearer what
needs to be done on failure.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 14 ++++++--------
src/secret/secret_driver.c | 9 +++++++--
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index ca13cf8..26646dd 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -918,7 +918,6 @@ virSecretLoad(virSecretObjListPtr secrets,
{
virSecretDefPtr def = NULL;
virSecretObjPtr obj = NULL;
- virSecretObjPtr ret = NULL;
if (!(def = virSecretDefParseFile(path)))
goto cleanup;
@@ -930,16 +929,15 @@ virSecretLoad(virSecretObjListPtr secrets,
goto cleanup;
def = NULL;
- if (virSecretLoadValue(obj) < 0)
- goto cleanup;
-
- ret = obj;
- obj = NULL;
+ if (virSecretLoadValue(obj) < 0) {
+ virSecretObjListRemove(secrets, obj);
+ virObjectUnref(obj);
+ obj = NULL;
+ }
cleanup:
- virSecretObjListRemove(secrets, obj);
virSecretDefFree(def);
- return ret;
+ return obj;
}
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 32cd8bb..14483fd 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -268,10 +268,13 @@ secretDefineXML(virConnectPtr conn,
* the backup. The current def will be handled below.
* Otherwise, this is a new secret, thus remove it.
*/
- if (backup)
+ if (backup) {
virSecretObjSetDef(obj, backup);
- else
+ } else {
virSecretObjListRemove(driver->secrets, obj);
+ virObjectUnref(obj);
+ obj = NULL;
+ }
cleanup:
virSecretDefFree(def);
@@ -411,6 +414,8 @@ secretUndefine(virSecretPtr secret)
virSecretObjDeleteData(obj);
virSecretObjListRemove(driver->secrets, obj);
+ virObjectUnref(obj);
+ obj = NULL;
ret = 0;
--
2.9.4
10:52 a.m.
New subject: [libvirt] [PATCH 8/8] secret: Handle object list removal and deletion properly
On 06/03/2017 03:27 PM, John Ferlan wrote:
Rather than rely on virSecretObjEndAPI to make the final
virObjectUnref
after the call virSecretObjListRemove, be more explicit by calling
virObjectUnref and setting @obj to NULL for secretUndefine and in
the error path of secretDefineXML.
This also fixes a leak during virSecretLoad if the virSecretLoadValue
fails the code jumps to cleanup without setting @ret = obj, thus calling
virSecretObjListRemove which only accounts for the object reference
related to adding the object to the list during virSecretObjListAdd,
but does not account for the reference to the object itself as the
return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
on the object recently added thus reducing the refcnt to zero. Thus
cleaning up the virSecretLoadValue error path to make it clearer what
needs to be done on failure.
I think the real reason is that we cannot call virSecretObjEndAPI()
because that *unlocks* the secret object. However, the object is already
unlocked at that point by virSecretObjListRemove() and thus we would
unlock twice while locking just once. Honestly, I'd rather see that
explanation in the commit message. But it's your call.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 14 ++++++--------
src/secret/secret_driver.c | 9 +++++++--
2 files changed, 13 insertions(+), 10 deletions(-)
ACK
Michal
12:19 p.m.
New subject: [libvirt] [PATCH 8/8] secret: Handle object list removal and deletion properly
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
On 06/03/2017 03:27 PM, John Ferlan wrote:
> Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
> after the call virSecretObjListRemove, be more explicit by calling
> virObjectUnref and setting @obj to NULL for secretUndefine and in
> the error path of secretDefineXML.
>
> This also fixes a leak during virSecretLoad if the virSecretLoadValue
> fails the code jumps to cleanup without setting @ret = obj, thus calling
> virSecretObjListRemove which only accounts for the object reference
> related to adding the object to the list during virSecretObjListAdd,
> but does not account for the reference to the object itself as the
> return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
> on the object recently added thus reducing the refcnt to zero. Thus
> cleaning up the virSecretLoadValue error path to make it clearer what
> needs to be done on failure.
I think the real reason is that we cannot call virSecretObjEndAPI()
because that *unlocks* the secret object. However, the object is already
unlocked at that point by virSecretObjListRemove() and thus we would
unlock twice while locking just once. Honestly, I'd rather see that
explanation in the commit message. But it's your call.
Partially true - although calling Unlock on something already Unlocked
by the same thread IIRC doesn't do much other than cause an error, but
we don't fail on that error.
It's ironic this relates to something Erik and I discussed during the
virNodeDev* changes w/r/t the "owner" (driver) after the Add/Append now
has a reference on the object and would always need to Unref it even
after removing it from the List.
This just happened to be where I had that oh sh*t moment and realized
that when calling Remove we were essentially unlocking and thus yes
calling EndAPI would unlock and unlocked object. I'll add something in
about that..
John
As an aside, this is exactly why I started down the path of common
objects. Consider how the callers to virDomainObjListAdd go through
great lengths to managed the returned object some quite differently
based on what they know about how many refs are returned and whether the
calling function calls virDomainObjEndAPI. If it does, there's always a
virObjectRef done on the object after the Add returns. It's a subtle
thing, but confusing nonetheless.
The thing is the *Remove API will call virHashRemoveEntry which
decrements a refcnt for each table; however, the *Add API only
increments the refcnt once for adding into each table. Callers are very
careful to understand and manage that.
I'd rather see that mgmt go away. It should be "safe" to call EndAPI obj
the @obj regardless if Add or Lookup was used. The callers shouldn't
have to know they need to either use *EndAPI or ObjUnlock. In any case,
I digress and that's a different issue for another day
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/virsecretobj.c | 14 ++++++--------
> src/secret/secret_driver.c | 9 +++++++--
> 2 files changed, 13 insertions(+), 10 deletions(-)
ACK
Michal
3:15 a.m.
New subject: [libvirt] [PATCH 8/8] secret: Handle object list removal and deletion properly
On 07/13/2017 07:19 PM, John Ferlan wrote:
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
>> after the call virSecretObjListRemove, be more explicit by calling
>> virObjectUnref and setting @obj to NULL for secretUndefine and in
>> the error path of secretDefineXML.
>>
>> This also fixes a leak during virSecretLoad if the virSecretLoadValue
>> fails the code jumps to cleanup without setting @ret = obj, thus calling
>> virSecretObjListRemove which only accounts for the object reference
>> related to adding the object to the list during virSecretObjListAdd,
>> but does not account for the reference to the object itself as the
>> return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
>> on the object recently added thus reducing the refcnt to zero. Thus
>> cleaning up the virSecretLoadValue error path to make it clearer what
>> needs to be done on failure.
>
> I think the real reason is that we cannot call virSecretObjEndAPI()
> because that *unlocks* the secret object. However, the object is already
> unlocked at that point by virSecretObjListRemove() and thus we would
> unlock twice while locking just once. Honestly, I'd rather see that
> explanation in the commit message. But it's your call.
>
Partially true - although calling Unlock on something already Unlocked
by the same thread IIRC doesn't do much other than cause an error, but
we don't fail on that error.
I don't think this is true. Just consider the following general example:
Thread A | Thread B
------------------------------
1) lock(X) |
2) | lock(x)
3) unlock(X) |
4) unlock(X) |
5) | work(X)
In this scenario, thread B starts working on X thinking it's locked
while it isn't. Also, man-page for pthread_mutex_lock says that
unlocking an unlocked lock leads to undefined behaviour (we use NORMAL
non-robust mutexes. And yes, I had to search for what does it mean
non-robust mutex).
It's ironic this relates to something Erik and I discussed during the
virNodeDev* changes w/r/t the "owner" (driver) after the Add/Append now
has a reference on the object and would always need to Unref it even
after removing it from the List.
About that. I think that if we do our ref counting right, we don't need
to do any special unref(). I mean:
undefine(X) {
obj = lookup(X); // obj now has refcnt = 2, one beacuse it's in the
// list, one for the undefine()
listRemove(obj); // obj.refcnt = 1, the list reference is gone
endApi(&obj); // undefine() reference is gone, obj is freed
}
Of course, if list uses two hash tables (one for UUIDs one for names) it
holds two references, but then listRemove() also unref()-s twice.
This just happened to be where I had that oh sh*t moment and realized
that when calling Remove we were essentially unlocking and thus yes
calling EndAPI would unlock and unlocked object. I'll add something in
about that..
John
As an aside, this is exactly why I started down the path of common
objects. Consider how the callers to virDomainObjListAdd go through
great lengths to managed the returned object some quite differently
based on what they know about how many refs are returned and whether the
calling function calls virDomainObjEndAPI. If it does, there's always a
virObjectRef done on the object after the Add returns. It's a subtle
thing, but confusing nonetheless.
The thing is the *Remove API will call virHashRemoveEntry which
decrements a refcnt for each table; however, the *Add API only
increments the refcnt once for adding into each table. Callers are very
careful to understand and manage that.
I haven't looked into the code, but if this is true then listAdd() and
listRemove() functions are broken because they mangle refcount. It
should be able to do the following and having the refcount of an object
unchanged at the end:
listAdd(obj);
listRemove(obj);
If the refcount is not the same at the end as it was at the beginning,
the list functions are broken and need to be fixed
I'd rather see that mgmt go away. It should be "safe" to call EndAPI obj
the @obj regardless if Add or Lookup was used. The callers shouldn't
have to know they need to either use *EndAPI or ObjUnlock. In any case,
I digress and that's a different issue for another day
I totally agree on this!
Michal
7:48 a.m.
New subject: [libvirt] [PATCH 8/8] secret: Handle object list removal and deletion properly
On 07/14/2017 04:15 AM, Michal Privoznik wrote:
On 07/13/2017 07:19 PM, John Ferlan wrote:
>
>
> On 07/11/2017 11:52 AM, Michal Privoznik wrote:
>> On 06/03/2017 03:27 PM, John Ferlan wrote:
>>> Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
>>> after the call virSecretObjListRemove, be more explicit by calling
>>> virObjectUnref and setting @obj to NULL for secretUndefine and in
>>> the error path of secretDefineXML.
>>>
>>> This also fixes a leak during virSecretLoad if the virSecretLoadValue
>>> fails the code jumps to cleanup without setting @ret = obj, thus calling
>>> virSecretObjListRemove which only accounts for the object reference
>>> related to adding the object to the list during virSecretObjListAdd,
>>> but does not account for the reference to the object itself as the
>>> return of @ret would be NULL so the caller wouldn't call
virSecretObjEndAPI
>>> on the object recently added thus reducing the refcnt to zero. Thus
>>> cleaning up the virSecretLoadValue error path to make it clearer what
>>> needs to be done on failure.
>>
>> I think the real reason is that we cannot call virSecretObjEndAPI()
>> because that *unlocks* the secret object. However, the object is already
>> unlocked at that point by virSecretObjListRemove() and thus we would
>> unlock twice while locking just once. Honestly, I'd rather see that
>> explanation in the commit message. But it's your call.
>>
>
> Partially true - although calling Unlock on something already Unlocked
> by the same thread IIRC doesn't do much other than cause an error, but
> we don't fail on that error.
I don't think this is true. Just consider the following general example:
Thread A | Thread B
------------------------------
1) lock(X) |
2) | lock(x)
3) unlock(X) |
4) unlock(X) |
5) | work(X)
In this scenario, thread B starts working on X thinking it's locked
while it isn't. Also, man-page for pthread_mutex_lock says that
unlocking an unlocked lock leads to undefined behaviour (we use NORMAL
non-robust mutexes. And yes, I had to search for what does it mean
non-robust mutex).
I assume that step 2 is supposed to be (X) and not (x)?
In your scenario above ThreadB gains the lock after step 3 since that's
the moment in time when ThreadA gives it up. When ThreadA goes to unlock
in step 4, it fails because it doesn't own the lock as only ThreadB can
unlock. Without ThreadB in the picture, then the second unlock would be
a failure because the thread isn't locked, cannot unlock an unlocked object.
IMO: undefined behavior is what I'd call a way to say you could get an
error that the lock is already unlocked or you could get an error that
you don't own the lock to unlock, but we don't want to explain that so
we'll just it's undefined. For some reason I have this recollection
about fork'd children that really make things crazy, but I'm not sure I
can put it into words.
>
> It's ironic this relates to something Erik and I discussed during the
> virNodeDev* changes w/r/t the "owner" (driver) after the Add/Append now
> has a reference on the object and would always need to Unref it even
> after removing it from the List.
About that. I think that if we do our ref counting right, we don't need
to do any special unref(). I mean:
undefine(X) {
obj = lookup(X); // obj now has refcnt = 2, one beacuse it's in the
// list, one for the undefine()
listRemove(obj); // obj.refcnt = 1, the list reference is gone
endApi(&obj); // undefine() reference is gone, obj is freed
}
Of course, if list uses two hash tables (one for UUIDs one for names) it
holds two references, but then listRemove() also unref()-s twice.
When one enters a listRemove function, there will be either 2 or 3 refs
on the object (1 or 2 hash tables) and the object is locked.
When one leaves a listRemove function, there is 1 reference on the
object and the object is unlocked.
So is the problem that listRemove shouldn't make that last Unlock? Or
that the caller shouldn't make an EndAPI call after calling Remove?
The corollary is that the listAdd would leave with 2 or 3 refs (again
number of hash tables) and the object locked. The caller is then
expected to run EndAPI. The obj is then left with 1 or 2 refs because
it's in a table and not because it's being used again (since EndAPI will
set *obj = NULL).
My "chosen" solution is to keep the Unlock in listRemove, but I can be
swayed to have listRemove keep the lock thus having callers use EndAPI
instead of Unref. That would be something I would want to propagate to
other series so as to keep things similar between all consumers.
>
> This just happened to be where I had that oh sh*t moment and realized
> that when calling Remove we were essentially unlocking and thus yes
> calling EndAPI would unlock and unlocked object. I'll add something in
> about that..
>
> John
>
> As an aside, this is exactly why I started down the path of common
> objects. Consider how the callers to virDomainObjListAdd go through
> great lengths to managed the returned object some quite differently
> based on what they know about how many refs are returned and whether the
> calling function calls virDomainObjEndAPI. If it does, there's always a
> virObjectRef done on the object after the Add returns. It's a subtle
> thing, but confusing nonetheless.
>
> The thing is the *Remove API will call virHashRemoveEntry which
> decrements a refcnt for each table; however, the *Add API only
> increments the refcnt once for adding into each table. Callers are very
> careful to understand and manage that.
I haven't looked into the code, but if this is true then listAdd() and
listRemove() functions are broken because they mangle refcount. It
should be able to do the following and having the refcount of an object
unchanged at the end:
listAdd(obj);
listRemove(obj);
If the refcount is not the same at the end as it was at the beginning,
the list functions are broken and need to be fixed
Go check callers of virDomainObjListAdd... Every single qemu case will
always virObjectRef(vm) after a call... Other consumers will sometimes
use EndAPI and other times use Unlock based primarily on whether they
called Ref as well. It's at best confusing. The problem in my mind is
that the Add was only refcnt'ing once for two lists while the Remove was
having two unref's.
I think if obj = Add is successful, then it's still up to whatever code
does the Remove(obj) code to subsequently indicate it's really done with
@obj by performing the Unref. At least that's the goal I've been
working towards...
John
>
> I'd rather see that mgmt go away. It should be "safe" to call EndAPI
obj
> the @obj regardless if Add or Lookup was used. The callers shouldn't
> have to know they need to either use *EndAPI or ObjUnlock. In any case,
> I digress and that's a different issue for another day
I totally agree on this!
Michal
8:29 p.m.
ping?
A bit involved, but encompasses a few things I have found or made more
common in other vir*obj's
Tks,
John
On 06/03/2017 09:27 AM, John Ferlan wrote:
A couple I forgot from earlier series, but a few things found since
then
along with some clarifications to make the ObjAdd processing a bit more
like I've found for nwfilter, nodedev, and interface as well as making
the ObjListRemove processing a bit clearer and more consistent.
Along with the other series and the virObject v2 adjustments - this brings
everything close enough to start sending the patches that will tie all this
stuff together to create/use a common object model.
John Ferlan (8):
secret: Whitespace modification for secret_driver
secret: Alter FindByUUID to expect the formatted uuidstr
secret: Rename variable in virSecretObjListAdd
secret: Clean up virSecretObjListAdd processing
secret: Remove need for local configFile and base64File in ObjectAdd
secret: Have virSecretObjNew consume newdef
secret: Properly handle @def after virSecretObjAdd in driver
secret: Handle object list removal and deletion properly
src/conf/virsecretobj.c | 125 +++++++++++++++++++++------------------------
src/conf/virsecretobj.h | 2 +-
src/secret/secret_driver.c | 50 ++++++++++++------
3 files changed, 94 insertions(+), 83 deletions(-)
5:26 p.m.
ping^2 ?
Tks,
John
Would help especially since I could post more for the recently posted
virObject adjustment...
On 06/14/2017 09:29 PM, John Ferlan wrote:
ping?
A bit involved, but encompasses a few things I have found or made more
common in other vir*obj's
Tks,
John
On 06/03/2017 09:27 AM, John Ferlan wrote:
> A couple I forgot from earlier series, but a few things found since then
> along with some clarifications to make the ObjAdd processing a bit more
> like I've found for nwfilter, nodedev, and interface as well as making
> the ObjListRemove processing a bit clearer and more consistent.
>
> Along with the other series and the virObject v2 adjustments - this brings
> everything close enough to start sending the patches that will tie all this
> stuff together to create/use a common object model.
>
>
> John Ferlan (8):
> secret: Whitespace modification for secret_driver
> secret: Alter FindByUUID to expect the formatted uuidstr
> secret: Rename variable in virSecretObjListAdd
> secret: Clean up virSecretObjListAdd processing
> secret: Remove need for local configFile and base64File in ObjectAdd
> secret: Have virSecretObjNew consume newdef
> secret: Properly handle @def after virSecretObjAdd in driver
> secret: Handle object list removal and deletion properly
>
> src/conf/virsecretobj.c | 125 +++++++++++++++++++++------------------------
> src/conf/virsecretobj.h | 2 +-
> src/secret/secret_driver.c | 50 ++++++++++++------
> 3 files changed, 94 insertions(+), 83 deletions(-)
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:45 a.m.
ping^3 ?
Tks,
John
On 06/22/2017 06:26 PM, John Ferlan wrote:
ping^2 ?
Tks,
John
Would help especially since I could post more for the recently posted
virObject adjustment...
On 06/14/2017 09:29 PM, John Ferlan wrote:
> ping?
>
> A bit involved, but encompasses a few things I have found or made more
> common in other vir*obj's
>
> Tks,
>
> John
>
> On 06/03/2017 09:27 AM, John Ferlan wrote:
>> A couple I forgot from earlier series, but a few things found since then
>> along with some clarifications to make the ObjAdd processing a bit more
>> like I've found for nwfilter, nodedev, and interface as well as making
>> the ObjListRemove processing a bit clearer and more consistent.
>>
>> Along with the other series and the virObject v2 adjustments - this brings
>> everything close enough to start sending the patches that will tie all this
>> stuff together to create/use a common object model.
>>
>>
>> John Ferlan (8):
>> secret: Whitespace modification for secret_driver
>> secret: Alter FindByUUID to expect the formatted uuidstr
>> secret: Rename variable in virSecretObjListAdd
>> secret: Clean up virSecretObjListAdd processing
>> secret: Remove need for local configFile and base64File in ObjectAdd
>> secret: Have virSecretObjNew consume newdef
>> secret: Properly handle @def after virSecretObjAdd in driver
>> secret: Handle object list removal and deletion properly
>>
>> src/conf/virsecretobj.c | 125 +++++++++++++++++++++------------------------
>> src/conf/virsecretobj.h | 2 +-
>> src/secret/secret_driver.c | 50 ++++++++++++------
>> 3 files changed, 94 insertions(+), 83 deletions(-)
>>
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2940
days inactive
2981
days old
31 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik