Re: [PATCH v2] QIOChannelSocket: Flush zerocopy socket error queue on ENOBUF failure for sendmsg

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 11, 2025 at 03:33:23PM +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 11, 2025 at 11:20:50AM -0400, Peter Xu wrote:
> > On Tue, Mar 11, 2025 at 08:13:16AM +0000, Daniel P. Berrangé wrote:
> > > On Mon, Mar 10, 2025 at 04:03:26PM -0400, Peter Xu wrote:
> > > > On Mon, Mar 10, 2025 at 07:48:16PM +0000, Daniel P. Berrangé wrote:
> > > > > Given this is in public API, the data needs to remain reported accurately
> > > > > for the whole deprecation period. IOW, the patch to qiochannel needs to
> > > > > preserve this data too.
> > > > 
> > > > :-(
> > > > 
> > > > We could potentially mark MigrationStats to be experimental as a whole and
> > > > declare that in deprecate.rst too, then after two releases, we can randomly
> > > > add / remove fields as wish without always need to go through the
> > > > deprecation process, am I right?
> > > 
> > > IMHO that would be an abuse of the process and harmful to applications
> > > and users consuming stats.
> > 
> > Ah I just noticed that's the exact same one we included in
> > query-migrate.. Then yes, the stable ABI is important here.
> > 
> > So for this specific case, maybe we shouldn't have exposed it in QMP from
> > the start.
> > 
> > To me, it's a question on whether we could have something experimental and
> > be exposed to QMP, where we don't need to guarantee a strict stable ABI, or
> > a very loose ABI (e.g. we can guarantee the command exists, and with
> > key-value string-integer pairs, nothing else).
> 
> QMP has the ability to tag commands/fields, etc as experimental.
> 
> libvirt will explicitly avoid consuming or exposing anything with
> an experimental tag on it.
> 
> > Maybe what we need is a new MigrationInfoOptional, to be embeded into
> > MigrationInfo (or not), marked experimental.  Then in the future whenever
> > we want to add some new statistics, we could decide whether it should be
> > part of stable ABI or not.
> 
> That is not required - individual struct fields can be marked
> experimental.

Yes that'll work too.  The important bit here is I think we should start to
seriously evaluate which to expose to QAPI as stable API when we add stats
into it.  We used to not pay too much attention.

With MigrationInfoOptional, we should suggest any new field to be added
there by default, then whatever needs to be put out of experimental needs
explicit justifications.  Or we can also document any new migration field
at least in the stats to be marked as experimental unless justified.

> 
> The key question is what the intended usage of the fields/stats/etc
> is to be. If you want it used by libvirt and mgmt apps it would need
> to be formally supported. If it is just for adhoc QEMU developer
> debugging and doesn't need libvirt / app support, then experimental
> is fine.

To my initial thoughts, I want Libvirt to fetch it.  However I don't want
Libvirt to parse it.

For example, for things like "whether zerocopy send succeeded or not", or
"how much time we spent on sending non-iterable device states", they're
almost not consumable for users, but great for debuggings.  It would be
great if Libvirt could know their existance, fetch it (e.g. once after
migration completes) then dump it to the logfile to help debugging and
triaging QEMU issues.  In that case parsing is not needed, the whole result
can be attached to the log as a JSON blob.  That releases the burden from
the need to maintain compatibility that we don't really need and nobody
cared (I bet it's the case here for zerocopy stats, but we got restricted
by our promises even if it may ultimately benefit nobody..).

Thanks,

-- 
Peter Xu