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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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–246

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 3214

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)];
char* data_path = (char*)malloc(nodelen + sizeof(PACKSUFFIX)); char data_path[nodelen + sizeof(PACKSUFFIX)];
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!
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;
} }