| Message ID | 20181012002217.2864-12-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | chardev: Convert IO handlers to use unsigned type | expand |
On 12/10/2018 02:22, Philippe Mathieu-Daudé wrote: > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > chardev/char-fd.c | 2 +- > include/chardev/char-fd.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/chardev/char-fd.c b/chardev/char-fd.c > index bb426fa4b1..900da2f935 100644 > --- a/chardev/char-fd.c > +++ b/chardev/char-fd.c > @@ -43,7 +43,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque) > { > Chardev *chr = CHARDEV(opaque); > FDChardev *s = FD_CHARDEV(opaque); > - int len; > + size_t len; > uint8_t buf[CHR_READ_BUF_LEN]; > ssize_t ret; > > diff --git a/include/chardev/char-fd.h b/include/chardev/char-fd.h > index e7c2b176f9..36c6b89cee 100644 > --- a/include/chardev/char-fd.h > +++ b/include/chardev/char-fd.h > @@ -31,7 +31,7 @@ typedef struct FDChardev { > Chardev parent; > > QIOChannel *ioc_in, *ioc_out; > - int max_size; > + size_t max_size; > } FDChardev; > > #define TYPE_CHARDEV_FD "chardev-fd" > This shouldn't be just for max_size, it should be for all variables that are set in the *_read_poll functions (those that you touch in patch 3). These variables are than used very little, basically only in a len = MAX(s->max_size, sizeof(buf)) statement, so this switch is safe. However, the order of the patches should be first 4, then this one (the assertion shows that the switch to unsigned is safe), then 5-6-9-10, then 7-8. If you convert implementations before users, the users could in principle overflow "int" when passing an arguments or storing its value. All this of course should be documented in commit messages, which are a bit... scant in this series. :) I'm usually okay with very short commit messages when the changes are spread across many commits (in that case, I usually document what all the repetitive changes are in the patches before and/or after those changes), but in this case you are leaving out completely the "why" for the changes, and that's not really a good idea. Finally, can you please include a patch to adjust the assertions in the USB smartcard code, as mentioned in my original reply to Prasad? Thanks, Paolo
diff --git a/chardev/char-fd.c b/chardev/char-fd.c index bb426fa4b1..900da2f935 100644 --- a/chardev/char-fd.c +++ b/chardev/char-fd.c @@ -43,7 +43,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque) { Chardev *chr = CHARDEV(opaque); FDChardev *s = FD_CHARDEV(opaque); - int len; + size_t len; uint8_t buf[CHR_READ_BUF_LEN]; ssize_t ret; diff --git a/include/chardev/char-fd.h b/include/chardev/char-fd.h index e7c2b176f9..36c6b89cee 100644 --- a/include/chardev/char-fd.h +++ b/include/chardev/char-fd.h @@ -31,7 +31,7 @@ typedef struct FDChardev { Chardev parent; QIOChannel *ioc_in, *ioc_out; - int max_size; + size_t max_size; } FDChardev; #define TYPE_CHARDEV_FD "chardev-fd"
Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- chardev/char-fd.c | 2 +- include/chardev/char-fd.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)