Re: [Qemu-devel] [PATCH v4 0/3] Introduce vhost-user-scsi and sample application

[Daniel P. Berrange]

On Thu, Mar 02, 2017 at 05:21:46PM +0000, Felipe Franciosi wrote:
> 
> > On 2 Mar 2017, at 17:17, Daniel P. Berrange <address@hidden> wrote:
> > 
> > On Thu, Mar 02, 2017 at 05:01:06PM +0000, Felipe Franciosi wrote:
> >> Hi Eric,
> >> 
> >> Ping on this?
> >> http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg06169.html
> > 
> > [snip]
> > 
> >>      CC      contrib/ivshmem-client/main.o
> >>    
> >> /var/tmp/patchew-tester-tmp-naxuy_ku/src/contrib/libvhost-user/libvhost-user.c:
> >>  In function ‘vu_panic’:
> >>    
> >> /var/tmp/patchew-tester-tmp-naxuy_ku/src/contrib/libvhost-user/libvhost-user.c:84:5:
> >>  error: ignoring return value of ‘vasprintf’, declared with attribute 
> >> warn_unused_result [-Werror=unused-result]
> >>         (void)vasprintf(&buf, msg, ap);
> >>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>    cc1: all warnings being treated as errors
> > 
> > You can side-step the issue entirely by simply not using the vasprintf
> > function. Instead do   'buf = g_strdup_vprintf(msg, ap);' - glib does
> > abort-on-oom, so there's no error scenario you need to handle.
> 
> Thanks for the pointer. That code is in libvhost-user already. I can send a 
> separate patch to fix that if you prefer.

Yeah, it wasn't previously noticed since these files weren't compiled
by default. Since you've added make rules to enable build, I'm afraid
you've got the unfortunate job of cleaning up pre-existing build
problems. I'd suggest just doing the conversion to g_strdup_vprintf()
in the first patch your series.

> Just think that for this kind of usage a simple ignore_value(vasprintf(...)) 
> is sufficient; that call is in a panic path already.

I think it is more correct to switch to g_strdup_vprintf so we get an immediate
abort, than to use ignore_value here.

> There were other cases where ignoring the return value seemed like the
> preferred thing to do.

Thre's a few scenarios where ignoring an OOM error is appropriate, but as a
general rule QEMU aims to abort on OOM rather than ignore it or try to handle
it.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|