Re: [libosinfo] Add 'persistent' parameter to OsinfoMedia

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Apr 13, 2017 at 11:15 AM, Daniel P. Berrange
<berrange@xxxxxxxxxx> wrote:
> On Thu, Apr 13, 2017 at 11:01:06AM +0200, Christophe Fergeau wrote:
>> On Wed, Apr 12, 2017 at 11:09:34AM +0200, Fabiano Fidêncio wrote:
>> > If media is an installer, thus specifies whether the media should be
>> > persistent accross the final reboot during its installation process.
>> > Default value is false.
>> >
>> > This is mainly needed for applications like GNOME Boxes (and maybe
>> > virt-install) to be able to decide whether the media should be ejected
>> > or not when the final reboot happens during its installation process.
>> >
>> > The latter case may happen when the installer leaves some packages to be
>> > installed after rebooting the OS for the last time.
>> >
>> > The situation this patch solves is a corner-case faced when adding the
>> > install scripts for SLES, as during its installation only one reboot is
>> > performed (so, installer-reboots attribute doesn't help us) and the media
>> > must persist after this reboot in order to finish the installation.
>>
>> The installer cannot use the network for that? Maybe it would be
>> acceptable to not add such a property, and document that libosinfo users
>> should remove the iso from the libvirt XML only after the last reboot?
>>
>> This would mean the installation ISO would always be visible in the
>> guest OS until the first user-initiated reboot, but maybe that's ok?
>> We could also add some ejection of the ISO to the post-install scripts,
>> but that's getting complex and ugly ;)
>
> A user initiated reboot after install wouldn't magically cause the ISO
> to be ejected. You would need the app to keep looking for that reboot
> and eject manually which I don't think is viable.
>
> Also, note that most installers will explicitly instruct the user to
> eject the CDROM at end of installation, so current app behaviour of
> rmoving the CDROM at this time matches that (of course that instruction
> is mostly about boot order priority in physical bios).

I'm not sure if I understand your point, Daniel, sorry. (I didn't get
whether your comment was just replying Christophe's messages or it
applies for this patchset).

>
>
>> > Signed-off-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
>> > ---
>> >  osinfo/libosinfo.syms  |  5 +++++
>> >  osinfo/osinfo_db.c     |  3 +++
>> >  osinfo/osinfo_loader.c |  9 +++++++++
>> >  osinfo/osinfo_media.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> >  osinfo/osinfo_media.h  |  2 ++
>> >  5 files changed, 67 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/osinfo/libosinfo.syms b/osinfo/libosinfo.syms
>> > index 68d54ff..e148d50 100644
>> > --- a/osinfo/libosinfo.syms
>> > +++ b/osinfo/libosinfo.syms
>> > @@ -520,6 +520,11 @@ LIBOSINFO_0.2.12 {
>> >     osinfo_media_get_volume_size;
>> >  } LIBOSINFO_0.2.11;
>> >
>> > +LIBOSINFO_0.2.13 {
>> > +    global:
>> > +    osinfo_media_get_persistent;
>> > +} LIBOSINFO_0.2.12;
>> > +
>> >  /* Symbols in next release...
>> >
>> >    LIBOSINFO_0.0.2 {
>> > diff --git a/osinfo/osinfo_db.c b/osinfo/osinfo_db.c
>> > index cf86ccc..62d54de 100644
>> > --- a/osinfo/osinfo_db.c
>> > +++ b/osinfo/osinfo_db.c
>> > @@ -628,6 +628,7 @@ static void fill_media(OsinfoDb *db, OsinfoMedia *media,
>> >  {
>> >      gboolean is_installer;
>> >      gboolean is_live;
>> > +    gboolean is_persistent;
>> >      gint reboots;
>> >      const gchar *id;
>> >      const gchar *kernel_path;
>> > @@ -663,9 +664,11 @@ static void fill_media(OsinfoDb *db, OsinfoMedia *media,
>> >          g_object_set(G_OBJECT(media), "initrd_path", initrd_path, NULL);
>> >      is_installer = osinfo_media_get_installer(matched_media);
>> >      is_live = osinfo_media_get_live(matched_media);
>> > +    is_persistent = osinfo_media_get_persistent(matched_media);
>> >      g_object_set(G_OBJECT(media),
>> >                   "installer", is_installer,
>> >                   "live", is_live,
>> > +                 "persistent", is_persistent,
>> >                   NULL);
>> >      if (is_installer) {
>> >          reboots = osinfo_media_get_installer_reboots(matched_media);
>> > diff --git a/osinfo/osinfo_loader.c b/osinfo/osinfo_loader.c
>> > index dba567f..d4c811a 100644
>> > --- a/osinfo/osinfo_loader.c
>> > +++ b/osinfo/osinfo_loader.c
>> > @@ -1050,6 +1050,8 @@ static OsinfoMedia *osinfo_loader_media(OsinfoLoader *loader,
>> >      xmlChar *installer = xmlGetProp(root, BAD_CAST OSINFO_MEDIA_PROP_INSTALLER);
>> >      xmlChar *installer_reboots =
>> >              xmlGetProp(root, BAD_CAST OSINFO_MEDIA_PROP_INSTALLER_REBOOTS);
>> > +    xmlChar *persistent =
>> > +            xmlGetProp(root, BAD_CAST OSINFO_MEDIA_PROP_PERSISTENT);
>> >      const OsinfoEntityKey keys[] = {
>> >          { OSINFO_MEDIA_PROP_URL, G_TYPE_STRING },
>> >          { OSINFO_MEDIA_PROP_KERNEL, G_TYPE_STRING },
>> > @@ -1082,6 +1084,13 @@ static OsinfoMedia *osinfo_loader_media(OsinfoLoader *loader,
>> >          xmlFree(installer_reboots);
>> >      }
>> >
>> > +    if (persistent) {
>> > +        osinfo_entity_set_param(OSINFO_ENTITY(media),
>> > +                                OSINFO_MEDIA_PROP_PERSISTENT,
>> > +                                (gchar *)persistent);
>> > +        xmlFree(persistent);
>> > +    }
>> > +
>> >      gint nnodes = osinfo_loader_nodeset("./variant", loader, ctxt, &nodes, err);
>> >      if (error_is_set(err)) {
>> >          g_object_unref(media);
>> > diff --git a/osinfo/osinfo_media.c b/osinfo/osinfo_media.c
>> > index af4bb14..eac48a8 100644
>> > --- a/osinfo/osinfo_media.c
>> > +++ b/osinfo/osinfo_media.c
>> > @@ -156,7 +156,8 @@ enum {
>> >      PROP_INSTALLER_REBOOTS,
>> >      PROP_OS,
>> >      PROP_LANGUAGES,
>> > -    PROP_VOLUME_SIZE
>> > +    PROP_VOLUME_SIZE,
>> > +    PROP_PERSISTENT
>> >  };
>> >
>> >  static void
>> > @@ -236,6 +237,11 @@ osinfo_media_get_property(GObject    *object,
>> >                            osinfo_media_get_volume_size(media));
>> >          break;
>> >
>> > +    case PROP_PERSISTENT:
>> > +        g_value_set_boolean(value,
>> > +                            osinfo_media_get_persistent(media));
>> > +        break;
>> > +
>> >      default:
>> >          /* We don't have any other property... */
>> >          G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
>> > @@ -332,6 +338,12 @@ osinfo_media_set_property(GObject      *object,
>> >                                        g_value_get_int64(value));
>> >          break;
>> >
>> > +    case PROP_PERSISTENT:
>> > +        osinfo_entity_set_param_boolean(OSINFO_ENTITY(media),
>> > +                                        OSINFO_MEDIA_PROP_PERSISTENT,
>> > +                                        g_value_get_boolean(value));
>> > +
>> > +        break;
>> >      default:
>> >          /* We don't have any other property... */
>> >          G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
>> > @@ -574,6 +586,24 @@ osinfo_media_class_init(OsinfoMediaClass *klass)
>> >                                 G_PARAM_READWRITE |
>> >                                 G_PARAM_STATIC_STRINGS);
>> >      g_object_class_install_property(g_klass, PROP_VOLUME_SIZE, pspec);
>> > +
>> > +    /**
>> > +     * OsinfoMedia:persistent:
>> > +     *
>> > +     * Whether the media should be persistent accross reboots done during
>> > +     * the installation process.
>> > +     *
>> > +     * Some distros need their media to be persistent accross reboots as some
>> > +     * packages are installed after the reboot (which may cause the media
>> > +     * ejection, depending on the application).
>> > +     */
>> > +    pspec = g_param_spec_boolean("persistent",
>> > +                                 "Persistent",
>> > +                                 _("The media persists reboots during its installation process"),
>> > +                                 FALSE /* default value */,
>> > +                                 G_PARAM_READWRITE |
>> > +                                 G_PARAM_STATIC_STRINGS);
>> > +    g_object_class_install_property(g_klass, PROP_PERSISTENT, pspec);
>> >  }
>> >
>> >  static void
>> > @@ -1269,6 +1299,21 @@ gint64 osinfo_media_get_volume_size(OsinfoMedia *media)
>> >          (OSINFO_ENTITY(media), OSINFO_MEDIA_PROP_VOLUME_SIZE, -1);
>> >  }
>> >
>> > +/**
>> > + * osinfo_media_get_persistent:
>> > + * @media: an #OsinfoMedia instance
>> > + *
>> > + * Whether @media should persist accross reboots done during its installation
>> > + * process.
>> > + *
>> > + * Returns: #TRUE if media should persist, #FALSE otherwise
>> > + */
>> > +gboolean osinfo_media_get_persistent(OsinfoMedia *media)
>> > +{
>> > +    return osinfo_entity_get_param_value_boolean_with_default
>> > +            (OSINFO_ENTITY(media), OSINFO_MEDIA_PROP_PERSISTENT, FALSE);
>> > +}
>> > +
>> >  /*
>> >   * Local variables:
>> >   *  indent-tabs-mode: nil
>> > @@ -1276,3 +1321,5 @@ gint64 osinfo_media_get_volume_size(OsinfoMedia *media)
>> >   *  c-basic-offset: 4
>> >   * End:
>> >   */
>> > +
>> > +
>> > diff --git a/osinfo/osinfo_media.h b/osinfo/osinfo_media.h
>> > index 09fbacd..b400fb5 100644
>> > --- a/osinfo/osinfo_media.h
>> > +++ b/osinfo/osinfo_media.h
>> > @@ -90,6 +90,7 @@ typedef struct _OsinfoMediaPrivate OsinfoMediaPrivate;
>> >  #define OSINFO_MEDIA_PROP_LANG_MAP       "l10n-language-map"
>> >  #define OSINFO_MEDIA_PROP_VARIANT        "variant"
>> >  #define OSINFO_MEDIA_PROP_VOLUME_SIZE    "volume-size"
>> > +#define OSINFO_MEDIA_PROP_PERSISTENT     "persistent"
>> >
>> >  /* object */
>> >  struct _OsinfoMedia
>> > @@ -140,6 +141,7 @@ gboolean osinfo_media_get_installer(OsinfoMedia *media);
>> >  gboolean osinfo_media_get_live(OsinfoMedia *media);
>> >  gint osinfo_media_get_installer_reboots(OsinfoMedia *media);
>> >  gint64 osinfo_media_get_volume_size(OsinfoMedia *media);
>> > +gboolean osinfo_media_get_persistent(OsinfoMedia *media);
>> >
>> >  #endif /* __OSINFO_MEDIA_H__ */
>> >  /*
>> > --
>> > 2.9.3
>> >
>> > _______________________________________________
>> > Libosinfo mailing list
>> > Libosinfo@xxxxxxxxxx
>> > https://www.redhat.com/mailman/listinfo/libosinfo
>
>
>
>> _______________________________________________
>> Libosinfo mailing list
>> Libosinfo@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libosinfo
>
>
> Regards,
> Daniel
> --
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
>
> _______________________________________________
> Libosinfo mailing list
> Libosinfo@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libosinfo



-- 
Fabiano Fidêncio

_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Fedora Users]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]

  Powered by Linux