Jag Raman <jag.ra...@oracle.com> writes:
> Hi Markus,
>
> Please see the comments inline below.
>
>> On May 5, 2023, at 10:58 AM, Markus Armbruster <arm...@redhat.com> wrote:
>>
>> I stumbled over this property, looked closer, and now I'm confused.
>>
>> Like most QOM properties, x-remote.auto-shutdown is virtually
>> undocumented. All we have is this comment in vfio-user-obj.c:
>>
>> /**
>> * Usage: add options:
>> * -machine x-remote,vfio-user=on,auto-shutdown=on
>> * -device <PCI-device>,id=<pci-dev-id>
>> * -object x-vfio-user-server,id=<id>,type=unix,path=<socket-path>,
>> * device=<pci-dev-id>
>> *
>> * Note that x-vfio-user-server object must be used with x-remote machine
>> only.
>> * This server could only support PCI devices for now.
>> *
>> * type - SocketAddress type - presently "unix" alone is supported.
>> Required
>> * option
>> *
>> * path - named unix socket, it will be created by the server. It is
>> * a required option
>> *
>> * device - id of a device on the server, a required option. PCI devices
>> * alone are supported presently.
>> *
>> * notes - x-vfio-user-server could block IO and monitor during the
>> * initialization phase.
>> */
>>
>> This differs from docs/system/multi-process.rst, which has
>>
>> - Example command-line for the remote process is as follows:
>>
>> /usr/bin/qemu-system-x86_64 \
>> -machine x-remote \
>> -device lsi53c895a,id=lsi0 \
>> -drive id=drive_image2,file=/build/ol7-nvme-test-1.qcow2 \
>> -device scsi-hd,id=drive2,drive=drive_image2,bus=lsi0.0,scsi-id=0 \
>> -object x-remote-object,id=robj1,devid=lsi0,fd=4,
>>
>> No mention of auto-shutdown here.
>>
>> It points to docs/devel/qemu-multiprocess, which doesn't exist. I guess
>> it's docs/devel/multi-process.rst. Please fix that. Anyway, no mention
>
> Sorry about this. I will fix it.
Thanks!
>> of auto-shutdown there, either.
>>
>> Let's try code instead. The only use of the property is here:
>>
>> static bool vfu_object_auto_shutdown(void)
>> {
>> bool auto_shutdown = true;
>> Error *local_err = NULL;
>>
>> if (!current_machine) {
>> return auto_shutdown;
>> }
>>
>> auto_shutdown = object_property_get_bool(OBJECT(current_machine),
>> "auto-shutdown",
>> &local_err);
>>
>> /*
>> * local_err would be set if no such property exists - safe to ignore.
>> * Unlikely scenario as auto-shutdown is always defined for
>> * TYPE_REMOTE_MACHINE, and TYPE_VFU_OBJECT only works with
>> * TYPE_REMOTE_MACHINE
>> */
>> if (local_err) {
>> auto_shutdown = true;
>> error_free(local_err);
>> }
>>
>> return auto_shutdown;
>> }
>>
>> The comment suggests auto-shutdown should always be set with machine
>> TYPE_REMOTE_MACHINE, i.e. -machine x-remote basically requires
>> auto-shutdown=on. Why isn't it the default then? Why is it even
>> configurable? Use cases?
>
> The "auto-shutdown" property tells the server if it should continue running
> after all the clients disconnect or if it should shut down automatically after
> the last client disconnects.
>
> The user can set this property to "off" when the server serves multiple
> QEMU clients. The server process will continue to run after the last
> client disconnects, waiting for more clients to connect in the future.
>
>>
>> Anyway, vfu_object_auto_shutdown() returns
>>
>> (1) true when we don't have a current machine
>>
>> (2) true when getting the current machine's auto-shutdown property fails
>>
>> (3) the value of its auto-shutdown property otherwise
>>
>> Two uses:
>>
>> * In vfu_object_finalize():
>>
>> if (!k->nr_devs && vfu_object_auto_shutdown()) {
>> qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> }
>>
>> I guess this requests shutdown when the last TYPE_VFU_OBJECT dies.
Double-checking:
1. Is TYPE_VFU_OBJECT intended only for TYPE_REMOTE_MACHINE?
2. If yes, what prevents use with another machine type?
>> SHUTDOWN_CAUSE_GUEST_SHUTDOWN is documented as
>>
>> # @guest-shutdown: Guest shutdown/suspend request, via ACPI or other
>> # hardware-specific means
>>
>> Can't say whether it's the right one to use here.
>>
>> * In VFU_OBJECT_ERROR():
>>
>> /**
>> * VFU_OBJECT_ERROR - reports an error message. If auto_shutdown
>> * is set, it aborts the machine on error. Otherwise, it logs an
>> * error message without aborting.
>> */
>> //
>> #define VFU_OBJECT_ERROR(o, fmt, ...)
>> \
>> {
>> \
>> if (vfu_object_auto_shutdown()) {
>> \
>> error_setg(&error_abort, (fmt), ## __VA_ARGS__);
>> \
>> } else {
>> \
>> error_report((fmt), ## __VA_ARGS__);
>> \
>> }
>> \
>> }
>> \
>>
>> Here, the property does something entirely different: it makes QEMU
>> *crash* on certain errors.
>>
>> Why are these errors so harmless when auto-shutdown is off that it's
>> safe to continue, but so serious when it's on that we must crash
>> immediately?
>>
>> Could you explain to me why it makes sense to tie "automatic shutdown"
>> and "crash on certain errors" together?
>
> When auto-shutdown is "off," we don't want the server to die automatically
> unless explicitly killed. As such, we report the error instead of crashing
> the server when auto-shutdown is off.
You report the error to stderr and then continue. This is safe only
when the error is recoverable.
Are you sure they all are?
Aborting on recoverable error is problematic. Can you explain in more
detail why you want to abort?
> -- Jag
>
>>
>> By the way, both uses of vfu_object_auto_shutdown() are in device code.
>> I can't see how its cases (1) and (2) can be reached.
Why do these cases exist?
