On 04/04/2014 02:34 PM, Michal Privoznik wrote:
While going through the security DAC code, I realized it's a ragbag with plenty of thing we try to avoid. For instance, callback data are passed as:
void params[2]; params[0] = mgr; params[1] = def;
Moreover, there's no need to pass the whole virDomainDef in the callback as the only thing needed in the callbacks is virSecurityLabelDefPtr. Okay, in some cases the callbacks report error with a domain name, but that can be changed.
About a third of this patch is not related to the params -> cbdata -> secdef change, like whitespace changes, the enum changes, and ATTRIBUTE_UNUSED and if (uidPtr) removals. Separating params -> cbdata and cbdata -> secdef changes would make this much easier to review.
The other thing, in switch() we ought use enum types as it is much safer when adding a new item to the enum.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 271 +++++++++++++++++++++++--------------------- 1 file changed, 144 insertions(+), 127 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
static int -virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv, +virSecurityDACGetIds(virSecurityLabelDefPtr seclabel, + virSecurityDACDataPtr priv, uid_t *uidPtr, gid_t *gidPtr, gid_t **groups, int *ngroups) { int ret;
- if (!def && !priv) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to determine default DAC seclabel " - "for an unknown object")); - return -1; - } - if (groups) *groups = priv ? priv->groups : NULL; if (ngroups) *ngroups = priv ? priv->ngroups : 0;
- if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0) + if ((ret = virSecurityDACParseIds(seclabel, uidPtr, gidPtr)) <= 0) return ret;
if (!priv) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("DAC seclabel couldn't be determined " - "for domain '%s'"), def->name); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("DAC seclabel couldn't be determined")); return -1; }
- if (uidPtr) - *uidPtr = priv->user; - if (gidPtr) - *gidPtr = priv->group; + *uidPtr = priv->user; + *gidPtr = priv->group;
It would be nice to mark these as ATTRIBUTE_NONNULL, even though it's a static function.
return 0; }
@@ -685,41 +656,65 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, static int virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - virDomainChrSourceDefPtr dev) + virDomainChrDefPtr dev, + virDomainChrSourceDefPtr dev_source)
{ virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr seclabel; + virSecurityDeviceLabelDefPtr chr_seclabel = NULL; char *in = NULL, *out = NULL; int ret = -1; uid_t user; gid_t group;
- if (virSecurityDACGetIds(def, priv, &user, &group, NULL, NULL)) - return -1; + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
- switch (dev->type) { + if (dev) + chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, + SECURITY_DAC_NAME);
This looks like a functional change - I don't see any lines removing virDomainChrDefGetSecurityLabelDef.
+ + if (chr_seclabel && chr_seclabel->label) { + if (virParseOwnershipIds(chr_seclabel->label, &user, &group) < 0) + return -1; + } else { + if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) + return -1; + } +
Jan