Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buffer: refactor buffer to be dynamically-sized #53

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
142 changes: 104 additions & 38 deletions src/async.c
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ typedef SSIZE_T ssize_t;
#define MSG_DONTWAIT 0
#endif

/* minimum buffer space available to initialize input data */
#define ASYNC_BUFFER_INIT_SIZE 4096
/* minimum buffer space available to receive input data */
#define ASYNC_BUFFER_MIN_SIZE 256
struct mpd_async {
mpd_socket_t fd;

Expand All @@ -78,7 +82,6 @@ mpd_async_new(int fd)

async->fd = fd;
mpd_error_init(&async->error);

mpd_buffer_init(&async->input);
mpd_buffer_init(&async->output);

Expand All @@ -92,6 +95,8 @@ mpd_async_free(struct mpd_async *async)

mpd_socket_close(async->fd);
mpd_error_deinit(&async->error);
mpd_buffer_deinit(&async->input);
mpd_buffer_deinit(&async->output);
free(async);
}

Expand Down Expand Up @@ -177,19 +182,22 @@ mpd_async_events(const struct mpd_async *async)
static bool
mpd_async_read(struct mpd_async *async)
{
size_t room;
ssize_t nbytes;

assert(async != NULL);
assert(async->fd != MPD_INVALID_SOCKET);
assert(!mpd_error_is_defined(&async->error));

room = mpd_buffer_room(&async->input);
if (room == 0)
return true;
/* make at least ASYNC_BUFFER_MIN_SIZE available for reading */
if (!mpd_buffer_make_room(&async->input, ASYNC_BUFFER_MIN_SIZE)) {
mpd_error_code(&async->error, MPD_ERROR_OOM);
mpd_error_message(&async->error,
"Out of memory for input buffer");
return false;
}

nbytes = recv(async->fd, mpd_buffer_write(&async->input), room,
MSG_DONTWAIT);
nbytes = recv(async->fd, mpd_buffer_write(&async->input),
mpd_buffer_room(&async->input), MSG_DONTWAIT);
if (nbytes < 0) {
/* I/O error */

Expand Down Expand Up @@ -276,39 +284,32 @@ mpd_async_io(struct mpd_async *async, enum mpd_async_event events)
return true;
}

bool
mpd_async_send_command_v(struct mpd_async *async, const char *command,
va_list args)
/* Insert the argument list args into buffer starting from start up to the
* available buffer space. The inserted arguments are quoted, and special
* characters are properly escaped. Because of these operations, the length to
* be written is only known after the fact.
*
* returns false if the current space available in buffer is insufficient or
* true otherwise.
*/
static bool
quote_vargs(struct mpd_buffer *buffer, char *start, char **end_pos,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this function do?
Documentation missing!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 14a9563

va_list args)
{
size_t room, length;
char *dest, *end, *p;
char *end, *p;
const char *arg;

assert(async != NULL);
assert(command != NULL);

if (mpd_error_is_defined(&async->error))
return false;

room = mpd_buffer_room(&async->output);
length = strlen(command);
if (room <= length)
return false;

dest = mpd_buffer_write(&async->output);
/* -1 because we reserve space for the \n character */
end = dest + room - 1;

/* copy the command (no quoting, we asumme it is "clean") */

memcpy(dest, command, length);
p = dest + length;
/* mpd_async_send_command_v() guarantees that mpd_buffer_room() has at
* least 1 position available (length + 1)
*/
assert(mpd_buffer_room(buffer) >= 1);
end = start + mpd_buffer_room(buffer) - 1;
p = start;

/* now append all arguments (quoted) */

while ((arg = va_arg(args, const char *)) != NULL) {
/* append a space separator */

if (p >= end)
return false;

Expand All @@ -317,17 +318,61 @@ mpd_async_send_command_v(struct mpd_async *async, const char *command,
/* quote the argument into the destination buffer */

p = quote(p, end, arg);
assert(p == NULL || (p >= dest && p <= end));
assert(p == NULL || (p >= start && p <= end));
if (p == NULL)
return false;
}


/* append the newline to finish this command */

*p++ = '\n';
*end_pos = p;
return true;
}

mpd_buffer_expand(&async->output, p - dest);
bool
mpd_async_send_command_v(struct mpd_async *async, const char *command,
va_list args)
{
const char *error_msg = "Out of memory for output buffer";
char *end_pos = NULL, *write_p;
bool success = false;
size_t length, newcap;
va_list cargs;

assert(async != NULL);
assert(command != NULL);

if (mpd_error_is_defined(&async->error))
return false;

length = strlen(command);
/* we need a '\n' at the end */
if (!mpd_buffer_make_room(&async->output, length + 1)) {
mpd_error_code(&async->error, MPD_ERROR_OOM);
mpd_error_message(&async->error, error_msg);
return false;
}

/* copy the command (no quoting, we assume it is "clean") */
memcpy(mpd_buffer_write(&async->output), command, length);
mpd_buffer_expand(&async->output, length);

while (!success) {
va_copy(cargs, args);
write_p = mpd_buffer_write(&async->output);
success = quote_vargs(&async->output, write_p, &end_pos, cargs);
va_end(cargs);
if (!success) {
newcap = mpd_buffer_capacity(&async->output) * 2;
if (!mpd_buffer_make_room(&async->output, newcap)) {
mpd_error_code(&async->error, MPD_ERROR_OOM);
mpd_error_message(&async->error, error_msg);
return false;
}
}
}

mpd_buffer_expand(&async->output, end_pos - write_p);
return true;
}

Expand All @@ -347,6 +392,22 @@ mpd_async_send_command(struct mpd_async *async, const char *command, ...)
return success;
}

/* Initialize the buffer if necessary
* Set the error code and message in case of error in mpd_buffer_make_room()
*/
static bool
mpd_async_init_buffer(struct mpd_buffer *buffer,
struct mpd_error_info *error)
{
if (mpd_buffer_capacity(buffer) == 0 &&
!mpd_buffer_make_room(buffer, ASYNC_BUFFER_INIT_SIZE)) {
mpd_error_code(error, MPD_ERROR_OOM);
mpd_error_message(error, "Out of memory for buffer");
return false;
} else
return true;
}

char *
mpd_async_recv_line(struct mpd_async *async)
{
Expand All @@ -355,12 +416,14 @@ mpd_async_recv_line(struct mpd_async *async)

assert(async != NULL);

if (!mpd_async_init_buffer(&async->input, &async->error))
return NULL;

size = mpd_buffer_size(&async->input);
if (size == 0)
return NULL;

src = mpd_buffer_read(&async->input);
assert(src != NULL);
newline = memchr(src, '\n', size);
if (newline == NULL) {
/* line is not finished yet */
Expand All @@ -371,7 +434,6 @@ mpd_async_recv_line(struct mpd_async *async)
mpd_error_message(&async->error,
"Response line too large");
}

return NULL;
}

Expand All @@ -385,6 +447,10 @@ size_t
mpd_async_recv_raw(struct mpd_async *async, void *dest, size_t length)
{
size_t max_size = mpd_buffer_size(&async->input);

if (!mpd_async_init_buffer(&async->input, &async->error))
return 0;

if (max_size == 0)
return 0;

Expand Down
69 changes: 63 additions & 6 deletions src/buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@
#include <assert.h>
#include <string.h>
#include <stdbool.h>
#include <stdlib.h>

/**
* A fixed 4kB buffer which can be appended at the end, and consumed
* at the beginning.
* A buffer which can be appended at the end, and consumed at the beginning.
*/
struct mpd_buffer {
/** the next buffer position to write to */
Expand All @@ -45,7 +45,10 @@ struct mpd_buffer {
unsigned read;

/** the actual buffer */
unsigned char data[4096];
unsigned char *data;

/** capacity of buffer */
size_t capacity;
};

/**
Expand All @@ -56,6 +59,26 @@ mpd_buffer_init(struct mpd_buffer *buffer)
{
buffer->read = 0;
buffer->write = 0;
buffer->data = NULL;
buffer->capacity = 0;
}

/**
* Free the buffer area.
*/
static inline void
mpd_buffer_deinit(struct mpd_buffer *buffer)
{
free(buffer->data);
}

/**
* Return the capacity of the buffer.
*/
static inline size_t
mpd_buffer_capacity(struct mpd_buffer *buffer)
{
return buffer->capacity;
}

/**
Expand All @@ -79,12 +102,13 @@ mpd_buffer_move(struct mpd_buffer *buffer)
static inline size_t
mpd_buffer_room(const struct mpd_buffer *buffer)
{
assert(buffer->write <= sizeof(buffer->data));
assert(buffer->write <= buffer->capacity);
assert(buffer->read <= buffer->write);

return sizeof(buffer->data) - (buffer->write - buffer->read);
return buffer->capacity - (buffer->write - buffer->read);
}


/**
* Checks if the buffer is full, i.e. nothing can be written.
*/
Expand Down Expand Up @@ -125,7 +149,7 @@ mpd_buffer_expand(struct mpd_buffer *buffer, size_t nbytes)
static inline size_t
mpd_buffer_size(const struct mpd_buffer *buffer)
{
assert(buffer->write <= sizeof(buffer->data));
assert(buffer->write <= buffer->capacity);
assert(buffer->read <= buffer->write);

return buffer->write - buffer->read;
Expand Down Expand Up @@ -154,4 +178,37 @@ mpd_buffer_consume(struct mpd_buffer *buffer, size_t nbytes)
buffer->read += nbytes;
}

/**
* Makes at least min_avail_len bytes available for reading/writing data.
* In other words, mpd_buffer_room() will be >= min_avail_len if called after
* this function.
*/
static inline bool
mpd_buffer_make_room(struct mpd_buffer *buffer, size_t min_avail_len)
{
size_t newcap;

if (mpd_buffer_room(buffer) >= min_avail_len)
return true;

if (mpd_buffer_capacity(buffer) == 0)
newcap = min_avail_len;
else {
newcap = buffer->capacity * 2;
while (newcap < min_avail_len)
newcap *= 2;
}

/* empty buffer */
if (mpd_buffer_room(buffer) == mpd_buffer_capacity(buffer)) {
mpd_buffer_deinit(buffer);
buffer->data = malloc(newcap);
} else
buffer->data = realloc(buffer->data, newcap);

if (buffer->data != NULL)
buffer->capacity = newcap;
return buffer->data != NULL;
}

#endif
22 changes: 1 addition & 21 deletions src/sync.c
Original file line number Diff line number Diff line change
Expand Up @@ -100,27 +100,7 @@ bool
mpd_sync_send_command_v(struct mpd_async *async, const struct timeval *tv0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: struct timeval is not used here because `mpd_sync_io' is not called anymore;
i have left it since all other functions of mpd_sync receive the timeval as well.
If you prefer that i remove it, let me know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an internal API, we can remove the parameter easily.
But do we want this? Does this function really need dynamic buffer sizes? What is your reason to refactor the output buffer to be dynamic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactoring is to allow libmpdclient to be limited in its message size only by MPD. If i'm right, currently MPD supports up to 8KiB messages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't answer my question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasons were: (i) At least one user would like to send messages > 4KiB (issue #51), and (ii) this code would also adapt in case MPD changes the limit of received messages.
You could also just bump the data size to 8KiB and call it a day (that is possible with that other PR, although, today, the limit is still 4KiB)
Since with this patch we can make room for the message, i don't see the need anymore for mpd_sync_io; so i would remove the tv0 parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#51 does not request being able to send large requests. He complains about the lousy error handling if he tries to send a huge malformed request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the debian bug report:
"Ideally, libmpdclient should support requests of arbitrary size (eventually
reaching the server limit), but if that is not feasible, at least a proper
error reporting would be nice."

I think for the majority of cases 4KiB is more than sufficient; The reason i submitted the other PR is in case the changes i'm proposing here are deemed undesirable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, that's where the Debian bug report deviates from #51, but we were talking about #51.
But even the Debian bug report doesn't explain how such a large request might be useful. Just like #51, the Debian bug report describes a malformed request, and supporting malformed requests is not libmpdclient's goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He also mentions this feature request in #51. The title is about the hard coded limit.

True, the test seems to be about fuzzing, but from the MPD protocol page i didn't find any mention of a limit to the find/search command. In theory, you could concatenate multiple find requests in a single huge line like: ((artist == 'FOO') AND (album == 'BAR')) AND ...

const char *command, va_list args)
{
struct timeval tv, *tvp;
va_list copy;
bool success;

if (tv0 != NULL) {
tv = *tv0;
tvp = &tv;
} else
tvp = NULL;

while (true) {
va_copy(copy, args);
success = mpd_async_send_command_v(async, command, copy);
va_end(copy);

if (success)
return true;

if (!mpd_sync_io(async, tvp))
return false;
}
return mpd_async_send_command_v(async, command, args);
}

bool
Expand Down