lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20231009063735-mutt-send-email-mst@kernel.org>
Date: Mon, 9 Oct 2023 06:52:32 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Liming Wu <liming.wu@...uarmicro.com>
Cc: Jason Wang <jasowang@...hat.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"virtualization@...ts.linux-foundation.org" <virtualization@...ts.linux-foundation.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"398776277@...com" <398776277@...com>
Subject: Re: [PATCH 2/2] tools/virtio: Add hints when module is not installed

On Mon, Oct 09, 2023 at 02:44:55AM +0000, Liming Wu wrote:
> 
> 
> > -----Original Message-----
> > From: Jason Wang <jasowang@...hat.com>
> > Sent: Sunday, October 8, 2023 12:36 PM
> > To: Liming Wu <liming.wu@...uarmicro.com>
> > Cc: Michael S . Tsirkin <mst@...hat.com>; kvm@...r.kernel.org;
> > virtualization@...ts.linux-foundation.org; netdev@...r.kernel.org; linux-
> > kernel@...r.kernel.org; 398776277@...com
> > Subject: Re: [PATCH 2/2] tools/virtio: Add hints when module is not installed
> > 
> > On Tue, Sep 26, 2023 at 1:00 PM <liming.wu@...uarmicro.com> wrote:
> > >
> > > From: Liming Wu <liming.wu@...uarmicro.com>
> > >
> > > Need to insmod vhost_test.ko before run virtio_test.
> > > Give some hints to users.
> > >
> > > Signed-off-by: Liming Wu <liming.wu@...uarmicro.com>
> > > ---
> > >  tools/virtio/virtio_test.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
> > > index 028f54e6854a..ce2c4d93d735 100644
> > > --- a/tools/virtio/virtio_test.c
> > > +++ b/tools/virtio/virtio_test.c
> > > @@ -135,6 +135,10 @@ static void vdev_info_init(struct vdev_info* dev,
> > unsigned long long features)
> > >         dev->buf = malloc(dev->buf_size);
> > >         assert(dev->buf);
> > >         dev->control = open("/dev/vhost-test", O_RDWR);
> > > +
> > > +       if (dev->control < 0)
> > > +               fprintf(stderr, "Install vhost_test module" \
> > > +               "(./vhost_test/vhost_test.ko) firstly\n");
> > 
> > There should be many other reasons to fail for open().
> > 
> > Let's use strerror()?
> Yes,  Thanks for the review. 
> Please rechecked the code as follow:
> --- a/tools/virtio/virtio_test.c
> +++ b/tools/virtio/virtio_test.c
> @@ -135,6 +135,11 @@ static void vdev_info_init(struct vdev_info* dev, unsigned long long features)
>         dev->buf = malloc(dev->buf_size);
>         assert(dev->buf);
>         dev->control = open("/dev/vhost-test", O_RDWR);
> +
> +       if (dev->control == NULL)


???
Why are you comparing a file descriptor to NULL?

> +               fprintf(stderr,
> +                       "%s: Check whether vhost_test.ko is installed.\n",
> +                       strerror(errno));


No, do not suggest checking unconditionally this is just wasting user's
time.  You would have to check the exact errno value. You will either
get ENOENT or ENODEV if module is not loaded. Other errors indicate
other problems.  And what matters is whether it's loaded, not installed
- vhost_test.ko will not get auto-loaded even if installed.


>         assert(dev->control >= 0);
>         r = ioctl(dev->control, VHOST_SET_OWNER, NULL);
>         assert(r >= 0);
>  
> Thanks
> 

In short, I am not applying this patch. If you really want to make
things a bit easier in case of errors, replace all assert r >= 0 with
a macro that prints out strerror(errno), that should be enough.
Maybe print file/line number too while we are at it.

-- 
MST


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ