Skip to content
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

No scratch for absPath? #9775

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

No scratch for absPath? #9775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 32 additions & 25 deletions src/libutil/file-system.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,36 +21,43 @@ namespace fs = std::filesystem;

namespace nix {

Path absPath(PathView path, std::optional<PathView> dir, bool resolveSymlinks)
#ifdef __GNUC__
std::unique_ptr<char, FreeDeleter> getCwd()
{
std::string scratch;
return std::unique_ptr<char, FreeDeleter> { getcwd(NULL, 0) };
Copy link
Contributor

Choose a reason for hiding this comment

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

is this called so often that it must be really fast and small, so that it is worth spilling this memory management over the code that wants the cwd string?

If no, then please create a function like std::string getCwd() that copies the content and then frees the original string. this way we don't need anyone to know about custom deleters for this purpose.

}
#endif

if (path[0] != '/') {
// In this case we need to call `canonPath` on a newly-created
// string. We set `scratch` to that string first, and then set
// `path` to `scratch`. This ensures the newly-created string
// lives long enough for the call to `canonPath`, and allows us
// to just accept a `std::string_view`.
if (!dir) {

Path absPath(PathView path, std::optional<PathView> dir, bool resolveSymlinks)
{
/* In some branches case we create strings that, as r-values, would
not live long enough. We instead assign them to these variables
so they are only deallocated at the end of this function, after
the call to `canonPath` where they are used. */
std::string scratch0;
std::unique_ptr<char, FreeDeleter> scratch1;
[[maybe_unused]] char buf[PATH_MAX];

return canonPath(
path[0] == '/'
? path
: (scratch0 = concatStrings(
dir
? *dir
:
#ifdef __GNU__
/* GNU (aka. GNU/Hurd) doesn't have any limitation on path
lengths and doesn't define `PATH_MAX'. */
char *buf = getcwd(NULL, 0);
if (buf == NULL)
/* GNU (aka. GNU/Hurd) doesn't have any limitation
on path lengths and doesn't define `PATH_MAX'. */
*(scratch1 = getCwd());
#else
char buf[PATH_MAX];
if (!getcwd(buf, sizeof(buf)))
#endif
throw SysError("cannot get cwd");
scratch = concatStrings(buf, "/", path);
#ifdef __GNU__
free(buf);
getcwd(buf, sizeof(buf))
? buf
: throw SysError("cannot get cwd"),
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we encapsulate these two branches and the whole legacy mem management issue simply into a function std::string getCwd() that does it all automagically (with the performance-question yes/no from earler in mind)?

this seems like an unnecessary dance for a function whose concern is making paths absolute.

#endif
} else
scratch = concatStrings(*dir, "/", path);
path = scratch;
}
return canonPath(path, resolveSymlinks);
"/",
path)),
resolveSymlinks);
Copy link
Contributor

Choose a reason for hiding this comment

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

i see the elegance in point-free programming in general, but the if-branched ersion that we had before was more readable.

}


Expand Down
8 changes: 8 additions & 0 deletions src/libutil/file-system.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "error.hh"
#include "logging.hh"
#include "file-descriptor.hh"
#include "util.hh"

#include <sys/types.h>
#include <sys/stat.h>
Expand Down Expand Up @@ -36,6 +37,13 @@ namespace nix {
struct Sink;
struct Source;

#ifdef __GNUC__
/**
* GNU libc can malloc the path for you with the right length.
*/
std::unique_ptr<char, FreeDeleter> getCwd();
#endif

/**
* @return An absolutized path, resolving paths relative to the
* specified directory, or the current directory otherwise. The path
Expand Down
10 changes: 10 additions & 0 deletions src/libutil/util.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,16 @@ template<class... Ts> overloaded(Ts...) -> overloaded<Ts...>;
std::string showBytes(uint64_t bytes);


/**
* For using `std::unique` with C functions.
*/
struct FreeDeleter
{
template <typename T>
void operator()(T *p) const { std::free(p); }
};


/**
* Provide an addition operator between strings and string_views
* inexplicably omitted from the standard library.
Expand Down
5 changes: 2 additions & 3 deletions src/nix/run.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,14 +259,13 @@ void chrootHelper(int argc, char * * argv)
createSymlink(readLink(src), dst);
}

char * cwd = getcwd(0, 0);
auto cwd = getCwd();
if (!cwd) throw SysError("getting current directory");
Finally freeCwd([&]() { free(cwd); });

if (chroot(tmpDir.c_str()) == -1)
throw SysError("chrooting into '%s'", tmpDir);

if (chdir(cwd) == -1)
if (chdir(&*cwd) == -1)
throw SysError("chdir to '%s' in chroot", cwd);
} else
if (mount(realStoreDir.c_str(), storeDir.c_str(), "", MS_BIND, 0) == -1)
Expand Down
Loading