[libvirt] [PATCH 2/2] security: update user and group parsing in security_dac.c

[Marcelo Cerri]

On Mon, Oct 08, 2012 at 02:11:26PM +0200, Peter Krempa wrote:
> On 10/06/12 04:52, Marcelo Cerri wrote:
> >The functions virGetUserID and virGetGroupID are now able to parse
> >user/group names and IDs in a similar way to coreutils' chown. So, user
> >and group parsing in security_dac can be simplified.
> >---
> >  src/security/security_dac.c | 40 ++++++++++------------------------------
> >  1 file changed, 10 insertions(+), 30 deletions(-)
> >
> >diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> >index a427e9d..e976144 100644
> >--- a/src/security/security_dac.c
> >+++ b/src/security/security_dac.c
> >@@ -95,39 +95,19 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
> >      group = sep + 1;
> >
> >      /* Parse owner */
> >-    if (*owner == '+') {
> >-        if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) {
> >-            virReportError(VIR_ERR_INVALID_ARG,
> >-                           _("Invalid uid \"%s\" in DAC label \"%s\""),
> >-                           owner, label);
> >-            goto cleanup;
> >-        }
> >-    } else {
> >-        if (virGetUserID(owner, &theuid) < 0 &&
> >-            virStrToLong_ui(owner, NULL, 10, &theuid) < 0) {
> >-            virReportError(VIR_ERR_INVALID_ARG,
> >-                           _("Invalid owner \"%s\" in DAC label \"%s\""),
> >-                           owner, label);
> >-            goto cleanup;
> >-        }
> >+    if (virGetUserID(owner, &theuid) < 0) {
> >+        virReportError(VIR_ERR_INVALID_ARG,
> >+                       _("Invalid owner \"%s\" in DAC label \"%s\""),
> >+                       owner, label);
> >+        goto cleanup;
> >      }
> >
> >      /* Parse group */
> >-    if (*group == '+') {
> >-        if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) {
> >-            virReportError(VIR_ERR_INVALID_ARG,
> >-                           _("Invalid gid \"%s\" in DAC label \"%s\""),
> >-                           group, label);
> >-            goto cleanup;
> >-        }
> >-    } else {
> >-        if (virGetGroupID(group, &thegid) < 0 &&
> >-            virStrToLong_ui(group, NULL, 10, &thegid) < 0) {
> >-            virReportError(VIR_ERR_INVALID_ARG,
> >-                           _("Invalid group \"%s\" in DAC label \"%s\""),
> >-                           group, label);
> >-            goto cleanup;
> >-        }
> >+    if (virGetGroupID(group, &thegid) < 0) {
> >+        virReportError(VIR_ERR_INVALID_ARG,
> >+                       _("Invalid group \"%s\" in DAC label \"%s\""),
> >+                       group, label);
> 
> Hm, this will mask the errors from virGetGroupID that might be
> useful. Should we remove this message in favor of the underlying
> messages or have this that has more relevant information where to
> find the issue but maybe mask the reason?

I thought about that when I was testing these changes and it seemed more
useful for me when the message points to where the problem is, in this
case the DAC label.

But you are right, it will mask the reason why a DAC label couldn't be
parsed.

> 
> Any opinions?

Maybe a solution with a nested error would be an alternative, what do
think? Not sure if it is a good idea and I don't know what is the best
way to implement it.

Here is an example of what I'm talking:

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index e976144..4f097cc 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -95,18 +95,24 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
     group = sep + 1;
 
     /* Parse owner */
+    virResetLastError();
     if (virGetUserID(owner, &theuid) < 0) {
+        virErrorPtr nested_error = virGetLastError();
         virReportError(VIR_ERR_INVALID_ARG,
-                       _("Invalid owner \"%s\" in DAC label \"%s\""),
-                       owner, label);
+                       _("Invalid owner \"%s\" in DAC label \"%s\"%s%s"),
+                       owner, label, nested_error ? ": "  : "", 
+                       nested_error ? nested_error->message  : ""); 
         goto cleanup;
     }
 
     /* Parse group */
+    virResetLastError();
     if (virGetGroupID(group, &thegid) < 0) {
+        virErrorPtr nested_error = virGetLastError();
         virReportError(VIR_ERR_INVALID_ARG,
-                       _("Invalid group \"%s\" in DAC label \"%s\""),
-                       group, label);
+                       _("Invalid group \"%s\" in DAC label \"%s\"%s%s"),
+                       group, label, nested_error ? ": "  : "", 
+                       nested_error ? nested_error->message  : ""); 
         goto cleanup;
     }
 
-- 
1.7.12


> 
> >+        goto cleanup;
> >      }
> >
> >      if (uidPtr)
> >
> 
> 
> Peter
>