importer: Rework API

Now that the importer *only* imports into OSTree repos, let's
clean up the API so that the `OstreeRepo` and `OstreeSePolicy`
are passed as constructor args.

Also rework things so there's only one constructor API that
steals the fd.

This is prep for adding another async import API.

Closes: #1124
Approved by: jlebon

src/daemon/rpmostreed-transaction-types.c

@@ -468,7 +468,7 @@ deploy_transaction_finalize (GObject *object)
 
 static gboolean
 import_local_rpm (OstreeRepo    *repo,
-                  int            fd,
+                  int           *fd,
                   char         **sha256_nevra,
                   GCancellable  *cancellable,
                   GError       **error)
@@ -482,12 +482,11 @@ import_local_rpm (OstreeRepo    *repo,
   if (policy == NULL)
     return FALSE;
 
-  g_autoptr(RpmOstreeImporter) unpacker = rpmostree_importer_new_fd (fd, NULL, 0, error);
+  g_autoptr(RpmOstreeImporter) unpacker = rpmostree_importer_new_take_fd (fd, repo, NULL, 0, policy, error);
   if (unpacker == NULL)
     return FALSE;
 
-  if (!rpmostree_importer_run (unpacker, repo, policy,
+  if (!rpmostree_importer_run (unpacker, NULL, cancellable, error))
-                               NULL, cancellable, error))
     return FALSE;
 
   g_autofree char *nevra = rpmostree_importer_get_nevra (unpacker);
@@ -497,6 +496,25 @@ import_local_rpm (OstreeRepo    *repo,
   return TRUE;
 }
 
+static void
+ptr_close_fd (gpointer fdp)
+{
+  int fd = GPOINTER_TO_INT (fdp);
+  glnx_close_fd (&fd);
+}
+
+/* GUnixFDList doesn't allow stealing individual members */
+static GPtrArray *
+unixfdlist_to_ptrarray (GUnixFDList *fdl)
+{
+  gint len;
+  gint *fds = g_unix_fd_list_steal_fds (fdl, &len);
+  GPtrArray *ret = g_ptr_array_new_with_free_func ((GDestroyNotify)ptr_close_fd);
+  for (int i = 0; i < len; i++)
+    g_ptr_array_add (ret, GINT_TO_POINTER (fds[i]));
+  return ret;
+}
+
 static gboolean
 import_many_local_rpms (OstreeRepo    *repo,
                         GUnixFDList   *fdl,
@@ -516,12 +534,15 @@ import_many_local_rpms (OstreeRepo    *repo,
 
   g_autoptr(GPtrArray) pkgs = g_ptr_array_new_with_free_func (g_free);
 
-  gint nfds = 0;
+  g_autoptr(GPtrArray) fds = unixfdlist_to_ptrarray (fdl);
-  const gint *fds = g_unix_fd_list_peek_fds (fdl, &nfds);
+  for (guint i = 0; i < fds->len; i++)
-  for (guint i = 0; i < nfds; i++)
     {
+      /* Steal fd from the ptrarray */
+      glnx_autofd int fd = GPOINTER_TO_INT (fds->pdata[i]);
+      fds->pdata[i] = GINT_TO_POINTER (-1);
       g_autofree char *sha256_nevra = NULL;
-      if (!import_local_rpm (repo, fds[i], &sha256_nevra, cancellable, error))
+      /* Transfer fd to import */
+      if (!import_local_rpm (repo, &fd, &sha256_nevra, cancellable, error))
         return FALSE;
 
       g_ptr_array_add (pkgs, g_steal_pointer (&sha256_nevra));

src/libpriv/rpmostree-core.c

@@ -2055,7 +2055,9 @@ import_one_package (RpmOstreeContext *self,
   }
 
   /* TODO - tweak the unpacker flags for containers */
-  g_autoptr(RpmOstreeImporter) unpacker = rpmostree_importer_new_fd (fd, pkg, flags, error);
+  OstreeRepo *ostreerepo = get_pkgcache_repo (self);
+  g_autoptr(RpmOstreeImporter) unpacker =
+    rpmostree_importer_new_take_fd (&fd, ostreerepo, pkg, flags, sepolicy, error);
   if (!unpacker)
     return FALSE;
 
@@ -2065,10 +2067,8 @@ import_one_package (RpmOstreeContext *self,
       rpmostree_importer_set_jigdo_mode (unpacker, jigdo_xattr_table, jigdo_xattrs);
     }
 
-  OstreeRepo *ostreerepo = get_pkgcache_repo (self);
   g_autofree char *ostree_commit = NULL;
-  if (!rpmostree_importer_run (unpacker, ostreerepo, sepolicy,
+  if (!rpmostree_importer_run (unpacker, &ostree_commit, cancellable, error))
-                               &ostree_commit, cancellable, error))
     return glnx_prefix_error (error, "Unpacking %s",
                               dnf_package_get_nevra (pkg));
 

src/libpriv/rpmostree-importer.c

@@ -53,9 +53,10 @@ typedef GObjectClass RpmOstreeImporterClass;
 struct RpmOstreeImporter
 {
   GObject parent_instance;
+  OstreeRepo *repo;
+  OstreeSePolicy *sepolicy;
   struct archive *archive;
   int fd;
-  gboolean owns_fd;
   Header hdr;
   rpmfi fi;
   off_t cpio_offset;
@@ -87,10 +88,11 @@ rpmostree_importer_finalize (GObject *object)
     archive_read_free (self->archive);
   if (self->fi)
     (void) rpmfiFree (self->fi);
-  if (self->owns_fd)
+  glnx_close_fd (&self->fd);
-    glnx_close_fd (&self->fd);
   g_string_free (self->tmpfiles_d, TRUE);
   g_free (self->ostree_branch);
+  g_clear_object (&self->repo);
+  g_clear_object (&self->sepolicy);
 
   g_clear_pointer (&self->rpmfi_overrides, (GDestroyNotify)g_hash_table_unref);
   g_clear_pointer (&self->doc_files, (GDestroyNotify)g_hash_table_unref);
@@ -115,6 +117,7 @@ rpmostree_importer_class_init (RpmOstreeImporterClass *klass)
 static void
 rpmostree_importer_init (RpmOstreeImporter *self)
 {
+  self->fd = -1;
   self->rpmfi_overrides = g_hash_table_new_full (g_str_hash, g_str_equal,
                                                  g_free, NULL);
   self->tmpfiles_d = g_string_new ("");
@@ -226,10 +229,12 @@ build_rpmfi_overrides (RpmOstreeImporter *self)
 }
 
 /*
- * rpmostree_importer_new_fd:
+ * rpmostree_importer_new_take_fd:
  * @fd: Fd
+ * @repo: repo
  * @pkg: (optional): Package reference, used for metadata
  * @flags: flags
+ * @sepolicy: (optional): SELinux policy
  * @error: error
  *
  * Create a new unpacker instance.  The @pkg argument, if
@@ -237,10 +242,12 @@ build_rpmfi_overrides (RpmOstreeImporter *self)
  * origin repo will be added to the final commit.
  */
 RpmOstreeImporter *
-rpmostree_importer_new_fd (int fd,
+rpmostree_importer_new_take_fd (int                     *fd,
-                           DnfPackage *pkg,
+                                OstreeRepo              *repo,
-                           RpmOstreeImporterFlags flags,
+                                DnfPackage              *pkg,
-                           GError **error)
+                                RpmOstreeImporterFlags   flags,
+                                OstreeSePolicy          *sepolicy,
+                                GError                 **error)
 {
   RpmOstreeImporter *ret = NULL;
   g_auto(Header) hdr = NULL;
@@ -248,15 +255,17 @@ rpmostree_importer_new_fd (int fd,
   struct archive *archive;
   gsize cpio_offset;
 
-  archive = rpmostree_unpack_rpm2cpio (fd, error);
+  archive = rpmostree_unpack_rpm2cpio (*fd, error);
   if (archive == NULL)
     goto out;
 
-  if (!rpmostree_importer_read_metainfo (fd, &hdr, &cpio_offset, &fi, error))
+  if (!rpmostree_importer_read_metainfo (*fd, &hdr, &cpio_offset, &fi, error))
     goto out;
 
   ret = g_object_new (RPMOSTREE_TYPE_IMPORTER, NULL);
-  ret->fd = fd;
+  ret->fd = glnx_steal_fd (fd);
+  ret->repo = g_object_ref (repo);
+  ret->sepolicy = sepolicy ? g_object_ref (sepolicy) : NULL;
   ret->fi = g_steal_pointer (&fi);
   ret->archive = g_steal_pointer (&archive);
   ret->flags = flags;
@@ -278,38 +287,6 @@ rpmostree_importer_new_fd (int fd,
   return ret;
 }
 
-/*
- * rpmostree_importer_new_at:
- * @dfd: Fd
- * @path: Path
- * @pkg: (optional): Package reference, used for metadata
- * @flags: flags
- * @error: error
- *
- * Create a new unpacker instance.  The @pkg argument, if
- * specified, will be inspected and metadata such as the
- * origin repo will be added to the final commit.
- */
-RpmOstreeImporter *
-rpmostree_importer_new_at (int dfd, const char *path,
-                           DnfPackage *pkg,
-                           RpmOstreeImporterFlags flags,
-                           GError **error)
-{
-  glnx_autofd int fd = -1;
-  if (!glnx_openat_rdonly (dfd, path, TRUE, &fd, error))
-    return FALSE;
-
-  RpmOstreeImporter *ret = rpmostree_importer_new_fd (fd, pkg, flags, error);
-  if (ret == NULL)
-    return NULL;
-
-  ret->owns_fd = TRUE;
-  fd = -1;
-
-  return g_steal_pointer (&ret);
-}
-
 void
 rpmostree_importer_set_jigdo_mode (RpmOstreeImporter                    *self,
                                    GVariant *xattr_table,
@@ -410,7 +387,6 @@ repo_metadata_for_package (DnfRepo *repo)
 
 static gboolean
 build_metadata_variant (RpmOstreeImporter *self,
-                        OstreeSePolicy    *sepolicy,
                         GVariant         **out_variant,
                         GCancellable      *cancellable,
                         GError           **error)
@@ -460,10 +436,10 @@ build_metadata_variant (RpmOstreeImporter *self,
   /* The current sepolicy that was used to label the unpacked files is important
    * to record. It will help us during future overlays to determine whether the
    * files should be relabeled. */
-  if (sepolicy)
+  if (self->sepolicy)
     g_variant_builder_add (&metadata_builder, "{sv}", "rpmostree.sepolicy",
                            g_variant_new_string
-                             (ostree_sepolicy_get_csum (sepolicy)));
+                             (ostree_sepolicy_get_csum (self->sepolicy)));
 
   /* let's be nice to our future selves just in case */
   g_variant_builder_add (&metadata_builder, "{sv}", "rpmostree.unpack_version",
@@ -788,12 +764,11 @@ handle_translate_pathname (OstreeRepo   *repo,
 
 static gboolean
 import_rpm_to_repo (RpmOstreeImporter *self,
-                    OstreeRepo        *repo,
-                    OstreeSePolicy    *sepolicy,
                     char             **out_csum,
                     GCancellable      *cancellable,
                     GError           **error)
 {
+  OstreeRepo *repo = self->repo;
   /* Passed to the commit modifier */
   GError *cb_error = NULL;
   cb_data fdata = { self, &cb_error };
@@ -821,12 +796,12 @@ import_rpm_to_repo (RpmOstreeImporter *self,
     {
       ostree_repo_commit_modifier_set_xattr_callback (modifier, jigdo_xattr_cb,
                                                       NULL, self);
-      g_assert (sepolicy == NULL);
+      g_assert (self->sepolicy == NULL);
     }
   else
     {
       ostree_repo_commit_modifier_set_xattr_callback (modifier, xattr_cb, NULL, self);
-      ostree_repo_commit_modifier_set_sepolicy (modifier, sepolicy);
+      ostree_repo_commit_modifier_set_sepolicy (modifier, self->sepolicy);
     }
 
   OstreeRepoImportArchiveOptions opts = { 0 };
@@ -883,7 +858,7 @@ import_rpm_to_repo (RpmOstreeImporter *self,
     return FALSE;
 
   g_autoptr(GVariant) metadata = NULL;
-  if (!build_metadata_variant (self, sepolicy, &metadata, cancellable, error))
+  if (!build_metadata_variant (self, &metadata, cancellable, error))
     return FALSE;
   g_variant_ref_sink (metadata);
 
@@ -903,18 +878,16 @@ import_rpm_to_repo (RpmOstreeImporter *self,
 
 gboolean
 rpmostree_importer_run (RpmOstreeImporter *self,
-                        OstreeRepo        *repo,
-                        OstreeSePolicy    *sepolicy,
                         char             **out_csum,
                         GCancellable      *cancellable,
                         GError           **error)
 {
   g_autofree char *csum = NULL;
-  if (!import_rpm_to_repo (self, repo, sepolicy, &csum, cancellable, error))
+  if (!import_rpm_to_repo (self, &csum, cancellable, error))
     return FALSE;
 
   const char *branch = rpmostree_importer_get_ostree_branch (self);
-  ostree_repo_transaction_set_ref (repo, NULL, branch, csum);
+  ostree_repo_transaction_set_ref (self->repo, NULL, branch, csum);
 
   if (out_csum)
     *out_csum = g_steal_pointer (&csum);

src/libpriv/rpmostree-importer.h

@@ -47,17 +47,12 @@ typedef enum {
 } RpmOstreeImporterFlags;
 
 RpmOstreeImporter*
-rpmostree_importer_new_fd (int fd,
+rpmostree_importer_new_take_fd (int                     *fd,
-                           DnfPackage *pkg,
+                                OstreeRepo              *repo,
-                           RpmOstreeImporterFlags flags,
+                                DnfPackage              *pkg,
-                           GError **error);
+                                RpmOstreeImporterFlags   flags,
-
+                                OstreeSePolicy          *sepolicy,
-RpmOstreeImporter*
+                                GError                 **error);
-rpmostree_importer_new_at (int dfd,
-                           const char *path,
-                           DnfPackage *pkg, /* for metadata */
-                           RpmOstreeImporterFlags flags,
-                           GError **error);
 
 void rpmostree_importer_set_jigdo_mode (RpmOstreeImporter *self,
                                         GVariant *xattr_table,
@@ -75,8 +70,6 @@ rpmostree_importer_get_ostree_branch (RpmOstreeImporter *unpacker);
 
 gboolean
 rpmostree_importer_run (RpmOstreeImporter *unpacker,
-                        OstreeRepo        *repo,
-                        OstreeSePolicy    *sepolicy,
                         char             **out_commit,
                         GCancellable      *cancellable,
                         GError           **error);

tests/check/test-utils.c

@@ -142,11 +142,14 @@ test_variant_to_nevra(void)
 
     g_autoptr(RpmOstreeImporter) importer = NULL;
     g_autofree char *foo_rpm = g_strdup_printf ("yumrepo/packages/%s/%s.rpm", arch, nevra);
-    importer = rpmostree_importer_new_at (AT_FDCWD, foo_rpm, NULL, 0, &error);
+    glnx_autofd int foo_fd = -1;
+    glnx_openat_rdonly (AT_FDCWD, foo_rpm, TRUE, &foo_fd, &error);
+    g_assert_no_error (error);
+    importer = rpmostree_importer_new_take_fd (&foo_fd, repo, NULL, 0, NULL, &error);
     g_assert_no_error (error);
     g_assert (importer);
 
-    ret = rpmostree_importer_run (importer, repo, NULL, NULL, NULL, &error);
+    ret = rpmostree_importer_run (importer, NULL, NULL, &error);
     g_assert_no_error (error);
     g_assert (ret);