Re: [libvirt] [PATCH] qemu: monitor: Add check that monitor is non-null to specific commands

On Fri, Apr 10, 2015 at 04:45:09PM +0200, Peter Krempa wrote: >Our monitor locking and cleanup code is weird as it allows to remove the >monitor object pointer while we are in the monitor. Additionally EVERY >api is using the vm private data after @vm was unlocked. This leads to >an unpleasant situation, where when qemu crashes during a monitor >session while a command is executed (and locks are down), the priv->mon >pointer gets cleared (@vm is unlocked). > >The next monitor call then crashes on commands that did not check if the >monitor is non-NULL. > >Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1209813 and few >other possible crashes >--- >Since this is a workaround rather than a proper fix I'd normally not go this >way, but untangling the monitor mess won't be easy so I'll fix the symptoms >first. > > src/qemu/qemu_monitor.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_monitor.h | 12 +++++------- > 2 files changed, 47 insertions(+), 7 deletions(-)

> You could've updated the ArbitraryCommand too. I haven't checked you fixed all of them, but most of them could use a run of attribute clean-ups ;) >@@ -774,7 +772,7 @@ int qemuMonitorBlockJobInfo(qemuMonitorPtr mon, > const char *device, > virDomainBlockJobInfoPtr info, > unsigned long long *bandwidth) >- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >+ ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > You removed the attribute but haven't fixed the function itself. ACK with at least functions qemuMonitorBlockJobInfo() and qemuMonitorArbitraryCommand() fixed as well. Pity there isn't an easy way of testing all the functions with NULLs.