On Wed, Dec 19, 2018 at 11:02:47AM +0000, Daniel P. Berrangé wrote:
> On Mon, Dec 17, 2018 at 08:10:57PM +0100, Fabiano Fidêncio wrote:
> > Those new methods are going to be used to tell the install-scripts
> > whether the injection-method that's going to be used is. We have to do
> > so as the command-line may be different depending on the
> > injection-method used, for example:
> > - fedora using cdrom, disk or floppy: ks=hd:/(vda|sda)/fedora.ks
> > - fedora using initrd: ks=file:/fedora.ks
> >
> > It's important to mention that although the methods are taking GFlags,
> > those are treated as GEnum and only one value is expected to be set, as
> > mentioned in the documentation.
> >
> > Also, mind that the usage of osinfo_entity_set_param() to store the
> > nick of the OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_* is intentional as
> > the nick is exactly what's going to be used in the install-scripts to
> > generate the proper command-line.
> >
> > Signed-off-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
> > ---
> > osinfo/libosinfo.syms | 3 ++
> > osinfo/osinfo_install_script.c | 95 ++++++++++++++++++++++++++++++++++
> > osinfo/osinfo_install_script.h | 4 ++
> > 3 files changed, 102 insertions(+)
> >
> > diff --git a/osinfo/libosinfo.syms b/osinfo/libosinfo.syms
> > index 45a7219..0272e94 100644
> > --- a/osinfo/libosinfo.syms
> > +++ b/osinfo/libosinfo.syms
> > @@ -543,6 +543,9 @@ LIBOSINFO_1.3.0 {
> > osinfo_imagelist_get_type;
> > osinfo_imagelist_new;
> >
> > + osinfo_install_script_get_preferred_injection_method;
> > + osinfo_install_script_set_preferred_injection_method;
> > +
> > osinfo_media_supports_installer_script;
> >
> > osinfo_os_add_image;
> > diff --git a/osinfo/osinfo_install_script.c b/osinfo/osinfo_install_script.c
> > index 016f850..68f0a9d 100644
> > --- a/osinfo/osinfo_install_script.c
> > +++ b/osinfo/osinfo_install_script.c
> > @@ -64,6 +64,7 @@ enum {
> > PROP_PRODUCT_KEY_FORMAT,
> > PROP_PATH_FORMAT,
> > PROP_AVATAR_FORMAT,
> > + PROP_PREFERRED_INJECTION_METHOD
> > };
> >
> > typedef struct _OsinfoInstallScriptGenerateData OsinfoInstallScriptGenerateData;
> > @@ -105,6 +106,11 @@ osinfo_install_script_set_property(GObject *object,
> > data);
> > break;
> >
> > + case PROP_PREFERRED_INJECTION_METHOD:
> > + osinfo_install_script_set_preferred_injection_method(script,
> > + g_value_get_flags(value));
> > + break;
> > +
> > default:
> > /* We don't have any other property... */
> > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> > @@ -151,6 +157,11 @@ osinfo_install_script_get_property(GObject *object,
> > osinfo_install_script_get_avatar_format(script));
> > break;
> >
> > + case PROP_PREFERRED_INJECTION_METHOD:
> > + g_value_set_flags(value,
> > + osinfo_install_script_get_preferred_injection_method(script));
> > + break;
> > +
> > default:
> > /* We don't have any other property... */
> > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> > @@ -254,6 +265,17 @@ osinfo_install_script_class_init(OsinfoInstallScriptClass *klass)
> > PROP_AVATAR_FORMAT,
> > pspec);
> >
> > + pspec = g_param_spec_flags("preferred-injection-method",
> > + "Preferred Injection Method",
> > + _("The preferred injection method"),
> > + OSINFO_TYPE_INSTALL_SCRIPT_INJECTION_METHOD,
> > + OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_DISK, /* default value */
> > + G_PARAM_READABLE |
>
> You've provided a setter, so why only G_PARAM_READABLE, and not WRITABLE too ?
>
> > + G_PARAM_STATIC_STRINGS);
> > + g_object_class_install_property(g_klass,
> > + PROP_PREFERRED_INJECTION_METHOD,
> > + pspec);
> > +
> > g_type_class_add_private(klass, sizeof(OsinfoInstallScriptPrivate));
> > }
> >
> > @@ -1765,6 +1787,79 @@ gboolean osinfo_install_script_get_needs_internet(OsinfoInstallScript *script)
> > FALSE);
> > }
> >
> > +/**
> > + * osinfo_install_script_set_preferred_injection_method:
> > + * @script: the install script
> > + * @method: one of the injection methods:
> > + * OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_CDROM,
> > + * OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_DISK,
> > + * OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_FLOPPY,
> > + * OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_INITRD,
> > + * OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_WEB
> > + *
> > + * Set the preferred injection method to be used with the @script
> > + */
> > +void osinfo_install_script_set_preferred_injection_method(OsinfoInstallScript *script,
> > + OsinfoInstallScriptInjectionMethod method)
> > +{
> > + GFlagsClass *flags_class;
> > + guint supported_methods;
> > + guint i;
> > +
> > + supported_methods = osinfo_install_script_get_injection_methods(script);
> > + if ((method & supported_methods) == 0) {
> > + g_warning("The injection-method passed is not supported by the install-script");
> > + return;
>
> g_warning is not appropriate for something which is likely to happen
> depending on data. We should have this return a gboolean error status.
> A caller can use g_warning if it desires.
On second thoughts, IMHO, we should not do this check at all.
Whether the preferred method matches supported methods is only
relevant when the data is actually used later on.
> > + }
> > +
>
> We should do a check here to see if multiple bits are set in method
> before trying to set the parameter and again return an error status
Lets ignore this, and then we can keep this method 'void'
> > + flags_class = g_type_class_ref(OSINFO_TYPE_INSTALL_SCRIPT_INJECTION_METHOD);
> > + for (i = 0; i < flags_class->n_values; i++) {
> > + if ((flags_class->values[i].value & method) != 0) {
> > + osinfo_entity_set_param(OSINFO_ENTITY(script),
> > + OSINFO_INSTALL_SCRIPT_PROP_PREFERRED_INJECTION_METHOD,
> > + flags_class->values[i].value_nick);
> > + break;
> > + }
> > + }
> > + g_type_class_unref(flags_class);
> > +}
> > +
> > +/**
> > + * osinfo_install_script_get_preferred_injection_method:
> > + * @script: the install script
> > + *
> > + * Returns: the preferred injection method for the script. If none is set and
> > + * OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_DISK is supported,
> > + * OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_DISK is returned, otherwise
> > + * OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_INITRD is returned.
> > + */
> > +OsinfoInstallScriptInjectionMethod
> > +osinfo_install_script_get_preferred_injection_method(OsinfoInstallScript *script)
> > +{
> > + GFlagsClass *flags_class;
> > + GFlagsValue *value;
> > + const gchar *nick;
> > + guint supported_methods;
> > +
> > + nick = osinfo_entity_get_param_value(OSINFO_ENTITY(script),
> > + OSINFO_INSTALL_SCRIPT_PROP_PREFERRED_INJECTION_METHOD);
> > +
> > + if (nick == NULL) {
> > + supported_methods = osinfo_install_script_get_injection_methods(script);
> > + if ((supported_methods & OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_DISK) != 0)
> > + return OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_DISK;
> > + else if ((supported_methods & OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_INITRD) != 0)
> > + return OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_INITRD;
> > + else
> > + g_return_val_if_reached(OSINFO_INSTALL_SCRIPT_INJECTION_METHOD_DISK);
>
> g_return_val_if_reached is pretty pointless. A simple 'return' is sufficient IMHO
>
> > + }
> > +
> > + flags_class = g_type_class_ref(OSINFO_TYPE_INSTALL_SCRIPT_INJECTION_METHOD);
> > + value = g_flags_get_value_by_nick(flags_class, nick);
> > + g_type_class_unref(flags_class);
> > +
> > + return value->value;
> > +}
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
> _______________________________________________
> Libosinfo mailing list
> Libosinfo@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libosinfo
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo
