| Message ID | 20230808132219.6422-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools: add some more configure options | expand |
On 08.08.2023 15:22, Juergen Gross wrote: > --- a/tools/xl/xl_parse.c > +++ b/tools/xl/xl_parse.c > @@ -1692,6 +1692,15 @@ void parse_config_data(const char *config_source, > xlu_cfg_get_defbool(config, "acpi", &b_info->acpi, 0); > > xlu_cfg_replace_string (config, "bootloader", &b_info->bootloader, 0); > +#ifndef HAVE_PYGRUB > + if (b_info->bootloader && > + (!strcmp(b_info->bootloader, "pygrub") || > + !strcmp(b_info->bootloader, "/usr/bin/pygrub"))) { And no other path combinations can occur? strstr() is perhaps too lax, but what about finding the last slash (if any) and comparing the rest of the string (the full string when there's no slash) against "pygrub"? > + fprintf(stderr, "ERROR: this instance of Xen has been built without support of \"pygrub\".\n"); The other question (I'm sorry for my ignorance here) is whether pygrub could come from anywhere else. Jan
On 08.08.23 15:32, Jan Beulich wrote: > On 08.08.2023 15:22, Juergen Gross wrote: >> --- a/tools/xl/xl_parse.c >> +++ b/tools/xl/xl_parse.c >> @@ -1692,6 +1692,15 @@ void parse_config_data(const char *config_source, >> xlu_cfg_get_defbool(config, "acpi", &b_info->acpi, 0); >> >> xlu_cfg_replace_string (config, "bootloader", &b_info->bootloader, 0); >> +#ifndef HAVE_PYGRUB >> + if (b_info->bootloader && >> + (!strcmp(b_info->bootloader, "pygrub") || >> + !strcmp(b_info->bootloader, "/usr/bin/pygrub"))) { > > And no other path combinations can occur? strstr() is perhaps too lax, > but what about finding the last slash (if any) and comparing the rest > of the string (the full string when there's no slash) against "pygrub"? "pygrub" is the preferred variant, "/usr/bin/pygrub" seems to be the legacy variant, which will result in a warning to use "pygrub" only (in case pygrub is enabled, of course). I don't think we should test for other non-standard paths. > >> + fprintf(stderr, "ERROR: this instance of Xen has been built without support of \"pygrub\".\n"); > > The other question (I'm sorry for my ignorance here) is whether pygrub > could come from anywhere else. It would be possible to use that in case it is e.g. installed in /usr/local/bin/pygrub (assuming the check above isn't modified). Juergen
On 08.08.2023 15:36, Juergen Gross wrote: > On 08.08.23 15:32, Jan Beulich wrote: >> On 08.08.2023 15:22, Juergen Gross wrote: >>> --- a/tools/xl/xl_parse.c >>> +++ b/tools/xl/xl_parse.c >>> @@ -1692,6 +1692,15 @@ void parse_config_data(const char *config_source, >>> xlu_cfg_get_defbool(config, "acpi", &b_info->acpi, 0); >>> >>> xlu_cfg_replace_string (config, "bootloader", &b_info->bootloader, 0); >>> +#ifndef HAVE_PYGRUB >>> + if (b_info->bootloader && >>> + (!strcmp(b_info->bootloader, "pygrub") || >>> + !strcmp(b_info->bootloader, "/usr/bin/pygrub"))) { >> >> And no other path combinations can occur? strstr() is perhaps too lax, >> but what about finding the last slash (if any) and comparing the rest >> of the string (the full string when there's no slash) against "pygrub"? > > "pygrub" is the preferred variant, "/usr/bin/pygrub" seems to be the > legacy variant, which will result in a warning to use "pygrub" only > (in case pygrub is enabled, of course). > > I don't think we should test for other non-standard paths. > >> >>> + fprintf(stderr, "ERROR: this instance of Xen has been built without support of \"pygrub\".\n"); >> >> The other question (I'm sorry for my ignorance here) is whether pygrub >> could come from anywhere else. > > It would be possible to use that in case it is e.g. installed in > /usr/local/bin/pygrub (assuming the check above isn't modified). Well, okay, I'll leave this to Anthony then. Jan
On Tue, Aug 08, 2023 at 03:22:19PM +0200, Juergen Gross wrote: > In case Xen has been configured with "--disable-pygrub", don't accept > the domain config option "bootloader=pygrub". > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Juergen Gross <jgross@suse.com> I'm unsure about this patch, but I guess we kind of expect pygrub to be built at the same time as `xl'. And it still allow to use "pygrub" if one specify the full path to it. So, Acked-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 1a5556d3bb..0e8c604bbf 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1692,6 +1692,15 @@ void parse_config_data(const char *config_source, xlu_cfg_get_defbool(config, "acpi", &b_info->acpi, 0); xlu_cfg_replace_string (config, "bootloader", &b_info->bootloader, 0); +#ifndef HAVE_PYGRUB + if (b_info->bootloader && + (!strcmp(b_info->bootloader, "pygrub") || + !strcmp(b_info->bootloader, "/usr/bin/pygrub"))) { + fprintf(stderr, "ERROR: this instance of Xen has been built without support of \"pygrub\".\n"); + exit(-ERROR_FAIL); + } +#endif + switch (xlu_cfg_get_list_as_string_list(config, "bootloader_args", &b_info->bootloader_args, 1)) { case 0:
In case Xen has been configured with "--disable-pygrub", don't accept the domain config option "bootloader=pygrub". Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - new patch --- tools/xl/xl_parse.c | 9 +++++++++ 1 file changed, 9 insertions(+)