[libvirt] [PATCHv2 1/2] security_dac: Rework to make it more readable

[Ján Tomko]

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 at 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140416/de8183a2/attachment-0001.sig>