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@xxxxxxxxx>
> > ---
> > 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
