| Message ID | 20190220010232.18731-24-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | chardev: Convert qemu_chr_write() to take a size_t argument | expand |
Hi On Wed, Feb 20, 2019 at 2:08 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > A througfull audit show that all time data is added to outbuf[], througfull? :) thorough? > 'outlen' is incremented. Then at creation and each time > continue_send() returns it pass thru check_reset which resets > 'outpos', thus we always have 'outlen >= outpos'. > Also due to the check on entry, we know outlen != 0. > We can then add an assertion on 'outlen > outpos', which will hmm, I think you could assert 'outlen >= outpos' only. I don't buy your argument about 'outlen > outpos' (because outlen != 0?) > helps the next patch to safely convert 'outlen - outpos' as an > unsigned type (size_t). > > Make this assertion explicit by casting 'outlen - outpos' size_t. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/ipmi/ipmi_bmc_extern.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c > index bf0b7ee0f5..ca61b04942 100644 > --- a/hw/ipmi/ipmi_bmc_extern.c > +++ b/hw/ipmi/ipmi_bmc_extern.c > @@ -107,8 +107,9 @@ static void continue_send(IPMIBmcExtern *ibe) > goto check_reset; > } > send: > + assert(ibe->outlen > ibe->outpos); > ret = qemu_chr_fe_write(&ibe->chr, ibe->outbuf + ibe->outpos, > - ibe->outlen - ibe->outpos); > + (size_t)(ibe->outlen - ibe->outpos)); > if (ret > 0) { > ibe->outpos += ret; > } > -- > 2.20.1 >
On Wed, Feb 20, 2019 at 02:02:30AM +0100, Philippe Mathieu-Daudé wrote: > A througfull audit show that all time data is added to outbuf[], > 'outlen' is incremented. Then at creation and each time > continue_send() returns it pass thru check_reset which resets > 'outpos', thus we always have 'outlen >= outpos'. Perhaps: "A thorough audit shows that outlen is always incremented when data is always added to outbuf[]. Then at creation and each time continus_send() returns it assures if outpos reaches outlen, both values are reset to zero, except in the case of sending a reset where a new command is added." This is certainly the design intent, thank you for the thorough audit. > Also due to the check on entry, we know outlen != 0. > We can then add an assertion on 'outlen > outpos', which will > helps the next patch to safely convert 'outlen - outpos' as an I was a little confused by "next patch", there is no following patch in the series for this. Maybe "next part of the patch"? > unsigned type (size_t). > > Make this assertion explicit by casting 'outlen - outpos' size_t. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Outside of the minor grammer issues, this looks good. I have noticed the inconsistent signed/unsigned usage in qemu and IMHO it's likely to lead to very bad bugs at some point. There have been studies that show that unsigned values tend to be more buggy in usage due to underflows, but for a length value that will eventually be converted to an unsigned value, what is here is better, I think. Both outpos and outlen are unsigned, so the size_t() cast is not really necessary, but I guess it makes it clear. Reviewed-by: Corey Minyard <cminyard@mvista.com> > --- > hw/ipmi/ipmi_bmc_extern.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c > index bf0b7ee0f5..ca61b04942 100644 > --- a/hw/ipmi/ipmi_bmc_extern.c > +++ b/hw/ipmi/ipmi_bmc_extern.c > @@ -107,8 +107,9 @@ static void continue_send(IPMIBmcExtern *ibe) > goto check_reset; > } > send: > + assert(ibe->outlen > ibe->outpos); > ret = qemu_chr_fe_write(&ibe->chr, ibe->outbuf + ibe->outpos, > - ibe->outlen - ibe->outpos); > + (size_t)(ibe->outlen - ibe->outpos)); > if (ret > 0) { > ibe->outpos += ret; > } > -- > 2.20.1 >
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c index bf0b7ee0f5..ca61b04942 100644 --- a/hw/ipmi/ipmi_bmc_extern.c +++ b/hw/ipmi/ipmi_bmc_extern.c @@ -107,8 +107,9 @@ static void continue_send(IPMIBmcExtern *ibe) goto check_reset; } send: + assert(ibe->outlen > ibe->outpos); ret = qemu_chr_fe_write(&ibe->chr, ibe->outbuf + ibe->outpos, - ibe->outlen - ibe->outpos); + (size_t)(ibe->outlen - ibe->outpos)); if (ret > 0) { ibe->outpos += ret; }
A througfull audit show that all time data is added to outbuf[], 'outlen' is incremented. Then at creation and each time continue_send() returns it pass thru check_reset which resets 'outpos', thus we always have 'outlen >= outpos'. Also due to the check on entry, we know outlen != 0. We can then add an assertion on 'outlen > outpos', which will helps the next patch to safely convert 'outlen - outpos' as an unsigned type (size_t). Make this assertion explicit by casting 'outlen - outpos' size_t. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/ipmi/ipmi_bmc_extern.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)