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.

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

Local extra-data #873

Merged
merged 1 commit into from Aug 21, 2017

Conversation

Projects
None yet
3 participants
@manuq
Contributor

manuq commented Jun 28, 2017

No description provided.

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Jun 28, 2017

Collaborator

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once
Collaborator

rh-atomic-bot commented Jun 28, 2017

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once
Show outdated Hide outdated common/flatpak-dir.c
extra_data_progress_report, &extra_data_progress,
cancellable, error);
home = g_file_new_for_path (g_get_home_dir ());
extra_local_filename = g_strconcat (".local/share/flatpak/extra-data/", extra_data_name, NULL);

This comment has been minimized.

@alexlarsson

alexlarsson Jun 28, 2017

Member

Use flatpak_get_user_base_dir_location () instead

@alexlarsson

alexlarsson Jun 28, 2017

Member

Use flatpak_get_user_base_dir_location () instead

Show outdated Hide outdated common/flatpak-dir.c
bytes = flatpak_load_http_uri (self->soup_session, extra_data_uri, NULL, NULL,
extra_data_progress_report, &extra_data_progress,
cancellable, error);
base_dir = flatpak_get_user_base_dir_location ();

This comment has been minimized.

@alexlarsson

alexlarsson Jun 30, 2017

Member

You should not free this return value.

@alexlarsson

alexlarsson Jun 30, 2017

Member

You should not free this return value.

Show outdated Hide outdated common/flatpak-dir.c
cancellable, error);
base_dir = flatpak_get_user_base_dir_location ();
extra_local_dir = g_file_get_child (base_dir, "extra-data");
extra_local_file = g_file_get_child (g_file_get_child (extra_local_dir, extra_data_sha256), extra_data_name);

This comment has been minimized.

@alexlarsson

alexlarsson Jun 30, 2017

Member

This leaks the intermediate child.
Use flatpak_build_file (base_dir, "extra-data", extra_data_sha256, extra_data_name, NULL) instead.

@alexlarsson

alexlarsson Jun 30, 2017

Member

This leaks the intermediate child.
Use flatpak_build_file (base_dir, "extra-data", extra_data_sha256, extra_data_name, NULL) instead.

Show outdated Hide outdated common/flatpak-dir.c
{
g_debug ("Loading extra-data from local file %s", g_file_get_path (extra_local_file));
extra_local_stream = (GInputStream *) g_file_read (extra_local_file, cancellable, error);
bytes = g_input_stream_read_bytes (extra_local_stream, download_size, cancellable, error);

This comment has been minimized.

@alexlarsson

alexlarsson Jun 30, 2017

Member

g_input_stream_read_bytes is just a more bindable version of g_input_stream_read(). It doesn't guarantee that you read the entire file, etc. Use g_file_load_contents instead and put the result (if the length is correct) in a GBytes.

@alexlarsson

alexlarsson Jun 30, 2017

Member

g_input_stream_read_bytes is just a more bindable version of g_input_stream_read(). It doesn't guarantee that you read the entire file, etc. Use g_file_load_contents instead and put the result (if the length is correct) in a GBytes.

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 30, 2017

Member

Also, can you drop the merge commits and squash this to a single commit.

Member

alexlarsson commented Jun 30, 2017

Also, can you drop the merge commits and squash this to a single commit.

@manuq

This comment has been minimized.

Show comment
Hide comment
@manuq

manuq Jun 30, 2017

Contributor

Thanks I pushed a squashed commit with new changes.

For base_dir "You should not free this return value.", I interpreted that as "move the call outside the for loop". If you didn't mean that, please tell me and I'll change it again. Sorry I'm a glib newbie.

Contributor

manuq commented Jun 30, 2017

Thanks I pushed a squashed commit with new changes.

For base_dir "You should not free this return value.", I interpreted that as "move the call outside the for loop". If you didn't mean that, please tell me and I'll change it again. Sorry I'm a glib newbie.

}
else
{
return flatpak_fail (error, _("Wrong size for extra-data %s"), g_file_get_path (extra_local_file));

This comment has been minimized.

@alexlarsson

alexlarsson Jun 30, 2017

Member

This will double-set error if it was set by g_file_load_contents, which will g_error

@alexlarsson

alexlarsson Jun 30, 2017

Member

This will double-set error if it was set by g_file_load_contents, which will g_error

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 30, 2017

Member

Oh, sorry, i thought it returned a value that was not ref:ed, but i was wrong.

Member

alexlarsson commented Jun 30, 2017

Oh, sorry, i thought it returned a value that was not ref:ed, but i was wrong.

extra-data: Support reading from local directory
Lookup extra-data files as
~/.local/share/flatpak/extra-data/SHA256/FILENAME, Similar to files
downloaded in the .flatpak-builder directory.

For now, if a corresponding file exists, assume it's the full download
and read bytes from it.  Then proceed to do the same checks as for the
bytes downloaded by Soup.

In the future this directory could be used to support resumed downloads
by storing partial downloads.
@manuq

This comment has been minimized.

Show comment
Hide comment
@manuq

manuq Jul 4, 2017

Contributor

@alexlarsson , any other review?

Contributor

manuq commented Jul 4, 2017

@alexlarsson , any other review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment