> On 25 Feb 2026, at 3:02 PM, Markus Armbruster <[email protected]> wrote:
>
> Daniel P. Berrangé <[email protected]> writes:
>
>> On Wed, Feb 25, 2026 at 11:35:15AM +0530, Prasad Pandit wrote:
>>> On Wed, 25 Feb 2026 at 09:23, Ani Sinha <[email protected]> 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.
>>>>
>>>> Suggested-by: Prasad Pandit <[email protected]>
>>>> Signed-off-by: Ani Sinha <[email protected]>
>>>> ---
>>>> migration/migration.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index a5b0465ed3..1eb75fb7fb 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned
>>>> modes, Error **errp)
>>
>> This method has an '**errp' parameter.....
>>
>>>> {
>>>> for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>>> if (modes & BIT(mode)) {
>>>> + if (g_slist_index(migration_blockers[mode],
>>>> + *reasonp) >= 0) {
>>>> + return -EEXIST;
>>
>> .... so using -errno for return values is not appropriate - it must
>> set 'errp' and return -1.
>
> Yes, it must set @errp on failure. Returning -1 would be wrong,
Ok so just
diff --git a/migration/migration.c b/migration/migration.c
index 1eb75fb7fb..dc75d2eeb6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1704,6 +1704,8 @@ static int add_blockers(Error **reasonp, unsigned modes,
Error **errp)
if (modes & BIT(mode)) {
if (g_slist_index(migration_blockers[mode],
*reasonp) >= 0) {
+ error_propagate_prepend(errp, *reasonp,
+ "migration blocker already added ");
return -EEXIST;
}
migration_blockers[mode] =
g_slist_prepend(migration_blockers[mode],
> because:
>
> int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error
> **errp)
> {
> if (is_only_migratable(reasonp, modes, errp)) {
> return -EACCES;
> } else if (is_busy(reasonp, errp)) {
> return -EBUSY;
> }
> return add_blockers(reasonp, modes, errp);
> }
>
> If callers don't actually care for error codes, the entire nest of
> functions could be simplified to return 0/-1 or true/false.
>
>>>> + }
>>>> migration_blockers[mode] =
>>>> g_slist_prepend(migration_blockers[mode],
>>>> *reasonp);
>>>> }
>>>
>>> * Looks okay.
>>> Reviewed-by: Prasad Pandit <[email protected]>
>>>
>>> Thank you.
>>> ---
>>> - Prasad
>>>
>>>
>>
>> With regards,
>> Daniel
