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

[Luke Yue]

On Mon, 2021-11-29 at 17:52 +0100, Martin Kletzander wrote:
> 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.
> 

Thanks for the review! I will split this in next version.

> > Signed-off-by: Luke Yue <lukedyue at gmail.com>
> > ---
> > 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.
> 

OK, I will fix these and take the suggestion for 1/3 

> Otherwise it looks good.

Thanks,
Luke