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
