Re: [PATCH v2 2/4] security: replace uses of label and VIR_FREE by g_autofree

On 11/13/24 07:28, Georgia Garcia wrote: > Moving towards full adoption of GLib APIs in the AppArmor code. > > Signed-off-by: Georgia Garcia <georgia.garcia(a)canonical.com&gt; > --- >   src/security/security_apparmor.c |  41 ++++--------- >   src/security/virt-aa-helper.c    | 100 ++++++++++--------------------- >   2 files changed, 45 insertions(+), 96 deletions(-) > > diff --git a/src/security/security_apparmor.c b/src/security/ > security_apparmor.c > index 7092724563..9e578b2526 100644 > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -115,37 +115,28 @@ profile_loaded(const char *str) >   static int >   profile_status_file(const char *str) >   { > -    char *profile = NULL; > -    char *content = NULL; > -    char *tmp = NULL; > -    int rc = -1; > +    g_autofree char *profile = NULL; > +    g_autofree char *content = NULL; > +    g_autofree char *tmp = NULL; >       int len; >         profile = g_strdup_printf("%s/%s", APPARMOR_DIR "/libvirt", str); >         if (!virFileExists(profile)) > -        goto failed; > +        return -1; >         if ((len = virFileReadAll(profile, MAX_FILE_LEN, &content)) < > 0) { >           virReportSystemError(errno, >                                _("Failed to read \'%1$s\'"), profile); > -        goto failed; > +        return -1; >       } >         /* create string that is ' <str> flags=(complain)\0' */ >       tmp = g_strdup_printf(" %s flags=(complain)", str); >         if (strstr(content, tmp) != NULL) > -        rc = 0; > -    else > -        rc = 1; > - > - failed: > -    VIR_FREE(tmp); > -    VIR_FREE(profile); > -    VIR_FREE(content); > - > -    return rc; > +        return 0; > +    return 1; >   } >     /* > @@ -218,7 +209,7 @@ static int >   use_apparmor(void) >   { >       int rc = -1; > -    char *libvirt_daemon = NULL; > +    g_autofree char *libvirt_daemon = NULL; >         if (virFileResolveLink("/proc/self/exe", &libvirt_daemon) < 0) { >           virReportError(VIR_ERR_INTERNAL_ERROR, > @@ -232,7 +223,7 @@ use_apparmor(void) >           return 1; >         if (access(APPARMOR_PROFILES_PATH, R_OK) != 0) > -        goto cleanup; > +        return rc; >         /* First check profile status using full binary path. If that > fails >        * check using profile name. > @@ -245,8 +236,6 @@ use_apparmor(void) >               rc = -1; >       } >   - cleanup: > -    VIR_FREE(libvirt_daemon); >       return rc; >   } >   @@ -948,7 +937,7 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, >                           virDomainChrSourceDef *dev_source, >                           bool chardevStdioLogd G_GNUC_UNUSED) >   { > -    char *in = NULL, *out = NULL; > +    g_autofree char *in = NULL, *out = NULL; While touching this code, we can follow the preference* of one variable per line. I can make that small tweak before pushing. Otherwise, the patch looks good! Reviewed-by: Jim Fehlig <jfehlig(a)suse.com&gt; Regards, Jim * I _think_ that's the project preference, and vaguely recall others mentioning it in the past :-).