| Message ID | 20240429141934.442154-1-f.ebner@proxmox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/copy-before-write: use uint64_t for timeout in nanoseconds | expand |
Hi Fiona, On 29/4/24 16:19, Fiona Ebner wrote: Not everybody uses an email client that shows the patch content just after the subject (your first lines wasn't making sense at first). Simply duplicating the subject helps to understand: Use uint64_t for timeout in nanoseconds ... > rather than the uint32_t for which the maximum is slightly more than 4 > seconds and larger values would overflow. The QAPI interface allows > specifying the number of seconds, so only values 0 to 4 are safe right > now, other values lead to a much lower timeout than a user expects. > > The block_copy() call where this is used already takes a uint64_t for > the timeout, so no change required there. > > Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option") > Reported-by: Friedrich Weber <f.weber@proxmox.com> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> > --- > block/copy-before-write.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/copy-before-write.c b/block/copy-before-write.c > index 8aba27a71d..026fa9840f 100644 > --- a/block/copy-before-write.c > +++ b/block/copy-before-write.c > @@ -43,7 +43,7 @@ typedef struct BDRVCopyBeforeWriteState { > BlockCopyState *bcs; > BdrvChild *target; > OnCbwError on_cbw_error; > - uint32_t cbw_timeout_ns; > + uint64_t cbw_timeout_ns; Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Am 29.04.24 um 16:36 schrieb Philippe Mathieu-Daudé: > Hi Fiona, > > On 29/4/24 16:19, Fiona Ebner wrote: > > Not everybody uses an email client that shows the patch content just > after the subject (your first lines wasn't making sense at first). > > Simply duplicating the subject helps to understand: > > Use uint64_t for timeout in nanoseconds ... > Oh, sorry. I'll try to remember that for the future. Should I re-send as a v2? Best Regards, Fiona
On 29.04.24 17:46, Fiona Ebner wrote: > Am 29.04.24 um 16:36 schrieb Philippe Mathieu-Daudé: >> Hi Fiona, >> >> On 29/4/24 16:19, Fiona Ebner wrote: >> >> Not everybody uses an email client that shows the patch content just >> after the subject (your first lines wasn't making sense at first). >> >> Simply duplicating the subject helps to understand: >> >> Use uint64_t for timeout in nanoseconds ... >> > > Oh, sorry. I'll try to remember that for the future. Should I re-send as > a v2? > Not necessary, I can touch up the message when applying to my block branch. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Am 29.04.2024 um 16:19 hat Fiona Ebner geschrieben: > rather than the uint32_t for which the maximum is slightly more than 4 > seconds and larger values would overflow. The QAPI interface allows > specifying the number of seconds, so only values 0 to 4 are safe right > now, other values lead to a much lower timeout than a user expects. > > The block_copy() call where this is used already takes a uint64_t for > the timeout, so no change required there. > > Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option") > Reported-by: Friedrich Weber <f.weber@proxmox.com> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Thanks, applied to the block branch. But I don't think our job is done yet with this. Increasing the limit is good and useful, but even if it's now unlikely to hit with sane values, we should still catch integer overflows in cbw_open() and return an error on too big values instead of silently wrapping around. Kevin
diff --git a/block/copy-before-write.c b/block/copy-before-write.c index 8aba27a71d..026fa9840f 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -43,7 +43,7 @@ typedef struct BDRVCopyBeforeWriteState { BlockCopyState *bcs; BdrvChild *target; OnCbwError on_cbw_error; - uint32_t cbw_timeout_ns; + uint64_t cbw_timeout_ns; /* * @lock: protects access to @access_bitmap, @done_bitmap and
rather than the uint32_t for which the maximum is slightly more than 4 seconds and larger values would overflow. The QAPI interface allows specifying the number of seconds, so only values 0 to 4 are safe right now, other values lead to a much lower timeout than a user expects. The block_copy() call where this is used already takes a uint64_t for the timeout, so no change required there. Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option") Reported-by: Friedrich Weber <f.weber@proxmox.com> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> --- block/copy-before-write.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)