Skip to content

commit tmpfile prep #957

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
cgwalters wants to merge 5 commits into from
Closed

commit tmpfile prep #957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e4e2d3
lib/commit: Clean up commit file type handling variables
cgwalters Jun 23, 2017
99de9c9
lib: Hoist unlinkat() cleanup API to fsutil, use in pull
cgwalters Jun 24, 2017
0c773e9
lib: Use OtTmpFile for static delta processing
cgwalters Jun 24, 2017
0f08bce
fixup! lib: Use OtTmpFile for static delta processing
cgwalters Jun 26, 2017
b2d8253
lib/commit: Fix fallocate size for bare-user symlinks
cgwalters Jun 26, 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
160 changes: 62 additions & 98 deletions src/libostree/ostree-repo-commit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,83 +446,62 @@ ot_fallocate (int fd, goffset size, GError **error)
return TRUE;
}

/* Combines a check for whether or not we already have the object with
* allocating a tempfile if we don't. Used by the static delta code.
*/
gboolean
_ostree_repo_open_content_bare (OstreeRepo *self,
const char *checksum,
guint64 content_len,
OstreeRepoContentBareCommit *out_state,
GOutputStream **out_stream,
OtTmpfile *out_tmpf,
gboolean *out_have_object,
GCancellable *cancellable,
GError **error)
{
gboolean ret = FALSE;
g_autofree char *temp_filename = NULL;
g_autoptr(GOutputStream) ret_stream = NULL;
gboolean have_obj;

if (!_ostree_repo_has_loose_object (self, checksum, OSTREE_OBJECT_TYPE_FILE, &have_obj,
cancellable, error))
goto out;

if (!have_obj)
return FALSE;
/* Do we already have this object? */
*out_have_object = have_obj;
if (have_obj)
{
int fd;

if (!glnx_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC,
&fd, &temp_filename, error))
goto out;

if (!ot_fallocate (fd, content_len, error))
goto out;

ret_stream = g_unix_output_stream_new (fd, TRUE);
/* Make sure the tempfile is unset */
out_tmpf->initialized = 0;
return TRUE;
}

ret = TRUE;
if (!have_obj)
{
out_state->temp_filename = temp_filename;
temp_filename = NULL;
out_state->fd = g_file_descriptor_based_get_fd ((GFileDescriptorBased*)ret_stream);
if (out_stream)
*out_stream = g_steal_pointer (&ret_stream);
}
*out_have_object = have_obj;
out:
return ret;
return ot_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC,
out_tmpf, error);
}

gboolean
_ostree_repo_commit_trusted_content_bare (OstreeRepo *self,
const char *checksum,
OstreeRepoContentBareCommit *state,
OtTmpfile *tmpf,
guint32 uid,
guint32 gid,
guint32 mode,
GVariant *xattrs,
GCancellable *cancellable,
GError **error)
{
gboolean ret = FALSE;

if (state->fd != -1)
{
if (!commit_loose_content_object (self, checksum,
state->temp_filename,
FALSE, uid, gid, mode,
xattrs, state->fd,
cancellable, error))
{
g_prefix_error (error, "Writing object %s.%s: ", checksum, ostree_object_type_to_string (OSTREE_OBJECT_TYPE_FILE));
goto out;
}
}
/* I don't think this is necessary, but a similar check was here previously,
* keeping it for extra redundancy.
*/
if (!tmpf->initialized || tmpf->fd == -1)
return TRUE;

ret = TRUE;
out:
g_free (state->temp_filename);
return ret;
if (!commit_loose_content_object (self, checksum,
tmpf->path,
FALSE, uid, gid, mode,
xattrs, tmpf->fd,
cancellable, error))
return glnx_prefix_error (error, "Writing object %s.%s", checksum, ostree_object_type_to_string (OSTREE_OBJECT_TYPE_FILE));
/* The path was consumed */
g_clear_pointer (&tmpf->path, g_free);
tmpf->initialized = FALSE;
return TRUE;
}

static gboolean
Expand Down Expand Up @@ -567,23 +546,6 @@ create_regular_tmpfile_linkable_with_content (OstreeRepo *self,
return TRUE;
}

/* A little helper to call unlinkat() as a cleanup
* function. Mostly only necessary to handle
* deletion of temporary symlinks.
*/
typedef struct {
int dfd;
const char *path;
} CleanupUnlinkat;

static void
cleanup_unlinkat (CleanupUnlinkat *cleanup)
{
if (cleanup->path)
(void) unlinkat (cleanup->dfd, cleanup->path, 0);
}
G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(CleanupUnlinkat, cleanup_unlinkat);

/* Write a content object. */
static gboolean
write_content_object (OstreeRepo *self,
Expand Down Expand Up @@ -614,29 +576,38 @@ write_content_object (OstreeRepo *self,
cancellable, error))
return FALSE;

gboolean temp_file_is_regular = g_file_info_get_file_type (file_info) == G_FILE_TYPE_REGULAR;
gboolean temp_file_is_symlink = g_file_info_get_file_type (file_info) == G_FILE_TYPE_SYMBOLIC_LINK;
gboolean object_is_symlink = temp_file_is_symlink;
gboolean phys_object_is_symlink = FALSE;
const GFileType object_file_type = g_file_info_get_file_type (file_info);
switch (object_file_type)
{
case G_FILE_TYPE_REGULAR:
break;
case G_FILE_TYPE_SYMBOLIC_LINK:
if (self->mode == OSTREE_REPO_MODE_BARE || self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY)
phys_object_is_symlink = TRUE;
break;
default:
return glnx_throw (error, "Unsupported file type %u", object_file_type);
}

guint64 size;

if (repo_mode == OSTREE_REPO_MODE_BARE_USER && object_is_symlink)
/* For bare-user, convert the symlink target to the input stream */
if (repo_mode == OSTREE_REPO_MODE_BARE_USER && object_file_type == G_FILE_TYPE_SYMBOLIC_LINK)
{
const char *target_str = g_file_info_get_symlink_target (file_info);
g_autoptr(GBytes) target = g_bytes_new (target_str, strlen (target_str) + 1);

/* For bare-user we can't store symlinks as symlinks, as symlinks don't
support user xattrs to store the ownership. So, instead store them
as regular files */
temp_file_is_regular = TRUE;
temp_file_is_symlink = FALSE;

if (file_input != NULL)
g_object_unref (file_input);
/* Include the terminating zero so we can e.g. mmap this file */
Copy link
Copy Markdown
Member

@jlebon jlebon Jun 26, 2017

Choose a reason for hiding this comment

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

One thing I just noticed here. We're creating a new stream that has just the target name + nul, but down there where we create the tmpfile for it, we're still using the size from the GFileInfo, so we don't actually end up writing the trailing zero, right?

Copy link
Copy Markdown
Member Author

@cgwalters cgwalters Jun 26, 2017

Choose a reason for hiding this comment

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

We do write it actually, because we drop into the g_output_stream_splice() path. But our fallocate() is wrong.

I'm actually kind of dubious about the value of writing it, since it doesn't make sense to mmap small files. But let's keep it. Fixup and new commit ⬇️

Copy link
Copy Markdown
Member Author

@cgwalters cgwalters Jun 26, 2017

Choose a reason for hiding this comment

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

(Also, nice catch!)

file_input = g_memory_input_stream_new_from_bytes (target);
size = g_bytes_get_size (target);
}

if (!(temp_file_is_regular || temp_file_is_symlink))
return glnx_throw (error, "Unsupported file type %u", g_file_info_get_file_type (file_info));
else if (!phys_object_is_symlink)
size = g_file_info_get_size (file_info);
else
size = 0;

/* For regular files, we create them with default mode, and only
* later apply any xattrs and setuid bits. The rationale here
Expand All @@ -649,31 +620,26 @@ write_content_object (OstreeRepo *self,
* temp_filename might also be a symlink. Hence the CleanupUnlinkat
* which handles that case.
*/
g_auto(CleanupUnlinkat) tmp_unlinker = { self->tmp_dir_fd, NULL };
g_auto(OtCleanupUnlinkat) tmp_unlinker = { self->tmp_dir_fd, NULL };
glnx_fd_close int temp_fd = -1;
g_autofree char *temp_filename = NULL;
gssize unpacked_size = 0;
gboolean indexable = FALSE;
if ((_ostree_repo_mode_is_bare (repo_mode)) && temp_file_is_regular)
if ((_ostree_repo_mode_is_bare (repo_mode)) && !phys_object_is_symlink)
{
guint64 size = g_file_info_get_size (file_info);

if (!create_regular_tmpfile_linkable_with_content (self, size, file_input,
&temp_fd, &temp_filename,
&temp_fd, &tmp_unlinker.path,
cancellable, error))
return FALSE;
tmp_unlinker.path = temp_filename;
}
else if (_ostree_repo_mode_is_bare (repo_mode) && temp_file_is_symlink)
else if (_ostree_repo_mode_is_bare (repo_mode) && phys_object_is_symlink)
{
/* Note: This will not be hit for bare-user mode because its converted to a
regular file and take the branch above */
if (!_ostree_make_temporary_symlink_at (self->tmp_dir_fd,
g_file_info_get_symlink_target (file_info),
&temp_filename,
&tmp_unlinker.path,
cancellable, error))
return FALSE;
tmp_unlinker.path = temp_filename;
}
else if (repo_mode == OSTREE_REPO_MODE_ARCHIVE_Z2)
{
Expand All @@ -686,11 +652,9 @@ write_content_object (OstreeRepo *self,
indexable = TRUE;

if (!glnx_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC,
&temp_fd, &temp_filename,
&temp_fd, &tmp_unlinker.path,
error))
return FALSE;
tmp_unlinker.path = temp_filename;
temp_file_is_regular = TRUE;
temp_out = g_unix_output_stream_new (temp_fd, FALSE);

file_meta = _ostree_zlib_file_header_new (file_info, xattrs);
Expand Down Expand Up @@ -757,18 +721,18 @@ write_content_object (OstreeRepo *self,
const guint32 gid = g_file_info_get_attribute_uint32 (file_info, "unix::gid");
const guint32 mode = g_file_info_get_attribute_uint32 (file_info, "unix::mode");
if (!commit_loose_content_object (self, actual_checksum,
temp_filename,
object_is_symlink,
tmp_unlinker.path,
object_file_type == G_FILE_TYPE_SYMBOLIC_LINK,
uid, gid, mode,
xattrs, temp_fd,
cancellable, error))
return glnx_prefix_error (error, "Writing object %s.%s: ", actual_checksum,
ostree_object_type_to_string (OSTREE_OBJECT_TYPE_FILE));
/* Clear the unlinker path, it was consumed */
tmp_unlinker.path = NULL;
/* Clear the unlinker, it was consumed */
ot_cleanup_unlinkat_clear (&tmp_unlinker);

/* Update size metadata if configured */
if (indexable && temp_file_is_regular)
if (indexable && object_file_type == G_FILE_TYPE_REGULAR)
{
struct stat stbuf;

Expand Down
12 changes: 3 additions & 9 deletions src/libostree/ostree-repo-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

#include "ostree-repo.h"
#include "ostree-remote-private.h"
#include "libglnx.h"
#include "otutil.h"

G_BEGIN_DECLS

Expand Down Expand Up @@ -294,25 +294,19 @@ _ostree_repo_commit_loose_final (OstreeRepo *self,
GCancellable *cancellable,
GError **error);

typedef struct {
int fd;
char *temp_filename;
} OstreeRepoContentBareCommit;

gboolean
_ostree_repo_open_content_bare (OstreeRepo *self,
const char *checksum,
guint64 content_len,
OstreeRepoContentBareCommit *out_state,
GOutputStream **out_stream,
OtTmpfile *out_tmpf,
gboolean *out_have_object,
GCancellable *cancellable,
GError **error);

gboolean
_ostree_repo_commit_trusted_content_bare (OstreeRepo *self,
const char *checksum,
OstreeRepoContentBareCommit *state,
OtTmpfile *tmpf,
guint32 uid,
guint32 gid,
guint32 mode,
Expand Down
Loading