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 1 commit
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
95 changes: 93 additions & 2 deletions hfile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,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 +615,47 @@ static hFILE *hopen_fd(const char *filename, const char *mode)
return NULL;
}

static hFILE *hpreload_fd(const char *filename, const char *mode)
{
if(!strchr(mode, 'r'))
Copy link
Copy Markdown
Contributor

@valeriuo valeriuo Sep 12, 2017

Choose a reason for hiding this comment

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

You should check mode value. If NULL, strchr will seg fault.

{
return NULL;
}

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

fseek(file, 0, SEEK_END);
Copy link
Copy Markdown
Contributor

@jkbonfield jkbonfield Sep 11, 2017

Choose a reason for hiding this comment

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

You need to check the return value of fseek, incase someone has given a non-seekable stream to this function. (Eg /dev/stdin)

int len = ftell(file);
fseek(file, 0, SEEK_SET);

char* buffer = malloc(len);
if(buffer == NULL)
{
errno = ENOMEM;
Copy link
Copy Markdown
Contributor

@jkbonfield jkbonfield Sep 11, 2017

Choose a reason for hiding this comment

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

Manually setting errno isn't recommended. Malloc will do this itself, as would the fread below. Indeed we're obscuring the fread return as it may have other more meaningful errno values.

goto error;
}
if(fread(buffer, 1, len, file) != len)
{
errno = EIO;
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 @@ -821,6 +862,41 @@ static int init_add_plugin(void *obj, int (*init)(struct hFILE_plugin *),
return 0;
}

hFILE *hopenv_mem(const char *filename, const char *mode, va_list args)
Copy link
Copy Markdown
Contributor

@jkbonfield jkbonfield Sep 11, 2017

Choose a reason for hiding this comment

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

I'm wondering if the existing hopen_mem, which is strictly URL based rather than generic memory based, should be reimplemented in terms of the generic hopenv_mem.

Copy link
Copy Markdown
Contributor Author

@ThomasHickman ThomasHickman Sep 12, 2017

Choose a reason for hiding this comment

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

Looks like a good idea, though in hopenv_mem there isn't (and I don't think it would be a good idea to) a way to specify a different buffer size from the length of the buffer that is filled - this is needed in hopen_mem. I think it would be a good to create a function create_hfile_mem, which would do the functionality in common with both functions (which I have implemented below)

{
char* buffer = va_arg(args, char*);
size_t sz = va_arg(args, size_t);
va_end(args);

hFILE_mem *fp = (hFILE_mem *) hfile_init_fixed(sizeof(hFILE_mem), mode, buffer, sz, sz);

fp->base.backend = &mem_backend;

return &fp->base;
}

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;
}

static void load_hfile_plugins()
{
static const struct hFILE_scheme_handler
Expand All @@ -833,6 +909,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 +956,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
32 changes: 32 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,38 @@ 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);
if (hclose(fin) != 0) fail("hclose mem for reading");

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 (hclose(fin) != 0) fail("hclose mem for writing");

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