Upcoming changes will use different LIBXL_API_VERSION variants.
Prepare libxl_domain_create_restore, which got a new parameter
"send_back_fd" in Xen 4.7. libvirt does not use this parameter.
No functional change intended.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
src/libxl/libxl_api.h | 44 ++++++++++++++++++++++++++++++++++++++++
src/libxl/libxl_domain.c | 5 +++--
2 files changed, 47 insertions(+), 2 deletions(-)
create mode 100644 src/libxl/libxl_api.h
diff --git a/src/libxl/libxl_api.h b/src/libxl/libxl_api.h
new file mode 100644
index 0000000000..64ccd7a428
--- /dev/null
+++ b/src/libxl/libxl_api.h
@@ -0,0 +1,44 @@
+/*
+ * libxl_api.h: handle various libxl API variants
+ *
+ * Copyright (C) 2021 SUSE LLC
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include <libxl.h>
+
+static inline int
+Libxl_Domain_Create_Restore(libxl_ctx *ctx,
+ libxl_domain_config *d_config,
+ uint32_t *domid,
+ int restore_fd,
+ const libxl_domain_restore_params *params,
+ const libxl_asyncprogress_how *aop_console_how)
+{
+ int ret;
+
+#if LIBXL_API_VERSION < 0x040700
+ ret = libxl_domain_create_restore(ctx, d_config, domid, restore_fd, params,
+ NULL, aop_console_how);
+#else
+ ret = libxl_domain_create_restore(ctx, d_config, domid, restore_fd, -1,
+ params, NULL, aop_console_how);
+#endif
+
+ return ret;
+}
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 014f6aceca..2fe1d34e19 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -22,6 +22,7 @@
#include <fcntl.h>
+#include "libxl_api.h"
#include "libxl_domain.h"
#include "libxl_capabilities.h"
@@ -1396,8 +1397,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
#ifdef LIBXL_HAVE_SRM_V2
params.stream_version = restore_ver;
#endif
- ret = libxl_domain_create_restore(cfg->ctx, &d_config, &domid,
- restore_fd, ¶ms, NULL,
+ ret = Libxl_Domain_Create_Restore(cfg->ctx, &d_config, &domid,
+ restore_fd, ¶ms,
&aop_console_how);
libxl_domain_restore_params_dispose(¶ms);
}
On 3/17/21 4:57 AM, Olaf Hering wrote:
> Upcoming changes will use different LIBXL_API_VERSION variants.
>
> Prepare libxl_domain_create_restore, which got a new parameter
> "send_back_fd" in Xen 4.7. libvirt does not use this parameter.
>
> No functional change intended.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
> src/libxl/libxl_api.h | 44 ++++++++++++++++++++++++++++++++++++++++
> src/libxl/libxl_domain.c | 5 +++--
> 2 files changed, 47 insertions(+), 2 deletions(-)
> create mode 100644 src/libxl/libxl_api.h
>
> diff --git a/src/libxl/libxl_api.h b/src/libxl/libxl_api.h
> new file mode 100644
> index 0000000000..64ccd7a428
> --- /dev/null
> +++ b/src/libxl/libxl_api.h
What do you think of a more descriptive name for this file? E.g.
libxl_api_quirks.h or libxl_api_wrap.h?
> @@ -0,0 +1,44 @@
> +/*
> + * libxl_api.h: handle various libxl API variants
Or maybe libxl_api_variants.h?
> + *
> + * Copyright (C) 2021 SUSE LLC
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#pragma once
> +
> +#include <libxl.h>
> +
> +static inline int
> +Libxl_Domain_Create_Restore(libxl_ctx *ctx,
libvirt doesn't use this format for function names. See 'Function names' under
naming conventions
https://libvirt.org/coding-style.html#naming-conventions
I struggled with naming when toying with this in the past. Maybe
libxlDomainCreateRestoreWrap? The 'Wrap' suffix compliments the libxl_api_wrap.h
name suggestion.
Regards,
Jim
> + libxl_domain_config *d_config,
> + uint32_t *domid,
> + int restore_fd,
> + const libxl_domain_restore_params *params,
> + const libxl_asyncprogress_how *aop_console_how)
> +{
> + int ret;
> +
> +#if LIBXL_API_VERSION < 0x040700
> + ret = libxl_domain_create_restore(ctx, d_config, domid, restore_fd, params,
> + NULL, aop_console_how);
> +#else
> + ret = libxl_domain_create_restore(ctx, d_config, domid, restore_fd, -1,
> + params, NULL, aop_console_how);
> +#endif
> +
> + return ret;
> +}
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 014f6aceca..2fe1d34e19 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -22,6 +22,7 @@
>
> #include <fcntl.h>
>
> +#include "libxl_api.h"
> #include "libxl_domain.h"
> #include "libxl_capabilities.h"
>
> @@ -1396,8 +1397,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
> #ifdef LIBXL_HAVE_SRM_V2
> params.stream_version = restore_ver;
> #endif
> - ret = libxl_domain_create_restore(cfg->ctx, &d_config, &domid,
> - restore_fd, ¶ms, NULL,
> + ret = Libxl_Domain_Create_Restore(cfg->ctx, &d_config, &domid,
> + restore_fd, ¶ms,
> &aop_console_how);
> libxl_domain_restore_params_dispose(¶ms);
> }
>
Am Thu, 18 Mar 2021 16:26:14 -0600 schrieb Jim Fehlig <jfehlig@suse.com>: > Maybe libxlDomainCreateRestoreWrap? > The 'Wrap' suffix compliments the libxl_api_wrap.h name suggestion. "Naming conventions" does not cover API wrapping. Some of the names are already taken, like libxl_domain_shutdown/libxlDomainShutdown. This is the reason why I just used uppercase for the first letter of the libxl function. I assume virLibxlDomainShutdown may work, because in the end the wrappers are a libvirt thing. Olaf
On 3/18/21 5:00 PM, Olaf Hering wrote: > Am Thu, 18 Mar 2021 16:26:14 -0600 > schrieb Jim Fehlig <jfehlig@suse.com>: > >> Maybe libxlDomainCreateRestoreWrap? >> The 'Wrap' suffix compliments the libxl_api_wrap.h name suggestion. > > "Naming conventions" does not cover API wrapping. I was referring to the use of '_' in the names. From the coding style doc: "Underscores should not be used in function names". The style doc doesn't dictate the words used to form function names, but does suggest a vir$object$verb$subject pattern. > Some of the names are already taken, like libxl_domain_shutdown/libxlDomainShutdown. In hindsight I would have probably used the 'vir' prefix in the driver entry points, e.g. virlibxlDomainShutdown (libxl_driver.c), giving some flexibility for driver-internal function naming. There is nothing preventing such change now, other than the future annoyance of backport conflicts. > This is the reason why I just used uppercase for the first letter of the libxl function. > > I assume virLibxlDomainShutdown may work, because in the end the wrappers are a libvirt thing. Unless someone listening has a better idea, I lean towards libxl_api_wrapper.h with function names libxl*Wrapper. Jim
Am Thu, 18 Mar 2021 21:51:18 -0600 schrieb Jim Fehlig <jfehlig@suse.com>: > Unless someone listening has a better idea, I lean towards libxl_api_wrapper.h > with function names libxl*Wrapper. I will rename the header, adjust the two style issues, and use these function names: libxlDomainCreateRestoreWrapper libxlRetrieveDomainConfigurationWrapper libxlDomainShutdownWrapper libxlDomainRebootWrapper libxlDomainPauseWrapper libxlDomainUnpauseWrapper libxlDomainNeedMemoryWrapper libxlGetFreeMemoryWrapper libxlSetVcpuonlineWrapper libxlSendTriggerWrapper libxlSetMemoryTargetWrapper Olaf
On Thu, Mar 18, 2021 at 09:51:18PM -0600, Jim Fehlig wrote: > On 3/18/21 5:00 PM, Olaf Hering wrote: > > Am Thu, 18 Mar 2021 16:26:14 -0600 > > schrieb Jim Fehlig <jfehlig@suse.com>: > > > > > Maybe libxlDomainCreateRestoreWrap? > > > The 'Wrap' suffix compliments the libxl_api_wrap.h name suggestion. > > > > "Naming conventions" does not cover API wrapping. > > I was referring to the use of '_' in the names. From the coding style doc: > "Underscores should not be used in function names". The style doc doesn't > dictate the words used to form function names, but does suggest a > vir$object$verb$subject pattern. > > > Some of the names are already taken, like libxl_domain_shutdown/libxlDomainShutdown. > > In hindsight I would have probably used the 'vir' prefix in the driver entry > points, e.g. virlibxlDomainShutdown (libxl_driver.c), giving some > flexibility for driver-internal function naming. There is nothing preventing > such change now, other than the future annoyance of backport conflicts. FWIW, in retrospect, I think we shouldn't have used "libxl" as a naming convention anywhere in libvirt - neither filenames or method names. This is a Xen driver, and libxl is just an impl detail. IOW, I we ought to have just use "virXen" as the method name / typedef prefix, and xen_driver.c as filename, etc. Obviously we avoided this originally to distinguish the new impl from the old XenD, but I think that was a mistake in retrospect, as we optimized for something that was only going to exist for a few further years, as opposed to optimizing for the long term where the libxl impl is the only one. I don't feel strongly about whether you stick with current naming conventions of change it to anything else - just wanted to throw this out there as a option. 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 :|
On 3/22/21 4:28 AM, Daniel P. Berrangé wrote:
> On Thu, Mar 18, 2021 at 09:51:18PM -0600, Jim Fehlig wrote:
>> On 3/18/21 5:00 PM, Olaf Hering wrote:
>>> Am Thu, 18 Mar 2021 16:26:14 -0600
>>> schrieb Jim Fehlig <jfehlig@suse.com>:
>>>
>>>> Maybe libxlDomainCreateRestoreWrap?
>>>> The 'Wrap' suffix compliments the libxl_api_wrap.h name suggestion.
>>>
>>> "Naming conventions" does not cover API wrapping.
>>
>> I was referring to the use of '_' in the names. From the coding style doc:
>> "Underscores should not be used in function names". The style doc doesn't
>> dictate the words used to form function names, but does suggest a
>> vir$object$verb$subject pattern.
>>
>>> Some of the names are already taken, like libxl_domain_shutdown/libxlDomainShutdown.
>>
>> In hindsight I would have probably used the 'vir' prefix in the driver entry
>> points, e.g. virlibxlDomainShutdown (libxl_driver.c), giving some
>> flexibility for driver-internal function naming. There is nothing preventing
>> such change now, other than the future annoyance of backport conflicts.
>
> FWIW, in retrospect, I think we shouldn't have used "libxl" as a naming
> convention anywhere in libvirt - neither filenames or method names. This
> is a Xen driver, and libxl is just an impl detail. IOW, I we ought to
> have just use "virXen" as the method name / typedef prefix, and
> xen_driver.c as filename, etc.
Agreed. I had a memory lapse about our previous discussions around s/libxl/xen/,
e.g.
https://listman.redhat.com/archives/libvir-list/2020-May/msg00081.html
It would have all come back if I started doing the work. Just thinking about it
again reminds me of all the "difficult" places libxl has leaked. I mentioned the
build option in above thread, but there's also deployment (/etc/libvirt/libxl*)
and runtime (/var/{lib,log,run}/libvirt/libxl*) leakage. If starting on such an
adventure I'm not sure where to stop. Handling the deployment/runtime leakages
likely requires symlinks, %post{un,trans} hacks, etc.
I'd be interested in the basic strategy you (or others) would take if embarking
on such adventure :-).
Regards,
Jim
© 2016 - 2026 Red Hat, Inc.