Re: [PATCH 2/3] virsh: Add '--full-seclabels' option for dominfo

On Thu, Sep 02, 2021 at 08:29:35PM +0800, Luke Yue wrote: > There is no virsh command uses virDomainGetSecurityLabelList API, > so add > an option for dominfo to call it and print full list of security > labels. > > Also realign some outputs as it's now "Security labels:" instead of > "Security label:". > This should go into separate patch as that makes it nicer to review and the separation of changes makes it more clear.

> Signed-off-by: Luke Yue <lukedyue(a)gmail.com&gt; > --- > docs/manpages/virsh.rst      |  5 +- > tests/virsh-undefine         |  8 ++-- > tests/virshtest.c            | 70 ++++++++++++++-------------- > tools/virsh-domain-monitor.c | 89 ++++++++++++++++++++++++--------- > --- > 4 files changed, 101 insertions(+), 71 deletions(-) > > diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain- > monitor.c > index f7cf82acdf..2b2746e713 100644 > --- a/tools/virsh-domain-monitor.c > +++ b/tools/virsh-domain-monitor.c > @@ -1299,29 +1304,53 @@ cmdDominfo(vshControl *ctl, const vshCmd > *cmd) >     } else { >         /* Only print something if a security model is active */ >         if (secmodel.model[0] != '\0') { > -            vshPrint(ctl, "%-15s %s\n", _("Security model:"), > secmodel.model); > -            vshPrint(ctl, "%-15s %s\n", _("Security DOI:"), > secmodel.doi); > - > -            /* Security labels are only valid for active domains > */ > -            seclabel = g_new0(virSecurityLabel, 1); > +            vshPrint(ctl, "%-16s %s\n", _("Security model:"), > secmodel.model); > +            vshPrint(ctl, "%-16s %s\n", _("Security DOI:"), > secmodel.doi); > + > +            if (fullseclabels) { > +                int len; > +                size_t i; > + > +                if ((len = virDomainGetSecurityLabelList(dom, > &seclabel)) < 0) { > +                    g_clear_pointer(&(seclabel), g_free); > +                    return false; > +                } else { No need for else branch when the first branch returns. > +                    for (i = 0; i < len; i++) > +                        if (seclabel[i].label[0] != '\0') > +                            vshPrint(ctl, "%-16s %s (%s)\n", > +                                     i == 0 ? _("Security > labels:") : "", > +                                     seclabel[i].label, > +                                     seclabel[i].enforcing ? > +                                     "enforcing" : > +                                     "permissive"); > +                } > > -            if (virDomainGetSecurityLabel(dom, seclabel) == -1) { > -                VIR_FREE(seclabel); > -                return false; > +                g_clear_pointer(&seclabel, g_free); >             } else { > -                if (seclabel->label[0] != '\0') > -                    vshPrint(ctl, "%-15s %s (%s)\n", _("Security > label:"), > -                             seclabel->label, seclabel->enforcing > ? "enforcing" : "permissive"); > -            } > +                /* Security labels are only valid for active > domains */ > +                seclabel = g_new0(virSecurityLabel, 1); > + > +                if (virDomainGetSecurityLabel(dom, seclabel) == - > 1) { You properly used `< 0` before, this should do the same then. > +                    g_clear_pointer(&seclabel, g_free); > +                    return false; > +                } else { Again, no need for an else branch when if branch returns.

Otherwise it looks good.