On Sun, May 9, 2021 at 11:32 PM Andrew Jones <drjones@xxxxxxxxxx> wrote:
>
> On Fri, May 07, 2021 at 01:51:45PM -0700, David Matlack wrote:
> > > > static void vm_open(struct kvm_vm *vm, int perm)
> > > > {
> > > > - vm->kvm_fd = open(KVM_DEV_PATH, perm);
> > >
> > > I don't think we should change this one, otherwise the user provided
> > > perms are ignored.
> >
> > Good catch. I don't see any reason to exclude this case, but we do need
> > to pass `perm` down to open_kvm_dev_path_or_exit().
> >
>
> I've reviewed v2 and gave it an r-b, since I don't have overly strong
> opinion about this, but I actually liked that open_kvm_dev_path_or_exit()
> didn't take any arguments. To handle this case I would have either left
> it open coded, like it was, or created something like
>
> int _open_kvm_dev_path_or_exit(int flags)
> {
> int fd = open(KVM_DEV_PATH, flags);
> if (fd < 0)
> ...exit skip...
> return fd;
> }
>
> int open_kvm_dev_path_or_exit(void)
> {
> return _open_kvm_dev_path_or_exit(O_RDONLY);
> }
I agree so long as hiding O_RDONLY does not decrease code readability.
I could not find any place in KVM where the R/W permissions on
/dev/kvm played a role, so I'm inclined to agree it's better to hide
the permissions.
I'll send another version with this change along with the comment fix.
Thanks.
>
> Thanks,
> drew
>
