( )⚙ D1125 cdatapack: print path on error

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1125

cdatapack: print path on error
ClosedPublic

Authored by quark on Oct 16 2017, 6:03 PM.
Tags
None
Subscribers
singhsrb

Details

Reviewers
durham
Group Reviewers
Restricted Project
Commits
rFBHGXbf081e811913: cdatapack: print path on error
Summary

This gives us more context about what pack file is problematic. It's useful
to detect issues like mmapping a 0-sized file.

Test Plan

Make sure the code compiles without warnings. Run related tests.

make local
rt test-cstore.t

Make sure setup.py specifies C99 so variable length array is available.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Oct 16 2017, 6:03 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 16 2017, 6:03 PM
singhsrb added a subscriber: singhsrb.Oct 16 2017, 6:55 PM
singhsrb added inline comments.
cstore/py-cdatapack.h
257

Wondering if something like this would be cleaner:

int ret_code = -1;
if (self->handle == NULL) {
  PyErr_NoMemory();
} else {
  switch (self->handle->status) {
    case DATAPACK_HANDLE_OK:
      ret_code = 0;
      break;
    case DATAPACK_HANDLE_VERSION_MISMATCH:
      PyErr_Format(PyExc_RuntimeError, "Unsupported version");
      break;
    default:
      PyErr_Format(
          PyExc_ValueError,
          "Error setting up datapack %s (status=%d)",
          data_path,
          self->handle->status);
  }
}

free(idx_path);
free(data_path);
free(self->handle);
self->handle = NULL;
return ret_code;
quark added inline comments.Oct 16 2017, 7:08 PM
cstore/py-cdatapack.h
257

Yes. I was just a bit lazy. I think we can use C99 variable length array here so it does not need to be freed explicitly.

quark edited the test plan for this revision. (Show Details)Oct 16 2017, 7:33 PM
quark updated this revision to Diff 2848.
quark updated this revision to Diff 2849.
singhsrb added inline comments.Oct 16 2017, 7:38 PM
cstore/py-cdatapack.h
245

That would really clean up a lot of this stuff. Great!

durham accepted this revision.Oct 31 2017, 12:25 PM
This revision is now accepted and ready to land.Oct 31 2017, 12:25 PM
Closed by commit rFBHGXbf081e811913: cdatapack: print path on error (authored by quark). · Explain WhyNov 2 2017, 3:04 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mcstore/py-cdatapack.h (16 lines)

Diff 2849

cstore/py-cdatapack.h

char *node; char *node;
Py_ssize_t nodelen; Py_ssize_t nodelen;
if (!PyArg_ParseTuple(args, "s#", &node, &nodelen)) { if (!PyArg_ParseTuple(args, "s#", &node, &nodelen)) {
return -1; return -1;
} }
char* idx_path = (char*)malloc(nodelen + sizeof(INDEXSUFFIX)); char idx_path[nodelen + sizeof(INDEXSUFFIX)];
singhsrbUnsubmitted
Not Done

That would really clean up a lot of this stuff. Great!

singhsrb: That would really clean up a lot of this stuff. Great!
char* data_path = (char*)malloc(nodelen + sizeof(PACKSUFFIX)); char data_path[nodelen + sizeof(PACKSUFFIX)];
if(idx_path == NULL || data_path == NULL) {
free(data_path);
free(idx_path);
PyErr_NoMemory();
return -1;
}
sprintf(idx_path, "%s%s", node, INDEXSUFFIX); sprintf(idx_path, "%s%s", node, INDEXSUFFIX);
sprintf(data_path, "%s%s", node, PACKSUFFIX); sprintf(data_path, "%s%s", node, PACKSUFFIX);
self->handle = open_datapack( self->handle = open_datapack(
idx_path, strlen(idx_path), idx_path, strlen(idx_path),
data_path, strlen(data_path)); data_path, strlen(data_path));
free(data_path);
free(idx_path);
if (self->handle == NULL) { if (self->handle == NULL) {
PyErr_NoMemory(); PyErr_NoMemory();
return -1; return -1;
singhsrbUnsubmitted
Not Done

Wondering if something like this would be cleaner:

int ret_code = -1;
if (self->handle == NULL) {
  PyErr_NoMemory();
} else {
  switch (self->handle->status) {
    case DATAPACK_HANDLE_OK:
      ret_code = 0;
      break;
    case DATAPACK_HANDLE_VERSION_MISMATCH:
      PyErr_Format(PyExc_RuntimeError, "Unsupported version");
      break;
    default:
      PyErr_Format(
          PyExc_ValueError,
          "Error setting up datapack %s (status=%d)",
          data_path,
          self->handle->status);
  }
}

free(idx_path);
free(data_path);
free(self->handle);
self->handle = NULL;
return ret_code;
singhsrb: Wondering if something like this would be cleaner: ``` int ret_code = -1; if (self->handle…
quarkAuthorUnsubmitted
Not Done

Yes. I was just a bit lazy. I think we can use C99 variable length array here so it does not need to be freed explicitly.

quark: Yes. I was just a bit lazy. I think we can use C99 variable length array here so it does not…
} else if (self->handle->status == DATAPACK_HANDLE_OK) { } else if (self->handle->status == DATAPACK_HANDLE_OK) {
return 0; return 0;
} }
if (self->handle->status == DATAPACK_HANDLE_VERSION_MISMATCH) { if (self->handle->status == DATAPACK_HANDLE_VERSION_MISMATCH) {
PyErr_Format(PyExc_RuntimeError, "Unsupported version"); PyErr_Format(PyExc_RuntimeError, "Unsupported version");
} else if (self->handle->status != DATAPACK_HANDLE_OK) { } else if (self->handle->status != DATAPACK_HANDLE_OK) {
PyErr_Format(PyExc_ValueError, PyErr_Format(PyExc_ValueError,
"Error setting up datapack (status=%d)", self->handle->status); "Error setting up datapack %s (status=%d)",
data_path,
self->handle->status);
} }
free(self->handle); free(self->handle);
self->handle = NULL; self->handle = NULL;
return -1; return -1;
} }