[haiku-commits] Re: BRANCH pdziepak-github.nfs4 - in src/add-ons/kernel/file_systems/nfs4: . idmapper
- From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Wed, 04 Jul 2012 16:06:50 +0200
Hi Paweł,
some nitpicking ahead, but I spotted a few problems as well. I did not
read the complete diff, though.
On 03.07.2012 22:49, pdziepak-github.nfs4 wrote:
+static status_t
+sParsePath(RequestBuilder& req, uint32* count, const char* _path)
Only variables get the 's' prefix, functions don't.
+{
+ char* path = strdup(_path);
You might want to return B_NO_MEMORY in case this fails; otherwise the
error is ignored.
+ (*count)++;
Why not use a reference instead?
+status_t
+FileInfo::UpdateFileHandles(Filesystem* fs)
AFAIK the word "filesystem" does not exist. Consequently, the class
should be called FileSystem instead.
+ sParsePath(req, &lookupCount, fs->Path());
+ sParsePath(req, &lookupCount, fPath);
Once it can fail, it probably makes sense to test the error code :-)
+ if (reply.LookUpUp() == B_ENTRY_NOT_FOUND) {
+ fParent = fHandle;
+ return B_OK;
+ } else
+ return reply.GetFH(&fParent);
The 'else' is superfluous, and should be avoided in this case.
struct FilesystemId {
We usually shorten it to ID, AFAICT.
+ if (code != MsgReply)
+ return 0;
+ else
+ return value;
Another case of a superfluous else (there are a couple more in this diff).
+status_t
+UIDToName(void* buffer)
+{
+ uid_t userId = *reinterpret_cast<uid_t*>(buffer);
+
+ const char* fullName = kNobodyName;
+
+ struct passwd* userInfo = getpwuid(userId);
+ if (userInfo != NULL) {
+ const char* name = userInfo->pw_name;
+ fullName = AddDomain(name);
+ }
+
+ status_t result = write_port(gReplyPort, MsgReply, fullName,
+ strlen(fullName) + 1);
+ free(const_cast<char*>(fullName));
Skimming over the code, this one doesn't look right: in the case
getpwuid() fails, you'll try to free a constant here.
+ gRequestPort = find_port(kRequestPortName);
+ if (gRequestPort == B_NAME_NOT_FOUND) {
+ fprintf(stderr, "%s\n", strerror(gRequestPort));
+ return gRequestPort;
+ }
+
+ gReplyPort = find_port(kReplyPortName);
+ if (gReplyPort == B_NAME_NOT_FOUND) {
+ fprintf(stderr, "%s\n", strerror(gReplyPort));
+ return gReplyPort;
+ }
You should never test for a specific error, just test < 0 here.
Bye,
Axel.
Other related posts:
