On Wed, 19 Apr 2023 16:10:13 +0100 Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx> wrote: Hi Jean-Philippe, thanks for having a look! > On Wed, Apr 19, 2023 at 02:58:32PM +0100, Jean-Philippe Brucker wrote: > > On Thu, Apr 13, 2023 at 05:57:55PM +0100, Andre Przywara wrote: > > > I am not sure we now really need patch 2 anymore (originally I had this > > > one before I switched to /dev/urandom). I *think* even a read from > > > /dev/urandom can return early (because of a signal, for instance), so > > > a return with 0 bytes read seems possible. > > > > Given that this should be very rare, maybe a simple loop would be better > > than switching the blocking mode? It's certainly a good idea to apply the > > "MUST" requirements from virtio. So originally I had this patch 2/2 on its own, still using /dev/random. And there a read() on the O_NONBLOCKed fd would return -EAGAIN immediately for the next 30 seconds straight, so doing this in a loop sounds very wrong. After all blocking fd's are there to solve exactly that problem. But indeed with /dev/urandom being much nicer to us already, and with the below mentioned special behaviour, just a simple second try (no loop) is sufficient. > Digging a bit more, the manpage [1] is helpful: > > The O_NONBLOCK flag has no effect when opening /dev/urandom. > When calling read(2) for the device /dev/urandom, reads of up to > 256 bytes will return as many bytes as are requested and will not > be interrupted by a signal handler. Reads with a buffer over > this limit may return less than the requested number of bytes or > fail with the error EINTR, if interrupted by a signal handler. Right, I saw references to that behaviour on the Internet(TM), but missed the manpage stanza. It still feels a bit awkward since this seems to rely on some Linux implementation detail, but that's certainly fine for kvmtool (being Linux only anyway). > So I guess you can also drop the O_NONBLOCK flag in patch 1. And for the > second one, maybe we could fallback to a 256 bytes read if the first one > fails Yes, that's certainly better and simplifies that patch. Thanks for digging this out! Cheers, Andre > > [1] https://man7.org/linux/man-pages/man4/urandom.4.html >
