Re: [Qemu-devel] [PATCH] audio: Convert use of atoi to qemu_strtoi

[nee]

On Mon, Mar 19, 2018 at 2:47 PM, Eric Blake  wrote:
> On 03/16/2018 09:40 AM, Nia Alarie wrote:
>>
>> If qemu_strtoi indicates an error, return the default value.
>
>
> Would it be better to diagnose the error instead of silently returning a
> default value?
>
>>
>> Signed-off-by: Nia Alarie 
>> ---
>>   audio/audio.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/audio/audio.c b/audio/audio.c
>> index 6eccdb17ee..d6e91901aa 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
>> @@ -335,9 +335,8 @@ static int audio_get_conf_int (const char *key, int
>> defval, int *defaultp)
>>   char *strval;
>> strval = getenv (key);
>> -if (strval) {
>> +if (strval && !qemu_strtoi(strval, NULL, 10, )) {
>>   *defaultp = 0;
>> -val = atoi (strval);
>>   return val;
>>   }
>>   else {
>>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Possibly, while writing these patches I was just going by what was
already there. I can see how that would be good.

Should the code provide a warning to the user and continue with the
default, or provide the warning and exit? And is it more correct to
use dolog() or AUD_log() in this context?

Re: [Qemu-devel] [PATCH] block/xen_disk: Convert atoi use to qemu_strtol to allow error checking

[nee]

On Fri, Mar 16, 2018 at 2:43 PM, Nia Alarie  wrote:
> Signed-off-by: Nia Alarie 
> ---
>  hw/block/xen_disk.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index f74fcd42d1..a9ec0ad6eb 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -32,6 +32,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qemu/cutils.h"
>  #include "trace.h"
>
>  /* - */
> @@ -1010,7 +1011,10 @@ static int blk_init(struct XenDevice *xendev)
>  blkdev->devtype = xenstore_read_be_str(>xendev, 
> "device-type");
>  }
>  directiosafe = xenstore_read_be_str(>xendev, "direct-io-safe");
> -blkdev->directiosafe = (directiosafe && atoi(directiosafe));
> +
> +if (directiosafe && qemu_strtoi(directiosafe, NULL, 10, 
> >directiosafe)) {
> +goto out_error;
> +}
>
>  /* do we have all we need? */
>  if (blkdev->params == NULL ||
> --
> 2.16.2
>

I've just realised that this patch is slightly wrong and doesn't match
the original code exactly. Please disregard.

Re: [Qemu-devel] [PATCH] 9p: Convert use of atoi to qemu_strtol to allow error checking

[nee]

On Mon, Mar 12, 2018 at 1:21 PM, Greg Kurz  wrote:
> On Mon, 12 Mar 2018 13:08:29 +
> Daniel P. Berrangé  wrote:
>
>> On Mon, Mar 12, 2018 at 02:02:29PM +0100, Greg Kurz wrote:
>> > On Mon, 12 Mar 2018 07:12:52 -0500
>> > Eric Blake  wrote:
>> >
>> > > On 03/11/2018 03:12 PM, Nia Alarie wrote:
>> > > > Signed-off-by: Nia Alarie 
>> > > > ---
>> > > >   hw/9pfs/9p.c | 12 ++--
>> > > >   1 file changed, 10 insertions(+), 2 deletions(-)
>> > > >
>> > >
>> > > >   } else if (perm & P9_STAT_MODE_LINK) {
>> > > > -int32_t ofid = atoi(extension.data);
>> > > > -V9fsFidState *ofidp = get_fid(pdu, ofid);
>> > > > +long ofid;
>> > > > +V9fsFidState *ofidp;
>> > > > +
>> > > > +if (qemu_strtol(extension.data, NULL, 10, ) ||
>> > > > +ofid > INT32_MAX || ofid < INT32_MIN) {
>> > >
>> > > Dan has a pending patch that will add qemu_strtoi, which might be a
>> > > nicer fit for this situation:
>> > >
>> > > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg00952.html
>> > >
>> > > int32_t is not necessarily int, but all platforms that compile qemu have
>> > > 'int32_t' and 'int' both at 32 bits, so it's simpler to change to 'int
>> > > ofid' and use Dan's function than it is to parse to long and then do
>> > > bounds checking.  Except that Dan still needs to post an updated version
>> > > of his patch...
>> > >
>> >
>> > I wasn't aware of Dan's patch but I agree it would result in a nicer
>> > change for 9p. This being said, tomorrow is soft freeze... is there
>> > a chance Dan's patch reaches master anytime soon ?
>>
>> I just sent an update of my series. If it gets acked by Eric, I'd be able
>> to send a pull request today.
>>
>> Regards,
>> Daniel
>
> Great !
>
> Nia,
>
> Could you please respin your patch on top of Daniel's series, using
> qemu_strtoui() ?
>
> Thanks,
>
> --
> Greg

I'm a bit unclear on how this works so I thought I'd ask here before I
send any more patches - this is CCed to my mentors as well. It's
"tomorrow" now, but I don't understand how soft freezes work or the
process beyond "I send this patch to qemu-devel and people say if I
should re-send it with changes, or that it's been accepted" :)

There are several more conversions from ato* to qemu_strto* I'd like
to submit from now on. Should I send them as using qemu_strtol now, to
get them "out there" before my own personal deadlines, and send
further patches to convert them to qemu_strtoi later, or should I
submit patches that use qemu_strtoi? I'm not sure of the current
status of Daniel's patches.

Thank you.

Re: [Qemu-devel] [PATCH v2] 9p: Convert use of atoi to qemu_strtoi to allow error checking

[nee]

On Mon, Mar 12, 2018 at 3:43 PM, Eric Blake  wrote:
> On 03/12/2018 10:33 AM, Nia Alarie wrote:
>>
>> Signed-off-by: Nia Alarie 
>> ---
>>   hw/9pfs/9p.c | 11 +--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>
>
> Helping out our CI tools:
> Based-on: <20180312124939.20562-1-berra...@redhat.com>
>
>>
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index 48fa48e720..254385dfa4 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -15,6 +15,7 @@
>>   #include 
>>   #include "hw/virtio/virtio.h"
>>   #include "qapi/error.h"
>> +#include "qemu/cutils.h"
>>   #include "qemu/error-report.h"
>>   #include "qemu/iov.h"
>>   #include "qemu/sockets.h"
>> @@ -2213,8 +2214,14 @@ static void coroutine_fn v9fs_create(void *opaque)
>>   }
>>   v9fs_path_copy(>path, );
>>   } else if (perm & P9_STAT_MODE_LINK) {
>> -int32_t ofid = atoi(extension.data);
>> -V9fsFidState *ofidp = get_fid(pdu, ofid);
>> +int ofid;
>
>
> 'unsigned int' and...
>
>> +V9fsFidState *ofidp;
>> +
>> +if (qemu_strtoi(extension.data, NULL, 10, )) {
>
>
> qemu_strtoui() might be smarter, per Greg's comments on v1.
>
>> +err = -EINVAL;
>> +goto out;
>> +}
>> +ofidp = get_fid(pdu, (int32_t)ofid);
>
>
> This cast is spurious.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

I did this because get_fid() takes an int32_t, not an unsigned int.
The struct V9fsFidState also uses an int32_t for its `fid` member. Do
you want me to change all these types, or just the function being used
here?

Re: [Qemu-devel] [PATCH] s390x/virtio: Convert virtio-ccw from *_exit to *_unrealize

[nee]

On Thu, Mar 8, 2018 at 4:13 PM, Stefan Hajnoczi  wrote:
> On Wed, Mar 07, 2018 at 04:29:58PM +, Nia Alarie wrote:
>> @@ -760,12 +760,12 @@ static int virtio_ccw_exit(VirtioCcwDevice *dev)
>>  if (sch) {
>>  css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, 
>> NULL);
>>  g_free(sch);
>> +ccw_dev->sch = NULL;
>
> This change is unrelated to the topic of the patch and not mentioned in
> the commit description.  I think it was probably made for consistency,
> rather than a bug fix or functional change.  That's a valid reason too
> but please mention secondary changes like this in future patches so
> reviewers know why you made them.
>
> Reviewed-by: Stefan Hajnoczi 

Indeed, this was done for consistency (but also, it feels like a good
practice to follow). Thanks for the review.

Re: [Qemu-devel] [PATCH v3 2/2] s390x: Change return type of virtio_ccw_exit to void.

[nee]

On Tue, Mar 6, 2018 at 2:40 PM, Cornelia Huck  wrote:
> On Tue,  6 Mar 2018 10:07:21 +
> Nia Alarie  wrote:
>
>> Allows a branch to be removed - this function always returns 0.
>>
>> Signed-off-by: Nia Alarie 
>> Reviewed-by: Christian Borntraeger 
>> ---
>>  hw/s390x/virtio-ccw.c | 6 +++---
>>  hw/s390x/virtio-ccw.h | 2 +-
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> While your patch is not wrong, I'd prefer to skip changing the exit
> functions and convert virtio-ccw to unrealize instead.
>
> Should not be too hard; do you want to take a stab at it?

Is there any difference in semantics between exit and unrealize aside
from the arguments and return type?