Re: [Libguestfs] [libnbd PATCH v3 07/29] lib/utils: add async-signal-safe assert()

[Laszlo Ersek]

On 2/20/23 19:21, Laszlo Ersek wrote:
> On 2/15/23 21:57, Eric Blake wrote:
>> On Wed, Feb 15, 2023 at 03:11:36PM +0100, Laszlo Ersek wrote:

>>> +
>>> +  xwrite (STDERR_FILENO, file, strlen (file));
>>> +  xwrite (STDERR_FILENO, ":", 1);
>>
>> Presumably, if our first best-effort xwrite() fails to produce full
>> output, all later xwrite() will hopefully hit the same error condition
>> so that we aren't producing even more-mangled output where later
>> syscalls succeed despite missing context earlier in the overall
>> output.  If it were something we truly wanted to worry about, the
>> solution would be pre-loading the entire message into a single buffer,
>> then calling xwrite() just once - but that's far more effort for
>> something we don't anticipate hitting in normal usage anyways.  I'm
>> happy if you ignore this whole paragraph of mine.
> 
> Any single buffer presents the problem of sizing the buffer
> appropriately, which we can't do in this context :)

Actually, we *can* do better:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/writev.html

What we're doing here is the textbook use case for scatter-gather (well,
in this case, gather). It's strange that it has taken me one night to
realize this (it occurred to me before falling asleep last night), given
that I heavily used scatter-gather in other, not-so-old, code. (Namely
the edk2 virtiofs driver.)

I'll attempt to replace xwrite() with xwritev().

Laszlo
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 01/29] use space consistently in function and function-like macro invocations

[Richard W.M. Jones]

On Mon, Feb 20, 2023 at 02:38:25PM +0100, Laszlo Ersek wrote:
> I certainly don't want to dismiss these observations, but I'll
> definitely forget about them unless we record them somewhere. Do these
> deserve a bugzilla (or multiple bugzillas)?

You could drop a note in the TODO file.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built

[Eric Blake]

On Mon, Feb 20, 2023 at 09:08:08PM +0200, Nir Soffer wrote:
> On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek  wrote:
> >
> > On 2/17/23 17:52, Eric Blake wrote:
> > > On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
> >
> > >> - Py_BuildValue with the "O" format specifier transfers the new list's
> > >> *sole* reference (= ownership) to the just-built higher-level object 
> > >> "args"
> > >
> > > Reference transfer is done with "N", not "O".  That would be an
> > > alternative to decreasing the refcount of py_array on success, but not
> > > eliminate the need to decrease the refcount on Py_BuildValue failure.
> > >
> > >>
> > >> - when "args" is killed (decref'd), it takes care of "py_array".
> > >>
> > >> Consequently, if Py_BuildValue fails, "py_array" continues owning the
> > >> new list -- and I believe that, if we take the new error branch, we leak
> > >> the object pointed-to by "py_array". Is that the case?
> > >
> > > Not quite.  "O" is different than "N".
> >
> > I agree with you *now*, looking up the "O" specification at
> > .
> >
> > However, when I was writing my email, I looked up Py_BuildValue at that
> > time as well, just elsewhere. I don't know where. Really. And then that
> > documentation said that the reference count would *not* be increased. I
> > distinctly remember that, because it surprised me -- I actually recalled
> > an *even earlier* experience reading the documentation, which had again
> > stated that "O" would increase the reference count.
> 
> Maybe here:
> https://docs.python.org/2/c-api/arg.html#building-values
> 
> Looks like another incompatibility between python 2 and 3.

Or maybe misreading the wrong part of the page.  PyArg_ParseTuple()
and Py_BuildValue() are listed on the same page, and intentionally use
similar-looking format strings, so you have to check _which_ part of
the page the "O" you are reading about is associated with.  The first
"O" is under PyArg_ParseTuple() and friends, and intentionally does
not increase reference count (the usesr passed us an Object, we are
parsing it into a placeholder where we don't need to clean up after
ourselves unless we want to add a reference to the object to make it
last beyond the caller), the latter says that "O" does increase the
reference count (we are building up a larger object that will now be
an additional reference path into the object we are passing in).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 07/29] lib/utils: add async-signal-safe assert()

[Eric Blake]

On Mon, Feb 20, 2023 at 07:21:05PM +0100, Laszlo Ersek wrote:
> On 2/15/23 21:57, Eric Blake wrote:
> > On Wed, Feb 15, 2023 at 03:11:36PM +0100, Laszlo Ersek wrote:
> >> Add an assert() variant that we may call between fork() and exec*().
> >>
> >> Signed-off-by: Laszlo Ersek 
> >> ---
> >>
> > 
> >> +++ b/lib/internal.h
> > 
> >> +
> >> +#ifdef NDEBUG
> >> +#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression) ((void)0)
> > 
> > May be affected by outcome of side discussion on 01/29 about whether
> > we want space after cast.
> > 
> >> +#else
> >> +#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression)\
> >> +  (nbd_internal_fork_safe_assert ((expression) != 0, __FILE__, __LINE__, \
> >> +  __func__, #expression))
> >> +#endif
> >> +
> >>  #endif /* LIBNBD_INTERNAL_H */
> > 
> >> +++ b/lib/utils.c
> >> @@ -431,13 +431,35 @@ nbd_internal_socketpair (int domain, int type, int 
> >> protocol, int *fds)
> >>if (ret == 0) {
> >>  for (i = 0; i < 2; i++) {
> >>if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
> >>  close (fds[0]);
> >>  close (fds[1]);
> >>  return -1;
> >>}
> >>  }
> >>}
> >>  #endif
> >>  
> >>return ret;
> >>  }
> >> +
> >> +void
> >> +nbd_internal_fork_safe_assert (int result, const char *file, long line,
> >> +   const char *func, const char *assertion)
> >> +{
> >> +  const char *line_out;
> >> +  char line_buf[32];
> >> +
> >> +  if (result)
> >> +return;
> > 
> > Since no code should ever directly call
> > nbd_internal_fork_safe_assert(0,...), but instead go through our macro
> > that has already filtered on expression's value,
> 
> I either don't understand, or I disagree. With NDEBUG not defined, the
> NBD_INTERNAL_FORK_SAFE_ASSERT macro is always expanded to a call to
> nbd_internal_fork_safe_assert(). And the expression to check is
> evaluated in the first argument of that function call:
> 
> nbd_internal_fork_safe_assert ((expression) != 0, ...
> 
> So I think the early return is definitely necessary here.

Oh, I was confused.  I went back and re-read your macro; I guess I was
thinking it was something like:

do if (!(expression)) nbd_internal_fork_safe_assert (args); while (0)

where the function is only called on failure, but you instead wrote it
as:

nbd_internal_fork_safe_assert ((expression) != 0), args)

So I stand corrected - we do need the early exit here, because we are
unconditionally calling the function (when assertions are enabled) at
least in the current spelling of the macro.

...
> 
> > At any rate, your
> > implementation looks reasonable, and more to the point, satisfies your
> > desire of being async-signal-safe and thus appropriate between fork()
> > and exec().
> > 
> > Reviewed-by: Eric Blake 
> > 
> 
> Thanks!

My R-b still stands, and I think you cleared up my confusion without
having to change any code on your part.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY

[Eric Blake]

On Mon, Feb 20, 2023 at 06:03:13PM +0100, Laszlo Ersek wrote:
> On 2/15/23 21:27, Eric Blake wrote:
> > On Wed, Feb 15, 2023 at 03:11:34PM +0100, Laszlo Ersek wrote:
> >> The "name##_iter" function is used 11 times in libnbd; in all those cases,
> >> "name" is "string_vector", and the "f" callback is "free":
> >>
> >>   string_vector_iter (..., (void *) free);
> >>
> >> Casting "free" to (void*) is ugly. (Well-defined by POSIX, but still.)
> > 
> > Tangentially related: casting function pointers in general may get
> > harder as more compilers move towards C23 and its newer rules (see for
> > example
> > https://lists.gnu.org/archive/html/bug-gnulib/2023-02/msg00055.html or
> > this gcc 13 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108694
> > which highlights some of the warnings that newer compilers will start
> > warning us about).  While this patch focuses on avoiding casts between
> > fn(*)(type*) and void*, I don't know off-hand if C23 will permit
> > fn(*)(void*) as a compatible function pointer with fn(*)(type).
> 
> My understanding is that, per C99 at least, ret_type(*)(void*) is
> compatible with ret_type(*)(type*) precisely if void* is compatible with
> type* (6.7.5.3p15).
> 
> Whether void* is compatible with type* depends on... ugh, that's hard to
> deduce from the standard. 6.7.5.1p2 says, "For two pointer types to be
> compatible, both shall be identically qualified and both shall
> be pointers to compatible types". I don't think "void" (as a type in
> itself) is compatible with any type!
> 
> Now, there is one particular statement on void* -- 6.2.5p27 says, "A
> pointer to void shall have the same representation and alignment
> requirements as a pointer to a character type."
> 
> (I think the statements about *converting* void* to type* and vice versa
> do not apply here; AFAICT "compatibility" is about reinterpreting the
> bit patterns, not converting values.)
> 
> In Annex I (Common warnings, "informative"), the following is listed:
> "An implicit narrowing conversion is encountered, such as the assignment
> of [...] a pointer to void to a pointer to any type other than a
> character type".
> 
> All in all I don't think ret_type(*)(type*) is compatible with
> ret_type(*)(void*) in the general case, and that's why in this patch I
> didn't want to go more general than I absolutely needed to.

Thanks for at least trying to find something definitive in the
standard.  Now you know why I skipped researching this particular
issue - it's not straightforward to figure out when function pointers
with differing parameter types are compatible.

> > 
> > Thinking higher-level now, your new macro is something where we have
> > to do a two-step declaration of macro types where we want the new
> > function.  Would it make more sense to change the signature of the
> > DEFINE_VECTOR_TYPE() macro to take a third argument containing the
> > function name to call on cleanup paths, with the ability to easily
> > write/reuse a no-op function for vectors that don't need to call
> > free(), where we can then unconditionally declare name##_empty() that
> > will work with all vector types?  That is, should we consider instead
> > doing something like:
> > 
> > DEFINE_VECTOR_TYPE (string_vector, char *, free);
> > 
> > DEFINE_VECTOR_TYPE (int_vector, int, noop);
> 
> My counter-arguments:
> 
> - this requires updates to all existent DEFINE_VECTOR_TYPE macro
> invocations,
> 
> - with "noop" passed to _reset, _reset and _empty become effectively the
> same, so we get (at least partially) duplicate APIs,
> 
> - this would be a step towards combinatorial explosion
> 
> - if "noop" does nothing, then why call it on each element of the vector
> anyway? It's not only the function call that becomes superfluous in the
> loop bodym with the function being "noop", but the loop *itself* becomes
> superfluous. So then we might want to compare the function pointer
> against "noop" outside of the loop... and that way we get a bunch of
> complications :)
> 
> I chose this approach because it is purely additive and precisely as
> generic/specific as it needs to be. We already have 11 use cases, so I
> don't think it's *too* specific.

We may still want some division of:

DEFINE_VECTOR_TYPE (int_vector, int);
DEFINE_POINTER_VECTOR_TYPE (string_vector, char *, free);

where under the hood, DEFINE_POINTER_VECTOR_TYPE(type, base, fn)
invokes both DEFINE_VECTOR_TYPE(type, base) and
DEFINE_VECTOR_EMPTY(type, fn), or whatever we name the second
function.

>From your other mail in this subthread:

> >> +#define DEFINE_VECTOR_EMPTY(name)  \
> >
> > I'm going to be that guy ...
> 
> Yes, someone has to be! I knew it was virtually impossible for me to get
> the name right at the first try :)
> 
> >
> > Can we call it ADD_VECTOR_EMPTY_METHOD or similar/better?
> 
> Eric, any comments?

ADD_VECTOR_EMPTY_METHOD() instead of DEFINE_VECTOR_EMPTY() works for
me.  Whether we hard-code 'free()' as the lone 

Re: [Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built

[Nir Soffer]

On Mon, Feb 20, 2023 at 10:45 AM Laszlo Ersek  wrote:
>
> On 2/17/23 17:52, Eric Blake wrote:
> > On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
>
> >> - Py_BuildValue with the "O" format specifier transfers the new list's
> >> *sole* reference (= ownership) to the just-built higher-level object "args"
> >
> > Reference transfer is done with "N", not "O".  That would be an
> > alternative to decreasing the refcount of py_array on success, but not
> > eliminate the need to decrease the refcount on Py_BuildValue failure.
> >
> >>
> >> - when "args" is killed (decref'd), it takes care of "py_array".
> >>
> >> Consequently, if Py_BuildValue fails, "py_array" continues owning the
> >> new list -- and I believe that, if we take the new error branch, we leak
> >> the object pointed-to by "py_array". Is that the case?
> >
> > Not quite.  "O" is different than "N".
>
> I agree with you *now*, looking up the "O" specification at
> .
>
> However, when I was writing my email, I looked up Py_BuildValue at that
> time as well, just elsewhere. I don't know where. Really. And then that
> documentation said that the reference count would *not* be increased. I
> distinctly remember that, because it surprised me -- I actually recalled
> an *even earlier* experience reading the documentation, which had again
> stated that "O" would increase the reference count.

Maybe here:
https://docs.python.org/2/c-api/arg.html#building-values

Looks like another incompatibility between python 2 and 3.

Nir

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 07/29] lib/utils: add async-signal-safe assert()

[Laszlo Ersek]

On 2/15/23 21:57, Eric Blake wrote:
> On Wed, Feb 15, 2023 at 03:11:36PM +0100, Laszlo Ersek wrote:
>> Add an assert() variant that we may call between fork() and exec*().
>>
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
> 
>> +++ b/lib/internal.h
> 
>> +
>> +#ifdef NDEBUG
>> +#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression) ((void)0)
> 
> May be affected by outcome of side discussion on 01/29 about whether
> we want space after cast.
> 
>> +#else
>> +#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression)\
>> +  (nbd_internal_fork_safe_assert ((expression) != 0, __FILE__, __LINE__, \
>> +  __func__, #expression))
>> +#endif
>> +
>>  #endif /* LIBNBD_INTERNAL_H */
> 
>> +++ b/lib/utils.c
>> @@ -431,13 +431,35 @@ nbd_internal_socketpair (int domain, int type, int 
>> protocol, int *fds)
>>if (ret == 0) {
>>  for (i = 0; i < 2; i++) {
>>if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
>>  close (fds[0]);
>>  close (fds[1]);
>>  return -1;
>>}
>>  }
>>}
>>  #endif
>>  
>>return ret;
>>  }
>> +
>> +void
>> +nbd_internal_fork_safe_assert (int result, const char *file, long line,
>> +   const char *func, const char *assertion)
>> +{
>> +  const char *line_out;
>> +  char line_buf[32];
>> +
>> +  if (result)
>> +return;
> 
> Since no code should ever directly call
> nbd_internal_fork_safe_assert(0,...), but instead go through our macro
> that has already filtered on expression's value,

I either don't understand, or I disagree. With NDEBUG not defined, the
NBD_INTERNAL_FORK_SAFE_ASSERT macro is always expanded to a call to
nbd_internal_fork_safe_assert(). And the expression to check is
evaluated in the first argument of that function call:

nbd_internal_fork_safe_assert ((expression) != 0, ...

So I think the early return is definitely necessary here.

> this conditional is
> technically dead code; but I'm okay whether it stays in or gets
> removed.
> 
>> +
>> +  xwrite (STDERR_FILENO, file, strlen (file));
>> +  xwrite (STDERR_FILENO, ":", 1);
> 
> Presumably, if our first best-effort xwrite() fails to produce full
> output, all later xwrite() will hopefully hit the same error condition
> so that we aren't producing even more-mangled output where later
> syscalls succeed despite missing context earlier in the overall
> output.  If it were something we truly wanted to worry about, the
> solution would be pre-loading the entire message into a single buffer,
> then calling xwrite() just once - but that's far more effort for
> something we don't anticipate hitting in normal usage anyways.  I'm
> happy if you ignore this whole paragraph of mine.

Any single buffer presents the problem of sizing the buffer
appropriately, which we can't do in this context :)

> 
>> +  line_out = nbd_internal_fork_safe_itoa (line, line_buf, sizeof line_buf);
>> +  xwrite (STDERR_FILENO, line_out, strlen (line_out));
>> +  xwrite (STDERR_FILENO, ": ", 2);
>> +  xwrite (STDERR_FILENO, func, strlen (func));
>> +  xwrite (STDERR_FILENO, ": Assertion `", 13);
>> +  xwrite (STDERR_FILENO, assertion, strlen (assertion));
>> +  xwrite (STDERR_FILENO, "' failed.\n", 10);
> 
> These days, the quoting style `' seems to be disappearing in favor of
> ''.  But a quick test showed me that at least glibc 2.36 is still
> producing the message with the quotes as you have spelled it here.

Personally I dislike `', and like '' (and ""), but I specifically
modeled the message on the standard assert() failure message on RHEL9.

> 
>> +  abort ();
> 
> Huh. I checked the source to glibc's assert.c, where it includes a
> comment about having to work cleanly even if a second assertion ends
> up being raised from within a SIGABRT handler active during the first
> attempt to abort().  But POSIX says that abort() shall attempt to
> override any SIGABRT handler in place, so I wonder if glibc's
> implementation is a bit too defensive.

What I did was, I checked POSIX on assert(), and the spec there simply
says that assert() calls abort():

"When it is executed, if expression (which shall have a scalar type) is
false (that is, compares equal to 0), assert() shall write information
about the particular call that failed on stderr and shall call abort()."

> At any rate, your
> implementation looks reasonable, and more to the point, satisfies your
> desire of being async-signal-safe and thus appropriate between fork()
> and exec().
> 
> Reviewed-by: Eric Blake 
> 

Thanks!
Laszlo
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 01/29] use space consistently in function and function-like macro invocations

[Eric Blake]

On Mon, Feb 20, 2023 at 02:38:25PM +0100, Laszlo Ersek wrote:
> > 
> > [1] This change widened out beyond 80 columns.  Is it worth splitting
> > that typedef line in two, perhaps as:
> > 
> > typedef char NBDKIT_UNIQUE_NAME (_x_has_uint_type)   \
> > [(typeof (x))-1 > 0 ? 1 : -1] __attribute__ ((__unused__));  \
> > 
> > or does that make it look worse?  At any rate, even if we do want to
> > reflow the line to be shorter, you have to consider the commit message
> > blurb about 'git show -w'.
> 
> This patch concerns itself with one thing only: the space character
> between the function designator (or macro name) and the paren that opens
> the argument list. Everything else is out of scope for the patch.

Fully agreed.  Doing JUST spacing is the only thing appropriate for this patch.

> 
> In the particular case, the original NBDKIT_UNIQUE_NAME line was already
> 84 characters long; in fact that original line, when introduced, broke
> the alignment of the backslashes at the right hand side. That state
> stems from nbdkit (not libnbd) commit cf2b6297646a
> ("common/include/unique-name.h: Rename UNIQUE_NAME macro", 2022-01-11).
> It was later ported to libnbd in commit 485f5ddf2d48
> ("common/include/unique-name.h: Rename UNIQUE_NAME macro", 2022-02-23).
> 
> So, this patch highlights another pre-existent task, and creates a new one:
> 
> - the overlong lines from the above-noted commits should be cleaned up
> 
> - the whitespace updates from the present patch should be reflected back
> to nbdkit.
> 
> I was aware of both of those tasks, it's just that my cleanup stack
> simply couldn't accommodate further recursions. I was already cleaning
> up a thing for a cleanup that I needed for another cleanup. I couldn't
> nest them any deeper, I had to stop the scope creep somewhere.

Fair enough. We do indeed seem good at adding to our TODO list without
meaning to.  We don't need to hold up this series waiting for other
cleanup tasks to be done.

> 
> More generally, we should probably invent a way to avoid porting such
> utility code back and forth between libnbd and nbdkit. For example,
> libguestfs, guestfs-tools, and virt-v2v share the "common" submodule.

The idea makes sense to me, although it does add a bit of a pain (git
submodules are not always the easiest to work with).

> > 
> > Hmm. Another potential cleanup patch (NOT for this one) would be
> > settling on a preferred style for whether the cast prefix operator has
> > a following space.  For example, in copy/file-ops.c, we use both
> > styles (git grep ' \*) \?[a-zA-Z]') when casting to a single pointer
> > type (I didn't check double pointers or casts to a type name):
> > 
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *) rw;
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> > copy/file-ops.c:data = (char *) data + r;
> > copy/file-ops.c:  struct rw_file *rwf = (struct rw_file *)rw;
> > copy/file-ops.c:data = (char *) data + r;
> 
> Yes, good observation.
> 
> > 
> > I think I'm a fan of having the space after the cast operator, and as
> > long as we're doing tree-wide cleanups, this would be a good time to
> > inject such a patch if we wanted.
> 
> I actually dislike the space there; the reason being that the cast
> operator has one of the strongest bindings in C, and the space evokes
> the opposite impression. We've had actual bugs introduced in edk2
> because someone was misled by this.
> 
> (The cast prefix op is in the same group as "*", "&", "!", "~", and
> unary "-"; we don't use a space with those either.
> 
> ... Well, I've seen a space character after "!" in some spots, and I
> happen to strongly disagree with that, for the exact same reason -- but
> that's another discussion. :))

You actually make a strong argument for not having the space after a
cast.  Thinking about it more, that also means that visually, you can
distinguish between
 (foo) (bar, baz)
 (foo)(bar, baz)

where the former is a function call through a function pointer foo
with two arguments, while the latter is an (unusual) cast of the
result of the comma operator to type foo.  Not that I'd ever expect to
encounter code where this visual distinction makes the code easier to
read (and proof that parsing C is not trivial, because how to parse
depends on the compiler knowing a priori whether the name on the left
is a type name or a function pointer name).

> 
> > 
> > Also, it might be cool to have a code formatting tool automatically
> > check that patches conform to a given style (but that presupposes that
> > there is a tool out there that gives us a style we like, or where the
> > differences to our current style are minimal to instead go with one of
> > its builtin styles - AND that such a tool can be present on at least
> > one of the CI machines to avoid regressions).  But 

Re: [Libguestfs] [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY

[Laszlo Ersek]

On 2/15/23 21:27, Eric Blake wrote:
> On Wed, Feb 15, 2023 at 03:11:34PM +0100, Laszlo Ersek wrote:
>> The "name##_iter" function is used 11 times in libnbd; in all those cases,
>> "name" is "string_vector", and the "f" callback is "free":
>>
>>   string_vector_iter (..., (void *) free);
>>
>> Casting "free" to (void*) is ugly. (Well-defined by POSIX, but still.)
> 
> Tangentially related: casting function pointers in general may get
> harder as more compilers move towards C23 and its newer rules (see for
> example
> https://lists.gnu.org/archive/html/bug-gnulib/2023-02/msg00055.html or
> this gcc 13 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108694
> which highlights some of the warnings that newer compilers will start
> warning us about).  While this patch focuses on avoiding casts between
> fn(*)(type*) and void*, I don't know off-hand if C23 will permit
> fn(*)(void*) as a compatible function pointer with fn(*)(type).

My understanding is that, per C99 at least, ret_type(*)(void*) is
compatible with ret_type(*)(type*) precisely if void* is compatible with
type* (6.7.5.3p15).

Whether void* is compatible with type* depends on... ugh, that's hard to
deduce from the standard. 6.7.5.1p2 says, "For two pointer types to be
compatible, both shall be identically qualified and both shall
be pointers to compatible types". I don't think "void" (as a type in
itself) is compatible with any type!

Now, there is one particular statement on void* -- 6.2.5p27 says, "A
pointer to void shall have the same representation and alignment
requirements as a pointer to a character type."

(I think the statements about *converting* void* to type* and vice versa
do not apply here; AFAICT "compatibility" is about reinterpreting the
bit patterns, not converting values.)

In Annex I (Common warnings, "informative"), the following is listed:
"An implicit narrowing conversion is encountered, such as the assignment
of [...] a pointer to void to a pointer to any type other than a
character type".

All in all I don't think ret_type(*)(type*) is compatible with
ret_type(*)(void*) in the general case, and that's why in this patch I
didn't want to go more general than I absolutely needed to.

> 
>>
>> Furthermore, in all 11 cases, the freeing of the vector's strings is
>> immediately followed by the release of the vector's array-of-pointers too.
>> (This additional step is not expressed consistently across libnbd: it's
>> sometimes spelled as free(vec.ptr), sometimes as
>> string_vector_reset().)
> 
> As Rich pointed out, just because libnbd is always passing 'free' does
> not mean that nbdkit is doing likewise, and we want to keep this file
> shared between the two projects.  Being able to conditionally add the
> cleanup function (where we don't want it on non-pointer vectors) makes
> sense, but removing the iterator function at the same time is a step
> too far.

OK.

> 
>>
>> Remove the generic "name##_iter" function definition, and introduce
>> "name##_empty", which performs both steps at the same time. Convert the
>> call sites. (Note that the converted code performs more cleanup steps in
>> some cases than strictly necessary, but the extra work is harmless, and
>> arguably beneficial for clarity / consistency.)
> 
> Agreed that the extra cleanup is not going to affect our hot path.
> 
>>
>> Expose the "name##_empty" function definition with a new, separate macro:
>> DEFINE_VECTOR_EMPTY(). The existent DEFINE_VECTOR_TYPE() macro permits
>> such element types that are not pointers, or are pointers to const- and/or
>> volatile-qualified objects. Whereas "name##_empty" requires that the
>> elements be pointers to dynamically allocated, non-const, non-volatile
>> objects.
>>
>> Signed-off-by: Laszlo Ersek 
> 
>> +++ b/common/utils/vector.h
> 
>>  
>> +/* This macro should only be used if:
>> + * - the vector contains pointers, and
>> + * - the pointed-to objects are:
>> + *   - neither const- nor volatile-qualified, and
>> + *   - allocated with malloc() or equivalent.
>> + */
>> +#define DEFINE_VECTOR_EMPTY(name)  \
>> +  /* Call free() on each element of the vector, then reset the vector. \
>> +   */  \
>> +  static inline void __attribute__ ((__unused__))  \
>> +  name##_empty (name *v)   \
>> +  {\
>> +size_t i;  \
>> +for (i = 0; i < v->len; ++i)   \
>> +  free (v->ptr[i]);\
>> +name##_reset (v);  \
>> +  }
>> +
> 
> Thinking higher-level now, your new macro is something where we have
> to do a two-step declaration of macro types where we want the new
> function.  Would it make more sense to change 

Re: [Libguestfs] [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY

[Laszlo Ersek]

On 2/15/23 17:23, Richard W.M. Jones wrote:
> On Wed, Feb 15, 2023 at 03:11:34PM +0100, Laszlo Ersek wrote:
>> The "name##_iter" function is used 11 times in libnbd; in all those cases,
>> "name" is "string_vector", and the "f" callback is "free":
>>
>>   string_vector_iter (..., (void *) free);
>>
>> Casting "free" to (void*) is ugly. (Well-defined by POSIX, but still.)
>>
>> Furthermore, in all 11 cases, the freeing of the vector's strings is
>> immediately followed by the release of the vector's array-of-pointers too.
>> (This additional step is not expressed consistently across libnbd: it's
>> sometimes spelled as free(vec.ptr), sometimes as
>> string_vector_reset().)
>>
>> Remove the generic "name##_iter" function definition, and introduce
>> "name##_empty", which performs both steps at the same time. Convert the
>> call sites. (Note that the converted code performs more cleanup steps in
>> some cases than strictly necessary, but the extra work is harmless, and
>> arguably beneficial for clarity / consistency.)
>>
>> Expose the "name##_empty" function definition with a new, separate macro:
>> DEFINE_VECTOR_EMPTY(). The existent DEFINE_VECTOR_TYPE() macro permits
>> such element types that are not pointers, or are pointers to const- and/or
>> volatile-qualified objects. Whereas "name##_empty" requires that the
>> elements be pointers to dynamically allocated, non-const, non-volatile
>> objects.
>>
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  common/utils/string-vector.h |  1 +
>>  common/utils/vector.h| 27 +---
>>  generator/states-connect-socket-activation.c |  9 +++
>>  lib/handle.c | 12 +++--
>>  lib/utils.c  |  6 ++---
>>  common/utils/test-vector.c   |  3 +--
>>  info/show.c  |  3 +--
>>  7 files changed, 30 insertions(+), 31 deletions(-)
>>
>> diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h
>> index aa33fd48ceb5..78d0b4f9bf10 100644
>> --- a/common/utils/string-vector.h
>> +++ b/common/utils/string-vector.h
>> @@ -38,5 +38,6 @@
>>  #include "vector.h"
>>  
>>  DEFINE_VECTOR_TYPE (string_vector, char *);
>> +DEFINE_VECTOR_EMPTY (string_vector);
>>  
>>  #endif /* STRING_VECTOR_H */
>> diff --git a/common/utils/vector.h b/common/utils/vector.h
>> index fb2482c853ff..14bf5b0ddbc0 100644
>> --- a/common/utils/vector.h
>> +++ b/common/utils/vector.h
>> @@ -150,15 +150,6 @@
>>  v->len = v->cap = 0;\
>>} \
>>  \
>> -  /* Iterate over the vector, calling f() on each element. */   \
>> -  static inline void __attribute__ ((__unused__))   \
>> -  name##_iter (name *v, void (*f) (type elem))  \
>> -  { \
>> -size_t i;   \
>> -for (i = 0; i < v->len; ++i)\
>> -  f (v->ptr[i]);\
>> -  } \
>> -\
> 
> What's the reason for removing iter?  It's used by nbdkit in ways that
> don't involve free().  I'd prefer if we leave this definition in.

Good point -- the header is shared by nbdkit and libnbd, but (because
the latter are two separate projects) any single git-grep will cover
only one.

I'll restore _iter.

> 
>>/* Sort the elements of the vector. */\
>>static inline void __attribute__ ((__unused__))   \
>>name##_sort (name *v, \
>> @@ -180,6 +171,24 @@
>>  
>>  #define empty_vector { .ptr = NULL, .len = 0, .cap = 0 }
>>  
>> +/* This macro should only be used if:
>> + * - the vector contains pointers, and
>> + * - the pointed-to objects are:
>> + *   - neither const- nor volatile-qualified, and
>> + *   - allocated with malloc() or equivalent.
>> + */
>> +#define DEFINE_VECTOR_EMPTY(name)  \
> 
> I'm going to be that guy ...

Yes, someone has to be! I knew it was virtually impossible for me to get
the name right at the first try :)

> 
> Can we call it ADD_VECTOR_EMPTY_METHOD or similar/better?

Eric, any comments?

> Otherwise it seems like DEFINE_VECTOR_EMPTY is an alternative to
> DEFINE_VECTOR_TYPE (which I thought was the case initially), rather
> than something you have to call in addition to DEFINE_VECTOR_TYPE.
> 
> The rest seems fine.

Thanks!
Laszlo

> 
> Rich.
> 
>> +  /* Call free() on each element of the vector, then reset the vector. \
>> +   */

Re: [Libguestfs] [libnbd PATCH v3 02/29] generator/C.ml: use space consistently in func. and func.-like macro calls

[Laszlo Ersek]

On 2/15/23 20:54, Eric Blake wrote:
> On Wed, Feb 15, 2023 at 03:11:31PM +0100, Laszlo Ersek wrote:
>> Apply the ideas in the previous patch to the C-language bindings
>> generator.
>>
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  generator/C.ml | 20 ++--
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
> 
>>pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n";
>> -  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 12 /* gcc >= 12.0 
>> */\n";
>> -  pr "#define LIBNBD_ATTRIBUTE_NONNULL(...) 
>> __attribute__((__nonnull__(__VA_ARGS__)))\n";
>> +  pr "#if defined (__GNUC__) && LIBNBD_GCC_VERSION >= 12 /* gcc >= 12.0 
>> */\n";
>> +  pr "#define LIBNBD_ATTRIBUTE_NONNULL(...) __attribute__ ((__nonnull__ 
>> (__VA_ARGS__)))\n";
> 
> Not only are these lines long in our source, they are long in the
> generated libnbd.h.  I would not be opposed to a followup patch that
> tries harder at keeping the generated file under 80 columns (but there
> are already places where that is harder than this snippet - such as
> the generated declaration of nbd_aio_opt_list_meta_context), but it
> does not need to happen in this patch.

Right, thanks -- if we agree we should have a new BZ for these
subsequent tasks, I'll go over the comments again and capture them in
the BZ.

Laszlo

> 
> And while I mentioned in 01/29 about the possibility of a C code
> formatter as a CI step for *.[ch], it would be even harder to insist
> that generated code matches a given style (it's always a nice goal for
> generated files to be human-readable where possible, but I'd much
> rather sacrifice that if it gets in the way of actually implementing
> the generator concisely).
> 
> Reviewed-by: Eric Blake 
> 

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 03/29] socket activation: rename sa_(tmpdir|sockpath) to sact_(tmpdir|sockpath)

[Laszlo Ersek]

On 2/15/23 21:00, Eric Blake wrote:
> On Wed, Feb 15, 2023 at 03:11:32PM +0100, Laszlo Ersek wrote:
>>  in POSIX reserves the "sa_" prefix:
>>
>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02
>>
>> Let's use "sact_" instead.
>>
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  lib/internal.h   |  4 ++--
>>  generator/states-connect-socket-activation.c | 16 
>>  lib/handle.c | 12 ++--
>>  3 files changed, 16 insertions(+), 16 deletions(-)
> 
> Reviewed-by: Eric Blake 
> 
> Stepping on reserved names is something that you can often get away
> with (if you don't conflict now, how likely is it that libc will
> actually introduce a future symbol that will conflict later?), so this
> is change is more on the theoretical side.  But I have no objections,
> and as you proved, it's a very easy mechanical fix once we even bother
> to identify that reserved name clashes could be a potential porting
> issue.  I've probably spent more time writing this email than you did
> in creating the patch proper (if you exclude the subsequent time
> testing that it builds and passes 'make check-valgrind'...)

I've actually reviewed (eyeballed) this patch myself *multiple times*,
very carefully. Mechanical changes can be deceptive :) Implementing them
seems easy, but their impact can be surprising.

In fact, whenever I need to review such a patch for someone else, I tend
to redo it myself from zero, then compare the patches (or the resultant
trees).

> Reviewed-by: Eric Blake 
> 

Thanks!
Laszlo
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 01/29] use space consistently in function and function-like macro invocations

[Laszlo Ersek]

On 2/15/23 20:48, Eric Blake wrote:
> On Wed, Feb 15, 2023 at 03:11:30PM +0100, Laszlo Ersek wrote:
>> We intend to place a space character between the function designator and
>> the opening parenthesis in a function call. We've been executing on that
>> mostly consistently; fix the few exceptions now.
>>
>> The same convention should be applied to the invocations of function-like
>> macros, and to instances of "__attribute__ ((attr))". (The latter is
>> exemplified, although not consistently, by the GCC manual.) Implement
>> this, by inserting the necessary spaces.
>>
>> Furthermore, some compiler attributes take multiple parameters, such as
>> "nonnull". The GCC manual clearly shows that arguments passed to such
>> attributes may be separated with ", " as well, not just ",". Use the
>> former, as it's more readable.
>>
>> Finally, the C standard calls "defined" -- as in "#if defined identifier"
>> and (equivalently) "#if defined (identifier)" -- a unary preprocessing
>> operator. We can spell the parenthesized form as "defined (identifier)"
>> rather than "defined(identifier)", so choose the former.
>>
>> I collected the locations possibly missing spaces with:
>>
>>   git grep -EHn '\<[a-zA-Z0-9_]+\(' -- '*.c' '*.h'
>>
>> and then manually updated each as necessary.
>>
>> I didn't change occurrences in comments, except where the comment clearly
>> indicated copying and pasting an expression into new code.
>>
>> "git show -w" outputs nothing for this patch.
> 
> Which would not remain the case if we reflow a long line made longer
> (see [1] below).
> 
>>
>> The test suite passes.
>>
>> Signed-off-by: Laszlo Ersek 
>> ---
> 
>> +++ b/lib/internal.h
>> @@ -46,7 +46,7 @@
>>   * debug and error handling code out of hot paths, making the hot path
>>   * into common functions use less instruction cache.
>>   */
>> -#if defined(__GNUC__)
>> +#if defined (__GNUC__)
> 
> In my experience with GNU code (which this is not), the style I've
> seen there is to omit () whenever possible, as in:
> 
> #if defined __GNUC__
> 
> or even
> 
> #ifdef __GNUC__
> 
> ...
> 
>> +++ b/common/include/checked-overflow.h
>> @@ -46,7 +46,7 @@
>>  #ifndef NBDKIT_CHECKED_OVERFLOW_H
>>  #define NBDKIT_CHECKED_OVERFLOW_H
>>  
>> -#if !defined(__GNUC__) && !defined(__clang__)
>> +#if !defined (__GNUC__) && !defined (__clang__)
> 
> ...obviously, the shorter #ifdef version can't be used here, but it
> could still be shortened to:
> 
> #if !defined __GNUC__ && !defined __clang__
> 
>> @@ -173,10 +173,10 @@
>>   *
>>   * The expression "x" is not evaluated, unless it has variably modified 
>> type.
>>   */
>> -#define STATIC_ASSERT_UNSIGNED_INT(x)   
>> \
>> -  do {  
>> \
>> -typedef char NBDKIT_UNIQUE_NAME(_x_has_uint_type)[(typeof (x))-1 > 0 ? 
>> 1 : -1] \
>> -  __attribute__((__unused__));  
>> \
>> +#define STATIC_ASSERT_UNSIGNED_INT(x)   
>> \
>> +  do {  
>> \
>> +typedef char NBDKIT_UNIQUE_NAME (_x_has_uint_type)[(typeof (x))-1 > 0 ? 
>> 1 : -1] \
>> +  __attribute__ ((__unused__)); 
>> \
> 
> [1] This change widened out beyond 80 columns.  Is it worth splitting
> that typedef line in two, perhaps as:
> 
> typedef char NBDKIT_UNIQUE_NAME (_x_has_uint_type)   \
> [(typeof (x))-1 > 0 ? 1 : -1] __attribute__ ((__unused__));  \
> 
> or does that make it look worse?  At any rate, even if we do want to
> reflow the line to be shorter, you have to consider the commit message
> blurb about 'git show -w'.

This patch concerns itself with one thing only: the space character
between the function designator (or macro name) and the paren that opens
the argument list. Everything else is out of scope for the patch.

In the particular case, the original NBDKIT_UNIQUE_NAME line was already
84 characters long; in fact that original line, when introduced, broke
the alignment of the backslashes at the right hand side. That state
stems from nbdkit (not libnbd) commit cf2b6297646a
("common/include/unique-name.h: Rename UNIQUE_NAME macro", 2022-01-11).
It was later ported to libnbd in commit 485f5ddf2d48
("common/include/unique-name.h: Rename UNIQUE_NAME macro", 2022-02-23).

So, this patch highlights another pre-existent task, and creates a new one:

- the overlong lines from the above-noted commits should be cleaned up

- the whitespace updates from the present patch should be reflected back
to nbdkit.

I was aware of both of those tasks, it's just that my cleanup stack
simply couldn't accommodate further recursions. I was already cleaning
up a thing for a cleanup that I needed for another cleanup. I couldn't
nest them any deeper, I had to stop the scope creep somewhere.

More generally, we should probably 

Re: [Libguestfs] [PATCH v2v v2 3/3] -o qemu: Always use -cpu host unless overridden by source hypervisor

[Richard W.M. Jones]

Thanks, this is now upstream in:

2cf337c26e2b6a99aa3cf1acd88ce637fa8fbbf7
83a6400aeaa00f930bee2e8aa1f9d25156204605
e4a7f08c406a588d6034242ce14c5efc832cb476

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v3 2/2] python: Avoid leaking py_array and py_args in event callbacks

[Richard W.M. Jones]

Thanks for the detailed review.  This is now upstream in these two
commits:

e07304de86defb8d3a09902b5f9f7593fa9863ac
afa4d64fca1672734ec474d6e8a1eeed4b34c6f4

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v v2 3/3] -o qemu: Always use -cpu host unless overridden by source hypervisor

[Richard W.M. Jones]

On Mon, Feb 20, 2023 at 11:36:36AM +0100, Laszlo Ersek wrote:
> On 2/17/23 12:44, Richard W.M. Jones wrote:
> > As with the prior commit, prefer -cpu host for all guests (except when
> > we have more information from the source hypervisor).  Although there
> > is the disadvantage that -cpu host is non-migratable, in practice it
> > would be very difficult to live migrate a host launched using direct
> > qemu commands.
> > 
> > Note that after this change, gcaps_arch_min_version is basically an
> > informational field.  No output uses it, but it will appear in debug
> > output and there's the possibility we might use it for a future output
> > mode.
> > 
> > Thanks: Laszlo Ersek
> > ---
> >  lib/types.mli | 6 +-
> >  output/output_qemu.ml | 6 +-
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/types.mli b/lib/types.mli
> > index 743daa8a14..4a183dd3ae 100644
> > --- a/lib/types.mli
> > +++ b/lib/types.mli
> > @@ -268,7 +268,11 @@ type guestcaps = {
> >minimum version.  Notably RHEL >= 9 requires at least x86_64-v2.
> >  
> >If the guest is capable of running on QEMU's default VCPU model
> > -  for the architecture ([-cpu qemu64]) then this is set to [0]. *)
> > +  for the architecture ([-cpu qemu64]) then this is set to [0].
> > +
> > +  Note this capability is not actually used by any current output
> > +  mode.  It is retained in case we might use it in future, but we
> > +  might remove it if it is not used. *)
> >  
> >gcaps_virtio_1_0 : bool;
> >(** The guest supports the virtio devices that it does at the virtio-1.0
> > diff --git a/output/output_qemu.ml b/output/output_qemu.ml
> > index 491906ebf9..2bbacb6eda 100644
> > --- a/output/output_qemu.ml
> > +++ b/output/output_qemu.ml
> > @@ -175,11 +175,7 @@ module QEMU = struct
> >  
> >  arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L));
> >  
> > -(match source.s_cpu_model, guestcaps.gcaps_arch_min_version with
> > -  | None, 0 -> ()
> > -  | None, _ -> arg "-cpu" "host"
> > -  | Some model, _ -> arg "-cpu" model
> > -);
> > +arg "-cpu" (Option.default "host" source.s_cpu_model);
> 
> (Ah yes, we have our own Option module from
> common/mlstdutils/std_utils.ml*, not the "standard" one
> .)

We should switch to using the standard one, but in this case it
depends on OCaml 4.08.  RHEL 7 has OCaml 4.05 and RHEL 8 has
OCaml 4.07.

Nevertheless I checked now if our module is needlessly different from
the official one and there are a few things so it's worth slowly
converging ours to make it easier to drop in future and reduce
surprises.

> >  if source.s_vcpu > 1 then (
> >(match source.s_cpu_topology with
> 
> Reviewed-by: Laszlo Ersek 

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v v2 2/3] -o libvirt: Always use host-model unless overridden by source hypervisor

[Richard W.M. Jones]

On Mon, Feb 20, 2023 at 11:30:54AM +0100, Laszlo Ersek wrote:
> On 2/17/23 12:44, Richard W.M. Jones wrote:
> > In the case where the source hypervisor doesn't specify a CPU model,
> > previously we chose qemu64 (qemu's most basic model), except for a few
> > guests that we know won't work on qemu64, eg. RHEL 9 requires
> > x86_64-v2 where we use 
> > 
> > However we recently encountered an obscure KVM bug related to this
> > (https://bugzilla.redhat.com/show_bug.cgi?id=2168082).  Windows 11
> > thinks the qemu64 CPU model when booted on real AMD Milan is an AMD
> > Phenom and tried to apply an ancient erratum to it.  Since KVM didn't
> > emulate the correct MSRs for this it caused the guest to fail to boot.
> > 
> > After discussion upstream we can't see any reason not to give all
> > guests host-model.  This should fix the bug above and also generally
> > improve performance by allowing the guest to exploit all host
> > features.
> > 
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=2168082#c19
> > Related: 
> > https://listman.redhat.com/archives/libguestfs/2023-February/030624.html
> > Thanks: Laszlo Ersek, Dr. David Alan Gilbert, Daniel Berrangé
> > ---
> >  output/create_libvirt_xml.ml | 59 +---
> >  tests/test-v2v-i-ova.xml |  1 +
> >  2 files changed, 28 insertions(+), 32 deletions(-)
> > 
> > diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
> > index e9c6c8c150..1d77139b45 100644
> > --- a/output/create_libvirt_xml.ml
> > +++ b/output/create_libvirt_xml.ml
> > @@ -184,41 +184,36 @@ let create_libvirt_xml ?pool source inspect
> >  e "vcpu" [] [PCData (string_of_int source.s_vcpu)]
> >];
> >  
> > -  if source.s_cpu_model <> None ||
> > - guestcaps.gcaps_arch_min_version >= 1 ||
> > - source.s_cpu_topology <> None then (
> > -let cpu_attrs = ref []
> > -and cpu = ref [] in
> > +  let cpu_attrs = ref []
> > +  and cpu = ref [] in
> >  
> > -(match source.s_cpu_model with
> > - | None ->
> > - if guestcaps.gcaps_arch_min_version >= 1 then
> > -   List.push_back cpu_attrs ("mode", "host-model");
> > - | Some model ->
> > - List.push_back cpu_attrs ("match", "minimum");
> > - if model = "qemu64" then
> > -   List.push_back cpu_attrs ("check", "none");
> > - (match source.s_cpu_vendor with
> > -  | None -> ()
> > -  | Some vendor ->
> > -  List.push_back cpu (e "vendor" [] [PCData vendor])
> > - );
> > - List.push_back cpu (e "model" ["fallback", "allow"] [PCData 
> > model])
> > -);
> > -(match source.s_cpu_topology with
> > - | None -> ()
> > - | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } ->
> > -let topology_attrs = [
> > -  "sockets", string_of_int s_cpu_sockets;
> > -  "cores", string_of_int s_cpu_cores;
> > -  "threads", string_of_int s_cpu_threads;
> > -] in
> > -List.push_back cpu (e "topology" topology_attrs [])
> > -);
> > -
> > -List.push_back_list body [ e "cpu" !cpu_attrs !cpu ]
> > +  (match source.s_cpu_model with
> > +   | None ->
> > +  List.push_back cpu_attrs ("mode", "host-model");
> 
> The indentation of "List.push_back" is off by one space here, as far as
> I can tell (only 1 space used instead of 2). If possible, please fix it
> up when you push the patches.
> 
> Reviewed-by: Laszlo Ersek 

Thanks - fixed the indentation in my local copy.

Rich.

> Thanks!
> Laszlo
> 
> 
> 
> > +   | Some model ->
> > +   List.push_back cpu_attrs ("match", "minimum");
> > +   if model = "qemu64" then
> > + List.push_back cpu_attrs ("check", "none");
> > +   (match source.s_cpu_vendor with
> > +| None -> ()
> > +| Some vendor ->
> > +List.push_back cpu (e "vendor" [] [PCData vendor])
> > +   );
> > +   List.push_back cpu (e "model" ["fallback", "allow"] [PCData model])
> > +  );
> > +  (match source.s_cpu_topology with
> > +   | None -> ()
> > +   | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } ->
> > +  let topology_attrs = [
> > +"sockets", string_of_int s_cpu_sockets;
> > +"cores", string_of_int s_cpu_cores;
> > +"threads", string_of_int s_cpu_threads;
> > +  ] in
> > +  List.push_back cpu (e "topology" topology_attrs [])
> >);
> >  
> > +  List.push_back_list body [ e "cpu" !cpu_attrs !cpu ];
> > +
> >let uefi_firmware =
> >  match target_firmware with
> >  | TargetBIOS -> None
> > diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
> > index da1db473e5..e5907ea1cc 100644
> > --- a/tests/test-v2v-i-ova.xml
> > +++ b/tests/test-v2v-i-ova.xml
> > @@ -10,6 +10,7 @@
> >2097152
> >2097152
> >1
> > +  
> >
> >  
> >  

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - 

Re: [Libguestfs] [PATCH v3 2/2] python: Avoid leaking py_array and py_args in event callbacks

[Laszlo Ersek]

On 2/17/23 18:45, Richard W.M. Jones wrote:
> See also:
> 
> https://listman.redhat.com/archives/libguestfs/2023-February/030730.html
> https://listman.redhat.com/archives/libguestfs/2023-February/030745.html
> https://listman.redhat.com/archives/libguestfs/2023-February/030746.html
> 
> Fixes: commit 6ef5837e2d8c5d4d83eff51c0201eb2e08f719de
> Thanks: Laszlo Ersek, Eric Blake
> ---
>  python/handle.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/python/handle.c b/python/handle.c
> index 8eeabe60a7..85089e6bce 100644
> --- a/python/handle.c
> +++ b/python/handle.c
> @@ -134,6 +134,7 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
>args = Py_BuildValue ("(Kiy#O)",
>  (unsigned PY_LONG_LONG) event, event_handle,
>  buf, buf_len, py_array);
> +  Py_DECREF (py_array);
>if (args == NULL) {
>  PyErr_PrintEx (0);
>  goto out;

I *think* doing it like this is safe in this instance.

I got concerned for a minute because of:

https://docs.python.org/3/c-api/refcounting.html?highlight=py_decref#c.Py_DECREF

Warning

The deallocation function can cause arbitrary Python code to be
invoked (e.g. when a class instance with a __del__() method is
deallocated). While exceptions in such code are not propagated, the
executed code has free access to all Python global variables. This
means that any object that is reachable from a global variable
should be in a consistent state before Py_DECREF() is invoked. For
example, code to delete an object from a list should copy a
reference to the deleted object in a temporary variable, update the
list data structure, and then call Py_DECREF() for the temporary
variable.

and:

https://docs.python.org/3/c-api/exceptions.html?highlight=pyerr_printex#c.PyErr_PrintEx

Call this function only when the error indicator is set. Otherwise
it will cause a fatal error!

So I had two concerns:

- can Py_DECREF() clear the error indicator?

- can Py_DECREF() lead to the exception (pending form Py_BuildValue())
being replaced (masked?) with a different exception?

I think the code is OK:

- My reading of the docs implies that the error indicator only gets
cleared with explicit PyErr_Clear() or PyErr_PrintEx() calls.

- We're only releasing a simple list of longs, so no particular
exception should be expected while destructing that.

And the testing described in the cover letter confirms this.

Reviewed-by: Laszlo Ersek 

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v3 1/2] Revert "python: fix call of Python handlers of events"

[Laszlo Ersek]

On 2/17/23 18:45, Richard W.M. Jones wrote:
> This reverts commit 85235aec837716f1ddb2926b9a59a02543195500.
> ---
>  python/handle.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/python/handle.c b/python/handle.c
> index bf639b5789..8eeabe60a7 100644
> --- a/python/handle.c
> +++ b/python/handle.c
> @@ -139,7 +139,6 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
>  goto out;
>}
>  
> -  Py_INCREF (args);
>py_r = PyObject_CallObject (py_callback, args);
>Py_DECREF (args);
>if (py_r != NULL)

The patch looks good, but I think the commit's subject line, and the
commit message, should be a bit more clear / verbose:

- we're only reverting a part of commit
85235aec837716f1ddb2926b9a59a02543195500

- the reason for doing this is that Py_BuildValue() always returns a new
reference

(when it succeeds).

With those notes:

Reviewed-by: Laszlo Ersek 

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v v2 3/3] -o qemu: Always use -cpu host unless overridden by source hypervisor

[Laszlo Ersek]

On 2/17/23 12:44, Richard W.M. Jones wrote:
> As with the prior commit, prefer -cpu host for all guests (except when
> we have more information from the source hypervisor).  Although there
> is the disadvantage that -cpu host is non-migratable, in practice it
> would be very difficult to live migrate a host launched using direct
> qemu commands.
> 
> Note that after this change, gcaps_arch_min_version is basically an
> informational field.  No output uses it, but it will appear in debug
> output and there's the possibility we might use it for a future output
> mode.
> 
> Thanks: Laszlo Ersek
> ---
>  lib/types.mli | 6 +-
>  output/output_qemu.ml | 6 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/types.mli b/lib/types.mli
> index 743daa8a14..4a183dd3ae 100644
> --- a/lib/types.mli
> +++ b/lib/types.mli
> @@ -268,7 +268,11 @@ type guestcaps = {
>minimum version.  Notably RHEL >= 9 requires at least x86_64-v2.
>  
>If the guest is capable of running on QEMU's default VCPU model
> -  for the architecture ([-cpu qemu64]) then this is set to [0]. *)
> +  for the architecture ([-cpu qemu64]) then this is set to [0].
> +
> +  Note this capability is not actually used by any current output
> +  mode.  It is retained in case we might use it in future, but we
> +  might remove it if it is not used. *)
>  
>gcaps_virtio_1_0 : bool;
>(** The guest supports the virtio devices that it does at the virtio-1.0
> diff --git a/output/output_qemu.ml b/output/output_qemu.ml
> index 491906ebf9..2bbacb6eda 100644
> --- a/output/output_qemu.ml
> +++ b/output/output_qemu.ml
> @@ -175,11 +175,7 @@ module QEMU = struct
>  
>  arg "-m" (Int64.to_string (source.s_memory /^ 1024L /^ 1024L));
>  
> -(match source.s_cpu_model, guestcaps.gcaps_arch_min_version with
> -  | None, 0 -> ()
> -  | None, _ -> arg "-cpu" "host"
> -  | Some model, _ -> arg "-cpu" model
> -);
> +arg "-cpu" (Option.default "host" source.s_cpu_model);

(Ah yes, we have our own Option module from
common/mlstdutils/std_utils.ml*, not the "standard" one
.)

>  
>  if source.s_vcpu > 1 then (
>(match source.s_cpu_topology with

Reviewed-by: Laszlo Ersek 


___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v v2 2/3] -o libvirt: Always use host-model unless overridden by source hypervisor

[Laszlo Ersek]

On 2/17/23 12:44, Richard W.M. Jones wrote:
> In the case where the source hypervisor doesn't specify a CPU model,
> previously we chose qemu64 (qemu's most basic model), except for a few
> guests that we know won't work on qemu64, eg. RHEL 9 requires
> x86_64-v2 where we use 
> 
> However we recently encountered an obscure KVM bug related to this
> (https://bugzilla.redhat.com/show_bug.cgi?id=2168082).  Windows 11
> thinks the qemu64 CPU model when booted on real AMD Milan is an AMD
> Phenom and tried to apply an ancient erratum to it.  Since KVM didn't
> emulate the correct MSRs for this it caused the guest to fail to boot.
> 
> After discussion upstream we can't see any reason not to give all
> guests host-model.  This should fix the bug above and also generally
> improve performance by allowing the guest to exploit all host
> features.
> 
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=2168082#c19
> Related: 
> https://listman.redhat.com/archives/libguestfs/2023-February/030624.html
> Thanks: Laszlo Ersek, Dr. David Alan Gilbert, Daniel Berrangé
> ---
>  output/create_libvirt_xml.ml | 59 +---
>  tests/test-v2v-i-ova.xml |  1 +
>  2 files changed, 28 insertions(+), 32 deletions(-)
> 
> diff --git a/output/create_libvirt_xml.ml b/output/create_libvirt_xml.ml
> index e9c6c8c150..1d77139b45 100644
> --- a/output/create_libvirt_xml.ml
> +++ b/output/create_libvirt_xml.ml
> @@ -184,41 +184,36 @@ let create_libvirt_xml ?pool source inspect
>  e "vcpu" [] [PCData (string_of_int source.s_vcpu)]
>];
>  
> -  if source.s_cpu_model <> None ||
> - guestcaps.gcaps_arch_min_version >= 1 ||
> - source.s_cpu_topology <> None then (
> -let cpu_attrs = ref []
> -and cpu = ref [] in
> +  let cpu_attrs = ref []
> +  and cpu = ref [] in
>  
> -(match source.s_cpu_model with
> - | None ->
> - if guestcaps.gcaps_arch_min_version >= 1 then
> -   List.push_back cpu_attrs ("mode", "host-model");
> - | Some model ->
> - List.push_back cpu_attrs ("match", "minimum");
> - if model = "qemu64" then
> -   List.push_back cpu_attrs ("check", "none");
> - (match source.s_cpu_vendor with
> -  | None -> ()
> -  | Some vendor ->
> -  List.push_back cpu (e "vendor" [] [PCData vendor])
> - );
> - List.push_back cpu (e "model" ["fallback", "allow"] [PCData model])
> -);
> -(match source.s_cpu_topology with
> - | None -> ()
> - | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } ->
> -let topology_attrs = [
> -  "sockets", string_of_int s_cpu_sockets;
> -  "cores", string_of_int s_cpu_cores;
> -  "threads", string_of_int s_cpu_threads;
> -] in
> -List.push_back cpu (e "topology" topology_attrs [])
> -);
> -
> -List.push_back_list body [ e "cpu" !cpu_attrs !cpu ]
> +  (match source.s_cpu_model with
> +   | None ->
> +  List.push_back cpu_attrs ("mode", "host-model");

The indentation of "List.push_back" is off by one space here, as far as
I can tell (only 1 space used instead of 2). If possible, please fix it
up when you push the patches.

Reviewed-by: Laszlo Ersek 

Thanks!
Laszlo



> +   | Some model ->
> +   List.push_back cpu_attrs ("match", "minimum");
> +   if model = "qemu64" then
> + List.push_back cpu_attrs ("check", "none");
> +   (match source.s_cpu_vendor with
> +| None -> ()
> +| Some vendor ->
> +List.push_back cpu (e "vendor" [] [PCData vendor])
> +   );
> +   List.push_back cpu (e "model" ["fallback", "allow"] [PCData model])
> +  );
> +  (match source.s_cpu_topology with
> +   | None -> ()
> +   | Some { s_cpu_sockets; s_cpu_cores; s_cpu_threads } ->
> +  let topology_attrs = [
> +"sockets", string_of_int s_cpu_sockets;
> +"cores", string_of_int s_cpu_cores;
> +"threads", string_of_int s_cpu_threads;
> +  ] in
> +  List.push_back cpu (e "topology" topology_attrs [])
>);
>  
> +  List.push_back_list body [ e "cpu" !cpu_attrs !cpu ];
> +
>let uefi_firmware =
>  match target_firmware with
>  | TargetBIOS -> None
> diff --git a/tests/test-v2v-i-ova.xml b/tests/test-v2v-i-ova.xml
> index da1db473e5..e5907ea1cc 100644
> --- a/tests/test-v2v-i-ova.xml
> +++ b/tests/test-v2v-i-ova.xml
> @@ -10,6 +10,7 @@
>2097152
>2097152
>1
> +  
>
>  
>  

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v v2 1/3] v2v: Rename gcaps_default_cpu to gcaps_arch_min_version

[Laszlo Ersek]

On 2/17/23 12:44, Richard W.M. Jones wrote:
> Some guests require not just a specific architecture, but cannot run
> on qemu's default CPU model, eg. requiring x86_64-v2.  Since we
> anticipate future guests requiring higher versions, let's encode the
> minimum architecture version instead of a simple boolean.
> 
> This patch essentially just remaps:
> 
>   gcaps_default_cpu = true  => gcaps_arch_min_version = 0
>   gcaps_default_cpu = false => gcaps_arch_min_version = 2
> 
> I removed a long comment about how this capability is used because we
> intend to completely remove use of the capability in a coming commit.
> 
> Updates: commit a50b975024ac5e46e107882e27fea498bf425685
> ---
>  lib/types.mli| 22 +++---
>  convert/convert_linux.ml |  9 +
>  convert/convert_windows.ml   |  2 +-
>  lib/types.ml |  6 +++---
>  output/create_libvirt_xml.ml |  4 ++--
>  output/output_qemu.ml|  6 +++---
>  6 files changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/types.mli b/lib/types.mli
> index 24a93760cf..743daa8a14 100644
> --- a/lib/types.mli
> +++ b/lib/types.mli
> @@ -263,24 +263,16 @@ type guestcaps = {
>gcaps_machine : guestcaps_machine; (** Machine model. *)
>gcaps_arch : string;  (** Architecture that KVM must emulate. *)
>  
> +  gcaps_arch_min_version : int;
> +  (** Some guest OSes require not just a specific architecture, but a
> +  minimum version.  Notably RHEL >= 9 requires at least x86_64-v2.
> +
> +  If the guest is capable of running on QEMU's default VCPU model
> +  for the architecture ([-cpu qemu64]) then this is set to [0]. *)
> +
>gcaps_virtio_1_0 : bool;
>(** The guest supports the virtio devices that it does at the virtio-1.0
>protocol level. *)
> -
> -  gcaps_default_cpu : bool;
> -  (** True iff the guest OS is capable of running on QEMU's default VCPU 
> model
> -  (eg. "-cpu qemu64" with the Q35 and I440FX machine types).
> -
> -  This capability only matters for the QEMU and libvirt output modules,
> -  where, in case the capability is false *and* the source hypervisor does
> -  not specify a VCPU model, we must manually present the guest OS with a
> -  VCPU that looks as close as possible to a physical CPU.  (In that 
> case, we
> -  specify host-passthrough.)
> -
> -  The management applications targeted by the RHV and OpenStack output
> -  modules have their own explicit VCPU defaults, overriding the QEMU 
> default
> -  model even in case the source hypervisor does not specify a VCPU model;
> -  those modules ignore this capability therefore.  Refer to 
> RHBZ#2076013. *)
>  }
>  (** Guest capabilities after conversion.  eg. Was virtio found or installed? 
> *)
>  
> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
> index 460cbff0ed..d5c0f24dbb 100644
> --- a/convert/convert_linux.ml
> +++ b/convert/convert_linux.ml
> @@ -203,9 +203,10 @@ let convert (g : G.guestfs) source inspect i_firmware 
> keep_serial_console _ =
>   * microarchitecture level, which the default QEMU VCPU model does not
>   * satisfy.  Refer to RHBZ#2076013 RHBZ#2166619.
>   *)
> -let default_cpu_suffices = family <> `RHEL_family ||
> -   inspect.i_arch <> "x86_64" ||
> -   inspect.i_major_version < 9 in
> +let arch_min_version =
> +  if family <> `RHEL_family || inspect.i_arch <> "x86_64" ||
> + inspect.i_major_version < 9
> +  then 0 else 2 in
>  
>  (* Return guest capabilities from the convert () function. *)
>  let guestcaps = {
> @@ -217,8 +218,8 @@ let convert (g : G.guestfs) source inspect i_firmware 
> keep_serial_console _ =
>gcaps_virtio_socket = kernel.ki_supports_virtio_socket;
>gcaps_machine = machine;
>gcaps_arch = Utils.kvm_arch inspect.i_arch;
> +  gcaps_arch_min_version = arch_min_version;
>gcaps_virtio_1_0 = virtio_1_0;
> -  gcaps_default_cpu = default_cpu_suffices;
>  } in
>  
>  guestcaps
> diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml
> index 8d3737995f..9d8d271d05 100644
> --- a/convert/convert_windows.ml
> +++ b/convert/convert_windows.ml
> @@ -265,8 +265,8 @@ let convert (g : G.guestfs) _ inspect i_firmware _ 
> static_ips =
>gcaps_machine = of_virtio_win_machine_type
>  virtio_win_installed.Inject_virtio_win.machine;
>gcaps_arch = Utils.kvm_arch inspect.i_arch;
> +  gcaps_arch_min_version = 0;
>gcaps_virtio_1_0 = virtio_win_installed.Inject_virtio_win.virtio_1_0;
> -  gcaps_default_cpu = true;
>  } in
>  
>  guestcaps
> diff --git a/lib/types.ml b/lib/types.ml
> index 98f8bc6fa5..6c019ae130 100644
> --- a/lib/types.ml
> +++ b/lib/types.ml
> @@ -397,8 +397,8 @@ type guestcaps = {
>gcaps_virtio_socket : bool;
>gcaps_machine : guestcaps_machine;
>gcaps_arch : string;
> 

Re: [Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built

[Richard W.M. Jones]

On Mon, Feb 20, 2023 at 09:45:33AM +0100, Laszlo Ersek wrote:
> On 2/17/23 17:52, Eric Blake wrote:
> > On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
> 
> >> - Py_BuildValue with the "O" format specifier transfers the new list's
> >> *sole* reference (= ownership) to the just-built higher-level object "args"
> > 
> > Reference transfer is done with "N", not "O".  That would be an
> > alternative to decreasing the refcount of py_array on success, but not
> > eliminate the need to decrease the refcount on Py_BuildValue failure.
> > 
> >>
> >> - when "args" is killed (decref'd), it takes care of "py_array".
> >>
> >> Consequently, if Py_BuildValue fails, "py_array" continues owning the
> >> new list -- and I believe that, if we take the new error branch, we leak
> >> the object pointed-to by "py_array". Is that the case?
> > 
> > Not quite.  "O" is different than "N".
> 
> I agree with you *now*, looking up the "O" specification at
> .
> 
> However, when I was writing my email, I looked up Py_BuildValue at that
> time as well, just elsewhere. I don't know where. Really. And then that
> documentation said that the reference count would *not* be increased. I
> distinctly remember that, because it surprised me -- I actually recalled
> an *even earlier* experience reading the documentation, which had again
> stated that "O" would increase the reference count.
> 
> So right now, I have three (inconsistent) memories:
> 
> - original (old) memory: "O" increments the refcount
> - recent memory: "O" does not increment the refcount
> - your reminder: "O" does increment the refcount

I looked at the source, and 'O' definitely increments the refcount
whereas 'N' does not.  See:

https://github.com/python/cpython/blob/b1b375e2670a58fc37cb4c2629ed73b045159918/Python/modsupport.c#L463

and:

https://github.com/python/cpython/blob/b1b375e2670a58fc37cb4c2629ed73b045159918/Python/modsupport.c#L475

Rich.

> I guess I must have misread something (I can't find the document now!).
> Sorry about that; I agree we need to drop the original py_array
> reference unconditionally.
> 
> Laszlo

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built

[Laszlo Ersek]

On 2/17/23 17:52, Eric Blake wrote:
> On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:

>> - Py_BuildValue with the "O" format specifier transfers the new list's
>> *sole* reference (= ownership) to the just-built higher-level object "args"
> 
> Reference transfer is done with "N", not "O".  That would be an
> alternative to decreasing the refcount of py_array on success, but not
> eliminate the need to decrease the refcount on Py_BuildValue failure.
> 
>>
>> - when "args" is killed (decref'd), it takes care of "py_array".
>>
>> Consequently, if Py_BuildValue fails, "py_array" continues owning the
>> new list -- and I believe that, if we take the new error branch, we leak
>> the object pointed-to by "py_array". Is that the case?
> 
> Not quite.  "O" is different than "N".

I agree with you *now*, looking up the "O" specification at
.

However, when I was writing my email, I looked up Py_BuildValue at that
time as well, just elsewhere. I don't know where. Really. And then that
documentation said that the reference count would *not* be increased. I
distinctly remember that, because it surprised me -- I actually recalled
an *even earlier* experience reading the documentation, which had again
stated that "O" would increase the reference count.

So right now, I have three (inconsistent) memories:

- original (old) memory: "O" increments the refcount
- recent memory: "O" does not increment the refcount
- your reminder: "O" does increment the refcount

I guess I must have misread something (I can't find the document now!).
Sorry about that; I agree we need to drop the original py_array
reference unconditionally.

Laszlo
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] python: Avoid leaking py_array and py_args in event callbacks

[Laszlo Ersek]

On 2/17/23 18:39, Richard W.M. Jones wrote:
> Version 1 was here:
> 
>   https://listman.redhat.com/archives/libguestfs/2023-February/030732.html
> 
> Following Eric's suggestion here:
> 
>   https://listman.redhat.com/archives/libguestfs/2023-February/030746.html
> 
> let's decrement the reference of py_array right after adding it to the
> args.  (This works even if args fails to be built).

I agree; (not having seen your new patch) I'd come to the same
conclusion after reading Eric's comment. We need to drop the initial
reference on py_array unconditionally -- if we succeed constructing the
tuple, it takes ownership, so we should drop our original (temporary)
reference; and if the tuple construction fails, there's nobody to foist
py_array upon, so we need to drop it just the same.

> 
> However the other part of Eric's suggestion is wrong as it ends up
> calling Py_DECREF (args) when args == NULL along the error path.  This
> lead me to look more closely at this patch:
> 
>   https://listman.redhat.com/archives/libguestfs/2019-January/020346.html
> 
> which I believe is wrong (at least, the part that fiddles with the
> reference to args).  I cannot reproduce the original problem, nor can
> I find any justification by looking at the documentation of
> PyObject_CallObject.
> 
> So we start by reverting that commit.

Yup, when I first looked at your v1 patch, git-blame had led me to
commit 85235aec8377 ("python: fix call of Python handlers of events",
2019-01-22), and I found the Py_INCREF() call dubious.

Laszlo


___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v 1/3] v2v: Rename gcaps_default_cpu to gcaps_arch_min_version

[Laszlo Ersek]

On 2/17/23 10:56, Richard W.M. Jones wrote:
> On Fri, Feb 17, 2023 at 08:53:42AM +0100, Laszlo Ersek wrote:
>> On 2/15/23 15:12, Richard W.M. Jones wrote:
>>> Some guests require not just a specific architecture, but cannot run
>>> on qemu's default CPU model, eg. requiring x86_64-v2.  Since we
>>> anticipate future guests requiring higher versions, let's encode the
>>> minimum architecture version instead of a simple boolean.
>>>
>>> This patch essentially just remaps:
>>>
>>>   gcaps_default_cpu = true  => gcaps_arch_min_version = 0
>>>   gcaps_default_cpu = false => gcaps_arch_min_version = 2
>>>
>>> and updates the comments.
>>>
>>> Updates: commit a50b975024ac5e46e107882e27fea498bf425685
>>> ---
>>>  lib/types.mli| 19 +++
>>>  convert/convert_linux.ml |  9 +
>>>  convert/convert_windows.ml   |  2 +-
>>>  lib/types.ml |  6 +++---
>>>  output/create_libvirt_xml.ml |  4 ++--
>>>  output/output_qemu.ml|  6 +++---
>>>  6 files changed, 25 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/lib/types.mli b/lib/types.mli
>>> index 24a93760cf..4a2bb5b371 100644
>>> --- a/lib/types.mli
>>> +++ b/lib/types.mli
>>> @@ -263,24 +263,27 @@ type guestcaps = {
>>>gcaps_machine : guestcaps_machine; (** Machine model. *)
>>>gcaps_arch : string;  (** Architecture that KVM must emulate. *)
>>>  
>>> -  gcaps_virtio_1_0 : bool;
>>> -  (** The guest supports the virtio devices that it does at the virtio-1.0
>>> -  protocol level. *)
>>> +  gcaps_arch_min_version : int;
>>> +  (** Some guest OSes require not just a specific architecture, but a
>>> +  minimum version.  Notably RHEL >= 9 requires at least x86_64-v2.
>>>  
>>> -  gcaps_default_cpu : bool;
>>> -  (** True iff the guest OS is capable of running on QEMU's default VCPU 
>>> model
>>> -  (eg. "-cpu qemu64" with the Q35 and I440FX machine types).
>>> +  If the guest is capable of running on QEMU's default VCPU model
>>> +  for the architecture then this is set to [0].
>>>  
>>>This capability only matters for the QEMU and libvirt output modules,
>>> -  where, in case the capability is false *and* the source hypervisor 
>>> does
>>> +  where, in case the capability is false  {b and} the source 
>>> hypervisor does
>>
>> One omission and one typo here:
>>
>> (1a) "false" should be replaced with "nonzero",
>> (1b) there is a superfluous space character before "{b and}".
>>
>>>not specify a VCPU model, we must manually present the guest OS with 
>>> a
>>>VCPU that looks as close as possible to a physical CPU.  (In that 
>>> case, we
>>> -  specify host-passthrough.)
>>> +  specify host-model.)
>>
>> Hmm yes this is probably a leftover from commit 12473bfcb749.
>>
>> (2) However, I think we can be a bit clearer here: we specify host-model
>> for libvirt, but host-passthrough for QEMU.
>>
>> (3) ... after reading through the series though, I've come to the same
>> conclusion that's stated in commit message #3: gcaps_arch_min_version
>> becomes effectively superfluous. I'd rather suggest removing it
>> altogether then! I think that will reduce the series to a single patch,
>> which in this particular case is good, I feel.
> 
> Do you think it should be kept for informational purposes?  I worry
> about losing this information for other outputs where it might matter
> in future.  Also we can easily delete it if we find we never need it.

Hm, OK, if it seems reasonable that other outputs might need it in the
near (?) future, then it might mean less churn to keep it now. Also it
doesn't incur big maintenance costs.

Laszlo

> 
> Rich.
> 
>> I'll have some more comments on the other patches (I figure the updates
>> from those will be squashed into the single version-2 patch, so they
>> remain relevant).
>>
>> Laszlo
>>
>>>  
>>>The management applications targeted by the RHV and OpenStack output
>>>modules have their own explicit VCPU defaults, overriding the QEMU 
>>> default
>>>model even in case the source hypervisor does not specify a VCPU 
>>> model;
>>>those modules ignore this capability therefore.  Refer to 
>>> RHBZ#2076013. *)
>>> +
>>> +  gcaps_virtio_1_0 : bool;
>>> +  (** The guest supports the virtio devices that it does at the virtio-1.0
>>> +  protocol level. *)
>>>  }
>>>  (** Guest capabilities after conversion.  eg. Was virtio found or 
>>> installed? *)
>>>  
>>> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
>>> index 460cbff0ed..d5c0f24dbb 100644
>>> --- a/convert/convert_linux.ml
>>> +++ b/convert/convert_linux.ml
>>> @@ -203,9 +203,10 @@ let convert (g : G.guestfs) source inspect i_firmware 
>>> keep_serial_console _ =
>>>   * microarchitecture level, which the default QEMU VCPU model does not
>>>   * satisfy.  Refer to RHBZ#2076013 RHBZ#2166619.
>>>   *)
>>> -let default_cpu_suffices = family <> `RHEL_family ||
>>> -