Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object

[Kevin Wolf]

Am 15.03.2021 um 15:15 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 13.03.2021 um 13:30 hat Markus Armbruster geschrieben:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> 
> >> > On 13/03/21 08:40, Markus Armbruster wrote:
> >> >>> +                if (!user_creatable_add_from_str(optarg, &local_err)) 
> >> >>> {
> >> >>> +                    if (local_err) {
> >> >>> +                        error_report_err(local_err);
> >> >>> +                        exit(2);
> >> >>> +                    } else {
> >> >>> +                        /* Help was printed */
> >> >>> +                        exit(EXIT_SUCCESS);
> >> >>> +                    }
> >> >>> +                }
> >> >>> +                break;
> >> >>>               }
> >> >>> -        }   break;
> >> >>>           case OPTION_IMAGE_OPTS:
> >> >>>               image_opts = true;
> >> >>>               break;
> >> >> Why is this one different?  The others all call
> >> >> user_creatable_process_cmdline().
> >> >> 
> >> >> 
> >> >
> >> > It's to exit with status code 2 instead of 1.
> >> 
> >> I see.  Worth a comment?
> >
> > There is a comment at the start of the function (which is just a few
> > lines above) that explains the exit codes:
> >
> >  * Compares two images. Exit codes:
> >  *
> >  * 0 - Images are identical or the requested help was printed
> >  * 1 - Images differ
> >  * >1 - Error occurred
> 
> I had in mind a comment that helps me over the "why is this not using
> user_creatable_process_cmdline()" hump.  Like so:
> 
>         case OPTION_OBJECT:
>             {
>                 /*
>                  * Can't use user_creatable_process_cmdline(), because
>                  * we need to exit(2) on error.
>                  */
>                 ... open-coded variation of
>                 user_creatable_process_cmdline() ...
>             }
> 
> Entirely up to you.

I see. This patch is already part of a pull request, but I wouldn't mind
a follow-up patch to add this comment if you want to send one.

Kevin