Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

Hi, Gerd

> -----Original Message-----
> From: Gerd Hoffmann [mailto:address@hidden
> Sent: Friday, July 25, 2014 5:46 PM
> 
>   Hi,
> 
> > +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
> > +                          const char *suffix)
> > +{
> > +    FWBootEntry *node, *i;
> > +
> > +    assert(dev != NULL || suffix != NULL);
> > +
> > +    QTAILQ_FOREACH(i, &fw_boot_order, link) {
> > +        if (i->bootindex == bootindex) {
> > +            qerror_report(ERROR_CLASS_GENERIC_ERROR,
> > +               "The bootindex %d has already been used", bootindex);
> > +            return;
> > +        }
> > +        /* delete the same original dev */
> > +        if (i->dev->id && !strcmp(i->dev->id, dev->id)) {
> > +            QTAILQ_REMOVE(&fw_boot_order, i, link);
> 
> Ok ...
> 
> > +            g_free(i->suffix);
> > +            g_free(i);
> 
> ... but you should free the old entry later ...
> 
> > +
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (bootindex >= 0) {
> > +        node = g_malloc0(sizeof(FWBootEntry));
> > +        node->bootindex = bootindex;
> > +        node->suffix = g_strdup(suffix);
> 
> ... because you can just copy the suffix from the old entry here,
> instead of expecting the caller pass it in.
> 
Okay, agreed.

But we should also think about the situation which a device don't have
old entry in global fw_boot_order list. So we can do like this:

if (suffix) {
    node->suffix = g_strdup(suffix);
} else if (has_old_entry) {
        node->suffix = g_strdup(old_entry->suffix);
} else {
        node->suffix = NULL;
}

Best regards,
-Gonglei