Skip to content

Add in-memory I/O using hFILE fixed buffers #590

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.

Sign up for GitHub

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

Closed
ThomasHickman wants to merge 9 commits into from
Closed

Add in-memory I/O using hFILE fixed buffers #590

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
80bb84f
Add in-memory I/O
ThomasHickman Sep 11, 2017
58fb979
Add better error handling in hpreload_fd
ThomasHickman Sep 12, 2017
29a6789
Add create_hfile_mem
ThomasHickman Sep 12, 2017
5cd9e07
Move mem backend code to the correct section
ThomasHickman Sep 12, 2017
3972015
Fix issues with mem hfile
ThomasHickman Sep 12, 2017
3d11377
Free hFILE buffer when erroring
ThomasHickman Sep 13, 2017
3656732
Stop freeing hfile mem buffers in hclose
ThomasHickman Sep 22, 2017
fde4dcf
Fixes to PR#590.
jkbonfield Dec 7, 2017
5fa67e9
Create preload scheme
ThomasHickman Dec 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 107 additions & 8 deletions hfile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,12 @@ hFILE *hfile_init_fixed(size_t struct_size, const char *mode,
return fp;
}

static const struct hFILE_backend mem_backend;

void hfile_destroy(hFILE *fp)
{
int save = errno;
if (fp) free(fp->buffer);
if (fp && fp->backend != &mem_backend) free(fp->buffer);
Copy link
Copy Markdown
Contributor

@jkbonfield jkbonfield Dec 6, 2017

Choose a reason for hiding this comment

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

This extra check means data: hopens now leak memory. Why are we not freeing here?

Changing this back makes test/hfile pass valgrind leak checks provided we also remove the now redundant free(internal_buf) calls.

I think it is probably fine to accept that obtaining the internal buffer doesn't equate to now also owning this buffer. If we wish to have the concept of taking ownership (and hence freeing responsibilities) we'd need an additional flag somewhere.

free(fp);
errno = save;
}
Expand Down Expand Up @@ -404,7 +406,7 @@ off_t hseek(hFILE *fp, off_t offset, int whence)
{
off_t curpos, pos;

if (writebuffer_is_nonempty(fp)) {
if (writebuffer_is_nonempty(fp) && fp->mobile) {
int ret = flush_buffer(fp);
if (ret < 0) return ret;
}
Expand Down Expand Up @@ -615,6 +617,39 @@ static hFILE *hopen_fd(const char *filename, const char *mode)
return NULL;
}

static hFILE *hpreload_fd(const char *filename, const char *mode)
{
if(mode == NULL || !strchr(mode, 'r'))
{
return NULL;
}

hFILE_fd *fp = NULL;
FILE *file = fopen(filename, mode);
if (!file) goto error;

if(fseek(file, 0, SEEK_END) != 0) goto error;
int len = ftell(file);
fseek(file, 0, SEEK_SET);

char* buffer = malloc(len);
if(buffer == NULL) goto error;
if(fread(buffer, 1, len, file) != len) goto error;

fp = (hFILE_fd *) hfile_init_fixed(sizeof (hFILE_fd), mode, buffer, len, len);
if (fp == NULL) goto error;

fp->fd = fileno(file);
fp->is_socket = 0;
fp->base.backend = &fd_backend;
return &fp->base;

error:
if (file) { int save = errno; (void) fclose(file); errno = save; }
hfile_destroy((hFILE *) fp);
return NULL;
}

hFILE *hdopen(int fd, const char *mode)
{
hFILE_fd *fp = (hFILE_fd*) hfile_init(sizeof (hFILE_fd), mode, blksize(fd));
Expand Down Expand Up @@ -711,6 +746,16 @@ static int cmp_prefix(const char *key, const char *s)
return 0;
}

static hFILE *create_hfile_mem(char* buffer, const char* mode, size_t buf_filled, size_t buf_size)
{
hFILE_mem *fp = (hFILE_mem *) hfile_init_fixed(sizeof(hFILE_mem), mode, buffer, buf_filled, buf_size);
if (fp == NULL)
return NULL;

fp->base.backend = &mem_backend;
return &fp->base;
}

static hFILE *hopen_mem(const char *url, const char *mode)
{
size_t length, size;
Expand All @@ -734,13 +779,52 @@ static hFILE *hopen_mem(const char *url, const char *mode)
if (buffer == NULL) return NULL;
hts_decode_percent(buffer, &length, data);
}
hFILE* hf;

if(!(hf = create_hfile_mem(buffer, mode, length, size))){
free(buffer);
return NULL;
}

hFILE_mem *fp = (hFILE_mem *)
hfile_init_fixed(sizeof (hFILE_mem), mode, buffer, length, size);
if (fp == NULL) { free(buffer); return NULL; }
return hf;
}

fp->base.backend = &mem_backend;
return &fp->base;
hFILE *hopenv_mem(const char *filename, const char *mode, va_list args)
{
char* buffer = va_arg(args, char*);
size_t sz = va_arg(args, size_t);
va_end(args);

hFILE* hf;

if(!(hf = create_hfile_mem(buffer, mode, sz, sz))){
free(buffer);
return NULL;
}

return hf;
}

int hfile_mem_get_buffer(hFILE *file, char **buffer, size_t *length){
if(file->backend != &mem_backend) {
errno = EINVAL;
return -1;
}

*buffer = file->buffer;
*length = file->buffer - file->limit;

return 0;
}

int hfile_plugin_init_mem(struct hFILE_plugin *self)
{
// mem files are declared remote so they work with a tabix index
static const struct hFILE_scheme_handler handler =
{NULL, hfile_always_remote, "mem", 2000 + 50, hopenv_mem};
self->name = "mem";
hfile_add_scheme_handler("mem", &handler);
return 0;
}


Expand Down Expand Up @@ -833,6 +917,7 @@ static void load_hfile_plugins()
hfile_add_scheme_handler("data", &data);
hfile_add_scheme_handler("file", &file);
init_add_plugin(NULL, hfile_plugin_init_net, "knetfile");
init_add_plugin(NULL, hfile_plugin_init_mem, "mem");

#ifdef ENABLE_PLUGINS
struct hts_path_itr path;
Expand Down Expand Up @@ -879,11 +964,25 @@ static hFILE *hopen_unknown_scheme(const char *fname, const char *mode)
return fp;
}

static hFILE *hopenv_unknown_scheme(const char *fname, const char *mode, va_list args)
{
char* method_type = va_arg(args, char*);
va_end(args);
if(!strcmp(method_type, "preload")){
errno = EPROTONOSUPPORT;
return NULL;
}

hFILE *fp = hpreload_fd(fname, mode);
if (fp == NULL && errno == ENOENT) errno = EPROTONOSUPPORT;
Copy link
Copy Markdown
Contributor

@jkbonfield jkbonfield Dec 6, 2017

Choose a reason for hiding this comment

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

I'm unsure why this line exists. hpreload_fd can return NULL/ENOENT for valid reasons, and ENOENT is suitable then I think.

return fp;
}

/* Returns the appropriate handler, or NULL if the string isn't an URL. */
static const struct hFILE_scheme_handler *find_scheme_handler(const char *s)
{
static const struct hFILE_scheme_handler unknown_scheme =
{ hopen_unknown_scheme, hfile_always_local, "built-in", 0 };
{ hopen_unknown_scheme, hfile_always_local, "built-in", 2000 + 50, hopenv_unknown_scheme };

char scheme[12];
int i;
Expand Down
17 changes: 15 additions & 2 deletions htslib/hfile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ hread(hFILE *fp, void *buffer, size_t nbytes)
if (n > nbytes) n = nbytes;
memcpy(buffer, fp->begin, n);
fp->begin += n;
return (n == nbytes)? (ssize_t) n : hread2(fp, buffer, nbytes, n);
return (n == nbytes || !fp->mobile)? (ssize_t) n : hread2(fp, buffer, nbytes, n);
}

/// Write a character to the stream
Expand Down Expand Up @@ -239,7 +239,15 @@ static inline ssize_t HTS_RESULT_USED
hwrite(hFILE *fp, const void *buffer, size_t nbytes)
{
extern ssize_t hwrite2(hFILE *, const void *, size_t, size_t);

extern int hfile_set_blksize(hFILE *fp, size_t bufsiz);

if(!fp->mobile){
if (fp->limit - fp->begin < nbytes){
hfile_set_blksize(fp, fp->limit - fp->buffer + nbytes);
fp->end = fp->limit;
}
}

size_t n = fp->limit - fp->begin;
if (n > nbytes) n = nbytes;
memcpy(fp->begin, buffer, n);
Expand All @@ -254,6 +262,11 @@ This includes low-level flushing such as via `fdatasync(2)`.
*/
int hflush(hFILE *fp) HTS_RESULT_USED;

/// For hfile_mem: get the internal buffer and it's size from a hfile
/** @return 0 if successful, or -1 if an error occurred
*/
int hfile_mem_get_buffer(hFILE *file, char **buffer, size_t *length);

#ifdef __cplusplus
}
#endif
Expand Down
42 changes: 42 additions & 0 deletions test/hfile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,48 @@ int main(void)
if ((c = hgetc(fin)) != EOF) fail("chars: hgetc (EOF) returned %d", c);
if (hclose(fin) != 0) fail("hclose(test/hfile_chars.tmp) for reading");

fin = hopen("test/hfile_chars.tmp", "r:", "preload");
if (fin == NULL) fail("preloading hopen(\"test/hfile_chars.tmp\") for reading");
for (i = 0; i < 256; i++)
if ((c = hgetc(fin)) != i)
fail("preloading chars: hgetc (%d = 0x%x) returned %d = 0x%x", i, i, c, c);
if ((c = hgetc(fin)) != EOF) fail("preloading chars: hgetc (EOF) returned %d", c);
if (hclose(fin) != 0) fail("preloading hclose(test/hfile_chars.tmp) for reading");

char* test_string = strdup("Test string");
fin = hopen("mem:", "r:", test_string, 12);
if (fin == NULL) fail("hopen(\"mem:\", \"r:\", ...)");
if (hread(fin, buffer, 12) != 12)
fail("hopen('mem:', 'r') failed read");
if(strcmp(buffer, test_string) != 0)
fail("hopen('mem:', 'r') missread '%s' != '%s'", buffer, test_string);
char* internal_buf;
size_t interval_buf_len;
if(hfile_mem_get_buffer(fin, &internal_buf, &interval_buf_len) != 0){
fail("hopen('mem:', 'r') failed to get internal buffer");
}
if (hclose(fin) != 0) fail("hclose mem for reading");
free(internal_buf);

test_string = strdup("Test string");
fin = hopen("mem:", "wr:", test_string, 12);
if (fin == NULL) fail("hopen(\"mem:\", \"w:\", ...)");
if (hseek(fin, -1, SEEK_END) < 0)
fail("hopen('mem:', 'wr') failed seek");
if (hwrite(fin, strdup(" extra"), 7) != 7)
fail("hopen('mem:', 'wr') failed write");
if (hseek(fin, 0, SEEK_SET) < 0)
fail("hopen('mem:', 'wr') failed seek");
if (hread(fin, buffer, 18) != 18)
fail("hopen('mem:', 'wr') failed read");
if (strcmp(buffer, "Test string extra") != 0)
fail("hopen('mem:', 'wr') misswrote '%s' != '%s'", buffer, "Test string extra");
if(hfile_mem_get_buffer(fin, &internal_buf, &interval_buf_len) != 0){
fail("hopen('mem:', 'wr') failed to get internal buffer");
}
if (hclose(fin) != 0) fail("hclose mem for writing");
free(internal_buf);

fin = hopen("data:,hello, world!%0A", "r");
if (fin == NULL) fail("hopen(\"data:...\")");
n = hread(fin, buffer, 300);
Expand Down