> On 3 Mar 2026, at 1:31 AM, Peter Xu <[email protected]> wrote:
>
> On Mon, Mar 02, 2026 at 04:58:50PM +0530, Prasad Pandit wrote:
>> Hello all,
>>
>> On Thu, 26 Feb 2026 at 21:46, Ani Sinha <[email protected]> wrote:
>>>>>> On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
>>>>>>> Currently the code that adds a migration blocker does not check if the
>>>>>>> same
>>>>>>> blocker already exists. Return an EEXIST error code if there is an
>>>>>>> attempt to
>>>>>>> add the same migration blocker again. This way the same migration
>>>>>>> blocker will
>>>>>>> not get added twice.
>>>>>>
>>>>>> Could you help explain why it will inject two identical errors in the
>>>>>> first
>>>>>> place, and why the caller cannot make sure it won't be injected twice?
>>>>>
>>>>> Likely due to a bug in the code. For example if the init function that
>>>>> adds the blocker is called again and the caller does not handle the
>>>>> second init call properly. This came up as a part of the coco reset work
>>>>> where migration blockers are added in init methods. They need not be
>>>>> added again when init methods are again called during the reset
>>>>> process. The caller can handle it of course but adding a check further
>>>>> down the call stack makes things more robust.
>>>>
>>>> IMHO if we want to make it more robust, we shouldn't return an error
>>>> because the caller may not always check for errors.
>>>>
>>>> Would assertion suites more here? Because migration blockers are not
>>>> something the user can manipulate, so it's a solid bug to fix when
>>>> triggered.
>>>
>>> If Prasad agrees, I will send something.
>>
>> * The majority of the places I see constructs like:
>> ===
>> if (migrate_add_blocker(&g->migration_blocker, errp) < 0) {
>> OR
>> ret = migrate_add_blocker(&hv_no_nonarch_cs_mig_blocker, &local_err);
>> if (ret < 0) {
>> error_report_err(local_err);
>> ===
>>
>> * So setting **errp and returning a negative value is consistent with
>> that. Aborting (assert(3)) for trying to add a duplicate blocker
>> object seems like a harsh punishment, considering that'll happen at
>> run time and the user won't be able to do much then.
>
> If it's a programming error, then it shouldn't happen at runtime. The
> current paths to fail this API was not for programming errors.
I have sent a patch with an assertion added
https://mail.gnu.org/archive/html/qemu-devel/2026-03/msg02602.html
>
> If this patch isn't required, IMHO we can at least drop it in this series
> and make it separate.
>
> Thanks,
>
> --
> Peter Xu
