| Message ID | 20170901180340.30009-9-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Preliminary libqtest cleanups | expand |
On 01.09.2017 20:03, Eric Blake wrote: > Rather than make multiple callers call strlen(), it's easier if > socket_send() itself can compute a length via strlen() if none > was provided (caller passes -1). Callers that can get at the > length more efficiently are left that way. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/libqtest.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) I have to say that I don't like this idea very much. socket_send() should IMHO not know about the type of the data that should be sent, i.e. it should not assume that the content is a zero-terminated string. This also could lead to some hard to detect bugs later in case somebody is calling the function like this: size = someotherfunction(); socket_send(fd, buf, size); ... and the someotherfunction() returned a negative error code instead of a correct size. So I'd like to suggest to simply drop this patch. Thomas
Thomas Huth <thuth@redhat.com> writes: > On 01.09.2017 20:03, Eric Blake wrote: >> Rather than make multiple callers call strlen(), it's easier if >> socket_send() itself can compute a length via strlen() if none >> was provided (caller passes -1). Callers that can get at the >> length more efficiently are left that way. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> tests/libqtest.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) > > I have to say that I don't like this idea very much. socket_send() > should IMHO not know about the type of the data that should be sent, > i.e. it should not assume that the content is a zero-terminated string. I agree. > This also could lead to some hard to detect bugs later in case somebody > is calling the function like this: > > size = someotherfunction(); > socket_send(fd, buf, size); > > ... and the someotherfunction() returned a negative error code instead > of a correct size. > > So I'd like to suggest to simply drop this patch. A separate wrapper function for sending zero-terminated strings would be fine with me.
On 09/05/2017 04:54 AM, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 01.09.2017 20:03, Eric Blake wrote: >>> Rather than make multiple callers call strlen(), it's easier if >>> socket_send() itself can compute a length via strlen() if none >>> was provided (caller passes -1). Callers that can get at the >>> length more efficiently are left that way. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- >>> tests/libqtest.c | 10 ++++++---- >>> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> I have to say that I don't like this idea very much. socket_send() >> should IMHO not know about the type of the data that should be sent, >> i.e. it should not assume that the content is a zero-terminated string. > > I agree. It doesn't assume that the content is zero-terminated unless you pass a negative length. > >> This also could lead to some hard to detect bugs later in case somebody >> is calling the function like this: >> >> size = someotherfunction(); >> socket_send(fd, buf, size); >> >> ... and the someotherfunction() returned a negative error code instead >> of a correct size. >> >> So I'd like to suggest to simply drop this patch. > > A separate wrapper function for sending zero-terminated strings would be > fine with me. I'm fine dropping the patch; computing the length in the callers is not that much more onerous (there aren't that many), so I don't think another wrapper is needed.
diff --git a/tests/libqtest.c b/tests/libqtest.c index 3f956f09fc..a6ce21d7f9 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -251,10 +251,13 @@ void qtest_quit(QTestState *s) g_free(s); } -static void socket_send(int fd, const char *buf, size_t size) +static void socket_send(int fd, const char *buf, ssize_t size) { size_t offset; + if (size < 0) { + size = strlen(buf); + } offset = 0; while (offset < size) { ssize_t len; @@ -274,9 +277,8 @@ static void socket_send(int fd, const char *buf, size_t size) static void socket_sendf(int fd, const char *fmt, va_list ap) { gchar *str = g_strdup_vprintf(fmt, ap); - size_t size = strlen(str); - socket_send(fd, str, size); + socket_send(fd, str, -1); g_free(str); } @@ -858,7 +860,7 @@ void qtest_bufwrite(QTestState *s, uint64_t addr, const void *data, size_t size) bdata = g_base64_encode(data, size); qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx ", addr, size); - socket_send(s->fd, bdata, strlen(bdata)); + socket_send(s->fd, bdata, -1); socket_send(s->fd, "\n", 1); qtest_rsp(s, 0); g_free(bdata);
Rather than make multiple callers call strlen(), it's easier if socket_send() itself can compute a length via strlen() if none was provided (caller passes -1). Callers that can get at the length more efficiently are left that way. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/libqtest.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)