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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list