Re: [PATCH 1/2] virt-aa-helper: add rules for shmem devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 20 Nov 2019, Christian Ehrhardt wrote:
> On Tue, Nov 19, 2019 at 10:25 PM Jamie Strandboge <jamie@xxxxxxxxxxxxx> wrote:
> > On Tue, 22 Oct 2019, Christian Ehrhardt wrote:
> > > +    for (i = 0; i < ctl->def->nshmems; i++) {
> > > +        if (ctl->def->shmems[i]) {
> > > +            virDomainShmemDef *shmem = ctl->def->shmems[i];
> > > +            /* server path can be on any type and overwrites defaults */
> > > +            if (shmem->server.enabled &&
> > > +                shmem->server.chr.data.nix.path) {
> > > +                    if (vah_add_file(&buf, shmem->server.chr.data.nix.path,
> > > +                            "rw") != 0)
> > > +                        goto cleanup;
> >
> > I'll let others comment on the code changes, but this apparmor rule
> > looks ok.
> >
> > > +            } else {
> >
> > That said, I wonder about the logic here since up above we have:
> >
> >   if (shmem->server.enabled && shmem->server.chr.data.nix.path)
> >
> > but here we just have 'else'. What happens if server.enabled is false
> > and server.chr.data.nix.path is set or vice versa? Does this 'else'
> > clause correctly handle those scenarios?
> 
> Yes if either of the above isn't fulfilled it will fall back to use
> the default paths.
> So a single else without other checks should be correct.
> The following switch then differs the default paths used base on the model.

Thanks! We agreed on irc a small code comment would clarify this. LGTM
provided you add a code comment similar to what we discussed on IRC.

-- 
Jamie Strandboge             | http://www.canonical.com

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux